[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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