[PATCH iwl-net] ice: fix incorrect PHY settings for 100 GB/s

[PATCH iwl-net] ice: fix incorrect PHY settings for 100 GB/s

[Przemyslaw Korba]

ptp4l application reports too high offset when ran on E823 device
with a 100GB/s link. Those values cannot go under 100ns, like in a
PTP working case when using 100 GB/s cable.
This is due to incorrect frequency settings on the PHY clocks for
100 GB/s speed. Changes are introduced to align with the internal
hardware documentation, and correctly initialize frequency in PHY
clocks with the frequency values that are in our HW spec.
To reproduce the issue run ptp4l as a Time Receiver on E823 device,
and observe the offset, which will never approach values seen
in the PTP working case.

Fixes: 3a7496234d17 ("ice: implement basic E822 PTP support")
Reviewed-by: Milena Olech <milena.olech@intel.com>
Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp_consts.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_consts.h b/drivers/net/ethernet/intel/ice/ice_ptp_consts.h
index 6620642077bb..bdb1020147d1 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_consts.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_consts.h
@@ -761,9 +761,9 @@ const struct ice_vernier_info_e82x e822_vernier[NUM_ICE_PTP_LNK_SPD] = {
 		/* rx_desk_rsgb_par */
 		644531250, /* 644.53125 MHz Reed Solomon gearbox */
 		/* tx_desk_rsgb_pcs */
-		644531250, /* 644.53125 MHz Reed Solomon gearbox */
+		390625000, /* 390.625 MHz Reed Solomon gearbox */
 		/* rx_desk_rsgb_pcs */
-		644531250, /* 644.53125 MHz Reed Solomon gearbox */
+		390625000, /* 390.625 MHz Reed Solomon gearbox */
 		/* tx_fixed_delay */
 		1620,
 		/* pmd_adj_divisor */

base-commit: 6ef5f61a4aa7d4df94a855a44f996bff08b0be83
-- 
2.31.1

Re: [Intel-wired-lan] [PATCH iwl-net] ice: fix incorrect PHY settings for 100 GB/s

[Paul Menzel]

Dear Przemyslaw,



Thank you for your patch.

Am 26.11.24 um 11:23 schrieb Przemyslaw Korba:
> ptp4l application reports too high offset when ran on E823 device
> with a 100GB/s link. Those values cannot go under 100ns, like in a
> PTP working case when using 100 GB/s cable.
> This is due to incorrect frequency settings on the PHY clocks for
> 100 GB/s speed. Changes are introduced to align with the internal
> hardware documentation, and correctly initialize frequency in PHY

It’d be great if you added the documentation name.

> clocks with the frequency values that are in our HW spec.
> To reproduce the issue run ptp4l as a Time Receiver on E823 device,
> and observe the offset, which will never approach values seen
> in the PTP working case.

(I’d add a blank line between paragraphs.)

Also, I always like to see pastes from the commands you ran to reproduce 
this. It’s always good for comparison.

> Fixes: 3a7496234d17 ("ice: implement basic E822 PTP support")

Any idea, where the wrong values came from? Will your test be added to 
the test procedure?

> Reviewed-by: Milena Olech <milena.olech@intel.com>
> Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_ptp_consts.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_consts.h b/drivers/net/ethernet/intel/ice/ice_ptp_consts.h
> index 6620642077bb..bdb1020147d1 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_consts.h
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_consts.h
> @@ -761,9 +761,9 @@ const struct ice_vernier_info_e82x e822_vernier[NUM_ICE_PTP_LNK_SPD] = {
>   		/* rx_desk_rsgb_par */
>   		644531250, /* 644.53125 MHz Reed Solomon gearbox */
>   		/* tx_desk_rsgb_pcs */
> -		644531250, /* 644.53125 MHz Reed Solomon gearbox */
> +		390625000, /* 390.625 MHz Reed Solomon gearbox */
>   		/* rx_desk_rsgb_pcs */
> -		644531250, /* 644.53125 MHz Reed Solomon gearbox */
> +		390625000, /* 390.625 MHz Reed Solomon gearbox */
>   		/* tx_fixed_delay */
>   		1620,
>   		/* pmd_adj_divisor */

Kind regards,

Paul

Re: [PATCH iwl-net] ice: fix incorrect PHY settings for 100 GB/s

[Andrew Lunn]

On Tue, Nov 26, 2024 at 11:23:11AM +0100, Przemyslaw Korba wrote:
> ptp4l application reports too high offset when ran on E823 device
> with a 100GB/s link. Those values cannot go under 100ns, like in a
> PTP working case when using 100 GB/s cable.
> This is due to incorrect frequency settings on the PHY clocks for
> 100 GB/s speed. Changes are introduced to align with the internal
> hardware documentation, and correctly initialize frequency in PHY
> clocks with the frequency values that are in our HW spec.
> To reproduce the issue run ptp4l as a Time Receiver on E823 device,
> and observe the offset, which will never approach values seen
> in the PTP working case.

You forgot to Cc: the PTP maintainer.

If i spent the time to measure the latency and configured ptp4l
correctly to take into account the latency, would i not see this
issue? And will this change then cause a regression because it changes
the latency invalidating my measurements?

    Andrew

RE: [PATCH iwl-net] ice: fix incorrect PHY settings for 100 GB/s

[Korba, Przemyslaw]

> On Tue, Nov 26, 2024 at 11:23:11AM +0100, Przemyslaw Korba wrote:
> > ptp4l application reports too high offset when ran on E823 device with
> > a 100GB/s link. Those values cannot go under 100ns, like in a PTP
> > working case when using 100 GB/s cable.
> > This is due to incorrect frequency settings on the PHY clocks for
> > 100 GB/s speed. Changes are introduced to align with the internal
> > hardware documentation, and correctly initialize frequency in PHY
> > clocks with the frequency values that are in our HW spec.
> > To reproduce the issue run ptp4l as a Time Receiver on E823 device,
> > and observe the offset, which will never approach values seen in the
> > PTP working case.
> 
> You forgot to Cc: the PTP maintainer.

Who is the PTP maintainer? Is it necessary? This is only Intel's driver, 
I am not sure if PTP maintainer is necessary. 

> 
> If i spent the time to measure the latency and configured ptp4l correctly to take
> into account the latency, would i not see this issue? And will this change then
> cause a regression because it changes the latency invalidating my measurements?
> 

I don't think ptp4l config, or configured latency has anything to do with that, you should
still see issue with any configured latency. due to incorrect PHY settings there was incorrect 
calculation in Vernier algorithm. Not much to do with latency. The Problem was that the offset 
was quite far off,  I will attach logs in commit message once the window is opened. I did not test 
with other ptp4l configs other than the standard one. Thanks for reviewing by the way! : )

>     Andrew

RE: [Intel-wired-lan] [PATCH iwl-net] ice: fix incorrect PHY settings for 100 GB/s

[Korba, Przemyslaw]

> 
> Dear Przemyslaw,
> 
> 
> 
> Thank you for your patch.
> 
> Am 26.11.24 um 11:23 schrieb Przemyslaw Korba:
> > ptp4l application reports too high offset when ran on E823 device with
> > a 100GB/s link. Those values cannot go under 100ns, like in a PTP
> > working case when using 100 GB/s cable.
> > This is due to incorrect frequency settings on the PHY clocks for
> > 100 GB/s speed. Changes are introduced to align with the internal
> > hardware documentation, and correctly initialize frequency in PHY
> 
> It’d be great if you added the documentation name.

I don't think I can, this is Intel's restricted secret document
 
> 
> > clocks with the frequency values that are in our HW spec.
> > To reproduce the issue run ptp4l as a Time Receiver on E823 device,
> > and observe the offset, which will never approach values seen in the
> > PTP working case.
> 
> (I’d add a blank line between paragraphs.)

Good point, will do.

> 
> Also, I always like to see pastes from the commands you ran to reproduce this. It’s
> always good for comparison.

Sure, will attach once window is opened.

> 
> > Fixes: 3a7496234d17 ("ice: implement basic E822 PTP support")
> 
> Any idea, where the wrong values came from? Will your test be added to the test
> procedure?

Wrong values come from incorrect vernier calculations. I will attach the ptp4l
logs to commit message once window is opened. We should have additional 
validation case for it in the near future. Thanks for reviewing! : )

> 
> > Reviewed-by: Milena Olech <milena.olech@intel.com>
> > Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
> > ---
> >   drivers/net/ethernet/intel/ice/ice_ptp_consts.h | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_consts.h
> > b/drivers/net/ethernet/intel/ice/ice_ptp_consts.h
> > index 6620642077bb..bdb1020147d1 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_ptp_consts.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_consts.h
> > @@ -761,9 +761,9 @@ const struct ice_vernier_info_e82x
> e822_vernier[NUM_ICE_PTP_LNK_SPD] = {
> >   		/* rx_desk_rsgb_par */
> >   		644531250, /* 644.53125 MHz Reed Solomon gearbox */
> >   		/* tx_desk_rsgb_pcs */
> > -		644531250, /* 644.53125 MHz Reed Solomon gearbox */
> > +		390625000, /* 390.625 MHz Reed Solomon gearbox */
> >   		/* rx_desk_rsgb_pcs */
> > -		644531250, /* 644.53125 MHz Reed Solomon gearbox */
> > +		390625000, /* 390.625 MHz Reed Solomon gearbox */
> >   		/* tx_fixed_delay */
> >   		1620,
> >   		/* pmd_adj_divisor */
> 
> Kind regards,
> 
> Paul

Re: [PATCH iwl-net] ice: fix incorrect PHY settings for 100 GB/s

[Przemek Kitszel]

On 11/27/24 14:19, Korba, Przemyslaw wrote:
>> On Tue, Nov 26, 2024 at 11:23:11AM +0100, Przemyslaw Korba wrote:
>>> ptp4l application reports too high offset when ran on E823 device with
>>> a 100GB/s link. Those values cannot go under 100ns, like in a PTP
>>> working case when using 100 GB/s cable.
>>> This is due to incorrect frequency settings on the PHY clocks for
>>> 100 GB/s speed. Changes are introduced to align with the internal
>>> hardware documentation, and correctly initialize frequency in PHY
>>> clocks with the frequency values that are in our HW spec.
>>> To reproduce the issue run ptp4l as a Time Receiver on E823 device,
>>> and observe the offset, which will never approach values seen in the
>>> PTP working case.
>>
>> You forgot to Cc: the PTP maintainer.
> 
> Who is the PTP maintainer? Is it necessary? This is only Intel's driver,
> I am not sure if PTP maintainer is necessary.

I was curious for a moment too, but just for a moment :)

We develop network drivers in the public, so we CC people which could
have a valuable feedback. For PTP code it's PTP Maintainer, and other
PTP folks. I'm not sure how interested they are though, @Richard?
@Vladimir?


In general it is similar to why we CC netdev. The difference is that
this code will not go via PTP Maintainer, but it does not matter that
much for the review/design purposes.

> 
>>
>> If i spent the time to measure the latency and configured ptp4l correctly to take
>> into account the latency, would i not see this issue? And will this change then
>> cause a regression because it changes the latency invalidating my measurements?
>>
> 
> I don't think ptp4l config, or configured latency has anything to do with that, you should
> still see issue with any configured latency. due to incorrect PHY settings there was incorrect
> calculation in Vernier algorithm. Not much to do with latency. The Problem was that the offset
> was quite far off,  I will attach logs in commit message once the window is opened. I did not test
> with other ptp4l configs other than the standard one. Thanks for reviewing by the way! : )
> 
>>      Andrew

Re: [PATCH iwl-net] ice: fix incorrect PHY settings for 100 GB/s

[Andrew Lunn]

> > > You forgot to Cc: the PTP maintainer.
> > 
> > Who is the PTP maintainer? Is it necessary? This is only Intel's driver,
> > I am not sure if PTP maintainer is necessary.
> 
> I was curious for a moment too, but just for a moment :)
> 
> We develop network drivers in the public, so we CC people which could
> have a valuable feedback. For PTP code it's PTP Maintainer, and other
> PTP folks. I'm not sure how interested they are though, @Richard?
> @Vladimir?
> 
> 
> In general it is similar to why we CC netdev. The difference is that
> this code will not go via PTP Maintainer, but it does not matter that
> much for the review/design purposes.

"only Intel's driver" is a bit of a worry. Part of being a Maintainer
is to ensure that all drivers behave the same. There should not be any
difference between an Intel PTP device, a Marvell PTP devices, a
Microchip PTP device etc. They should all implement the API in a
uniform way. A developer for the Intel driver probably does not know
the Marvell and Microchip code, and so could easily do something
different. The PTP Maintainer however has an overview over all
implementations and can point out when the driver is not implementing
the APIs in a uniform way, etc.

What i have also seen is that if one driver gets something wrong,
other drivers might as well. Being a Maintainer you are not blinkered
by corporate identity. You might take a quick look at the other PTP
drivers and see if they have the same problem. You can then ask the
developer to go fix the same problem in all the other PTP drivers.

	Andrew

Re: [PATCH iwl-net] ice: fix incorrect PHY settings for 100 GB/s

[Richard Cochran]

On Thu, Nov 28, 2024 at 05:20:44PM +0100, Andrew Lunn wrote:

> "only Intel's driver" is a bit of a worry. Part of being a Maintainer
> is to ensure that all drivers behave the same. There should not be any
> difference between an Intel PTP device, a Marvell PTP devices, a
> Microchip PTP device etc. They should all implement the API in a
> uniform way.

Yes, and I appreciate it being on CC even for driver changes.

> What i have also seen is that if one driver gets something wrong,
> other drivers might as well.

Yeah, unfortunately there are many device drivers (not just
PTP/network drivers) that get things wrong.  These are then copied by
the authors of new drivers, and so on.

Thanks,
Richard

RE: [PATCH iwl-net] ice: fix incorrect PHY settings for 100 GB/s

[Korba, Przemyslaw]

> On Thu, Nov 28, 2024 at 05:20:44PM +0100, Andrew Lunn wrote:
> 
> > "only Intel's driver" is a bit of a worry. Part of being a Maintainer
> > is to ensure that all drivers behave the same. There should not be any
> > difference between an Intel PTP device, a Marvell PTP devices, a
> > Microchip PTP device etc. They should all implement the API in a
> > uniform way.
> 
> Yes, and I appreciate it being on CC even for driver changes.
> 
> > What i have also seen is that if one driver gets something wrong,
> > other drivers might as well.
> 
> Yeah, unfortunately there are many device drivers (not just PTP/network drivers)
> that get things wrong.  These are then copied by the authors of new drivers, and
> so on.
> 
> Thanks,
> Richard

Sure, thanks! Will keep in mind for the future : )
Thanks, 
Przmek