The ridiculous constant buffer size is only one of the problems with that code...
https://github.com/STMicroelectronics/STM32CubeF4/blob/3d6be4bd406f275728e0a321cc371c62a3100533/Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_eth.c#L1176
The code is setting the OWN bit before modifying other descriptor values in the subsequent lines. And, instead of SET_BIT() macro, it uses WRITE_REG() macro, which corrupts (clears) all other bits configured before. These are two critical and absolutely idiotic bugs in a single trivial line of code! As HAL developers have hard time understanding even the most basic issues and solutions, I'm writing the corrected code here:
if (heth->RxDescList.ItMode != 0U)
{
WRITE_REG(dmarxdesc->DESC1, ETH_RX_BUF_SIZE | ETH_DMARXDESC_RCH);
}
else
{
WRITE_REG(dmarxdesc->DESC1, ETH_DMARXDESC_DIC | ETH_RX_BUF_SIZE | ETH_DMARXDESC_RCH);
}
/* Ensure rest of descriptor is written to RAM before the OWN bit */
__DMB();
SET_BIT(dmarxdesc->DESC0, ETH_DMARXDESC_OWN);
And I can even tell how all of this disaster happened... Let's take a look on the new rewritten H7 driver:
https://github.com/STMicroelectronics/STM32CubeH7/blob/b340b13929e36a3427b8d94e8b1006022f82273f/Drivers/STM32H7xx_HAL_Driver/Src/stm32h7xx_hal_eth.c#L1188
This code is correct. For H7 the buffer size is configured in a register and setting OWN bit in that IF construction is the last modification of the descriptor values. But then the H7 code was ported to F4, for which the hardware and descriptor format differs.
For buffer size the H7 code hasn't "a similar name to copy-paste" close to this code location, therefore the developer just put in the "nice" 1000 just to get it compiling. The fact, that the developer didn't use the ETH_RX_BUF_SIZE macro, shows that he/she doesn't understand HAL software architecture.
As for the OWN bit, the developer was probably instructed to read my article about barriers, where part of the conclusion is: "The universal fix is to put __DMB() macro just before: Setting OWN bit." So he/she added the necessary setting of OWN bit literally after the __DMB(). Most likely because the developer doesn't even understand the concept of a descriptor.
https://github.com/STMicroelectronics/STM32CubeH7/blob/b340b13929e36a3427b8d94e8b1006022f82273f/Drivers/STM32H7xx_HAL_Driver/Src/stm32h7xx_hal_eth.c#L3314
https://github.com/STMicroelectronics/STM32CubeF4/blob/3d6be4bd406f275728e0a321cc371c62a3100533/Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_eth.c#L3072
After disabling interrupts, one cannot just unconditionally enable them. They have to be restored to the previous state like it is shown in this topic. Another critical bug for both - H7 and F4!
That's about bugs. But wait, that's not all...
https://github.com/STMicroelectronics/STM32CubeF4/blob/3d6be4bd406f275728e0a321cc371c62a3100533/Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_eth.c#L999
This code is not even from the new H7 driver, but from the previous F4 driver. Testing DMA status is useless as a simple write to ETH_DMATPDR register is correct and is more efficient. It's done efficiently for ETH_DMARPDR, so why are there different approaches? But the comment "Set the Tail pointer address" on the line before is copied from H7 and is wrong for the F4. Chaos!
https://github.com/STMicroelectronics/STM32CubeF4/blob/3d6be4bd406f275728e0a321cc371c62a3100533/Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_eth.c#L927
This line is also copied over from H7 without reading the reference manual. On F4 any value issues a poll demand and therefore calculating some complex pointer is waste of resources and bloated code. Actually even writing zero is not the most most efficient code, but this one is:
ETH->DMATPDR = (uint32_t)ETH; // Any value issues a descriptor list poll demand.
For memory barriers the new drivers mostly correctly uses the more efficient DMB instruction, but why there is DSB at lines 918 and 992? This time it comes from the lines 938 and 1012 of the new H7 driver. There is no reason for using DSB in any of these!
https://github.com/STMicroelectronics/STM32CubeF4/blob/3d6be4bd406f275728e0a321cc371c62a3100533/Drivers/STM32F4xx_HAL_Driver/Inc/stm32f4xx_hal_eth.h#L69
Those additional BackupAddr0 and BackupAddr1 elements of the descriptor structure are necessary for H7, because it's hardware modifies the descriptor completely and looses those addresses. But for F4 those are completely useless waste of memory, because the F4 hardware doesn't modify the fields, where addresses are stored. Obviously the developer has not read the reference manual either.
Apparently the H7 code was written by someone, who at least to some level understands what he is doing, but then porting to F4 was done by someone, who definitely doesn't understand what he/she is doing. Is this disgusting attitude for software considered normal for ST?
@Imen DAHMEN, the fun with the new rewritten driver starts...
P.S. All of these issues of F4 code are also the same for F7, F2 and F1. Therefore, if someone is currently porting that code to those series, fix it for those also.
