| Message ID | 20240130-rxc_bugfix-v2-2-5e6c3168e5f0@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Fix missing PHY-to-MAC RX clock | expand |
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.
Hello Russell, On Tue, 30 Jan 2024, Russell King (Oracle) wrote: ... > > +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... > ... > 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? If a PCS can set the needed clock signal without configuring the details of a particular link mode, then passing the interface mode to pcs_pre_init() would indeed not be relevant. Generally, I agree that setting the interface mode shouldn't be the concern of the pre-initialization function. I'll dig a bit more into the PCS datasheet and run more tests to see if I can get away with enabling the RX clock selectively for this particular PCS model. If not, then maybe I can hardcode a "default" interface mode for the pre-initialization that will not interfere with the rest of the link setup process. Best Regards,
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); + + return ret; +} + static void phylink_mac_config(struct phylink *pl, const struct phylink_link_state *state) { 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 @@ -396,6 +396,10 @@ struct phylink_pcs_ops; * @phylink: pointer to &struct phylink_config * @neg_mode: provide PCS neg mode via "mode" argument * @poll: poll the PCS for link changes + * @rxc_always_on: The MAC driver requires the reference clock + * to always be on. Standalone PCS drivers which + * do not have access to a PHY device can check + * this instead of PHY_F_RXC_ALWAYS_ON. * * This structure is designed to be embedded within the PCS private data, * and will be passed between phylink and the PCS. @@ -408,6 +412,7 @@ struct phylink_pcs { struct phylink *phylink; bool neg_mode; bool poll; + bool rxc_always_on; }; /** @@ -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. */ 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); }; #if 0 /* For kernel-doc purposes only. */ @@ -568,6 +577,8 @@ void phylink_disconnect_phy(struct phylink *); void phylink_mac_change(struct phylink *, bool up); void phylink_pcs_change(struct phylink_pcs *, bool up); +int phylink_pcs_pre_init(struct phylink *pl, struct phylink_pcs *pcs); + void phylink_start(struct phylink *); void phylink_stop(struct phylink *);
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(+)