drivers: ethernet: add support for microchip lan9250 #77035

mariopaja · 2024-08-13T18:05:47Z

This PR adds support for LAN9250 spi ethernet controller.
This driver is tested on the Mikroe ETH Click 3

Console:

[00:00:00.079,000] <inf> eth_lan9250: LAN9250 Initialized
*** Booting Zephyr OS build v3.7.0-565-g2c4498a094de ***
[00:00:00.079,000] <inf> net_dhcpv4_client_sample: Run dhcpv4 client
[00:00:00.079,000] <inf> net_dhcpv4_client_sample: Start on eth_lan9250@0: index=1
[00:00:05.679,000] <inf> net_dhcpv4: Received: 192.168.8.242
[00:00:05.679,000] <inf> net_dhcpv4_client_sample:    Address[1]: 192.168.8.242
[00:00:05.679,000] <inf> net_dhcpv4_client_sample:     Subnet[1]: 255.255.255.0
[00:00:05.679,000] <inf> net_dhcpv4_client_sample:     Router[1]: 192.168.8.1
[00:00:05.679,000] <inf> net_dhcpv4_client_sample: Lease time[1]: 43200 seconds
uart:~$ net iface

Interface eth0 (0x20000e50) (Ethernet) [1]
===================================
Link addr : 80:34:28:01:02:05
MTU       : 1500
Flags     : AUTO_START,IPv4
Device    : eth_lan9250@0 (0x8017248)
Ethernet capabilities supported:
        10 Mbits
        100 Mbits
Ethernet PHY device: <none> (0)
IPv4 unicast addresses (max 1):
        192.168.8.242/255.255.255.0 DHCP preferred
IPv4 multicast addresses (max 2):
        224.0.0.1
IPv4 gateway : 192.168.8.1
DHCPv4 lease time : 43200
DHCPv4 renew time : 21600
DHCPv4 server     : 192.168.8.1
DHCPv4 requested  : 192.168.8.242
DHCPv4 state      : bound
DHCPv4 attempts   : 2
uart:~$ net ping 8.8.8.8
PING 8.8.8.8
28 bytes from 8.8.8.8 to 192.168.8.242: icmp_seq=1 ttl=53 time=30 ms
28 bytes from 8.8.8.8 to 192.168.8.242: icmp_seq=2 ttl=53 time=30 ms
28 bytes from 8.8.8.8 to 192.168.8.242: icmp_seq=3 ttl=53 time=30 ms

Nucleo-H563 Overlay:

&spi1 {
    eth_lan9250: eth_lan9250@0 {
        compatible = "microchip,lan9250";
        spi-max-frequency = <30000000>;
        reg = <0>;
        int-gpios = <&arduino_header 15 GPIO_ACTIVE_LOW>;
        status = "okay";
        local-mac-address = [ 80 34 28 01 02 05  ];
    };
};

&mac {
    status = "disabled";
};

&mdio {
    status = "disabled";
};

drivers/ethernet/Kconfig.lan9250

drivers/ethernet/eth_lan9250.c

jukkar · 2024-08-14T06:50:52Z

drivers/ethernet/eth_lan9250.c

+	uint32_t tmp;
+	int wait_time = 0;
+
+	while (true) {


There exists WAIT_FOR macro that could perhaps used here and the other wait loops in other functions, could you check if it would make sense to use that here.

I had a look into it and I tried to implement it. I don't know if I did something wrong but I noticed a performance hit. Receiving an IP took double the time after implementing WAIT_FOR

WAIT_FOR:

[00:00:00.019,000] <inf> eth_lan9250: LAN9250 Initialized *** Booting Zephyr OS build v3.7.0-945-ga7dd4cb9ce3d *** [00:00:00.019,000] <inf> net_dhcpv4_client_sample: Run dhcpv4 client [00:00:00.019,000] <inf> net_dhcpv4_client_sample: Start on eth_lan9250@0: index=1 [00:00:10.669,000] <inf> net_dhcpv4: Received: 192.168.8.242 [00:00:10.669,000] <inf> net_dhcpv4_client_sample: Address[1]: 192.168.8.242 [00:00:10.669,000] <inf> net_dhcpv4_client_sample: Subnet[1]: 255.255.255.0 [00:00:10.669,000] <inf> net_dhcpv4_client_sample: Router[1]: 192.168.8.1 [00:00:10.669,000] <inf> net_dhcpv4_client_sample: Lease time[1]: 43200 seconds uart:~$

while loop:

[00:00:00.074,000] <inf> eth_lan9250: LAN9250 Initialized *** Booting Zephyr OS build v3.7.0-945-ga7dd4cb9ce3d *** [00:00:00.074,000] <inf> net_dhcpv4_client_sample: Run dhcpv4 client [00:00:00.074,000] <inf> net_dhcpv4_client_sample: Start on eth_lan9250@0: index=1 [00:00:05.659,000] <inf> net_dhcpv4: Received: 192.168.8.242 [00:00:05.659,000] <inf> net_dhcpv4_client_sample: Address[1]: 192.168.8.242 [00:00:05.659,000] <inf> net_dhcpv4_client_sample: Subnet[1]: 255.255.255.0 [00:00:05.659,000] <inf> net_dhcpv4_client_sample: Router[1]: 192.168.8.1 [00:00:05.659,000] <inf> net_dhcpv4_client_sample: Lease time[1]: 43200 seconds

drivers/ethernet/eth_lan9250.c

jukkar · 2024-08-14T06:53:52Z

drivers/ethernet/eth_lan9250.c

+
+	/* Reference: Microchip Ethernet LAN9250
+	 * https://github.com/microchip-pic-avr-solutions/ethernet-lan9250/
+	 * ((phy_add & 0x1F) << 11) | ((index & 0x1F) << 6)


This comment line is a bit lonely, perhaps you could add more information what it means (I see it is being used in next line so the comment refers to that)

I tried to give a bit more information this time

jukkar · 2024-08-14T06:56:27Z

drivers/ethernet/eth_lan9250.c

+
+	lan9250_write_sys_reg(dev, LAN9250_RESET_CTL,
+			      LAN9250_RESET_CTL_HMAC_RST | LAN9250_RESET_CTL_PHY_RST |
+				      LAN9250_RESET_CTL_DIGITAL_RST);


indentation is wrong

jukkar · 2024-08-14T06:56:36Z

drivers/ethernet/eth_lan9250.c

+
+	lan9250_write_sys_reg(dev, LAN9250_IRQ_CFG,
+			      LAN9250_IRQ_CFG_INT_DEAS_100US | LAN9250_IRQ_CFG_IRQ_EN |
+				      LAN9250_IRQ_CFG_IRQ_TYPE_PP);


jukkar · 2024-08-14T06:57:00Z

drivers/ethernet/eth_lan9250.c

+	 */
+	lan9250_write_sys_reg(dev, LAN9250_INT_EN,
+			      LAN9250_INT_EN_PHY_INT_EN | LAN9250_INT_EN_TDFA_EN |
+				      LAN9250_INT_EN_RSFL_EN);


and here and other similar lines too, could you check them all

drivers/ethernet/eth_lan9250.c

lmajewski · 2024-08-14T08:03:06Z

drivers/ethernet/eth_lan9250.c

+
+	ret |= lan9250_write_mac_reg(dev, LAN9250_HMAC_ADDRH,
+				     ctx->mac_address[4] | (ctx->mac_address[5] << 8));
+


Please explicitly check the return code (i.e. do not OR the ret)

drivers/ethernet/eth_lan9250.c

lmajewski · 2024-08-14T08:05:29Z

drivers/ethernet/eth_lan9250.c

+		ret |= lan9250_read_sys_reg(dev, LAN9250_HW_CFG, &tmp);
+		k_busy_wait(USEC_PER_MSEC * 1U);
+	} while ((tmp & LAN9250_HW_CFG_DEVICE_READY) == 0);
+


My overall impression is that you do use a lot of busy waiting. Is there any special rationale for it?

It is the datasheet that specifies that there are timing rules back to back read-write operations 5.2.1 BACK-TO-BACK WRITE-READ CYCLES, and then I took as a baseline the delays in the ethernet-lan9250 driver.

drivers/ethernet/eth_lan9250.c

lmajewski · 2024-08-14T08:08:39Z

drivers/ethernet/eth_lan9250.c

