drivers: eth_mcux: adding i.mx-rt support #10875

agansari · 2018-10-26T13:09:22Z

Enables Networking hardware on i.MX-RT 1050-EVKB board.
Fixes #8900

enables board specific pinout for ethernet connection
fixed initialization sequence in eth_mcux
pinmux enablement pushed later so sysclock is initalized before

Signed-off-by: Andrei Gansari andrei.gansari@nxp.com

codecov-io · 2018-10-26T13:38:50Z

Codecov Report

Merging #10875 into master will increase coverage by 0.04%.
The diff coverage is 30.95%.

@@            Coverage Diff             @@
##           master   #10875      +/-   ##
==========================================
+ Coverage   48.37%   48.41%   +0.04%     
==========================================
  Files         265      270       +5     
  Lines       42188    42121      -67     
  Branches    10137    10139       +2     
==========================================
- Hits        20408    20394      -14     
+ Misses      17703    17645      -58     
- Partials     4077     4082       +5

Impacted Files	Coverage Δ
include/logging/log_core.h	`100% <ø> (ø)`	⬆️
include/kernel.h	`98.48% <ø> (ø)`	⬆️
include/bluetooth/gatt.h	`100% <ø> (ø)`	⬆️
boards/posix/nrf52_bsim/k_busy_wait.c	`0% <0%> (ø)`	⬆️
subsys/net/lib/mqtt_sock/mqtt_rx.c	`0% <0%> (ø)`
subsys/net/lib/mqtt_sock/mqtt_transport.c	`0% <0%> (ø)`
subsys/bluetooth/host/conn.c	`32.38% <0%> (-0.33%)`	⬇️
subsys/logging/log_msg.c	`86.36% <0%> (-0.5%)`	⬇️
...bsys/net/lib/mqtt_sock/mqtt_transport_socket_tcp.c	`0% <0%> (ø)`
include/net/net_context.h	`78.94% <100%> (+0.37%)`	⬆️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f60b580...599da04. Read the comment docs.

jukkar · 2018-10-26T19:15:01Z

drivers/ethernet/eth_mcux.c

+	int irq_lock_key = irq_lock();
+
+
+	if (EIR & (kENET_RxBufferInterrupt | kENET_RxFrameInterrupt))


This does not follow Zephyr coding style. Please place { in the same line as the if() statement like this

if (xxx) { } else if () { } else { }

jukkar · 2018-10-26T19:15:16Z

drivers/ethernet/eth_mcux.c

+
+	irq_unlock(irq_lock_key);
+}
+#endif //CONFIG_SOC_SERIES_IMX_RT


No // comments

jukkar

LGTM, but please check the issues sanitychecker is reporting at https://app.shippable.com/github/zephyrproject-rtos/zephyr/runs/25592/5/tests

galak · 2018-10-30T09:36:02Z

boards/arm/mimxrt1050_evk/pinmux.c

 #endif

 	return 0;
 }

-SYS_INIT(mimxrt1050_evk_init, PRE_KERNEL_1, 0);
+SYS_INIT(mimxrt1050_evk_init, PRE_KERNEL_2, 0);


Curious, why the change to PRE_KERNEL_2

Pushed pin initialization later so i can use k_busy_wait to reset ethernet PHY chip.

Does a similar change need to be made on frdm_k64f?

K64F doesn't need this change, PHY is similar, a bit different and differently wired.

galak

Can you fixup the white space issues that checkpatch is reporting
Also, can you add more details about what changes you are making to the ethernet driver itself to support i.MX-RT (i'd pull these changes out into their own commit).

pfalcon · 2018-10-30T12:39:26Z

From commit message:

Enables Newtworking hardware on i.MX-RT 1050-EVKB board.

Please fix typo in "Newtworking".

And the commit message is very bare. Even description of the PR here more detailed than the commit message, even though the commit message is what goes into posterity (into git log, git bisect, etc.). At the very least, it could mention that existing eth_mcux driver is reused for this hardware (which is not obvious and pleasantly (I hope) surprising fact for me).

(Note that "more detailed commit message" is my usual suggestion to pretty big share of patches I review.)

pfalcon

Some suggestions from my side too. At least splitting off ext/ change to a separate commit should be done. (Unless overridden by @galak, @MaureenHelm or someone else in the know.)

pfalcon · 2018-10-30T12:41:08Z

boards/arm/mimxrt1050_evk/Kconfig.defconfig

+if NETWORKING
+
+config NET_L2_ETHERNET
+	def_bool y if !MODEM_WNCM14A2A


Is this truly a good idea here?

pfalcon · 2018-10-30T12:43:36Z

boards/arm/mimxrt1050_evk/pinmux.c

-			    IOMUXC_SW_PAD_CTL_PAD_PKE_MASK |
-			    IOMUXC_SW_PAD_CTL_PAD_SPEED(2) |
-			    IOMUXC_SW_PAD_CTL_PAD_DSE(6));
+				IOMUXC_SW_PAD_CTL_PAD_PKE_MASK |


So, in such cases, Zephyr codestyle requires to align args "in column", using tabs for as long as possible, then spaces. In other words, what was before seems to be right. Also, IMHO, codestyle changes should be separated from functional changes. YMMV

pfalcon · 2018-10-30T12:45:00Z

boards/arm/mimxrt1050_evk/pinmux.c

+#endif
+
+#ifdef CONFIG_ETH_MCUX_0
+	IOMUXC_SetPinMux(IOMUXC_GPIO_B1_04_ENET_RX_DATA00, 0U);


Why do we have uppercase "U" here and "u" below?

Also, so far Zephyr doesn't use that "suffixed U" style. (So far, yes).

MaureenHelm

Please make the driver changes in a separate commit from the soc and board changes.

MaureenHelm · 2018-10-30T19:46:44Z

boards/arm/mimxrt1050_evk/pinmux.c

+#include <fsl_gpio.h>
+
+#ifdef CONFIG_ETH_MCUX_0
+	gpio_pin_config_t enet_gpio_config = {


Dedent this block and make the variable static const.

MaureenHelm · 2018-10-30T19:48:07Z

boards/arm/mimxrt1050_evk/pinmux.c

+	GPIO_PinInit(GPIO1, 9, &enet_gpio_config);
+	GPIO_PinInit(GPIO1, 10, &enet_gpio_config);
+
+	/* pull up the ENET_INT before RESET. */


Please add to the comment that you're resetting the PHY

MaureenHelm · 2018-10-30T19:48:53Z

boards/arm/mimxrt1050_evk/pinmux.c

 #endif

 	return 0;
 }

-SYS_INIT(mimxrt1050_evk_init, PRE_KERNEL_1, 0);
+SYS_INIT(mimxrt1050_evk_init, PRE_KERNEL_2, 0);


Does a similar change need to be made on frdm_k64f?

MaureenHelm · 2018-10-30T20:00:10Z

drivers/ethernet/eth_mcux.c

@@ -932,6 +951,30 @@ static void eth_mcux_ptp_isr(void *p)
 }
 #endif

+#ifdef CONFIG_SOC_SERIES_IMX_RT


I'd like to avoid direct dependencies on the SoC like this. If you add interrupt-names = "COMMON" to the dts, you can make this condition #ifdef ETH_IRQ_COMMON instead.

MaureenHelm · 2018-10-30T20:08:13Z

drivers/ethernet/eth_mcux.c

+}
+#endif /* CONFIG_SOC_SERIES_IMX_RT */
+
+#ifdef CONFIG_SOC_SERIES_KINETIS_K6X


Change this to #ifdef ETH_IRQ_RX and wrap the other isr's similarly with #ifdef ETH_IRQ_TX and #ifdef ETH_IRQ_ERR_MISC

MaureenHelm · 2018-10-30T20:17:26Z

drivers/ethernet/eth_mcux.c

