-
Notifications
You must be signed in to change notification settings - Fork 8.3k
NXP i.MX NETC switch DSA driver #82307
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
Conversation
|
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
drivers/ethernet/eth_nxp_imx_netc.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just making sure I understand this- will all instances of this IP with an internal PHY support DSA? Or are these functions unrelated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This eth_nxp_imx_netc.c driver is for normal ENETC MAC of NETC. There may be several instances designed for external usage. For example, i.MX95, there are 3 ENETC MACs externally but without switch IP in NETC.
For SoC like i.MX RT1180, there are 5 ports switch IP in NETC together with 2 ENETC MACs. So, it's designed that one ENETC MAC will connect to one switch MAC inside SoC for DSA management. The MACs connected inside SoC is called pseudo MAC in reference manual. Outside the SoC, we could see 4 switch ports and one normal eth port.
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this driver Kconfig select DSA support, or should the DSA_NXP_IMX_NETC Kconfig select it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I think it makes sense to rework this.
config ETH_DSA_SUPPORT
bool
help
Set by an ethernet driver that supports DSA.
menuconfig NET_DSA
bool "Distributed Switch Architecture support"
depends on ETH_DSA_SUPPORT
help
Enable Distributed Switch Architecture support. For now it
only supports Kinetics and STM32 ENET drivers.
NET_DSA should select ETH_DSA_SUPPORT. Then this is handle in common NET option, and device driver no longer needs to handle. Also it's possible eth device driver supports DSA master mode without any code wrappered with ETH_DSA_SUPPORT.
./drivers/ethernet/Kconfig.sam_gmac: select ETH_DSA_SUPPORT
./drivers/ethernet/nxp_enet/Kconfig: select ETH_DSA_SUPPORT
./drivers/ethernet/nxp_enet/Kconfig: select ETH_DSA_SUPPORT
./drivers/ethernet/Kconfig.stm32_hal: select ETH_DSA_SUPPORT
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it makes sense to set the NET_DSA in config and then let other parts of the system either depend on it (recommended) or select it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this driver Kconfig select DSA support, or should the
DSA_NXP_IMX_NETCKconfig select it?
This driver is supposed to set it, not the DSA driver, it's for indicating the master port driver was implemented to have the DSA wrapper as the send function and registering the master tx function. I think the kconfig is poorly documented and as @yangbolu1991 points out it would be ideal to not need to add DSA support for the cpu side drivers . But I think we should not be removing this config in this PR and definitely without further solution because for now it's true that only some of the drivers have implemented support for the DSA architecture
samples/net/dsa/src/main.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we instead make this a Kconfig?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. That's better. I will do. Or we define max number with 10 for Kconfig? This may cover most cases in the future.
Thanks.
samples/net/dsa/prj.conf
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe these should be set by a board overlay for the RT1180? I doubt many boards are this capable as a switch, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different boards with different ethernet port number. This number here is not just for switch ports. There may be also other normal ethernet ports. For RT1180, there are 4 switch ports + 1 normal eth port.
If max number 10 is accepted here, I think future boards no longer define in board overlay. 10 should be enough for most boards.
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an overkill. The IPv4 and IPv6 structs take up lot of space so we should avoid allocating them just in case. You can add these settings to board overlay if you need more than default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay.
west.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit with HAL changes should be first in this PR- also please update the commit message to be more clear- something like:
west.yml: update hal_nxp to include DSA switch driver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it makes sense to set the NET_DSA in config and then let other parts of the system either depend on it (recommended) or select it.
drivers/ethernet/dsa_nxp_imx_netc.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend putting () around this complex macro in order to avoid possible weird issues when the macro is expanded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thanks.
drivers/ethernet/dsa_nxp_imx_netc.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thanks.
samples/net/dsa/prj.conf
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an overkill. The IPv4 and IPv6 structs take up lot of space so we should avoid allocating them just in case. You can add these settings to board overlay if you need more than default.
56dd439 to
ee5dc23
Compare
Added NETC switch node. Signed-off-by: Yangbo Lu <[email protected]>
Added pinctrl for NETC switch ports. Signed-off-by: Yangbo Lu <[email protected]>
Moved netc node from m33 dts to common dtsi. ENETC PSI0 kept disabled for M7 for now. Signed-off-by: Yangbo Lu <[email protected]>
Enabled switch ports in dts. Signed-off-by: Yangbo Lu <[email protected]>
Added board init for switch ports. Signed-off-by: Yangbo Lu <[email protected]>
Documented the NETC DSA support. Signed-off-by: Yangbo Lu <[email protected]>
The multiple ports DSA device may have some ports disabled in dts. This should be handled in the sample. Signed-off-by: Yangbo Lu <[email protected]>
994de6a to
a3d322a
Compare
|
Updated to v8. Changes include,
Thanks a lot. |
samples/net/dsa/src/dsa_lldp.c
Outdated
| * check_ll_ether_addr table | ||
| */ | ||
| if (check_ll_ether_addr(lladst.addr, ð_filter_l2_addr_base[0][0])) { | ||
| return 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not return "plain" values here and in the final return, but either NET_OK, NET_CONTINUE or NET_DROP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
| struct sockaddr_ll dst; | ||
| struct ethernet_context *ctx = net_if_l2_data(iface); | ||
| struct net_eth_hdr *eth_hdr = (struct net_eth_hdr *) buffer; | ||
| struct net_eth_hdr *eth_hdr = (struct net_eth_hdr *)buffer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you place the cosmetic changes in this file to a separate commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Thanks.
samples/net/dsa/boards/ip_k66f.conf
Outdated
| CONFIG_NET_CONFIG_MY_IPV4_ADDR="192.168.0.2" | ||
| CONFIG_NET_CONFIG_MY_IPV4_GW="192.168.0.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the documentation IPv4 address space here so 192.0.2.2 and 192.0.2.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
Fixed style problem for dsa_lldp.c with clang-format. Signed-off-by: Yangbo Lu <[email protected]>
Current DSA sample is initially for ip_k66f board with a switch PHY, while it is expected as a common DSA sample for all DSA devices. This patch does not change any function. It only reworks the sample with new added CONFIG_NET_SAMPLE_DSA_LLDP for ip_k66f with 3 LAN ports. Ideally this should be common for all DSA devices, but for now, ip_k66f is the only platform supporting it. As there will be more DSA functions supported, better to allow each function enablement via option to suit hardware or device driver support status. Also improved dsa_ll_addr_switch_cb return value with enum value instead of plain value. Signed-off-by: Yangbo Lu <[email protected]>
Moved platform specific options to board. Also changed IP/Gateway to align documentation. Signed-off-by: Yangbo Lu <[email protected]>
Added mimxrt1180_evk board support. Signed-off-by: Yangbo Lu <[email protected]>
a3d322a to
d982f84
Compare
|
Updated to v9 for dsa sample code improvement per @jukkar suggestion . Changes include,
Thank you very much. |

Take i.MX RT1180 NETC hardware as an example.
The PR also reworked dsa sample for flexibility and extensibility.
Current DSA sample is initially for ip_k66f board with a switch PHY,
while it is expected as a common DSA sample for all DSA devices.
This PR only reworks the sample
with new added CONFIG_NET_SAMPLE_DSA_LLDP for ip_k66f with 3 LAN
ports.
Ideally this should be common for all DSA devices, but for now, ip_k66f
is the only platform supporting it.
As there will be more DSA functions supported, better to allow each
function enablement via option to suit hardware or device driver support
status.
Dependency:
For dsa sample built for RT1180, the exported 4 switch ports worked as switch with MAC learning.
hotplug cable will have information in the console. Here is the sample log.
Thanks.