Re: [PATCH net-next] 1588 support on bcm54210pe

[Florian Fainelli <f.fainelli@gmail.com>]

On 4/22/2022 12:48 PM, Richard Cochran wrote:
> On Fri, Apr 22, 2022 at 05:00:07PM +0200, Andrew Lunn wrote:
> 
>>> I am confident that this code is relevant exclusively to the
>>> BCM54210PE.
> 
> Not true.
> 
>> It will not even work with the BCM54210, BCM54210S and
>>> BCM54210SE PHYs.
> 
> The registers you used are also present in the BCM541xx devices.
> Pretty sure your code would work on those devices (after adjusting
> register offsets).
> 
>> Florian can probably tell us more, but often hardware like this is
>> shared by multiple devices. If it is, you might want to use a more
>> generic prefix.
> 
> My understanding is that there are two implementions, gen1 and gen2.
> Your bcm542xx and the bcm541xx are both gen1, and both support inband
> Rx time stamping.

That is correct. Lasse for your future submission please address the 
following:

- conform to the usual patch submission style and break up your changes 
between bcmgenet.c (although I doubt you need to change it), broadcom.c 
and bcm-phy-lib.[ch]

- do not create a PHY device entry specifically for BCM54210PE, use the 
existing BCM54210 entry and add checks using the revision field of 
phydev->phy_id where necessary. There are already many entries in this 
driver, adding more does not help maintaining it. Also, I went through 
several months of work fixing bugs and adding decent power management 
features to this driver that all PHYs should leverage, adding a new 
entry means we need to verify whether all code paths are hit or not

- move generic code, such as all of the PTP code into bcm-phy-lib.[ch] 
where it can easily be re-used across multiple PHY device driver entries 
(54810, 54210 etc.)

Thanks!

> 
> Because the registers are all the same (just the offsets are
> different), I'd like to see a common module that can be used by all
> gen1 devices.  The module could be named bcm-ptp-gen1.c for example.

I would prefer that we just stick to adding that code to 
bcm-phy-lib.[ch] which all Broadcom PHY drivers can use and we can 
decide whether we want to add a Kconfig option specifically for PTP.

Cheers
-- 
Florian