Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

driver: eth: Support for lan8651 T1S ETH (with OA TC6 library) #63614

Merged

Conversation

lmajewski
Copy link
Collaborator

@lmajewski lmajewski commented Oct 6, 2023

This patch set provides support for T1S ethernet device - LAN8651.

For communication the approach described in Open Alliance TC6 specification is used. This code (from oa_tc6.[ch]) can be reused with other ICs following this standard.

Test HW:

  • nucleo_g491re development board from STM
  • Microe LAN8651 module
  • LAN8670-USB dongle with debian12 as host

Build:

west build -p always -b nucleo_g431rb samples/net/sockets/echo_server/ \
	-- -DCONF_FILE=boards/nucleo_g431rb.conf

Test on host PC (one needs to first build echo-client program from net-tools):

while :; do ./echo-client -r -i enx001ec0d1cc4d  192.168.3.3; sleep 1; \
	ping -c3 192.168.3.3; done

Code required for testing and debugging:
https://github.com/lmajewski/zephyr/commits/t1s-lan8651-nucleo_g431rb-test

@lmajewski
Copy link
Collaborator Author

List of interested developers: @GeorgeCGV @uwearzt @lunn

@lmajewski
Copy link
Collaborator Author

lmajewski commented Oct 6, 2023

Setup LAN8670-USB dongle (to be connected to T1S host) - debian12 (kernel 6.1):
###################################################################

cd lan867x-linux-driver-1v0/linux-v6.1.21-support
make
sudo --preserve-env=KBUILD_SIGN_PIN /usr/src/linux-kbuild-6.1/scripts/sign-file sha256 /var/lib/shim-signed/mok/MOK.priv /var/lib/shim-signed/mok/MOK.der microchip_t1s.ko
sudo insmod microchip_t1s.ko enable=1 node_id=0 node_count=8 max_bc=0 burst_timer=128 to_timer=32

Note:
The smsc95xx module shall be loaded first (before the usb dongle is inserted) - otherwise it will
use default PHY driver (not the one for microchip).

To fix it use:
$ echo <usb_device_name> | sudo tee /sys/bus/usb/drivers/smsc95xx/unbind > /dev/null
$ sudo insmod microchip_t1s.ko
$ echo <usb_device_name> | sudo tee /sys/bus/usb/drivers/smsc95xx/bind > /dev/null

Design decisions:
#################

  • The OA_TC6 routines are using the struct tc6 and struct net_pkt as input.
    It was on purpose to tie it with net_pkt as this is what we do receive to send
    and what is read to upper layers.

  • The driver supports half-duplex in terms of the data transmission to/from LAN8651
    via SPI (i.e. when TX is performed, no valid data is read).

    It can be added, but would increase the design of driver and memory footprint.

    I need to assess if this is worth the effort - for now the driver is as simple
    as possible.

  • The driver design differs from Linux one [1], as I had to take into account the
    constrained RAM evironment in Cortex-M4 based CPUs (STM32G491 from nucleo_g491re
    has only 32KiB of RAM to fit network stack and LAN865x driver).

  • The PHY part of LAN8651 is very limited - for example the link status is always 1,
    hence there was no good reason to add separate PHY driver (as it was done for
    adin2111).

Test scenarios:
###############

  • ARP and ICMP from HOST (ping)
    ping 192.168.3.3

  • echo_client (on my Linux HOST PC) -> UDP only
    while :; do ./echo-client -r -i enx001ec0d1cc4d 192.168.3.3; sleep 1; ping -c3 192.168.3.3; done

  • echo_server run on nucleo_g491:

    west build -p always -b nucleo_g431rb samples/net/sockets/echo_server/ -- -DSHIELD="mikroe_twowire_eth_click_arduino" -DEXTRA_CONF_FILE=boards/nucleo_g431rb_microe_twowire_eth_click.conf

  • zperf (CONFIG_NET_ZPERF) cannot be used as it turns out that RAM size is exceeded
    (nucleo_g431re has STM32G491 with only 32 KiB or RAM)

