Re: [PATCH net-next v2 4/4] net: dp83869: Add RGMII internal delay configuration

From: Dan Murphy
Date: Tue May 26 2020 - 13:48:20 EST


Andrew

On 5/23/20 5:07 PM, Andrew Lunn wrote:
Any why is your PHY special, in that is does care about out of range
delays, when others using new the new core helper don't?
We are not rounding to nearest here. Basically the helper works to find the
best match

If the delay passed in is less than or equal to the smallest delay then
return the smallest delay index

If the delay passed in is greater then the largest delay then return the max
delay index
+ /* Find an approximate index by looking up the table */
+ if (delay > delay_values[i - 1] &&
+ delay < delay_values[i]) {
+ if (delay - delay_values[i - 1] < delay_values[i] - delay)
+ return i - 1;
+ else
+ return i;

This appears to round to the nearest value when it is not an exact
match.

The documentation is a hint to the DT developer what value to put in
DT. By saying it rounders, the developer does not need to go digging
through the source code to find an exact value, otherwise -EINVAL will
be returned. They can just use the value the HW engineer suggested,
and the PHY will pick whatever is nearest.

Not sure what you mean about this PHY being special. This helper is
not PHY specific.
As you said, if out of range, the helper returns the top/bottom
value. Your PHY is special, the top/bottom value is not good enough,
you throw an error.

The point of helpers is to give uniform behaviour. We have one line
helpers, simply because they give uniform behaviour, rather than have
each driver do it subtlety different. But it also means drivers should
try to not add additional constraints over what the helper already
has, unless it is actually required by the hardware.

After I think about this more I am thinking a helper may be over kill here
and the delay to setting should be done within the PHY driver itself
The helper is useful, it will result in uniform handling of rounding
between DT values and what the PHY can actually do. But please also
move your range check and error message inside the helper.


I re-worked v3 to be a bit more of a helper and incorporated Florian's and you comments

Dan


Andrew