+#if CONFIG_SOC_SERIES_IMX_RT
+	IRQ_CONNECT(ENET_IRQn, CONFIG_ETH_MCUX_0_IRQ_PRI,
+		    eth_mcux_dispacher_isr, DEVICE_GET(eth_mcux_0), 0);
+	irq_enable(ENET_IRQn);


Use the irq number from dts instead of CMSIS

MaureenHelm · 2018-11-05T16:14:39Z

drivers/ethernet/eth_mcux.c

@@ -33,7 +33,9 @@ LOG_MODULE_REGISTER(LOG_MODULE_NAME);

 #include "fsl_enet.h"
 #include "fsl_phy.h"
+#ifdef CONFIG_SOC_SERIES_KINETIS_K6X
 #include "fsl_port.h"


Is this include needed at all?

It was needed in case of unique mac, SIM definition was not included.
No longer required.

Let's remove it then

MaureenHelm · 2018-11-05T16:28:18Z

dts/arm/nxp/nxp_rt.dtsi

@@ -30,15 +30,15 @@
 			#size-cells = <1>;

 			itcm0: itcm@0 {
-				reg = <0x00000000 0x20000>;
+				reg = <0x00000000 0x80000>;


This doesn't make sense. We have 512 KB SRAM that gets partitioned into ITCM, DTCM, and OCRAM. They can't all be 512 KB.

Rather than changing these partitions, we need to add a min_flash and/or min_ram requirement to the failing sample/test yaml.

My bad, i just looked at memory map and not FlexRam.
Thank you for yaml suggestion.

agansari · 2018-11-07T11:37:16Z

@pfalcon and @galak can you please help me test this pull request on both i.MX RT and K64F for backward compatibility?

MaureenHelm

Looks pretty good. I still need to test on hardware

MaureenHelm · 2018-11-07T12:45:37Z

drivers/ethernet/eth_mcux.c

@@ -33,7 +33,9 @@ LOG_MODULE_REGISTER(LOG_MODULE_NAME);

 #include "fsl_enet.h"
 #include "fsl_phy.h"
+#ifdef CONFIG_SOC_SERIES_KINETIS_K6X
 #include "fsl_port.h"


Let's remove it then

MaureenHelm · 2018-11-07T12:49:00Z

drivers/ethernet/eth_mcux.c

@@ -33,9 +33,6 @@ LOG_MODULE_REGISTER(LOG_MODULE_NAME);

 #include "fsl_enet.h"
 #include "fsl_phy.h"
-#ifdef CONFIG_SOC_SERIES_KINETIS_K6X
-#include "fsl_port.h"


Move this change to the previous commit

MaureenHelm · 2018-11-07T12:50:17Z

boards/arm/mimxrt1050_evk/mimxrt1050_evk.yaml

@@ -15,3 +15,4 @@ ram: 128
 flash: 128
 supported:
  - spi
+  - netif:eth


Could you also update the board doc to reflect that ethernet is now supported?

dbkinder

+1 for doc changes

galak · 2018-11-14T16:10:21Z

Some suggestions from my side too. At least splitting off ext/ change to a separate commit should be done. (Unless overridden by @galak, @MaureenHelm or someone else in the know.)

I don't think it needs its own commit

galak · 2018-11-14T16:11:35Z

@pfalcon still have issues?

Oops, sorry, forgot I had -1 here.

pfalcon · 2018-11-14T16:16:33Z

Dismissed my -1, sorry, completely forgot I had it there.

(Submitters, feel free to ping reviewers in such cases.)

galak · 2018-11-14T16:52:44Z

soc/arm/nxp_imx/rt/dts_fixup.h

@@ -41,4 +41,13 @@
 #define DT_UART_MCUX_LPUART_3_CLOCK_NAME	DT_NXP_KINETIS_LPUART_4018C000_CLOCK_CONTROLLER
 #define DT_UART_MCUX_LPUART_3_CLOCK_SUBSYS	DT_NXP_KINETIS_LPUART_4018C000_CLOCK_NAME

+#define DT_ETH_MCUX_0_NAME		ETH_LABEL


Would prefer to see this be:

#define DT_ETH_MCUX_0_NAME DT_NXP_KINETIS_ETHERNET_402D8000_LABEL

@galak I've made the change you requested
@galak @pfalcon Thank you for your reviews!

Sadly, I don't have much to be thanked for - it's on my TODO to test this patch on real hw, but unfortunately I can't to it due to other tasks I got into. I just hope that other folks tested it. (I'll get to it to of course.)

