Re: [PATCH net-next v2 2/7] net: phy: add rxc_always_on flag to phylink_pcs

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Jan 30, 2024 at 10:28:37AM +0100, Romain Gantois wrote:
> Some MAC drivers (e.g. stmmac) require a continuous receive clock signal to
> be generated by a PCS that is handled by a standalone PCS driver.
> 
> Such a PCS driver does not have access to a PHY device, thus cannot check
> the PHY_F_RXC_ALWAYS_ON flag. They cannot check max_requires_rxc in the
> phylink config either, since it is a private member. Therefore, a new flag
> is needed to signal to the PCS that it should keep the RX clock signal up
> at all times.
> 
> Suggested-by: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
> ---
>  drivers/net/phy/phylink.c | 14 ++++++++++++++
>  include/linux/phylink.h   | 11 +++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 851049096488..6fcc0a8ba122 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1042,6 +1042,20 @@ static void phylink_pcs_poll_start(struct phylink *pl)
>  		mod_timer(&pl->link_poll, jiffies + HZ);
>  }
>  
> +int phylink_pcs_pre_init(struct phylink *pl, struct phylink_pcs *pcs)
> +{
> +	int ret = 0;
> +
> +	/* Signal to PCS driver that MAC requires RX clock for init */
> +	if (pl->config->mac_requires_rxc)
> +		pcs->rxc_always_on = true;
> +
> +	if (pcs->ops->pcs_pre_init)
> +		ret = pcs->ops->pcs_pre_init(pcs, pl->link_config.interface);

Given that:
1) phylink supports switching between mutliple different interfaces,
2) from what I can see you are only calling this from stmmac's
   initialisation path,
3) you pass the interface mode to the PCS here

then we don't want the PCS to configure itself for the interface mode
passed in, because this function won't be called when the interface
mode changes - and PCS driver authors will have to bear that in mind.
So...

> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index fcee99632964..71e970271fd3 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -422,6 +427,8 @@ struct phylink_pcs {
>   * @pcs_an_restart: restart 802.3z BaseX autonegotiation.
>   * @pcs_link_up: program the PCS for the resolved link configuration
>   *               (where necessary).
> + * @pcs_pre_init: configure PCS components necessary for MAC hardware
> + *                initialization e.g. RX clock for stmmac.

This is fine as a short description.

>   */
>  struct phylink_pcs_ops {
>  	int (*pcs_validate)(struct phylink_pcs *pcs, unsigned long *supported,
> @@ -441,6 +448,8 @@ struct phylink_pcs_ops {
>  	void (*pcs_an_restart)(struct phylink_pcs *pcs);
>  	void (*pcs_link_up)(struct phylink_pcs *pcs, unsigned int neg_mode,
>  			    phy_interface_t interface, int speed, int duplex);
> +	int (*pcs_pre_init)(struct phylink_pcs *pcs,
> +			    phy_interface_t interface);

... I would prefer this to be called initial_interface to make it
clear that it's just the initial interface mode.

However, do we really need it - if the PCS is supplying the RXC to
the MAC, then is the interface mode between the PCS and PHY all that
relevant at this point?

Also, please note that this is poor documentation for this function
(encouraged by broken kernel doc not able to properly describe "ops"
structures). See further down in the #if 0..#endif block where each
and every function in this ops structure is fully documented. Please
do the same for any new functions added.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!