Skip to content

Conversation

@marwaiehm-st
Copy link
Contributor

@marwaiehm-st marwaiehm-st commented Feb 5, 2025

Add STM32N6 ethernet support:

  • Add the compatible of the STM32N6 series
  • Update STM32N6 pinctrl dtsi files with the ETH pins
  • Align Ethernet descriptors to 32 bytes for STM32N6 to ensure efficient DMA operations and improve cache line efficiency
    and overall performance
  • Update the ETH_STM32_HAL menu configuration to conditionally select USE_STM32_HAL_RIF if SOC_SERIES_STM32N6X is enabled.
  • Add RISAF configuration in eth_initialize function for STM32N6 series to set up master and slave security attributes
    for the Ethernet peripheral.
  • Ensure RISAF configuration is done before enabling the Ethernet clock to maintain proper security attributes.

This PR supports only the Nucleo_N657X0_Q. I will add support for the STM32N6570_DK in a separate PR.

@zephyrbot
Copy link

zephyrbot commented Feb 5, 2025

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
hal_stm32 zephyrproject-rtos/hal_stm32@55043bc zephyrproject-rtos/hal_stm32@04ccc77 zephyrproject-rtos/[email protected]

All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-hal_stm32 DNM (manifest) This PR should not be merged (controlled by action-manifest) labels Feb 5, 2025
@marwaiehm-st marwaiehm-st force-pushed the upstream_eth branch 3 times, most recently from 7e4e629 to 3703c06 Compare February 6, 2025 10:53
@zephyrbot zephyrbot removed the DNM (manifest) This PR should not be merged (controlled by action-manifest) label Feb 6, 2025
@marwaiehm-st marwaiehm-st force-pushed the upstream_eth branch 6 times, most recently from be7aa97 to 6d35efe Compare February 12, 2025 21:02
@marwaiehm-st marwaiehm-st marked this pull request as ready for review February 13, 2025 09:21
@maass-hamburg
Copy link
Member

This does not seem right, adding a compatible to reduce code. That is more like a missuse of the DT.
Maybe use the Kconfig CONFIG_SOC_SERIES_STM32N6X for filtering in the driver. this would also eliminale having to have a seperate dt compatible.

@maass-hamburg This is perfectly fine. Here is the specification description of "compatible" property:

The compatible property value consists of one or more strings that define the specific programming model for
the device. This list of strings should be used by a client program for device driver selection. The property
value consists of a concatenated list of null terminated strings, from most specific to most general. They allow
a device to express its compatibility with a family of similar devices, potentially allowing a single device driver
to match against several devices.

So here most specific is "st,stm32n6-ethernet" while most general is "st,stm32-ethernet". "st,stm32-ethernet" is used to select the driver that will be used and "st,stm32n6-ethernet" allows to make some customization on top of it. This model is used in other drivers as well. Filtering using CONFIG_SOC_SERIES_STM32N6X as you propose will only make driver harder to read and maintain. This is where we're coming from and use of compatible make drivers much easier to maintain (as for instance you don't need to update the driver the next time a compatible series should be added. You just need to provide the right compatible in the .dtsi).

I was referring the suggestion to have st,stm32n6-ethernet" and "st,stm32h7-ethernet" in the compatible. the generic "st,stm32-ethernet". was fine.

@erwango
Copy link
Member

erwango commented Feb 18, 2025

I was referring the suggestion to have st,stm32n6-ethernet" and "st,stm32h7-ethernet" in the compatible. the generic "st,stm32-ethernet". was fine.

Ok, but I still think it is correct, st,stm32n6-ethernet being seen as a specific case of st,stm32h7-ethernet.
And as could be seen this allows to get rid of all the #if DT_HAS_COMPAT_STATUS_OKAY(st_stm32h7_ethernet) || DT_HAS_COMPAT_STATUS_OKAY(st_stm32n6_ethernet)

@maass-hamburg
Copy link
Member

maass-hamburg commented Feb 18, 2025

I was referring the suggestion to have st,stm32n6-ethernet" and "st,stm32h7-ethernet" in the compatible. the generic "st,stm32-ethernet". was fine.

Ok, but I still think it is correct, st,stm32n6-ethernet being seen as a specific case of st,stm32h7-ethernet. And as could be seen this allows to get rid of all the #if DT_HAS_COMPAT_STATUS_OKAY(st_stm32h7_ethernet) || DT_HAS_COMPAT_STATUS_OKAY(st_stm32n6_ethernet)

would the st,stm32n6 work (even limited) with just st,stm32h7-ethernet and st,stm32-ethernet as the compatible?

@erwango
Copy link
Member

erwango commented Feb 18, 2025

would the st,stm32n6 work (even limited) with just st,stm32h7-ethernet and st,stm32-ethernet as the compatible?

You can replace #if DT_HAS_COMPAT_STATUS_OKAY(st_stm32n6_ethernet) by #ifdef CONFIG_SOC_SERIES_STM32N6X.
But then, when adding support for a next series fullycompatible with N6 IP, you'll have to do:
#ifdef CONFIG_SOC_SERIES_STM32N6X || CONFIG_SOC_SERIES_STM32YZX.
Using compat method, you'll just need to provide the right compatibles in new series .dtsi and you'll be done. No need to touch the driver.

@maass-hamburg
Copy link
Member