What is missing:
################

  • Setup of PLCA parameters from user programs (only DTS is used now)
    (if there would be need a to add the special network management callback

  • Callback to adjust MAC address

  • SW based reset - at least on LAN8651 rev B0 simple OA_RESET bit 0
    set has some issues. GPIO based reset works correctly.

  • For performance improvement: Modify the code to use k_fifo for RX/TX and perform full-duplex SPI transmission in a separate thread based on in the FIFO data available. This is ongoing work and can be added after the "base" OA TC6 code is added.

Dependencies:
#############
#63239
lmajewski@912115f

Open Questions:
#############

  • where to place the oa_tc.[ch] agnostic files?

Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks quite resonable. Please fix the compliance issues, there are several whitespace issues in the code.
Please change the driver commit subject to
driver: eth: Support for lan8651 T1S ETH

drivers/ethernet/Kconfig.lan865x Outdated Show resolved Hide resolved
drivers/ethernet/eth_lan865x.c Show resolved Hide resolved
drivers/ethernet/eth_lan865x.c Outdated Show resolved Hide resolved
drivers/ethernet/oa_tc6.c Show resolved Hide resolved
@@ -0,0 +1,52 @@
# Generic networking options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sample board config should be in a separate commit.

Please only place here board specific stuff so that the main prj.conf in that sample will be the master information and here we just tweak things needed for this board. Do not place config options specific to your local setup here (like IP addresses etc).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prj.conf file needs changes as shown in nucleo_g431rb.conf, otherwise the sample will not build (excess of on-board RAM).

IMHO, it would be better to have a complete setup there for this particular development board, so other users can reproduce this setup easily.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but this is not a way here. The prj.conf will be the master of the config and the config in board/ directory can then add or adjust the configuration parameters. This way if any new config options are added in prj.conf, the compilation for this board will automatically use them and we do not need to adjust two files.

samples/net/sockets/echo_server/boards/nucleo_g431rb.conf Outdated Show resolved Hide resolved
samples/net/sockets/echo_server/boards/nucleo_g431rb.conf Outdated Show resolved Hide resolved
@jukkar
Copy link
Member

jukkar commented Oct 6, 2023

  • where to place the oa_tc.[ch] agnostic files?

I suppose we could start with drivers/ethernet directory, or a sub-directory there

@lmajewski lmajewski changed the title RFC: net: eth: Support for lan8651 T1S ETH (with OA TC6 library) driver: eth: Support for lan8651 T1S ETH (with OA TC6 library) Oct 6, 2023
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the compliance issues

samples/net/sockets/echo_server/boards/nucleo_g431rb.conf Outdated Show resolved Hide resolved
@@ -0,0 +1,52 @@
# Generic networking options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but this is not a way here. The prj.conf will be the master of the config and the config in board/ directory can then add or adjust the configuration parameters. This way if any new config options are added in prj.conf, the compilation for this board will automatically use them and we do not need to adjust two files.

@erwango erwango assigned rlubos and unassigned erwango Nov 2, 2023
@erwango erwango removed the area: Shields Shields (add-on boards) label Nov 2, 2023
@lmajewski
Copy link
Collaborator Author

Sorry, but I shouldn't be the assignee on such PR no shield, noSTM32). Assigning @rlubos as PR has already been reviewed by @jukkar.

The aforementioned "shield" has been removed with this PR, as it has been pointed out that Zephyr will not accept non compatible combination of base board (nucleo) and MicroBus shield.

Moreover, I would appreciate if we could finish with review of this PR.

@jukkar
Copy link
Member

jukkar commented Nov 3, 2023

@erwango if you are not reviewing this any more, please remove your -1

@lmajewski
Copy link
Collaborator Author

@jukkar - I had to rebase those patches on top of newest origin/main branch. The only change was with Kconfig.

@lmajewski
Copy link
Collaborator Author

@jukkar - Please find list of enhancements for this driver (as I had to rebase it on top of newest 'main' branch):

  • Add support for setting MAC address
  • Reduce IRQ stack size
  • Check if received frame shall be dropped (based on footer information)
  • Optimize 'sbo' and 'ebo' usage when receiving packets
  • Handle special case when frame ends and starts in the same chunk

Those files provide generic functions to handle transmission between chip
conforming OA TC6 standard and Zephyr's network stack represented by
struct net_pkt.

The communication is performed via SPI and is focused on reduced memory
footprint (works with SOC equipped with 32 KiB of RAM) and robustness of
operation.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
jukkar
jukkar previously approved these changes Nov 7, 2023
Copy link
Collaborator

@jfischer-no jfischer-no left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

drivers/ethernet/eth_lan865x.c Outdated Show resolved Hide resolved
This patch set provides support for T1S ethernet device - LAN8651.

For SPI communication the implementation of Open Alliance TC6
specification is used.

The driver implementation focuses mostly on reducing memory footprint,
as the used SoC (STM32G491) for development has only 32 KiB RAM in total.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
@carlescufi carlescufi merged commit 12665a0 into zephyrproject-rtos:main Nov 9, 2023
19 checks passed
@lmajewski lmajewski deleted the t1s-lan8651-nucleo_g431rb branch November 9, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Binding PR modifies or adds a Device Tree binding area: Ethernet area: Networking area: Sockets Networking sockets platform: Microchip MEC Microchip MEC Platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants