[PATCH] net: stmmac: Fix PTP ref clock for Tegra234

[PATCH] net: stmmac: Fix PTP ref clock for Tegra234

[Jon Hunter]

Since commit 030ce919e114 ("net: stmmac: make sure that ptp_rate is not
0 before configuring timestamping") was added the following error is
observed on Tegra234:

 ERR KERN tegra-mgbe 6800000.ethernet eth0: Invalid PTP clock rate
 WARNING KERN tegra-mgbe 6800000.ethernet eth0: PTP init failed

It turns out that the Tegra234 device-tree binding defines the PTP ref
clock name as 'ptp-ref' and not 'ptp_ref' and the above commit now
exposes this and that the PTP clock is not configured correctly.

Ideally, we would rename the PTP ref clock for Tegra234 to fix this but
this will break backward compatibility with existing device-tree blobs.
Therefore, fix this by using the name 'ptp-ref' for devices that are
compatible with 'nvidia,tegra234-mgbe'.

Fixes: d8ca113724e7 ("net: stmmac: tegra: Add MGBE support")
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index b80c1efdb323..f82a7d55ea0a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -635,8 +635,12 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 	}
 	clk_prepare_enable(plat->pclk);
 
+	if (of_device_is_compatible(np, "nvidia,tegra234-mgbe"))
+		plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp-ref");
+	else
+		plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp_ref");
+
 	/* Fall-back to main clock in case of no PTP ref is passed */
-	plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp_ref");
 	if (IS_ERR(plat->clk_ptp_ref)) {
 		plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk);
 		plat->clk_ptp_ref = NULL;
-- 
2.43.0

Re: [PATCH] net: stmmac: Fix PTP ref clock for Tegra234

[Subbaraya Sundeep]

Hi,

On 2025-06-12 at 06:20:32, Jon Hunter (jonathanh@nvidia.com) wrote:
> Since commit 030ce919e114 ("net: stmmac: make sure that ptp_rate is not
> 0 before configuring timestamping") was added the following error is
> observed on Tegra234:
> 
>  ERR KERN tegra-mgbe 6800000.ethernet eth0: Invalid PTP clock rate
>  WARNING KERN tegra-mgbe 6800000.ethernet eth0: PTP init failed
> 
> It turns out that the Tegra234 device-tree binding defines the PTP ref
> clock name as 'ptp-ref' and not 'ptp_ref' and the above commit now
> exposes this and that the PTP clock is not configured correctly.
> 
> Ideally, we would rename the PTP ref clock for Tegra234 to fix this but
> this will break backward compatibility with existing device-tree blobs.
> Therefore, fix this by using the name 'ptp-ref' for devices that are
> compatible with 'nvidia,tegra234-mgbe'.
AFAIU for Tegra234 device from the beginning, entry in dts is ptp-ref.
Since driver is looking for ptp_ref it is getting 0 hence the crash
and after the commit 030ce919e114 result is Invalid error instead of crash.
For me PTP is not working for Tegra234 from day 1 so why to bother about
backward compatibility and instead fix dts.
Please help me understand it has been years I worked on dts.

Thanks,
Sundeep
> 
> Fixes: d8ca113724e7 ("net: stmmac: tegra: Add MGBE support")
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index b80c1efdb323..f82a7d55ea0a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -635,8 +635,12 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
>  	}
>  	clk_prepare_enable(plat->pclk);
>  
> +	if (of_device_is_compatible(np, "nvidia,tegra234-mgbe"))
> +		plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp-ref");
> +	else
> +		plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp_ref");
> +
>  	/* Fall-back to main clock in case of no PTP ref is passed */
> -	plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp_ref");
>  	if (IS_ERR(plat->clk_ptp_ref)) {
>  		plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk);
>  		plat->clk_ptp_ref = NULL;
> -- 
> 2.43.0
> 

Re: [PATCH] net: stmmac: Fix PTP ref clock for Tegra234

[Andrew Lunn]