+			/* Review the space available for the new frag */
+			lan9250_read_buf(dev, data_ptr, spi_frame_len);
+			net_buf_add(pkt_buf, pkt_len);
+


You may want to check if CONFIG_NET_BUF_DATA_SIZE is in sync with your setup as you use net_buf_add()

I don't think it is needed because it is already checked how much space is available at the end of the buffer with net_buf_tailroom(). Similar to the other SPI Ethernet modules (w5500, enc28j60, eth_enc424j600)

lmajewski · 2024-08-14T08:09:10Z

drivers/ethernet/eth_lan9250.c

+	uint32_t regval;
+	uint16_t free_size;
+	uint8_t status_size;
+


The LOG_DBG() shall be after declarations

lmajewski · 2024-08-14T08:13:49Z

drivers/ethernet/eth_lan9250.c

+	}
+
+	k_sem_give(&ctx->tx_rx_sem);
+


It looks like you followed the paradigm from LAN8651 (T1S) driver. The rationale for half-duplex transmission there was the issue with driver complexity versus potential gain.

I'm just wondering if the LAN9250 has the opportunity to work full duplex (as other drivers connected via SPI)?

And another question - out of curiosity - have you tested the driver with zperf Zephyr's test program?

And just to point out - the SPI transmission with chained different buffers was the reason for considerable slow down for STM32 based SoC.

Last but not least - the LAN9250 seems to be 10/100 Mbps - I do assume that it works with 100 Mbps?

And another question - out of curiosity - have you tested the driver with zperf Zephyr's test program?

No I did not test it, but I can do that too, to test it out.
However, I had a short look on the switch and the device is working at 100Mbps (FE):

Last but not least - the LAN9250 seems to be 10/100 Mbps - I do assume that it works with 100 Mbps?

I have enabled auto-negotiation & advertisement capability for both 10/100 Mbps HD/FD

zephyr/drivers/ethernet/eth_lan9250.c

Lines 357 to 379 in 3b70da4

/* Configure PHY basic control:

*

* - Auto-Negotiation for 10/100 Mbits and Half/Full Duplex

*/

lan9250_write_phy_reg(dev, LAN9250_PHY_BASIC_CONTROL,

LAN9250_PHY_BASIC_CONTROL_PHY_AN |

LAN9250_PHY_BASIC_CONTROL_PHY_SPEED_SEL_LSB |

LAN9250_PHY_BASIC_CONTROL_PHY_DUPLEX);

/* Configure PHY auto-negotiation advertisement capability:

*

* - Asymmetric pause

* - Symmetric pause

* - 100Base-X half/full duplex

* - 10Base-X half/full duplex

* - Select IEEE802.3

*/

lan9250_write_phy_reg(dev, LAN9250_PHY_AN_ADV,

LAN9250_PHY_AN_ADV_ASYM_PAUSE | LAN9250_PHY_AN_ADV_SYM_PAUSE |

LAN9250_PHY_AN_ADV_100BTX_HD | LAN9250_PHY_AN_ADV_100BTX_FD |

LAN9250_PHY_AN_ADV_10BT_HD | LAN9250_PHY_AN_ADV_10BT_FD |

LAN9250_PHY_AN_ADV_SELECTOR_DEFAULT);

lmajewski · 2024-08-14T08:36:36Z

drivers/ethernet/Kconfig.lan9250

Maybe a bit off-topic. It looks like second MicroClick modules are used as based board. Is there any plan to add them to zephyr as "capes"? IIRC for LAN8651 I also wanted to add it - in a way similar to:
https://github.com/zephyrproject-rtos/zephyr/tree/main/boards/shields/mikroe_wifi_bt_click
or
https://github.com/zephyrproject-rtos/zephyr/tree/main/boards/shields/mikroe_eth_click

