[RFC] Pull request for ip_k66f board 13.04

[lmajewski]

Dear Community,

This PR is more a RFC for proposed changes for NXP's Kinetics MCUX ENET to add support for
KSZ8794 DSA switch.

Most notable changes:

• Fixed clock timing - sleep now works correctly on ip_k66f
• Restructure the Kconfig.defconfig and ip_k66f_defconfig
• The MII interrupt has been disabled. It is not needed as we don't use SMI for PHY communication and we solely use SPI.
• Support for ETH MCUX fixed PHY (Similar to Linux kernel)
• Introduction of DSA infrastructure to Zephyr
• KSZ8794 setup and configuration

I would like to hear your opinion on the FIXED_PHY and proposed DSA support.

[zephyrbot]

Some checks failed. Please fix and resubmit.

checkpatch issues

-:27: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#27: FILE: boards/arm/ip_k66f/Kconfig.defconfig:32:
+        default y if !MODEM$

-:30: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#30: FILE: boards/arm/ip_k66f/Kconfig.defconfig:35:
+        default y if NET_L2_ETHERNET$

-:33: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#33: FILE: boards/arm/ip_k66f/Kconfig.defconfig:38:
+        default y if ETH_MCUX$

-:36: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#36: FILE: boards/arm/ip_k66f/Kconfig.defconfig:41:
+        default y if ETH_MCUX$

-:39: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#39: FILE: boards/arm/ip_k66f/Kconfig.defconfig:44:
+        default y if ETH_MCUX$

-:42: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#42: FILE: boards/arm/ip_k66f/Kconfig.defconfig:47:
+        default y if ETH_MCUX$

-:49: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#49: FILE: boards/arm/ip_k66f/Kconfig.defconfig:54:
+        default y if SPI$

-:94: ERROR:CODE_INDENT: code indent should use tabs where possible
#94: FILE: boards/arm/ip_k66f/ip_k66f.dts:87:
+        status = "okay";$

-:94: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#94: FILE: boards/arm/ip_k66f/ip_k66f.dts:87:
+        status = "okay";$

-:98: ERROR:CODE_INDENT: code indent should use tabs where possible
#98: FILE: boards/arm/ip_k66f/ip_k66f.dts:91:
+        status = "okay";$

-:98: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#98: FILE: boards/arm/ip_k66f/ip_k66f.dts:91:
+        status = "okay";$

-:288: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#288: FILE: drivers/ethernet/Kconfig.mcux:53:
+       bool "Set fixed PHY transmission speed to 100 Mbps"$

-:291: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#291: FILE: drivers/ethernet/Kconfig.mcux:56:
+       bool "Set fixed PHY transmission speed to 10 Mbps"$

-:302: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#302: FILE: drivers/ethernet/Kconfig.mcux:67:
+       bool "Set fixed PHY transmission to full duplex"$

-:305: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#305: FILE: drivers/ethernet/Kconfig.mcux:70:
+       bool "Set fixed PHY transmission to half duplex"$

-:424: ERROR:SPACING: space required before the open parenthesis '('
#424: FILE: drivers/ethernet/dsa_ksz8794.c:84:
+	if(port < KSZ8794_PORT1 || port > KSZ8794_PORT3)