On Thu, Jun 12, 2025 at 10:57:49AM +0000, Subbaraya Sundeep wrote:
> Hi,
> 
> On 2025-06-12 at 06:20:32, Jon Hunter (jonathanh@nvidia.com) wrote:
> > Since commit 030ce919e114 ("net: stmmac: make sure that ptp_rate is not
> > 0 before configuring timestamping") was added the following error is
> > observed on Tegra234:
> > 
> >  ERR KERN tegra-mgbe 6800000.ethernet eth0: Invalid PTP clock rate
> >  WARNING KERN tegra-mgbe 6800000.ethernet eth0: PTP init failed
> > 
> > It turns out that the Tegra234 device-tree binding defines the PTP ref
> > clock name as 'ptp-ref' and not 'ptp_ref' and the above commit now
> > exposes this and that the PTP clock is not configured correctly.
> > 
> > Ideally, we would rename the PTP ref clock for Tegra234 to fix this but
> > this will break backward compatibility with existing device-tree blobs.
> > Therefore, fix this by using the name 'ptp-ref' for devices that are
> > compatible with 'nvidia,tegra234-mgbe'.

> AFAIU for Tegra234 device from the beginning, entry in dts is ptp-ref.
> Since driver is looking for ptp_ref it is getting 0 hence the crash
> and after the commit 030ce919e114 result is Invalid error instead of crash.
> For me PTP is not working for Tegra234 from day 1 so why to bother about
> backward compatibility and instead fix dts.
> Please help me understand it has been years I worked on dts.

Please could you expand on that, because when i look at the code....


  	/* Fall-back to main clock in case of no PTP ref is passed */
 	plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp_ref");
  	if (IS_ERR(plat->clk_ptp_ref)) {
  		plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk);
  		plat->clk_ptp_ref = NULL;

if the ptp_ref does not exist, it falls back to stmmac_clk. Why would
that cause a crash?

While i agree if this never worked, we can ignore backwards
compatibility and just fix the DT, but i would like a fuller
explanation why the fallback is not sufficient to prevent a crash.

	Andrew

Re: [PATCH] net: stmmac: Fix PTP ref clock for Tegra234

[Jon Hunter]

On 12/06/2025 13:10, Andrew Lunn wrote:
> On Thu, Jun 12, 2025 at 10:57:49AM +0000, Subbaraya Sundeep wrote:
>> Hi,
>>
>> On 2025-06-12 at 06:20:32, Jon Hunter (jonathanh@nvidia.com) wrote:
>>> Since commit 030ce919e114 ("net: stmmac: make sure that ptp_rate is not
>>> 0 before configuring timestamping") was added the following error is
>>> observed on Tegra234:
>>>
>>>   ERR KERN tegra-mgbe 6800000.ethernet eth0: Invalid PTP clock rate
>>>   WARNING KERN tegra-mgbe 6800000.ethernet eth0: PTP init failed
>>>
>>> It turns out that the Tegra234 device-tree binding defines the PTP ref
>>> clock name as 'ptp-ref' and not 'ptp_ref' and the above commit now
>>> exposes this and that the PTP clock is not configured correctly.
>>>
>>> Ideally, we would rename the PTP ref clock for Tegra234 to fix this but
>>> this will break backward compatibility with existing device-tree blobs.
>>> Therefore, fix this by using the name 'ptp-ref' for devices that are
>>> compatible with 'nvidia,tegra234-mgbe'.
> 
>> AFAIU for Tegra234 device from the beginning, entry in dts is ptp-ref.
>> Since driver is looking for ptp_ref it is getting 0 hence the crash
>> and after the commit 030ce919e114 result is Invalid error instead of crash.
>> For me PTP is not working for Tegra234 from day 1 so why to bother about
>> backward compatibility and instead fix dts.
>> Please help me understand it has been years I worked on dts.
> 
> Please could you expand on that, because when i look at the code....
> 
> 
>    	/* Fall-back to main clock in case of no PTP ref is passed */
>   	plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp_ref");
>    	if (IS_ERR(plat->clk_ptp_ref)) {
>    		plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk);
>    		plat->clk_ptp_ref = NULL;
> 
> if the ptp_ref does not exist, it falls back to stmmac_clk. Why would
> that cause a crash?
 >  > While i agree if this never worked, we can ignore backwards
> compatibility and just fix the DT, but i would like a fuller
> explanation why the fallback is not sufficient to prevent a crash.

The problem is that in the 'ptp-ref' clock name is also defined in the 
'mgbe_clks' array in dwmac-tegra.c driver. All of these clocks are 
requested and enabled using the clk_bulk_xxx APIs and so I don't see how 
we can simply fix this now without breaking support for older device-trees.

Jon

-- 
nvpublic

Re: [PATCH] net: stmmac: Fix PTP ref clock for Tegra234

[Andrew Lunn]

On Thu, Jun 12, 2025 at 01:26:55PM +0100, Jon Hunter wrote:
> 
> On 12/06/2025 13:10, Andrew Lunn wrote:
> > On Thu, Jun 12, 2025 at 10:57:49AM +0000, Subbaraya Sundeep wrote:
> > > Hi,
> > > 
> > > On 2025-06-12 at 06:20:32, Jon Hunter (jonathanh@nvidia.com) wrote:
> > > > Since commit 030ce919e114 ("net: stmmac: make sure that ptp_rate is not
> > > > 0 before configuring timestamping") was added the following error is
> > > > observed on Tegra234:
> > > > 
> > > >   ERR KERN tegra-mgbe 6800000.ethernet eth0: Invalid PTP clock rate
> > > >   WARNING KERN tegra-mgbe 6800000.ethernet eth0: PTP init failed
> > > > 
> > > > It turns out that the Tegra234 device-tree binding defines the PTP ref
> > > > clock name as 'ptp-ref' and not 'ptp_ref' and the above commit now
> > > > exposes this and that the PTP clock is not configured correctly.
> > > > 
> > > > Ideally, we would rename the PTP ref clock for Tegra234 to fix this but
> > > > this will break backward compatibility with existing device-tree blobs.
> > > > Therefore, fix this by using the name 'ptp-ref' for devices that are
> > > > compatible with 'nvidia,tegra234-mgbe'.
> > 
> > > AFAIU for Tegra234 device from the beginning, entry in dts is ptp-ref.
> > > Since driver is looking for ptp_ref it is getting 0 hence the crash
> > > and after the commit 030ce919e114 result is Invalid error instead of crash.
> > > For me PTP is not working for Tegra234 from day 1 so why to bother about
> > > backward compatibility and instead fix dts.
> > > Please help me understand it has been years I worked on dts.
> > 
> > Please could you expand on that, because when i look at the code....
> > 
> > 
> >    	/* Fall-back to main clock in case of no PTP ref is passed */
> >   	plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp_ref");
> >    	if (IS_ERR(plat->clk_ptp_ref)) {
> >    		plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk);
> >    		plat->clk_ptp_ref = NULL;
> > 
> > if the ptp_ref does not exist, it falls back to stmmac_clk. Why would
> > that cause a crash?
> >  > While i agree if this never worked, we can ignore backwards
> > compatibility and just fix the DT, but i would like a fuller
> > explanation why the fallback is not sufficient to prevent a crash.
> 
> The problem is that in the 'ptp-ref' clock name is also defined in the
> 'mgbe_clks' array in dwmac-tegra.c driver. All of these clocks are requested
> and enabled using the clk_bulk_xxx APIs and so I don't see how we can simply
> fix this now without breaking support for older device-trees.

So you can definitively say, PTP does actually work? You have ptp4l
running with older kernels and DT blob, and it has sync to a grand
master?

	Andrew

Re: [PATCH] net: stmmac: Fix PTP ref clock for Tegra234

[Jon Hunter]

On 12/06/2025 13:45, Andrew Lunn wrote:
> On Thu, Jun 12, 2025 at 01:26:55PM +0100, Jon Hunter wrote:
>>
>> On 12/06/2025 13:10, Andrew Lunn wrote:
>>> On Thu, Jun 12, 2025 at 10:57:49AM +0000, Subbaraya Sundeep wrote:
>>>> Hi,
>>>>
>>>> On 2025-06-12 at 06:20:32, Jon Hunter (jonathanh@nvidia.com) wrote:
>>>>> Since commit 030ce919e114 ("net: stmmac: make sure that ptp_rate is not
>>>>> 0 before configuring timestamping") was added the following error is
>>>>> observed on Tegra234:
>>>>>
>>>>>    ERR KERN tegra-mgbe 6800000.ethernet eth0: Invalid PTP clock rate
>>>>>    WARNING KERN tegra-mgbe 6800000.ethernet eth0: PTP init failed
>>>>>
>>>>> It turns out that the Tegra234 device-tree binding defines the PTP ref
>>>>> clock name as 'ptp-ref' and not 'ptp_ref' and the above commit now
>>>>> exposes this and that the PTP clock is not configured correctly.
>>>>>
>>>>> Ideally, we would rename the PTP ref clock for Tegra234 to fix this but
>>>>> this will break backward compatibility with existing device-tree blobs.
>>>>> Therefore, fix this by using the name 'ptp-ref' for devices that are
>>>>> compatible with 'nvidia,tegra234-mgbe'.
>>>
>>>> AFAIU for Tegra234 device from the beginning, entry in dts is ptp-ref.
>>>> Since driver is looking for ptp_ref it is getting 0 hence the crash
>>>> and after the commit 030ce919e114 result is Invalid error instead of crash.
>>>> For me PTP is not working for Tegra234 from day 1 so why to bother about
>>>> backward compatibility and instead fix dts.
>>>> Please help me understand it has been years I worked on dts.
>>>
>>> Please could you expand on that, because when i look at the code....
>>>
>>>
>>>     	/* Fall-back to main clock in case of no PTP ref is passed */
>>>    	plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp_ref");
>>>     	if (IS_ERR(plat->clk_ptp_ref)) {
>>>     		plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk);
>>>     		plat->clk_ptp_ref = NULL;
>>>
>>> if the ptp_ref does not exist, it falls back to stmmac_clk. Why would
>>> that cause a crash?
>>>   > While i agree if this never worked, we can ignore backwards
>>> compatibility and just fix the DT, but i would like a fuller
>>> explanation why the fallback is not sufficient to prevent a crash.
>>
>> The problem is that in the 'ptp-ref' clock name is also defined in the
>> 'mgbe_clks' array in dwmac-tegra.c driver. All of these clocks are requested
>> and enabled using the clk_bulk_xxx APIs and so I don't see how we can simply
>> fix this now without breaking support for older device-trees.
> 
> So you can definitively say, PTP does actually work? You have ptp4l
> running with older kernels and DT blob, and it has sync to a grand
> master?

So no I can't say that and I have not done any testing with PTP to be 
clear. However, the problem I see, is that because the driver defines 
the name as 'ptp-ref', if we were to update both the device-tree and the 
driver now to use the expected name 'ptp_ref', then and older 
device-tree will no longer work with the new driver regardless of the 
PTP because the devm_clk_bulk_get() in tegra_mgbe_probe() will fail.

I guess we could check to see if 'ptp-ref' or 'ptp_ref' is present 
during the tegra_mgbe_probe() and then update the mgbe_clks array as 
necessary.

Jon

-- 
nvpublic

Re: [PATCH] net: stmmac: Fix PTP ref clock for Tegra234

[Andrew Lunn]

> > So you can definitively say, PTP does actually work? You have ptp4l
> > running with older kernels and DT blob, and it has sync to a grand
> > master?
> 
> So no I can't say that and I have not done any testing with PTP to be clear.
> However, the problem I see, is that because the driver defines the name as
> 'ptp-ref', if we were to update both the device-tree and the driver now to
> use the expected name 'ptp_ref', then and older device-tree will no longer
> work with the new driver regardless of the PTP because the
> devm_clk_bulk_get() in tegra_mgbe_probe() will fail.
> 
> I guess we could check to see if 'ptp-ref' or 'ptp_ref' is present during
> the tegra_mgbe_probe() and then update the mgbe_clks array as necessary.

Lets just consider for the moment, that it never worked.

If we change the device tree to the expected 'ptp_ref', some devices
actually start working. None regress, because none ever worked. We can
also get the DT change added to stable, so older devices start
working. We keep the code nice and clean, no special case.

Now, lets consider the case some devices do actually work. How are
they working? Must it be the fallback? The ptp-ref clock is actually
turned on, and if the ptp-ref clock and the main clock tick at the
same rate, ptp would work. I _guess_, if the main clock and the
ptp-ref clock tick at different rates, you get something from the ptp
hardware, but it probably does not get sync with a grand master, or if
it does, the jitter is high etc. So in effect it is still broken.

Can somebody with the datasheet actually determine where ptp-ref clock
comes from? Is it just a gated main clock? Is it from a pin?

If it does actually work, can we cause a regression by renaming the
clock in DT? I _guess_ so, if the DT also has the clock wrong. So it
is a fixed-clock, and that fixed clock has the wrong frequency set. It
is not used at the moment, so being wrong does not matter. But when we
start using it, things break. Is this possible? I don't know, i've not
looked at the DT.

Before we decide how to fix this, we need a proper understanding of
what is actually broken/works.

	Andrew

Re: [PATCH] net: stmmac: Fix PTP ref clock for Tegra234

[Jon Hunter]

On 13/06/2025 14:22, Andrew Lunn wrote:
>>> So you can definitively say, PTP does actually work? You have ptp4l
>>> running with older kernels and DT blob, and it has sync to a grand
>>> master?
>>
>> So no I can't say that and I have not done any testing with PTP to be clear.
>> However, the problem I see, is that because the driver defines the name as
>> 'ptp-ref', if we were to update both the device-tree and the driver now to
>> use the expected name 'ptp_ref', then and older device-tree will no longer
>> work with the new driver regardless of the PTP because the
>> devm_clk_bulk_get() in tegra_mgbe_probe() will fail.
>>
>> I guess we could check to see if 'ptp-ref' or 'ptp_ref' is present during
>> the tegra_mgbe_probe() and then update the mgbe_clks array as necessary.
> 
> Lets just consider for the moment, that it never worked.

To be clear, by 'it never worked', you are referring to only PTP 
support? Then yes that is most likely.

> If we change the device tree to the expected 'ptp_ref', some devices
> actually start working. None regress, because none ever worked. We can
> also get the DT change added to stable, so older devices start
> working. We keep the code nice and clean, no special case.

Although PTP may not work, basic ethernet support does and 'correcting' 
the device-tree only, will break basic ethernet support for this device. 
That is what I am more concerned about and trying to avoid.

> Now, lets consider the case some devices do actually work. How are
> they working? Must it be the fallback? The ptp-ref clock is actually
> turned on, and if the ptp-ref clock and the main clock tick at the
> same rate, ptp would work. I _guess_, if the main clock and the
> ptp-ref clock tick at different rates, you get something from the ptp
> hardware, but it probably does not get sync with a grand master, or if
> it does, the jitter is high etc. So in effect it is still broken.

Given that we are seeing the error ...

  ERR KERN tegra-mgbe 6800000.ethernet eth0: Invalid PTP clock rate

Doesn't that imply that if we did attempt to use PTP on this device we 
would hit the bug reported by commit 030ce919e114 ("net: stmmac: make 
sure that ptp_rate is not 0 before configuring timestamping") and 
therefore, I would not expect PTP to work?

> 
> Can somebody with the datasheet actually determine where ptp-ref clock
> comes from? Is it just a gated main clock? Is it from a pin?

I can ask.

> If it does actually work, can we cause a regression by renaming the
> clock in DT? I _guess_ so, if the DT also has the clock wrong. So it
> is a fixed-clock, and that fixed clock has the wrong frequency set. It
> is not used at the moment, so being wrong does not matter. But when we
> start using it, things break. Is this possible? I don't know, i've not
> looked at the DT.
> 
> Before we decide how to fix this, we need a proper understanding of
> what is actually broken/works.

Yes that makes sense. Ethernet definitely works. I am not sure we have 
ever explicitly tested PTP with this driver, but I can ask.

Jon

-- 
nvpublic

Re: [PATCH] net: stmmac: Fix PTP ref clock for Tegra234

[Andrew Lunn]

On Mon, Jun 16, 2025 at 11:06:54AM +0100, Jon Hunter wrote:
> 
> On 13/06/2025 14:22, Andrew Lunn wrote:
> > > > So you can definitively say, PTP does actually work? You have ptp4l
> > > > running with older kernels and DT blob, and it has sync to a grand
> > > > master?
> > > 
> > > So no I can't say that and I have not done any testing with PTP to be clear.
> > > However, the problem I see, is that because the driver defines the name as
> > > 'ptp-ref', if we were to update both the device-tree and the driver now to
> > > use the expected name 'ptp_ref', then and older device-tree will no longer
> > > work with the new driver regardless of the PTP because the
> > > devm_clk_bulk_get() in tegra_mgbe_probe() will fail.
> > > 
> > > I guess we could check to see if 'ptp-ref' or 'ptp_ref' is present during
> > > the tegra_mgbe_probe() and then update the mgbe_clks array as necessary.
> > 
> > Lets just consider for the moment, that it never worked.
> 
> To be clear, by 'it never worked', you are referring to only PTP support?
> Then yes that is most likely.

Yes, i'm just referring to PTP. I would be very surprised if
sending/receiving Ethernet frames is broken, that gets lots of
testing.

> > If we change the device tree to the expected 'ptp_ref', some devices
> > actually start working. None regress, because none ever worked. We can
> > also get the DT change added to stable, so older devices start
> > working. We keep the code nice and clean, no special case.
> 
> Although PTP may not work, basic ethernet support does and 'correcting' the
> device-tree only, will break basic ethernet support for this device.

We clearly don't want to do that. But we should be able to come up
with a fix which does not make things worse. The obvious one is that
we have both ptp-ref and ptp_ref in tegra234.dtsi, so that both
mgbe_clks in dwmac-tegra.c and stmmac_probe_config_dt is happy.

	Andrew

Re: [PATCH] net: stmmac: Fix PTP ref clock for Tegra234

[Jon Hunter]

On 13/06/2025 14:22, Andrew Lunn wrote:
>>> So you can definitively say, PTP does actually work? You have ptp4l
>>> running with older kernels and DT blob, and it has sync to a grand
>>> master?
>>
>> So no I can't say that and I have not done any testing with PTP to be clear.
>> However, the problem I see, is that because the driver defines the name as
>> 'ptp-ref', if we were to update both the device-tree and the driver now to
>> use the expected name 'ptp_ref', then and older device-tree will no longer
>> work with the new driver regardless of the PTP because the
>> devm_clk_bulk_get() in tegra_mgbe_probe() will fail.
>>
>> I guess we could check to see if 'ptp-ref' or 'ptp_ref' is present during
>> the tegra_mgbe_probe() and then update the mgbe_clks array as necessary.
> 
> Lets just consider for the moment, that it never worked.
> 
> If we change the device tree to the expected 'ptp_ref', some devices
> actually start working. None regress, because none ever worked. We can
> also get the DT change added to stable, so older devices start
> working. We keep the code nice and clean, no special case.
> 
> Now, lets consider the case some devices do actually work. How are
> they working? Must it be the fallback? The ptp-ref clock is actually
> turned on, and if the ptp-ref clock and the main clock tick at the
> same rate, ptp would work. I _guess_, if the main clock and the
> ptp-ref clock tick at different rates, you get something from the ptp
> hardware, but it probably does not get sync with a grand master, or if
> it does, the jitter is high etc. So in effect it is still broken.
> 
> Can somebody with the datasheet actually determine where ptp-ref clock
> comes from? Is it just a gated main clock? Is it from a pin?

Looking at the datasheet, this is a pin to the controller and sourced 
from an external clock.

> If it does actually work, can we cause a regression by renaming the
> clock in DT? I _guess_ so, if the DT also has the clock wrong. So it
> is a fixed-clock, and that fixed clock has the wrong frequency set. It
> is not used at the moment, so being wrong does not matter. But when we
> start using it, things break. Is this possible? I don't know, i've not
> looked at the DT.
> 
> Before we decide how to fix this, we need a proper understanding of
> what is actually broken/works.

AFAIK we have never tested PTP with this driver on this device. So the 
risk of breaking something is low for this device.

Jon

-- 
nvpublic

Re: [PATCH] net: stmmac: Fix PTP ref clock for Tegra234

[Jon Hunter]

On 16/06/2025 14:06, Andrew Lunn wrote:
> On Mon, Jun 16, 2025 at 11:06:54AM +0100, Jon Hunter wrote:
>>
>> On 13/06/2025 14:22, Andrew Lunn wrote:
>>>>> So you can definitively say, PTP does actually work? You have ptp4l
>>>>> running with older kernels and DT blob, and it has sync to a grand
>>>>> master?
>>>>
>>>> So no I can't say that and I have not done any testing with PTP to be clear.
>>>> However, the problem I see, is that because the driver defines the name as
>>>> 'ptp-ref', if we were to update both the device-tree and the driver now to
>>>> use the expected name 'ptp_ref', then and older device-tree will no longer
>>>> work with the new driver regardless of the PTP because the
>>>> devm_clk_bulk_get() in tegra_mgbe_probe() will fail.
>>>>
>>>> I guess we could check to see if 'ptp-ref' or 'ptp_ref' is present during
>>>> the tegra_mgbe_probe() and then update the mgbe_clks array as necessary.
>>>
>>> Lets just consider for the moment, that it never worked.
>>
>> To be clear, by 'it never worked', you are referring to only PTP support?
>> Then yes that is most likely.
> 
> Yes, i'm just referring to PTP. I would be very surprised if
> sending/receiving Ethernet frames is broken, that gets lots of
> testing.
> 
>>> If we change the device tree to the expected 'ptp_ref', some devices
>>> actually start working. None regress, because none ever worked. We can
>>> also get the DT change added to stable, so older devices start
>>> working. We keep the code nice and clean, no special case.
>>
>> Although PTP may not work, basic ethernet support does and 'correcting' the
>> device-tree only, will break basic ethernet support for this device.
> 
> We clearly don't want to do that. But we should be able to come up
> with a fix which does not make things worse. The obvious one is that
> we have both ptp-ref and ptp_ref in tegra234.dtsi, so that both
> mgbe_clks in dwmac-tegra.c and stmmac_probe_config_dt is happy.

Yes that's a possibility. OK I will have a think about this some more.

Jon

-- 
nvpublic

Re: [PATCH] net: stmmac: Fix PTP ref clock for Tegra234

[Andrew Lunn]

> > Now, lets consider the case some devices do actually work. How are
> > they working? Must it be the fallback? The ptp-ref clock is actually
> > turned on, and if the ptp-ref clock and the main clock tick at the
> > same rate, ptp would work. I _guess_, if the main clock and the
> > ptp-ref clock tick at different rates, you get something from the ptp
> > hardware, but it probably does not get sync with a grand master, or if
> > it does, the jitter is high etc. So in effect it is still broken.
> > 
> > Can somebody with the datasheet actually determine where ptp-ref clock
> > comes from? Is it just a gated main clock? Is it from a pin?
> 
> Looking at the datasheet, this is a pin to the controller and sourced from
> an external clock.

So the fallback of the main clock is not likely to help, unless by
chance the external clock and the main clock happen to be the same
frequency.

> AFAIK we have never tested PTP with this driver on this device. So the risk
> of breaking something is low for this device.

So it seems like the simple fix is to list both ptp-ref and ptp_ref,
pointing to the same clock, along with a comment explaining why you
have this odd construction.

Please could you test that?

	Andrew

Re: [PATCH] net: stmmac: Fix PTP ref clock for Tegra234

[Jon Hunter]

On 25/06/2025 17:24, Andrew Lunn wrote:
>>> Now, lets consider the case some devices do actually work. How are
>>> they working? Must it be the fallback? The ptp-ref clock is actually
>>> turned on, and if the ptp-ref clock and the main clock tick at the
>>> same rate, ptp would work. I _guess_, if the main clock and the
>>> ptp-ref clock tick at different rates, you get something from the ptp
>>> hardware, but it probably does not get sync with a grand master, or if
>>> it does, the jitter is high etc. So in effect it is still broken.
>>>
>>> Can somebody with the datasheet actually determine where ptp-ref clock
>>> comes from? Is it just a gated main clock? Is it from a pin?
>>
>> Looking at the datasheet, this is a pin to the controller and sourced from
>> an external clock.
> 
> So the fallback of the main clock is not likely to help, unless by
> chance the external clock and the main clock happen to be the same
> frequency.

Actually the fallback to the main clock is not applicable here for this 
Tegra device because we don't have a clock called 'stmmaceth'. In fact, 
that is something else we should fix because for Tegra234, I see ...

  tegra-mgbe 6800000.ethernet: Cannot get CSR clock

However, this is expected because there is no 'stmmaceth'. The fallback 
to the main clock is only applicable to !snps,dwc-qos-ethernet-4.10 
devices but this should also be applicable to 'nvidia,tegra234-mgbe' as 
well.

>> AFAIK we have never tested PTP with this driver on this device. So the risk
>> of breaking something is low for this device.
> 
> So it seems like the simple fix is to list both ptp-ref and ptp_ref,
> pointing to the same clock, along with a comment explaining why you
> have this odd construction.
> 
> Please could you test that?

I am sure it will work and yes I can test. I am a bit concerned that the 
device-tree maintainers will not want this because strictly device-tree 
should describe the hardware and adding workarounds is not ideal. Don't 
get me wrong this is a massive oversight on our behalf to begin with. 
Ideally we would handle this somewhere in the dwmac-tegra.c driver.

Jon

-- 
nvpublic