It's taking me some time to test i.MX RT and backward checking K64F, this i why I didn't ping earlier. Patch looks okay so far.

Enables Networking hardware on i.MX-RT type drivers. Reuses the same eth_mcux driver used by Kinetis family; initialization sequence refactored to work with this board as well. Unlike Kinetis family, i.MX has a single ENET interrupt and we need to discriminate between interrupts using a status register. Signed-off-by: Andrei Gansari <andrei.gansari@nxp.com>

Enables Networking hardware on i.MX-RT 1050-EVKB board. Pinout enabled board specific etherenet connection, also pin initialization was moved later to PRE_KERNEL_2 in order to have sysclock initialized before. Signed-off-by: Andrei Gansari <andrei.gansari@nxp.com>

In order to prevent failing tests on i.MX RT the following samples were limited to a minimum 140k of flash: http_client, http_server, socket/echo_client and sockets/echo_server. Signed-off-by: Andrei Gansari <andrei.gansari@nxp.com>

Board documentation updated with Networking related infomation. Signed-off-by: Andrei Gansari <andrei.gansari@nxp.com>

pfalcon · 2018-11-19T19:04:57Z

@agansari : FYI, I'm very, very close to help with testing this (at least on frdm_k64f). I actually hoped to do this today, but as usually happens, discovered a trail of regressions on approaching that. For you reference, my standard test plans are described in https://github.com/pfalcon/zephyr/wiki/NetworkingTestPlan (again, they don't work with current master, and require patches - new and old).

pfalcon · 2018-11-20T11:37:44Z

Ok, testing: frdm_k64f with samples/net/sockets/dumb_http_server, master 57a6858 + this patch + #7831, as required for reliable frdm_k64f operation (not related to the changes in this PR in any way, master is broken).

ab -n1000 http://192.0.2.1:8080/ works as expected. Specifically:

Complete requests:      1000
Failed requests:        0
Total transferred:      2181000 bytes
HTML transferred:       2122000 bytes
Requests per second:    118.71 [#/sec] (mean)
Time per request:       8.424 [ms] (mean)
Time per request:       8.424 [ms] (mean, across all concurrent requests)
Transfer rate:          252.85 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    1   0.1      1       2
Processing:     5    8   0.2      8      11
Waiting:        4    7   0.2      7       7
Total:          6    8   0.2      8      11

Percentage of the requests served within a certain time (ms)
  50%      8
  66%      8
  75%      8
  80%      8
  90%      8
  95%      9
  98%      9
  99%      9
 100%     11 (longest request)

pfalcon · 2018-11-20T12:01:17Z

Ok, now frdm_k64f + big_http_download:

First iteration went ok:

***** Booting Zephyr OS zephyr-v1.13.0-2040-g4b22b4c6ce *****
Preparing HTTP GET request for http://archive.ubuntu.com:80/ubuntu/dists/xenial/main/installer-amd64/current/images/hd-media/vmlinuz
addrinfo @0x20008238: ai_family=2, ai_socktype=1, ai_protocol=6, sa_family=2, sin_port=5000
sock = 0
7015896 bytes
Hash: 337c37d7ec00348414224baa6bdb2d43f2a34ef5676bafcdcad916f148b5b317
Total downloaded so far: 6MB

It seems to be even faster with this patch ;-). (I'm kidding, it's as slow as usual. In all fairness, I don't add d/l speed calculation to not scare off people.)

But on 2nd iter:

sock = 0
[00:00:01.642,867] <err> eth_mcux.eth_rx: Failed to get fragment buf
1158170 bytes

Well, that unlikely has anything to do with the driver changes, and should be a transient error.

pfalcon · 2018-11-20T12:18:16Z

Ok, as there're no regressions with frdm_k64f, let me approve this. Otherwise, I of course take a chance to bootstrap me with mimxrt1050_evk testing.

pfalcon

Tested on frdm_k64f

agansari · 2018-11-20T12:22:07Z

@pfalcon i.MX RT will have some issues, some samples are too big for iTCM memory, but thye will run if they fit.
Thank you for taking time on regressive testing. Can we automate these tests?

pfalcon · 2018-11-20T13:46:30Z

Can we automate these tests?

Well, that's definitely the aim various parties want to achieve, e.g. I know that NXP has some internal system (@dleach02 told), on Linaro level we want to integrate more HW testing into mostly barebones testing, Zephyr recently added real-board testing into its "sanitycheck" testsuite. But what can I say more if the current situation is that mainline has networking stack setting which semi-break frmd_k64f (#7831), and for half a year (it's actually longer than that) it can't come to resolution?

All in all, before we'll be able to run tests automatically, we need to get used to run tests consistently manually. And https://github.com/pfalcon/zephyr/wiki/NetworkingTestPlan is my modest contribution towards that.

pfalcon · 2018-11-20T13:49:10Z

@MaureenHelm, on https://www.nxp.com/support/developer-resources/run-time-software/i.mx-developer-resources/i.mx-rt1050-evaluation-kit:MIMXRT1050-EVK?tab=In-Depth_Tab (which is click away from "MIMXRT1050-EVK Website" link in https://docs.zephyrproject.org/latest/boards/arm/mimxrt1050_evk/doc/mimxrt1050_evk.html), clicking on "ARM mbed" pane leads to https://os.mbed.com/platforms/FRDM-KW41Z/ , instead of https://os.mbed.com/platforms/MIMXRT1050-EVK/ . Let me know if I should submit this issue in another way ;-).

pfalcon · 2018-11-20T14:03:15Z

@agansari, Maybe you can confirm something to me. So, when I work with a board which has mbed bootloader, I usually follow the least-resistance way, and just copy zephyr.bin files to mass-storage device the said bootloader offers. So far, that doesn't work with mimxrt1050_evk. Symptoms are as described here: https://community.nxp.com/thread/486834 , and I wonder if that reply nails:

For I.MXRT1050 EVKB, Drag and drop function is not supported.

Did you ever use the I.MXRT1050 in mbed-like way?

agansari · 2018-11-20T14:11:17Z

@pfalcon I only used j-link debugging, haven't tried mbed bootloader, let me test that.

pfalcon · 2018-11-20T22:42:18Z

@agansari: Ok, "mystery" solved. With default-as-shipped mbet bootloader, there's no jlink support. After jlink firmware installed, there's no mbed flash-drive any longer. I was able to get hello_world running all in all. Will continue with eth testing tomorrow. And hope to submit update to the board doc.

pfalcon · 2018-11-21T11:08:07Z

@agansari : So far, bad news - no response from Eth on imxrt, nothing in stats after pings. What I noticed is that both LEDs on Eth jack always light up for me, even without cable. And they never blink. Do you have the same?

And just reinsuring - I didn't see all the jumpers on the board being described in user guide. I don't need change any jumpers, do I?

agansari · 2018-11-21T11:59:14Z

@pfalcon yes, the leds light up from the begining, but they blink on pings for example. There is no need for jumpers or deep switches changes.

pfalcon · 2018-11-21T20:57:48Z

Ok, let's continue in #11586

agansari requested review from jukkar, MaureenHelm, pfalcon and tbursztyka as code owners October 26, 2018 13:09

pfalcon added area: Networking area: Ethernet labels Oct 26, 2018

jukkar requested changes Oct 26, 2018

View reviewed changes

agansari force-pushed the imxrt_netw branch 2 times, most recently from 8b61c82 to 17cf8fd Compare October 29, 2018 13:48

jukkar approved these changes Oct 29, 2018

View reviewed changes

galak reviewed Oct 30, 2018

View reviewed changes

galak requested changes Oct 30, 2018

View reviewed changes

agansari force-pushed the imxrt_netw branch from 17cf8fd to b3a6904 Compare October 30, 2018 11:39

pfalcon previously requested changes Oct 30, 2018

View reviewed changes

agansari force-pushed the imxrt_netw branch from b3a6904 to bdab628 Compare October 30, 2018 13:56

MaureenHelm requested changes Oct 30, 2018

View reviewed changes

agansari force-pushed the imxrt_netw branch 2 times, most recently from f3e7f81 to f41267f Compare November 5, 2018 11:10

MaureenHelm requested changes Nov 5, 2018

View reviewed changes

agansari force-pushed the imxrt_netw branch from 29e34ac to 1d0d083 Compare November 6, 2018 11:30

MaureenHelm reviewed Nov 7, 2018

View reviewed changes

agansari force-pushed the imxrt_netw branch 2 times, most recently from 59c501b to d3dc1c0 Compare November 7, 2018 16:14

agansari requested a review from dbkinder as a code owner November 7, 2018 16:14

agansari force-pushed the imxrt_netw branch 2 times, most recently from 61dd607 to 45fc2dc Compare November 7, 2018 16:19

dbkinder approved these changes Nov 8, 2018

View reviewed changes

agansari force-pushed the imxrt_netw branch from 6206b65 to c358d29 Compare November 14, 2018 13:59

galak reviewed Nov 14, 2018

View reviewed changes

agansari added 4 commits November 15, 2018 17:27

samples: net: some samples min_flash = 140k

1ea851e

In order to prevent failing tests on i.MX RT the following samples were limited to a minimum 140k of flash: http_client, http_server, socket/echo_client and sockets/echo_server. Signed-off-by: Andrei Gansari <andrei.gansari@nxp.com>

boards: mimxrt1050_evk: networking added to documentation

599da04

Board documentation updated with Networking related infomation. Signed-off-by: Andrei Gansari <andrei.gansari@nxp.com>

agansari force-pushed the imxrt_netw branch from c358d29 to 599da04 Compare November 15, 2018 16:31

pfalcon approved these changes Nov 20, 2018

View reviewed changes

tbursztyka approved these changes Nov 20, 2018

View reviewed changes

galak approved these changes Nov 20, 2018

View reviewed changes

galak merged commit bf988dc into zephyrproject-rtos:master Nov 20, 2018

pfalcon mentioned this pull request Nov 21, 2018

mimxrt1050_evb board: Can't get Ethernet to work #11586

Closed

agansari deleted the imxrt_netw branch December 9, 2019 09:34

		int irq_lock_key = irq_lock();


		if (EIR & (kENET_RxBufferInterrupt \| kENET_RxFrameInterrupt))

drivers: eth_mcux: adding i.mx-rt support #10875

drivers: eth_mcux: adding i.mx-rt support #10875

Uh oh!

Conversation

agansari commented Oct 26, 2018 • edited by pfalcon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jukkar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galak left a comment

Choose a reason for hiding this comment

Uh oh!

pfalcon commented Oct 30, 2018

Uh oh!

pfalcon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaureenHelm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaureenHelm Nov 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agansari commented Nov 7, 2018

Uh oh!

MaureenHelm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbkinder left a comment

Choose a reason for hiding this comment

agansari commented Oct 26, 2018 •

edited by pfalcon

Loading

codecov-io commented Oct 26, 2018 •

edited

Loading

MaureenHelm Nov 5, 2018 •

edited

Loading

pfalcon commented Nov 20, 2018 •

edited

Loading