but the reply was that we cannot tie to any specific board (like with microe_wifi_bt_click.

@lmajewski I was also thinking adding it as a shield in a second PR. At the moment I want to pass only a minimal implementation of the driver.

but the reply was that we cannot tie to any specific board (like with microe_wifi_bt_click.

I know that we should not tie drivers to specific shields but also Microship itself is using this module to demonstrate LAN9250 on their Quick Start Guide

mariopaja · 2024-08-17T16:49:10Z

Converted to draft because I noticed some unexpected behaviour, while connected to different switches/routers

This PR adds support for LAN9250 spi ethernet controller. This driver is tested on the Mikroe ETH Click 3 https://www.mikroe.com/eth-3-click Signed-off-by: Mario Paja <[email protected]>

mariopaja · 2024-09-01T14:42:31Z

@jukkar @lmajewski

Issue mentioned earlier is fixed

[00:00:00.292,000] <inf> eth_lan9250: LAN9250 Initialized
*** Booting Zephyr OS build v3.7.0-945-gfd55383524db ***
[00:00:00.293,000] <inf> net_dhcpv4_client_sample: Run dhcpv4 client
[00:00:00.293,000] <inf> net_dhcpv4_client_sample: Start on eth_lan9250@0: index=1
[00:00:02.895,000] <inf> net_dhcpv4: Received: 192.168.8.240
[00:00:02.896,000] <inf> net_dhcpv4_client_sample:    Address[1]: 192.168.8.240
[00:00:02.896,000] <inf> net_dhcpv4_client_sample:     Subnet[1]: 255.255.255.0
[00:00:02.896,000] <inf> net_dhcpv4_client_sample:     Router[1]: 192.168.8.1
[00:00:02.896,000] <inf> net_dhcpv4_client_sample: Lease time[1]: 43200 seconds
uart:~$ net iface
Hostname: zephyr

Interface eth0 (0x3fc94d60) (Ethernet) [1]
===================================
Link addr : 80:34:28:01:02:03
MTU       : 1500
Flags     : AUTO_START,IPv4
Device    : eth_lan9250@0 (0x3c026bc8)
Ethernet capabilities supported:
        10 Mbits
        100 Mbits
Ethernet PHY device: <none> (0)
IPv4 unicast addresses (max 1):
        192.168.8.240/255.255.255.0 DHCP preferred
IPv4 multicast addresses (max 2):
        224.0.0.1
IPv4 gateway : 192.168.8.1
DHCPv4 lease time : 43200
DHCPv4 renew time : 21600
DHCPv4 server     : 192.168.8.1
DHCPv4 requested  : 192.168.8.240
DHCPv4 state      : bound
DHCPv4 attempts   : 1
uart:~$ net ping 8.8.8.8
PING 8.8.8.8
28 bytes from 8.8.8.8 to 192.168.8.240: icmp_seq=1 ttl=111 time=68 ms
28 bytes from 8.8.8.8 to 192.168.8.240: icmp_seq=2 ttl=111 time=64 ms
28 bytes from 8.8.8.8 to 192.168.8.240: icmp_seq=3 ttl=111 time=64 ms
uart:~$

xiao esp32s3 overlay:

&spi2 {
    cs-gpios=<&gpio0 3 GPIO_ACTIVE_LOW>;
    eth_lan9250: eth_lan9250@0 {
        compatible = "microchip,lan9250";
        spi-max-frequency = <60000000>;
        reg = <0>; 
        int-gpios = <&gpio0 4 GPIO_ACTIVE_LOW>; 
        status = "okay";
        local-mac-address = [80 34 28 01 02 03];
    };
};

This was the best performance I could get:

jukkar

Some very minor style nits from me, other than that looks reasonable.

jukkar · 2024-10-04T08:25:35Z

drivers/ethernet/eth_lan9250.c

+	/* Disable TX data FIFO available interrupt */
+	lan9250_write_sys_reg(dev, LAN9250_FIFO_INT,
+			      LAN9250_FIFO_INT_TX_DATA_AVAILABLE_LEVEL |
+				      LAN9250_FIFO_INT_TX_STATUS_LEVEL);


Indentation is wrong here and in many similar statements below

zephyrbot added platform: Microchip MEC Microchip MEC Platform area: Ethernet labels Aug 13, 2024

zephyrbot requested review from albertofloyd, decsny, jvasanth1, lmajewski, pdgendt and VenkatKotakonda August 13, 2024 18:06

zephyrbot assigned jvasanth1 Aug 13, 2024

mariopaja force-pushed the lan9250 branch 2 times, most recently from 31ad344 to 3b70da4 Compare August 14, 2024 06:02

jukkar reviewed Aug 14, 2024

View reviewed changes

lmajewski reviewed Aug 14, 2024

View reviewed changes

mariopaja force-pushed the lan9250 branch 3 times, most recently from 01f1782 to 1d35288 Compare August 17, 2024 15:45

mariopaja marked this pull request as draft August 17, 2024 16:48

mariopaja force-pushed the lan9250 branch 2 times, most recently from b309efd to fd55383 Compare September 1, 2024 14:00

drivers: ethernet: add support for microchip lan9250

117d071

This PR adds support for LAN9250 spi ethernet controller. This driver is tested on the Mikroe ETH Click 3 https://www.mikroe.com/eth-3-click Signed-off-by: Mario Paja <[email protected]>

mariopaja force-pushed the lan9250 branch from fd55383 to 117d071 Compare September 1, 2024 14:06

mariopaja marked this pull request as ready for review September 1, 2024 14:32

albertofloyd removed their request for review September 20, 2024 20:50

mariopaja requested review from lmajewski and jukkar October 2, 2024 06:48

jukkar reviewed Oct 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: ethernet: add support for microchip lan9250 #77035

drivers: ethernet: add support for microchip lan9250 #77035

mariopaja commented Aug 13, 2024

jukkar Aug 14, 2024

mariopaja Aug 17, 2024

jukkar Aug 14, 2024

mariopaja Aug 17, 2024

jukkar Aug 14, 2024

jukkar Aug 14, 2024

jukkar Aug 14, 2024

lmajewski Aug 14, 2024

lmajewski Aug 14, 2024

mariopaja Aug 16, 2024

lmajewski Aug 14, 2024

mariopaja Aug 17, 2024

lmajewski Aug 14, 2024

lmajewski Aug 14, 2024

lmajewski Aug 14, 2024

mariopaja Aug 14, 2024 •

edited

Loading

lmajewski Aug 14, 2024

mariopaja Aug 14, 2024 •

edited

Loading

mariopaja commented Aug 17, 2024 •

edited

Loading

mariopaja commented Sep 1, 2024

jukkar left a comment

jukkar Oct 4, 2024


		ret \|= lan9250_write_mac_reg(dev, LAN9250_HMAC_ADDRH,
		ctx->mac_address[4] \| (ctx->mac_address[5] << 8));

	/* Configure PHY basic control:
	*
	* - Auto-Negotiation for 10/100 Mbits and Half/Full Duplex
	*/

	lan9250_write_phy_reg(dev, LAN9250_PHY_BASIC_CONTROL,
	LAN9250_PHY_BASIC_CONTROL_PHY_AN \|
	LAN9250_PHY_BASIC_CONTROL_PHY_SPEED_SEL_LSB \|
	LAN9250_PHY_BASIC_CONTROL_PHY_DUPLEX);

	/* Configure PHY auto-negotiation advertisement capability:
	*
	* - Asymmetric pause
	* - Symmetric pause
	* - 100Base-X half/full duplex
	* - 10Base-X half/full duplex
	* - Select IEEE802.3
	*/
	lan9250_write_phy_reg(dev, LAN9250_PHY_AN_ADV,
	LAN9250_PHY_AN_ADV_ASYM_PAUSE \| LAN9250_PHY_AN_ADV_SYM_PAUSE \|
	LAN9250_PHY_AN_ADV_100BTX_HD \| LAN9250_PHY_AN_ADV_100BTX_FD \|
	LAN9250_PHY_AN_ADV_10BT_HD \| LAN9250_PHY_AN_ADV_10BT_FD \|
	LAN9250_PHY_AN_ADV_SELECTOR_DEFAULT);

drivers: ethernet: add support for microchip lan9250 #77035

Are you sure you want to change the base?

drivers: ethernet: add support for microchip lan9250 #77035

Conversation

mariopaja commented Aug 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariopaja Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariopaja Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

mariopaja commented Aug 17, 2024 • edited Loading

mariopaja commented Sep 1, 2024

jukkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariopaja Aug 14, 2024 •

edited

Loading

mariopaja Aug 14, 2024 •

edited

Loading

mariopaja commented Aug 17, 2024 •

edited

Loading