Message-ID: <7d51efd8-2789-4cd8-935f-ea5bdf2329f1@broadcom.com>
Date: Mon, 9 Oct 2023 15:23:58 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Köry Maincent <kory.maincent@...tlin.com>,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org
Cc: Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Jonathan Corbet <corbet@....net>,
 Jay Vosburgh <j.vosburgh@...il.com>, Andy Gospodarek <andy@...yhouse.net>,
 Nicolas Ferre <nicolas.ferre@...rochip.com>,
 Claudiu Beznea <claudiu.beznea@...on.dev>,
 Horatiu Vultur <horatiu.vultur@...rochip.com>, UNGLinuxDriver@...rochip.com,
 Broadcom internal kernel review list
 <bcm-kernel-feedback-list@...adcom.com>, Andrew Lunn <andrew@...n.ch>,
 Heiner Kallweit <hkallweit1@...il.com>, Russell King
 <linux@...linux.org.uk>, Richard Cochran <richardcochran@...il.com>,
 Radu Pirea <radu-nicolae.pirea@....nxp.com>,
 Willem de Bruijn <willemdebruijn.kernel@...il.com>,
 Vladimir Oltean <vladimir.oltean@....com>, Michael Walle <michael@...le.cc>,
 Jacob Keller <jacob.e.keller@...el.com>,
 Maxime Chevallier <maxime.chevallier@...tlin.com>
Subject: Re: [PATCH net-next v5 13/16] net: Change the API of PHY default
 timestamp to MAC

On 10/9/23 08:51, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@...tlin.com>
> 
> Change the API to select MAC default time stamping instead of the PHY.
> Indeed the PHY is closer to the wire therefore theoretically it have less
> delay than the MAC timestamping but the reality is different.

s/have/has/ or you need to make the subject plural.

> Due to lower
> time stamping clock frequency, latency in the MDIO bus and no PHC hardware
> synchronization between different PHY, the PHY PTP is often less precise
> than the MAC. 

-> different PHYs

> The exception is for PHY designed specially for PTP case but
> these board are not very widespread. 

s/board/devices/ maybe?

> For not breaking the compatibility I
> introduce an allowlist to reference all current PHYs that support
> time stamping and let them keep the old API behavior.
> 
> The phy_set_timestamp function is called at each call of phy_attach_direct.
> In case of MAC driver using phylink this function is called when the
> interface is turned up. Then if the interface goes down and up again the
> last choice of timestamp will be overwritten by the default choice.
> A solution could be to cache the timestamp status but it can bring other
> issues. In case of SFP, if we change the module, it doesn't make sense to
> blindly re-set the timestamp back to PHY, if the new module has a PHY with
> mediocre timestamping capabilities.
> 
> Signed-off-by: Kory Maincent <kory.maincent@...tlin.com>
> ---
> 
> Changes in v5:
> - Extract the API change in this patch.
> - Rename whitelist to allowlist.
> - Set NETDEV_TIMESTAMPING in register_netdevice function.
> - Add software timestamping case description in ts_info.
> ---
>   drivers/net/phy/phy_device.c | 68 ++++++++++++++++++++++++++++++++++++
>   include/linux/netdevice.h    |  5 +++
>   net/core/dev.c               |  3 ++
>   net/core/dev_ioctl.c         | 36 +++++++++++--------
>   net/core/timestamping.c      |  9 +++++
>   net/ethtool/common.c         | 16 +++++++--
>   6 files changed, 121 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 2ce74593d6e4..2d5a6d57acb3 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1411,6 +1411,68 @@ int phy_sfp_probe(struct phy_device *phydev,
>   }
>   EXPORT_SYMBOL(phy_sfp_probe);
>   
> +/* An allowlist for PHYs selected as default timesetamping.
> + * Its use is to keep compatibility with old PTP API which is selecting
> + * these PHYs as default timestamping.
> + * The new API is selecting the MAC as default timestamping.
> + */
> +const char * const phy_timestamping_allowlist[] = {
> +	"Broadcom BCM5411",
> +	"Broadcom BCM5421",
> +	"Broadcom BCM54210E",
> +	"Broadcom BCM5461",
> +	"Broadcom BCM54612E",
> +	"Broadcom BCM5464",
> +	"Broadcom BCM5481",
> +	"Broadcom BCM54810",
> +	"Broadcom BCM54811",
> +	"Broadcom BCM5482",
> +	"Broadcom BCM50610",
> +	"Broadcom BCM50610M",
> +	"Broadcom BCM57780",
> +	"Broadcom BCM5395",
> +	"Broadcom BCM53125",
> +	"Broadcom BCM53128",
> +	"Broadcom BCM89610",

The list of Broadcom PHYs that you have is too broad, if you look at 
drivers/net/phy/bcm-phy-ptp.c only PHY_ID_BCM54210E is actually 
supported as of now.

The allowlist is not maintainable using the PHY device name, especially 
not without having a shared header file that is used by both 
phy_device.c and the individual PHY device driver. You are guaranteed 
that a name change on one side can be missed on the other.

Using PHY OUIs can be a tad complicated with C45 PHYs, so rather, I 
would suggest that each of those PHY device drivers be responsible for 
overidding the timestamping selection, you could even consider inventing 
a specific PHYLIB_TIMESTAMPING_LEGACY and then issue a warning within 
the PHY library to encourage a change (if this is even relevant).

>   #endif
> +
> +	u32			ts_layer;

Likewise, would be preferable to use an enum type.
-- 
Florian


Download attachment "smime.p7s" of type "application/pkcs7-signature" (4221 bytes)

Powered by blists - more mailing lists