stm32/ethernet: A few improvements. #13630
Conversation
af20cee to
c470a20
Compare
ports/stm32/eth.c
Outdated
| mp_hal_pin_config_alt_static(MICROPY_HW_ETH_RMII_TXD0, MP_HAL_PIN_MODE_ALT, MP_HAL_PIN_PULL_NONE, STATIC_AF_ETH_RMII_TXD0); | ||
| mp_hal_pin_config_alt_static(MICROPY_HW_ETH_RMII_TXD1, MP_HAL_PIN_MODE_ALT, MP_HAL_PIN_PULL_NONE, STATIC_AF_ETH_RMII_TXD1); | ||
| mp_hal_pin_config_alt_static_speed(MICROPY_HW_ETH_RMII_TXD0, MP_HAL_PIN_MODE_ALT, MP_HAL_PIN_PULL_NONE, MP_HAL_PIN_SPEED_MEDIUM, STATIC_AF_ETH_RMII_TXD0); | ||
| mp_hal_pin_config_alt_static_speed(MICROPY_HW_ETH_RMII_TXD1, MP_HAL_PIN_MODE_ALT, MP_HAL_PIN_PULL_NONE, MP_HAL_PIN_SPEED_MEDIUM, STATIC_AF_ETH_RMII_TXD1); |
There was a problem hiding this comment.
The default when using just mp_hal_pin_config_alt_static() is to use "high speed", so this reduces the speed. Why not configure it to "very high"?
There was a problem hiding this comment.
Actually I was not sure what state it is and changed it to the slowest speed that fits according to the data sheet and that worked with my "ball of wires" test set-up. But if the default is "high speed", it can be left as that. "Very high" speed is according to the data sheet not needed at 3.3V and it just increases the radiation and cross-talk. So I can drop this commit.
ports/stm32/eth.c
Outdated
| @@ -165,14 +167,14 @@ STATIC void eth_phy_write(uint32_t reg, uint32_t val) { | |||
| } | |||
| ETH->MACMIIDR = val; | |||
| uint32_t ar = ETH->MACMIIAR; | |||
| ar = reg << ETH_MACMIIAR_MR_Pos | (ar & ETH_MACMIIAR_CR_Msk) | ETH_MACMIIAR_MW | ETH_MACMIIAR_MB; | |||
| ar = reg << ETH_MACMIIAR_MR_Pos | (ar & ETH_MACMIIAR_CR_Msk) | ETH_MACMIIAR_MW | ETH_MACMIIAR_MB | (phy_addr << ETH_MACMIIAR_PA_Pos); | |||
There was a problem hiding this comment.
please put the pyh_addr bit at the start of this expression, to retain the ordering that most-significant-bits are on the left
There was a problem hiding this comment.
Done, as well for the H7 case.
ports/stm32/eth.c
Outdated
| @@ -190,17 +194,18 @@ STATIC uint32_t eth_phy_read(uint32_t reg) { | |||
| while (ETH->MACMIIAR & ETH_MACMIIAR_MB) { | |||
| } | |||
| uint32_t ar = ETH->MACMIIAR; | |||
| ar = reg << ETH_MACMIIAR_MR_Pos | (ar & ETH_MACMIIAR_CR_Msk) | ETH_MACMIIAR_MB; | |||
| ar = reg << ETH_MACMIIAR_MR_Pos | (ar & ETH_MACMIIAR_CR_Msk) | ETH_MACMIIAR_MB | (phy_addr << ETH_MACMIIAR_PA_Pos); | |||
There was a problem hiding this comment.
Done, as well for the H7 case.
ports/stm32/eth.c
Outdated
| @@ -26,12 +26,14 @@ | |||
|
|
|||
| #include <string.h> | |||
| #include "py/mphal.h" | |||
| #include "py/runtime.h" | |||
There was a problem hiding this comment.
For raising an error about a wrong phy_type:
mp_raise_ValueError(MP_ERROR_TEXT("Invalid phy_type"));
There was a problem hiding this comment.
I see.
It would be better to separate the layers here: low-level hardware init vs higher-level Python interface.
I suggest changing eth_init() to return int, with 0 being success and a negative number being an error code. Then callers of this function must test this error code and raise as appropriate.
As part of this, note that ARDUINO_PORTENTA_H7's board init function calls eth_init(), and that should definitely not raise an exception (because that code runs outside the main MicroPython framework). So that use will need to be updated to pass the correct PHY.
There was a problem hiding this comment.
Moved the raise() call to network_lan.c and added the proper parameters to board_init.c of the Portenta H7. Thank you for the hint about the Portenta H7. I did not expect that.
| PHY_ANAR_SPEED_10FULL | | ||
| PHY_ANAR_SPEED_100HALF | | ||
| PHY_ANAR_SPEED_100FULL | | ||
| PHY_ANAR_IEEE802_3); |
There was a problem hiding this comment.
This new setting is a change from the previous behaviour, which just left this register in its default state. How does the change you've made impact things? (It looks like a good change, I'm just wondering why you made it.)
There was a problem hiding this comment.
I made it to be sure that all possible modes that are supported by the Ethernet controller are announced. The default mode can as well be set by strapping pins at the PHY, which by chance/bad luck may be more restrictive.
|
The PR is updated. Tested with a PYBD_SF6 and STM32H750 device and LAN8720 and DP83848 PHY devices. The performance using iperf3 is about 80MBit/s for TCP and 10.5MBit/s for UDP. |
I think this is just the default bitrate setting for UDP tests. You should be able to get a lot more bandwidth than this by using the |
|
At the board version the bandwidth can be set as well. Setting it to 80M results in a data rate of 80M for udp. Using |
|
Updated. |
The default value is 0, which is compatible with the existing behaviour. Implementing that required changes to eth.c as well. The value of phy_addr is added to the eth_t data type. Tested with a STM32F767 and a STM32H750 device. Signed-off-by: robert-hh <robert@hammelrath.com>
The MAC clock was initialized both in eth_init() and eth_mac_init(). The latter is not required. Signed-off-by: robert-hh <robert@hammelrath.com>
With LAN8742, LAN8720, LAN83825 and DP83848 as possible options, and the symbols PHY_LAN8720, PHY_LAN8742, PHY_DP83825 and PHY_DP8348. The default is PHY_LAN8742 which is the existing behaviour. The eth_init() parameters for the Portenta H7 board are set to phy_addr=0 and phy_type=LAN8742, which matches the previous defaults and the schematics. Tested with LAN8720 and DP83848 breakout boards at 10M Duplex and 100M Duplex modes. Signed-off-by: robert-hh <robert@hammelrath.com>
|
Thanks for updating. Now merged. |
|
Thank you very much. I tested and it works. |
phy_addr=x, e.g.lan = network.LAN(phy_addr=1. The default value is 0, matching the current behavior.phy_type=<model>. Supported model names are PHY_LAN8742, PHY_LAN8720 and PHY_DP83848. The default is PHY_LAN8742. Tested with a LAN8720 (=LAN8742) and DP83848 device.