-:456: WARNING:LONG_LINE: line over 80 characters
#456: FILE: drivers/ethernet/dsa_ksz8794.c:116:
+static int dsa_ksz8794_spi_setup(struct dsa_ksz8794_spi *sdev, u32_t freq, u8_t cs_num,

-:460: ERROR:CODE_INDENT: code indent should use tabs where possible
#460: FILE: drivers/ethernet/dsa_ksz8794.c:120:
+        u8_t val[2], tmp;$

-:460: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#460: FILE: drivers/ethernet/dsa_ksz8794.c:120:
+        u8_t val[2], tmp;$

-:498: WARNING:LONG_LINE: line over 80 characters
#498: FILE: drivers/ethernet/dsa_ksz8794.c:158:
+	LOG_DBG("KSZ8794: ID0: 0x%x ID1: 0x%x timeout: %d", val[1], val[0], timeout);

-:513: ERROR:OPEN_BRACE: that open brace { should be on the previous line
#513: FILE: drivers/ethernet/dsa_ksz8794.c:173:
+	for (i = KSZ8794_PORT1; i <= KSZ8794_PORT3; i++)
+	{

-:808: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#808: FILE: drivers/ethernet/dsa_ksz8794.h:248:
+      KSZ8794_PORT1=1,$

-:808: ERROR:SPACING: spaces required around that '=' (ctx:VxV)
#808: FILE: drivers/ethernet/dsa_ksz8794.h:248:
+      KSZ8794_PORT1=1,
                    ^

-:809: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#809: FILE: drivers/ethernet/dsa_ksz8794.h:249:
+      KSZ8794_PORT2,$

-:810: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#810: FILE: drivers/ethernet/dsa_ksz8794.h:250:
+      KSZ8794_PORT3,$

-:922: WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (16, 23)
#922: FILE: drivers/ethernet/eth_mcux.c:436:
+		if (context->iface)
+		       context->phy_state = eth_mcux_phy_state_reset;

-:925: ERROR:CODE_INDENT: code indent should use tabs where possible
#925: FILE: drivers/ethernet/eth_mcux.c:439:
+^I        k_delayed_work_submit(&context->delayed_phy_work, 1);$

-:997: ERROR:CODE_INDENT: code indent should use tabs where possible
#997: FILE: drivers/ethernet/eth_mcux.c:517:
+^I^I        k_work_submit(&context->phy_work);$

-:1007: ERROR:CODE_INDENT: code indent should use tabs where possible
#1007: FILE: drivers/ethernet/eth_mcux.c:536:
+^I        eth_mcux_fixed_phy_duplex_and_speed(&phy_duplex, &phy_speed);$

- total: 9 errors, 20 warnings, 998 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

NOTE: Whitespace errors detected.
      You may wish to use scripts/cleanpatch or scripts/cleanfile

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Gitlint issues

Commit 5d77d03a67:
1: UC6 Body has no content, should at least have 1 line.

Commit 68369d7eac:
3: UC4 Line exceeds max length (73>72): "This patch adds support for DSA devices - like ksz8794 switch to the mcux"

Commit 5f1aa14e1b:
3: UC4 Line exceeds max length (73>72): "This change introduces a set of new Kconfig options necessary for setting"

Commit a907ac8b1b:
1: UC6 Body has no content, should at least have 1 line.

Commit 1c815b05ff:
1: UC5 Title exceeds max length (81>75): "eth: Kconfig: Add ETH_MCUX_NO_PHY_SMI option to disable PHY communication via SMI"

Commit 7dbcbd9a4d:
1: UC5 Title exceeds max length (85>75): "eth: mcux: Add support to NXP's Kinetics ENET for devices not using SMI for PHY setup"
6: UC4 Line exceeds max length (74>72): "This change introduces a new Kconfig define - CONFIG_ETH_MCUX_NO_PHY_SMI -"

Commit dc176695e2:
1: UC6 Body has no content, should at least have 1 line.

Commit c106000230:
1: UC5 Title exceeds max length (76>75): "Kconfig: Cleanup the ip_k66f_defconfig and move defines to Kconfig.defconfig"
1: UC6 Body has no content, should at least have 1 line.

Commit 93bb26839d:
6: UC4 Line exceeds max length (74>72): "Now, this board uses 120MHz as already done for k64f SoC (setting PLL freq"

Commit 7ba63dd5b1:
3: UC4 Line exceeds max length (78>72): "This change enables the RMII external clock source at the clock initialization"

Commit 2a88019c2b:
1: UC5 Title exceeds max length (76>75): "eth: Kconfig: Add ETH_MCUX_RMII_EXT_CLK option to enable external RMII clock"
3: UC4 Line exceeds max length (74>72): "This option will configure MCUX block (by setting RMIISRC [19] bit to 1 in"
4: UC4 Line exceeds max length (80>72): "SIM_SOPT2 register) to use external clock source for RMII from ENET_1588_CLKIN)."

Commit 9710571d19:
11: UC4 Line exceeds max length (94>72): "./JLink_V664/JLinkRTTLogger -Device MK66FN2M0XXX18 -RTTChannel 1 -if SWD -Speed 4000 ~/rtt.log"

Commit eca8bd84d0:
1: UC6 Body has no content, should at least have 1 line.

Commit 91d8104c3f:
1: UC6 Body has no content, should at least have 1 line.

Commit e18207046a:
1: UC6 Body has no content, should at least have 1 line.

Commit 9cdce893ae:
1: UC6 Body has no content, should at least have 1 line.

Commit 97587f27e5:
1: UC6 Body has no content, should at least have 1 line.

Commit abe936987d:
1: UC6 Body has no content, should at least have 1 line.

Commit ad78ff9c2a:
1: UC6 Body has no content, should at least have 1 line.

Commit 0883e3f6a1:
1: UC6 Body has no content, should at least have 1 line.

Commit d3f7af956a:
1: UC6 Body has no content, should at least have 1 line.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

[jukkar]

No objections to adding DSA support to Zephyr. There is actually an issue #22061 for this so you could mention that issue in one of the commits and add "Fixes ..." to it.
In order to help reviewing this thing, I would split the PR into multiple pieces:

• the DSA infrastructure support
• changes to boards
• changes to mcux

Currently many of the commits are somewhat unrelated to each other so in order to ease the review, it would be great if you could split the PR into more managable pieces.

[AntoineZen]

Hi @lmajewski,

I'm about going the same route but with KSZ9893. In our setup, the MCU (i.MX-RT 1062) has two ethernet (MAC) ports, where one port is having a single PHY (KSZ8081) and the second port is connected to the KSZ9893 trought RMII. The management of the switch is done over I2C and SMI (MDIO) is not used. With you changes to eth_mcux.c, the configuration the where to use SMI or not is static (CONFIG_ETH_MCUX_NO_PHY_SMI), which enforce to use the same kind of management interface on both Ethernet port. This would be a problem with heterogeneous management interfaces. Do you think it worth to make this configuration option a run-time/boot from the device-tree configuration ?

[lmajewski]

Hi @lmajewski,

I'm about going the same route but with KSZ9893. In our setup, the MCU (i.MX-RT 1062) has two ethernet (MAC) ports, where one port is having a single PHY (KSZ8081) and the second port is connected to the KSZ9893 trought RMII. The management of the switch is done over I2C and SMI (MDIO) is not used. With you changes to eth_mcux.c, the configuration the where to use SMI or not is static (CONFIG_ETH_MCUX_NO_PHY_SMI), which enforce to use the same kind of management interface on both Ethernet port. This would be a problem with heterogeneous management interfaces. Do you think it worth to make this configuration option a run-time/boot from the device-tree configuration ?

In my use case (the single eth port on K66F) the static (in Kconfig) option was the most appropriate and straightforward.
However, as you described, you do have a different use case where both SMI and I2C are required.
That would be a good rationale to convert this option to be configurable via device tree, so each ETH port could be configured.

Note 1:
Just informative - I'm going to sent soon patches providing DSA infrastructure for Zephyr - I'm using KSZ8794 IC.

Note 2:
For your use case you can also use "fixed-phy" functionality recently merged:
#24460

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.