[BUG] STM32 HAL ETH driver sets wrong MDIO frequency

[Piranha]

The code in HAL_ETH_SetMDIOClockRange() function for H7 series, starting from HCLK frequency of 150 MHz and higher, sets the MDIO clock divider to 102. Per IEEE 802.3 specification the MDIO clock must not exceed 2,5 MHz, which means that it will be valid for HCLK up to 2,5 MHz * 102 = 255 MHz. But for H72x/H73x parts the maximum AHB frequency is 275 MHz and for H7Ax/H7Bx parts 280 MHz. Obviously the solution is to add a range with a divider 124, which will be valid up to 2,5 MHz * 124 = 310 MHz. The code for H5 series is correct, although ironically the maximum HCLK speed for those MCUs is 250 MHz anyway. Also the comment on the line 2405 incorrectly mentions 200 MHz instead of 250 MHz.

In addition for HCLK frequencies below 20 MHz the code sets the divider to the highest value, which is the worst possible one. Instead it should set it to the appropriate or lowest possible value. For Fx series that means setting the divider to 16, but for Hx series the divider can be as low as 4. While very low HCLK frequencies are not appropriate for MAC operation, they are still valid for MDIO operation and PHY configuration, which could be useful for some projects.

And, as almost always, this HAL code is just plain stupid. Not only the first (hclk >= 20000000U) is damaging, but all of the (hclk >= ...) conditions are just useless and increases the bloat. The code clearly shows that the developer doesn't really understand even the basic logic operations.

[LCE]

Good find, thanks for posting. Immediately checked my code and found that I'm using the highest divider ETH_MACMDIOAR_CR_DIV124. :D

> Not only the first (hclk >= 20000000U) is damaging

Why's that "damaging"? With the divider of 16, MDIO clock is set to 1.25 MHz, that a problem?

[Piranha]

It's damaging for HCLK frequencies below 20 MHz, because it sets the highest divider, which results in unnecessarily low MDIO frequencies. Read the second paragraph of the main post. :)

[LCE]

I got that, my question was about the "damage", which sounds to me as if any hardware could take some real damage.

But so you mean it's just veeeery slow.
And maybe unexpected things concerning communication with the PHY might happen?

[MWB_CHa]

Hi @Piranha ,

Thanks for spotting this point.

I confirm that the STM32H7 ETH driver shall be fixed for the MDIO clocks range. We missed the update in the latest version and it will be done in next release. (files enclosed).

"Not only the first (hclk >= 20000000U) is damaging, but all of the (hclk >= ...) conditions are just useless and increases the bloat"

Regarding the 20MHz frequency: it is not clear to me, because I think that frequencies below 20MHz should not be supported. But I need to check then get back with a confirmation.

But could you please explain why you think that all the other conditions are useless/bloat ?

Regarding the If/Else layout and coverage, the rationale is keeping the code easily readable and understandable and to align it with reference manual description. So it shall be generic (covering all derivatives of the same series) and simple to read.

 

[Piranha]

My main post explains why the (hclk >= 20000000U) condition is harmful regardless of the specific hardware requirements. Apparently you didn't read it. It also explains for what purpose the low HCLK frequencies are still useful. Again you didn't read it. The DIV102 range in your "fixed" files still contain the wrong comment about 200 MHz upper limit. And again you didn't read my post. In addition, calculating the DIV124 range with your suboptimal undocumented formula, gives the upper limit of 305 MHz, not 300 MHz. But, of course, for some geniuses the "better looking" numbers are more important than actual engineering or even just a basic consistency.

 

 

if ((...) && (x < 5)) {
	...
} else if ((x >= 5) && (...)) {

 

Post edited to adhere community guidelines.

[MWB_CHa]

Anyway, I've already read and acknowledged the other points, my question was simple: "Why you think that all the other conditions are useless/bloat ?". Your answer is that repeating the condition of the upper limit (ie. (x<5)->(x>=5)) is useless. Yes, that's true. But, as I already explained, it's simply a coding style allowing clear visibility of the code (table-like) and it doesn't add any useless bits of code since the compiler eventually transforms it into optimal sequence. Good for readers with no bad for footprint or functionality.

 

Sure, the comment line 2440 will be fixed

/* CSR Clock Range between 150-200 MHz */

should be

/* CSR Clock Range between 150-250 MHz */

 

Thanks for your contribution.

Post edited to adhere community guidelines.

[Piranha]

 

it doesn't add any useless bits of code since the compiler eventually transforms it into optimal sequence

First, debugging is typically done without optimization. Second - oh, so you are relying on a compiler optimization. There is just a "minor" issue - with optimization your HAL/Cube junk falls apart even quicker than it does without it.

 

it's simply a coding style allowing clear visibility of the code (table-like)

The only people, for whom it can increase the readability, are the ones, who don't understand code. For a normal developers your bloatware "style" decreases the readability, increases the cognitive load and expands the surface for potential bugs.

The HAL is full of code, which is copy-pasted in multiple instances like this one. The code for CM7 is exactly the same for both cases, so why is it duplicated? Either the developer cannot see the pattern or is incapable of restructuring the code accordingly. Or will you call it also a "style"? In addition the comments on lines 2002, 2016 are wrong and lines 1980, 1996, 2011 are missing comments.

The variable names - does the HAL use lower_case, CamelCase or a Hungarian notation for variable names?  That's a lack of style and consistency! And those useless Captain Obvious comments like "Set own bit" for code SET_BIT(..., ETH_DMATXCDESC_OWN) - a clear indicator of a beginner writing code and not understanding the whole point of making comments.

Then there is code like (uint32_t)(0x0U). Why is there an U suffix, if the value is cast to an unsigned type anyway? Looking all through the code it shows that the developers don't understand where the suffixes are actually necessary and they just put them almost everywhere. And why is a single constant put in parentheses?

Post edited to adhere community guidelines.