[RFC] option to use proper skew timings for Micrel KSZ9021

[RFC] option to use proper skew timings for Micrel KSZ9021

[Ian Abbott]

Hi all,

Currently the skew timings in the PHY OF device node for KSZ9021 are 
specified in 200ps steps for historical reasons (due to an error in the 
original KSZ9021 datasheet), but the hardware actually uses 120ps steps. 
  (This is all explained in 
"Documentation/devicetree/bindings/net/micrel-ksz90x1.txt".)

I would like to add an optional boolean property to indicate that the 
skew timing properties are to be interpreted as "proper" skew timings 
(in 120ps steps) rather than fake skew timings.  When this property is 
true, the driver can divide the specified skew timing values by 120 
instead of 200.  The advantage of this is that the same skew timing 
property values can be used in the device node and will apply to both 
KSZ9021 and KSZ9031 as long as the values are in range for both chips. 
(The KSZ9021 supports a larger range that the KSZ9031 for 
"rxdX-skew-ps", "txdX-skew-ps", "rxdv-skew-ps" and "txen-skew-ps", but 
the KSZ9031 has a finer resolution of 60ps compared to the KSZ9021's 120ps.)

I'd like to know if this is a sensible suggestion, and if so, what would 
be a sensible name for the new property?

Best regards,
Ian Abbott

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [RFC] option to use proper skew timings for Micrel KSZ9021

[Andrew Lunn]

> I would like to add an optional boolean property to indicate that the skew
> timing properties are to be interpreted as "proper" skew timings (in 120ps
> steps) rather than fake skew timings.  When this property is true, the
> driver can divide the specified skew timing values by 120 instead of 200.
> The advantage of this is that the same skew timing property values can be
> used in the device node and will apply to both KSZ9021 and KSZ9031 as long
> as the values are in range for both chips.

Hi Ian

I don't see why this is an advantage. Yes, it is all messed up, but it
is a well defined and documented mess. All this boolean will do is
make it a more complex mess, leading it more errors with the wrong
value set.

      Andrew

Re: [RFC] option to use proper skew timings for Micrel KSZ9021

[Ian Abbott]

On 09/11/2022 17:27, Andrew Lunn wrote:
>> I would like to add an optional boolean property to indicate that the skew
>> timing properties are to be interpreted as "proper" skew timings (in 120ps
>> steps) rather than fake skew timings.  When this property is true, the
>> driver can divide the specified skew timing values by 120 instead of 200.
>> The advantage of this is that the same skew timing property values can be
>> used in the device node and will apply to both KSZ9021 and KSZ9031 as long
>> as the values are in range for both chips.
> 
> Hi Ian
> 
> I don't see why this is an advantage. Yes, it is all messed up, but it
> is a well defined and documented mess. All this boolean will do is
> make it a more complex mess, leading it more errors with the wrong
> value set.

Hi Andrew,

I can think of a couple of use cases.  For example:

1. Forward planning to replace KSZ9021 with KSZ9031 in a future hardware 
iteration.  As long as the device tree and kernel driver (and possibly 
the bootloader if it uses the same device tree blob as the kernel 
internally) are upgraded at the same time, software upgrades of existing 
hardware with KSZ9021 will continue to work correctly.  Upgraded 
hardware with KSZ9031 will work properly with the updated software.

2. Due to KSZ9031 chip shortages, it may be useful to replace KSZ9031 
with KSZ9021 for a few manufacturing runs.  This can be done as long as 
the device tree and driver are updated to know about the new property in 
time for those manufacturing runs.

In both cases, the skew timings chosen would need to apply to both 
KSZ9021 and KSZ9031.

(At the time of writing, KSZ9031RNX chips are very difficult to get hold 
of except at exorbitant prices from scalpers (~US$200 per chip!), but 
the older generation KSZ9021RN chips are much easier to get hold of 
(~US$5 per chip).)

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [RFC] option to use proper skew timings for Micrel KSZ9021

[Andrew Lunn]

> 1. Forward planning to replace KSZ9021 with KSZ9031 in a future hardware
> iteration.  As long as the device tree and kernel driver (and possibly the
> bootloader if it uses the same device tree blob as the kernel internally)
> are upgraded at the same time, software upgrades of existing hardware with
> KSZ9021 will continue to work correctly.  Upgraded hardware with KSZ9031
> will work properly with the updated software.
> 
> 2. Due to KSZ9031 chip shortages, it may be useful to replace KSZ9031 with
> KSZ9021 for a few manufacturing runs.  This can be done as long as the
> device tree and driver are updated to know about the new property in time
> for those manufacturing runs.
> 
> In both cases, the skew timings chosen would need to apply to both KSZ9021
> and KSZ9031.

So you are saying that as it is pin compatible ( i assume), you can
swap the PHY and still call it the same board, and still use the same
device tree blob.

If you are going to do this, i think you really should fix all the
bugs, not just the step. KSZ9021 has an offset of -840ps. KSZ9031 has
an offset of -900ps. So both are broke, in that the skew is expected
to be a signed value, 0 meaning 0.

I would suggest a bool property something like:

micrel,skew-equals-real-picoseconds

and you need to update the documentation in a way it is really clear
what is going on.

I would also consider adding a phydev_dbg() which prints the actual ps
skew being used, with/without the bug.

And since you are adding more foot guns, please validate the values in
DT as strictly as possible, without breaking the existing binding.

   Andrew

Re: [RFC] option to use proper skew timings for Micrel KSZ9021

[Ian Abbott]

On 11/11/2022 14:25, Andrew Lunn wrote:
>> 1. Forward planning to replace KSZ9021 with KSZ9031 in a future hardware
>> iteration.  As long as the device tree and kernel driver (and possibly the
>> bootloader if it uses the same device tree blob as the kernel internally)
>> are upgraded at the same time, software upgrades of existing hardware with
>> KSZ9021 will continue to work correctly.  Upgraded hardware with KSZ9031
>> will work properly with the updated software.
>>
>> 2. Due to KSZ9031 chip shortages, it may be useful to replace KSZ9031 with
>> KSZ9021 for a few manufacturing runs.  This can be done as long as the
>> device tree and driver are updated to know about the new property in time
>> for those manufacturing runs.
>>
>> In both cases, the skew timings chosen would need to apply to both KSZ9021
>> and KSZ9031.
> 
> So you are saying that as it is pin compatible ( i assume), you can
> swap the PHY and still call it the same board, and still use the same
> device tree blob.

Yes, but you've convinced me that it is slightly more involved than I 
initially thought!

> If you are going to do this, i think you really should fix all the
> bugs, not just the step. KSZ9021 has an offset of -840ps. KSZ9031 has
> an offset of -900ps. So both are broke, in that the skew is expected
> to be a signed value, 0 meaning 0.
> 
> I would suggest a bool property something like:
> 
> micrel,skew-equals-real-picoseconds
> 
> and you need to update the documentation in a way it is really clear
> what is going on.

Perhaps it could allow the renamed properties for the KSZ9131 to be 
used. (They have similar names to the KSZ9021/KSZ9031 properties, but 
change the `-ps` suffix to `-psec`.)  Those are already absolute 
timings.  There would need to be a decision on what to do if the node 
contains a mixture of `foo-ps` and `foo-psec` properties.

> I would also consider adding a phydev_dbg() which prints the actual ps
> skew being used, with/without the bug.
> 
> And since you are adding more foot guns, please validate the values in
> DT as strictly as possible, without breaking the existing binding.

Yes, some min/max clamping of skew values would be good.  The code for 
KSZ9131 does that already.

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [RFC] option to use proper skew timings for Micrel KSZ9021

[Andrew Lunn]

> Yes, but you've convinced me that it is slightly more involved than I
> initially thought!
> 
> > If you are going to do this, i think you really should fix all the
> > bugs, not just the step. KSZ9021 has an offset of -840ps. KSZ9031 has
> > an offset of -900ps. So both are broke, in that the skew is expected
> > to be a signed value, 0 meaning 0.
> > 
> > I would suggest a bool property something like:
> > 
> > micrel,skew-equals-real-picoseconds
> > 
> > and you need to update the documentation in a way it is really clear
> > what is going on.
> 
> Perhaps it could allow the renamed properties for the KSZ9131 to be used.
> (They have similar names to the KSZ9021/KSZ9031 properties, but change the
> `-ps` suffix to `-psec`.)

That is also broken. Go check, everything else in DT uses -ps.

> > I would also consider adding a phydev_dbg() which prints the actual ps
> > skew being used, with/without the bug.
> > 
> > And since you are adding more foot guns, please validate the values in
> > DT as strictly as possible, without breaking the existing binding.
> 
> Yes, some min/max clamping of skew values would be good.  The code for
> KSZ9131 does that already.

I would want much more strict checking than that. The old and the new
values probably don't intersect. So if you see an old value while
micrel,skew-equals-real-picoseconds is in force, fail the probe with
-EINVAL. It looks like the old binding silently preforms rounding to
the nearest delay. So you probably should not do the opposite, error
out for a new value when micrel,skew-equals-real-picoseconds is not in
force. But you can add range checks. A negative value is clearly wrong
for the old values and should be -EINVAL. You just need to watch out
for that the current code reads the values as u32, not s32, so you
won't actually see a negative value.

Sometimes it is better to leave broke but working stuff broken and
just live with it.

      Andrew

Re: [RFC] option to use proper skew timings for Micrel KSZ9021

[Ian Abbott]

On 11/11/2022 17:53, Andrew Lunn wrote:
>>> And since you are adding more foot guns, please validate the values in
>>> DT as strictly as possible, without breaking the existing binding.
>>
>> Yes, some min/max clamping of skew values would be good.  The code for
>> KSZ9131 does that already.
> 
> I would want much more strict checking than that. The old and the new
> values probably don't intersect. So if you see an old value while
> micrel,skew-equals-real-picoseconds is in force, fail the probe with
> -EINVAL. It looks like the old binding silently preforms rounding to
> the nearest delay. So you probably should not do the opposite, error
> out for a new value when micrel,skew-equals-real-picoseconds is not in
> force. But you can add range checks. A negative value is clearly wrong
> for the old values and should be -EINVAL. You just need to watch out
> for that the current code reads the values as u32, not s32, so you
> won't actually see a negative value.

I'm not sure how to tell old values and new values apart (except for 
negative new values).  A divisibility test won't work for values that 
are divisible by 600 (lcm(120, 200)).

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [RFC] option to use proper skew timings for Micrel KSZ9021

