Ethernet GPIOs not configured correctly

[SytechDesigns]

This is a follow on from issue #373 .
Bug : Gpio pins for Ethernet are having their speed set to 0x00 ( low speed) instead of high speed, causing the phy interface to fail for high speed operations.

History : we have never been able to get ETH working correctly on the rev1.2 keil board. everything looks right, it just doesn't seem to be able to send and receive messages. We tried a native STMCube base app on the board and it works fine - so not hardware, network etc issues. To try and simplify the problem we laid out some new dev boards. One with an MII phy and one with the RMII (KSZ8081). Both have STMF427 2Mflash and external SDRam. We started off with something more familiar so went back to MF4.3 - We already had ports using the MII phy, and this worked just fine. When we came to the RMII chip, we had the same problem as we have with the keil 1.2 board - works with a 'native' lwip app ( so hardware fine) but not with MF4.3 LWIP - after much debugging I found the problem..
In DeviceCode...\STM32F4_ETH_gpio.cpp each gpio pin required is initialised by a call to CPU_GPIO_DisablePin - passing in pin, altfunction setting etc. The value passed in for the 'alternate' has the gpio speed, and alt function encoded and is a 32bit value ( 0x2b2) - The alternate param is of type GPIO_ALT_MODE - which is an enum with range 0 - 8. The alternate vale of 0xb (ETH), being out of range is not a problem, the compiler is happy with this, but the MDK compiler is optimising the enum storage type, based on the contained values - meaning, it makes it 8 bit - so when we pass in 0x2b2, we get 0xb2. Inside CPU_GPIO_DisablePin, this is fine until we call STM32F4_GPIO_Pin_Config, where all params are correct - except the gpio speed - which now gets passed as 'low' (00) not 'High' (02).

When we use a MII phy, data and clock between processor and phy is 25Mhz or less, and we get away with the 'low' clock' setting. But on the RMII, it is 50Mhz - and we no longer get away with this!
Possibly this is why the Keil STM400 rev 1.1 board works, as the code forces it connect at 10M and half duplex, it seems it does not work at 100M full duplex on MF4.4 - I suspect this is the same problem, when you go down to 10M connection speed - all clocks etc. between processor and phy are 10 times slower than at 100M. I don't think it is a phy problem on this old phy

We have corrected this gpio config problem on our 4.3 port and magically the RMII phy now works.

Temporary work round example can be seen in the MCBSTM32F400 solution - devicecode\Init. If you have a look at the gpio init for external Sram etc. you will see this problem has been seen before!! Answer is
after you call CPU_GPIO_DisablePin() - call STM32F4_GPIO_PinConfig() which will accept the alt function param as an int32 and reconfigure the gpio pin with the correct speed setting.

Not the best, as we effectively call STM32F4..PinConfig twice - but for now it is a simple work round.

I have not had a chance to try this on the MCBSTM32F400 port yet - with a rev 1.2 board - but I am 99% certain it will fix the problem.

[SytechDesigns]

Ok, I managed to find a little time to update the eth gpio config and rebuild. with the eth gpio's speed set to 'High' ( 0x02) the rev 1.2 board with the KSZ8081 now works just fine - connected at 100M full duplex.

[smaillet-ms]

Is there a PR related to this?

[SytechDesigns]

I guess I was looking for some guidance on the best way to fix this first. This issue comes up with any requirement to set up STM pins to their alt function - such as Ext SDRam - ETH or LCD. One solution is to modify the GPIO_ALT_MODE enum - which is in the hardware independent part of code, currently it only goes up to 0 - 7, so would be wrong to start defining it for STM specific alt functions. But there are a couple of possibilities here, we could add a UNKNOWN enum member and make it 0xffff - which would force the enum storage to get optimised as 16 bit rather than 8. Then we pass in whatever we need as the altmode value ( but this is a bit of a hack - it should really have a value in the enum range) Another possibility is to map the generic enum values of 0 - 7 to STM specific values in the STMF4 gpio code - so we map 0 to be 'normal gpio', 1 to be analog, 2, to be eth, 3 to be fmc etc. Then when we handle the passed in GPIO_ALT_MODE param in STM32F4Gpio functions we convert the generic enum value to a custom value for the STM processor - so a value of 2 passed in, gets converted to 0x2b2 etc. The later is most likely more the way things were intended.
If I was just doing this for myself, I would do it any way I liked, but if it is for the 'official' code base, then I would want to do it in the 'correct way'.

