Add support for 'fixed link' configuration on Kinetics MCUX ENET driver

[lmajewski]

Following changes to Kinetics MCUX ENET driver:

• Disable usage of SMI to read status of PHY device.
  Rationale:
  Some devices - like DSA switches (ksz8794) use SPI for configuration and reading its status
  (instead of MDIO). The current implementation of ENET driver (in eth_enet.c ) is very tightly coupled with SMI interrupts as they allow moving to the next stage of this driver's state machine. This change decouples driver logic from them when required.

• Add support for 'fixed-link' DTS property
  With this patch set the enet driver now supports setting fixed transmission parameters between
  PHY (DSA) and ENET.

Please consider those changes as a preparatory work for DSA support for Zephyr.

[jukkar]

Please fix also checkpatch warnings.

[galak]

I need to look at the DT stuff, but think it can be handled possibly as a child. you can see something like dts/bindings/mtd/partition.yaml

[pfalcon]

@lmajewski: I usually all for having well-split, dine-detail commits, but sometimes, you can split too much. Your commit "eth: mcux: Support devices not using SMI for PHY setup" says in description "This change introduces a new Kconfig define - CONFIG_ETH_MCUX_NO_PHY_SMI", but it actually doesn't, and thus references an unknown CONFIG_* option. Bottom line: please squash "eth: Kconfig: Add new ETH_MCUX_NO_PHY_SMI Kconfig option" into that commit. (The rest seems to be ok.)

[pfalcon]

@lmajewski: Also, while doing the above, please consider whether it's worth to name the option CONFIG_ETH_MCUX_NO_PHY_SMI, maybe it makes sense to cut the negation, name it CONFIG_ETH_MCUX_PHY_SMI, have it "y" be default, and just disable for platforms where needed. Well, might worth asking @MaureenHelm whether she thinks that's anyhow important.

[pfalcon]

@jukkar: It seems that your previous comments were addressed, can you have a look and maybe dismiss your -1 to motivate other reviewers to look further into this?

[lmajewski]

@lmajewski: I usually all for having well-split, dine-detail commits, but sometimes, you can split too much. Your commit "eth: mcux: Support devices not using SMI for PHY setup" says in description "This change introduces a new Kconfig define - CONFIG_ETH_MCUX_NO_PHY_SMI", but it actually doesn't, and thus references an unknown CONFIG_* option. Bottom line: please squash "eth: Kconfig: Add new ETH_MCUX_NO_PHY_SMI Kconfig option" into that commit. (The rest seems to be ok.)

I will squash those commits - thanks for pointing this out

@lmajewski: Also, while doing the above, please consider whether it's worth to name the option CONFIG_ETH_MCUX_NO_PHY_SMI, maybe it makes sense to cut the negation, name it CONFIG_ETH_MCUX_PHY_SMI, have it "y" be default, and just disable for platforms where needed. Well, might worth asking @MaureenHelm whether she thinks that's anyhow important.

As the eth_mcux.c driver uses PHY_SMI for all NXP SoCs excluding only DSA case - I think that it is more readable to use straight logic and put all the necessary code under
#ifdef CONFIG_ETH_MCUX_NO_PHY_SMI

Otherwise, we would have negation in the #ifdef - #ifndef which IMHO is far more difficult to read correctly. Additionally, with current approach we can only enable this option when we do need it - in the DSA case.

To sum up:
I would prefer to keep the name CONFIG_ETH_MCUX_NO_PHY_SMI (and current approach) as it shows that this code doesn't use
SMI interrupts for eth_mcux operation.

[pfalcon]

Otherwise, we would have negation in the #ifdef - #ifndef which IMHO is far more difficult to read correctly.

I would disagree with that, it's easier to deal with standard language syntax of #ifdef/#ifndef which applies to any situation, than to adhocly named options which embed some kind of negation somewhere in their names.

That said, as long as that option is considered, and you have arguments to prefer the current situation, it sounds good to me.

[lmajewski]

Otherwise, we would have negation in the #ifdef - #ifndef which IMHO is far more difficult to read correctly.

I would disagree with that, it's easier to deal with standard language syntax of #ifdef/#ifndef which applies to any situation, than to adhocly named options which embed some kind of negation somewhere in their names.

That said, as long as that option is considered, and you have arguments to prefer the current situation, it sounds good to me.

Please correct me if I've misunderstood something - you are OK to keep things as they are proposed in this patch set (as I've pointed out arguments for such situation).

However, you disagree with the generic approach to have CONFIG_* names with negation in them and you prefer to use #ifndef instead when applicable?

[pfalcon]

Please correct me if I've misunderstood something - you are OK to keep things as they are proposed in this patch set (as I've pointed out arguments for such situation).

Yes.

However, you disagree with the generic approach to have CONFIG_* names with negation in them and you prefer to use #ifndef instead when applicable?

Yes, I just share my personal opinion on that matter. Maybe you'll find some merit in it and consider it next time (maybe not). Likewise, I'm listening to arguments of others. All that should help make the project better/more consistent/easier to use for everyone.

Bottom line: please make the commit squash discussed above. Then, from my side, I'd just like to test it on frdm_k64f (keeping fingers crossed that I'll have time for that this week).

[agansari]

Code looks okay, waiting for @pfalcon to test it to approve.

My comment related to Kconfig: I find the double negation sometimes harder to follow
#ifndef CONFIG_ETH_MCUX_NO_PHY_SMI
In logic a double negation is a truth statement, but it's not as clear and straight forward as a direct truth claim when following code.

[pfalcon]

Sorry for the delay here, tested on frdm_k64f with my usual tests - dumb_http_server, total ~5000 requests, and big_http_download, 3 iterations, total 20MB downloaded.

Ideally, I wish this was reviewed/approved by @agansari and/ot @MaureenHelm, but as this waited in queue for so long, let me cast +1 from me now.

[MaureenHelm]

I agree the double negative is confusing and I generally prefer to see positive logic in Kconfigs. But given that 3 networking folks have +1-ed this, I'm ok to keep it as it is.