[Andrew Lunn]

On Fri, Nov 11, 2022 at 07:03:08PM +0000, Ian Abbott wrote:
> On 11/11/2022 17:53, Andrew Lunn wrote:
> > > > And since you are adding more foot guns, please validate the values in
> > > > DT as strictly as possible, without breaking the existing binding.
> > > 
> > > Yes, some min/max clamping of skew values would be good.  The code for
> > > KSZ9131 does that already.
> > 
> > I would want much more strict checking than that. The old and the new
> > values probably don't intersect. So if you see an old value while
> > micrel,skew-equals-real-picoseconds is in force, fail the probe with
> > -EINVAL. It looks like the old binding silently preforms rounding to
> > the nearest delay. So you probably should not do the opposite, error
> > out for a new value when micrel,skew-equals-real-picoseconds is not in
> > force. But you can add range checks. A negative value is clearly wrong
> > for the old values and should be -EINVAL. You just need to watch out
> > for that the current code reads the values as u32, not s32, so you
> > won't actually see a negative value.
> 
> I'm not sure how to tell old values and new values apart (except for
> negative new values).  A divisibility test won't work for values that are
> divisible by 600 (lcm(120, 200)).

I might have this wrong, but i think they are:


 Old	New
===== ======
0	-840
200	-720
400	-600
600	-480
800	-360
1000	-240
1200	-120
1400	0
1600	120
1800	240
2000	360
2200	480
2400	600
2600	720
2800	840
3000	960

The only overlap is 0 and 600. You can just special case those two
values.

	Andrew