If I get some feedback on the preferred way to fix it, I don't mind doing the work and generating a PR

[smaillet-ms]

The "correct" way in my view is any way that isolates such idiosyncrasies to each board while minimizing the amount of copy/paste/tweak code. (copy/paste/tweak is fine for data tables in this sort of case), That is, to me the best design is one that uses shared code for all STM devices including the same GPIO controller, but using a unique data table that is easily understood and modified for each actual board. The HAL GPIO initialize routine can call the common code providing the per board data table to do the init as needed for a given board. (IIRC it isn't appropriate to just set everything to high speed as a default as there are power consumption implications)

The more generalized CPU GPIO APIs are in need of some re-thinking/re-design as they were based on a vastly narrower set of silicon options and simpler design variations. (you know back in the days when dinosaurs roamed the land and we carved code into stone tablets 😁 ) The complexity of options in GPIO pins has significantly increased beyond what the original design ever considered.

Given that one of the goals is to be able to use ANY CMSIS implementation AS-IS it would be interesting to see what, if anything, CMSIS v5 (due out this month) has for support of GPIO. (It has mostly ignored the issue of defining a GPIO API in previous releases)

[smaillet-ms]

It is worth mentioning that the original intent of the GPIO_ALT_MODE was to select amongst alternate external pin usage when the pin isn't used for GPIO. (e.g. many micros have GPIO pin multiplexing such that a given external pin might be used for GPIO or it could be used from some other purpose tied to an internal peripheral such as an I2S data line, SPI Chip Select, etc... At the time the number of alternate functions a given pin would multiplex to was limited (still is really), however, this limited view of things leaves out the real world case of additional settings. In the short term it may be worth modifying the API to accept an integral value with support for implicit casting from the enum type. Thus a full set of 32 bits is available while retaining support for the legacy more simplistic model.

[pilidj]

Hi! this interests me! If your phy works now, does that mean if you use socket class in managed code it works?
I didn't understand where you re-call STM32F4_GPIO_PinConfig.
In my case I have this in STM32F4_ETH_gpio.cpp file:

``#elif STM32F4_ETH_PHY_RMII
GPIO_PIN Eth_Rmii_TxD0 = ETH_PB12;//ETH_PG13;
GPIO_PIN Eth_Rmii_TxD1 = ETH_PB13;//ETH_PG14;
GPIO_PIN Eth_Rmii_Tx_En = ETH_PB11;//ETH_PG11;
GPIO_PIN Eth_Rmii_RxD0 = ETH_PC4;
GPIO_PIN Eth_Rmii_Rxd1 = ETH_PC5;
GPIO_PIN Eth_Rmii_Ref_Clk = ETH_PA1;
GPIO_PIN Ethr_Rmii_Crs_Div = ETH_PA7;

CPU_GPIO_DisablePin( Eth_Rmii_TxD0     , RESISTOR_DISABLED, 0, (GPIO_ALT_MODE)ETH_AF11 );
CPU_GPIO_DisablePin( Eth_Rmii_TxD1     , RESISTOR_DISABLED, 0, (GPIO_ALT_MODE)ETH_AF11 );
CPU_GPIO_DisablePin( Eth_Rmii_Tx_En    , RESISTOR_DISABLED, 0, (GPIO_ALT_MODE)ETH_AF11 );
CPU_GPIO_DisablePin( Eth_Rmii_RxD0     , RESISTOR_DISABLED, 0, (GPIO_ALT_MODE)ETH_AF11 );
CPU_GPIO_DisablePin( Eth_Rmii_Rxd1     , RESISTOR_DISABLED, 0, (GPIO_ALT_MODE)ETH_AF11 );
CPU_GPIO_DisablePin( Eth_Rmii_Ref_Clk  , RESISTOR_DISABLED, 0, (GPIO_ALT_MODE)ETH_AF11 );
CPU_GPIO_DisablePin( Ethr_Rmii_Crs_Div , RESISTOR_DISABLED, 0, (GPIO_ALT_MODE)ETH_AF11 );``

Do I have to call STM32F4_GPIO_PinConfig after each call of CPU_GPIO_DisablePin?
Other question, is there an project editor to open native code files with auto completion?

[SytechDesigns]

@pilidj
In the STM32F4_ETH_GPIO_PinConfig...
Add an extern ref to STM32F4_ETH_gpio.h - so linker can find function
Then as follows..
#include <tinyhal.h>

#include "STM32F4_ETH_gpio.h"
// allow direct access to stm gpio config
extern void STM32F4_GPIO_Pin_Config( GPIO_PIN pin, UINT32 mode, GPIO_RESISTOR resistor, UINT32 alternate );
//--------------------------------------------------------------------------------------------
// Constant defines
//--------------------------------------------------------------------------------------------

#define ETH_PA0 0x00U
#define ETH_PA3 0x03U
#define ETH_PB0 0x10U
...........
#define ETH_PG13 0x6DU
#define ETH_PG14 0x6EU

#define ETH_AF11 0x2B2U // speed = 2 (50MHz), af = 11 (Ethernet)
...........

void eth_initEthGpio()
{
// Configure ethernet GPIOs
const uint32_t pinMode = ETH_AF11 & 0xF;

GPIO_PIN Eth_Mdio = ETH_PA2;
GPIO_PIN Eth_Mdc = ETH_PC1;

GPIO_PIN Eth_Rmii_Txd0 = ETH_PB12;
GPIO_PIN Eth_Rmii_Txd1 = ETH_PB13;
GPIO_PIN Eth_Rmii_Txen = ETH_PB11;

GPIO_PIN Eth_Rmii_Rxd0 = ETH_PC4;
GPIO_PIN Eth_Rmii_Rxd1 = ETH_PC5;

GPIO_PIN Eth_Rmii_Ref_Clk = ETH_PA1;
GPIO_PIN Eth_Rmii_Crs_Div = ETH_PA7;

GPIO_PIN Eth_Mii_Rxer = ETH_PB10;

CPU_GPIO_DisablePin(Eth_Mdio, RESISTOR_DISABLED, 0, (GPIO_ALT_MODE)ETH_AF11);
STM32F4_GPIO_Pin_Config( Eth_Mdio , pinMode, RESISTOR_DISABLED, ETH_AF11 );

CPU_GPIO_DisablePin(Eth_Mdc, RESISTOR_DISABLED, 0, (GPIO_ALT_MODE)ETH_AF11);
STM32F4_GPIO_Pin_Config( Eth_Mdc , pinMode, RESISTOR_DISABLED, ETH_AF11 )

...............................................................

It is just a work round, as you end up calling STM32F4_GPIO_Pin_Config twice, once in CPU_GPIO_DisablePin ( with wrong ALT_MODE Value) and then with correct value.

I have been contemplating the 'correct' way to do this. GPIO_ALT_MODE is a platform independent enum, and (at least in MDK, I don't know about GCC), it will be optimised in size to be the smallest type possible ( i.e byte). So when you cast a 16 bit value to the enum, it drops the ms nibble.
One way I am thinking of is to define a number of byte type enum values of AF1 to AF_MAX, and then have these in the msb 5 bits, then the lsb 3 bits can be speed. When this enum value is cast to a GPIO_ALT_MODE, its value will not be corrupted. Then in CPU_GPIO_Disable parse the 8 bit enum value and generate the correct af and speed settings for the pin. This way the 'generic' mechanism is not modified, and only change is at the STM platform gpio driver level.

[cw2]

As I have hit the same problem, I vote for @smaillet-ms suggestion to change the parameter type (in #514).

[smaillet-ms]

@SytechDesigns It may be worth using C++ strongly typed enums to enforce the size of the enum. That could be a short term fix. The longer term "fix" is a redesign of the GPIO API support as the current design was based on hardware architectures that aren't around anymore and modern SoCs work very differently.