would the st,stm32n6 work (even limited) with just st,stm32h7-ethernet and st,stm32-ethernet as the compatible?

You can replace #if DT_HAS_COMPAT_STATUS_OKAY(st_stm32n6_ethernet) by #ifdef CONFIG_SOC_SERIES_STM32N6X. But then, when adding support for a next series fullycompatible with N6 IP, you'll have to do: #ifdef CONFIG_SOC_SERIES_STM32N6X || CONFIG_SOC_SERIES_STM32YZX. Using compat method, you'll just need to provide the right compatibles in new series .dtsi and you'll be done. No need to touch the driver.

I understand you, but that was not the point. A compatible should not depend on another compatible being present. The second and later compatibles are meant as a fallback, if there is no diver available for the first compatible available. I don't like having a chain of compatibles, that all depend on the next.-

@erwango
Copy link
Member

erwango commented Feb 19, 2025

I understand you, but that was not the point. A compatible should not depend on another compatible being present. The second and later compatibles are meant as a fallback, if there is no diver available for the first compatible available. I don't like having a chain of compatibles, that all depend on the next.-

This fallback mechanism is valid for Linux but hardly applies in Zephyr, right ?
What other solution do you propose that avoids to update driver code each time a new series with compatible IP should be supported by the driver code (which is the final vocation of "compatible" in the end) ?

@maass-hamburg
Copy link
Member

I understand you, but that was not the point. A compatible should not depend on another compatible being present. The second and later compatibles are meant as a fallback, if there is no diver available for the first compatible available. I don't like having a chain of compatibles, that all depend on the next.-

This fallback mechanism is valid for Linux but hardly applies in Zephyr, right ? What other solution do you propose that avoids to update driver code each time a new series with compatible IP should be supported by the driver code (which is the final vocation of "compatible" in the end) ?

If it is compatible with a former compatible, use that. If there are new features, that requires the creation of a new compatible, create it and then make the appropriate changes to the driver and add || DT_HAS_COMPAT_STATUS_OKAY(st_stm32n6_ethernet) where it belongs. Maybe create a define for DT_HAS_COMPAT_STATUS_OKAY(st_stm32h7_ethernet) || DT_HAS_COMPAT_STATUS_OKAY(st_stm32n6_ethernet) at be beginning of the file to shorten it.

@maass-hamburg
Copy link
Member

Are there cases where users have to set the compatible themself for Ethernet? Or do they always have to include the dtsi?

If they never have to set it themself (in out of tree boards f.e.), I still don't like it, but can tolerate it.

@erwango
Copy link
Member

erwango commented Feb 19, 2025

If it is compatible with a former compatible, use that. If there are new features, that requires the creation of a new compatible, create it and then make the appropriate changes to the driver and add || DT_HAS_COMPAT_STATUS_OKAY(st_stm32n6_ethernet) where it belongs.

That is exactly what is done in the PR currently:

If it is compatible with a former compatible, use that.

N6 Eth IP makes use of existing STM32 driver (requires st,stm32-ethernet) and is a H7 compatible variant of it, which hence requires use of st,stm32h7-ethernet on top.

If there are new features, that requires the creation of a new compatible, create it and then make the appropriate changes to the driver and add || DT_HAS_COMPAT_STATUS_OKAY(st_stm32n6_ethernet) where it belongs.

Done as well. Hence the introduction of st,stm32n6-ethernet.

Finally, following these steps, we end up with compatible = st,stm32n6-ethernet, st,stm32h7-ethernet, st,stm32-ethernet.

@maass-hamburg
Copy link
Member

@marwaiehm-st this should be rebased

Update STM32N6 pinctrl dtsi files with the ETH pins
Add STM32N6 Ethernet pinctrl config to stm32-pinctrl-config.yaml

Signed-off-by: IBEN EL HADJ MESSAOUD Marwa <[email protected]>
Add the ethernet node

Signed-off-by: IBEN EL HADJ MESSAOUD Marwa <[email protected]>
Add ethernet node

Signed-off-by: IBEN EL HADJ MESSAOUD Marwa <[email protected]>
-Update the ETH_STM32_HAL menu configuration to conditionally
 select USE_STM32_HAL_RIF if SOC_SERIES_STM32N6X is enabled.

-Align Ethernet descriptors to 32 bytes for STM32N6 to ensure
 efficient DMA operations and improve cache line efficiency
 and overall performance

-Add RISAF configuration in eth_initialize function for STM32N6
 series to set up master and slave security attributes
 for the Ethernet peripheral.

-Ensure RISAF configuration is done before enabling
 the Ethernet clock to maintain proper security attributes.

Signed-off-by: IBEN EL HADJ MESSAOUD Marwa <[email protected]>
@maass-hamburg
Copy link
Member

@marwaiehm-st RGMII support is now missing, was that intended?

@marwaiehm-st
Copy link
Contributor Author

@marwaiehm-st RGMII support is now missing, was that intended?

The RGMII is used on the board STM32N6570_DK, and the support for the STM32N6570_DK will be added in a separate PR.

@maass-hamburg
Copy link
Member

Please add the st,stm32n6-ethernet.yaml file.

Add the compatible of the STM32N6 series

Signed-off-by: IBEN EL HADJ MESSAOUD Marwa <[email protected]>
@kartben kartben merged commit fb0e6e9 into zephyrproject-rtos:main Mar 18, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants