| Message ID | 20240103142827.168321-6-romain.gantois@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix missing PHY-to-MAC RX clock | expand |
On Wed, Jan 03, 2024 at 03:28:25PM +0100, Romain Gantois wrote: > The GMAC1 controller in the RZN1 IP requires the RX MII clock signal to be > started before it initializes its own hardware, thus before it calls > phylink_start. > > Check the rxc_always_on pcs flag and enable the clock signal during the > link validation phase. However, validation is *not* supposed to change the configuration of the hardware. Validation may fail. The "interface" that gets passed to validation may never ever be selected. This change feels like nothing more than a hack. Since the MAC driver has to itself provide the PCS to phylink via the mac_select_pcs() method, the MAC driver already has knowledge of which PCS it is going to be using. Therefore, I think it may make sense to do something like this: int phylink_pcs_preconfig(struct phylink *pl, struct phylink_pcs *pcs) { if (pl->config->mac_requires_rxc) pcs->rxc_always_on = true; if (pcs->ops->preconfig) pcs->ops->pcs_preconfig(pcs); } and have stmmac call phylink_pcs_preconfig() for each PCS that it will be using during initialisation / resume paths?
Hello Russel, On Wed, 3 Jan 2024, Russell King (Oracle) wrote: ... > Since the MAC driver has to itself provide the PCS to phylink via the > mac_select_pcs() method, the MAC driver already has knowledge of which > PCS it is going to be using. Therefore, I think it may make sense > to do something like this: > > int phylink_pcs_preconfig(struct phylink *pl, struct phylink_pcs *pcs) > { > if (pl->config->mac_requires_rxc) > pcs->rxc_always_on = true; > > if (pcs->ops->preconfig) > pcs->ops->pcs_preconfig(pcs); > } > > and have stmmac call phylink_pcs_preconfig() for each PCS that it will > be using during initialisation / resume paths? Yes, that is definitely a much cleaner solution. I'll reimplement the PCS changes in this way. Best Regards,
diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c index 97139c07130f..bf796491b826 100644 --- a/drivers/net/pcs/pcs-rzn1-miic.c +++ b/drivers/net/pcs/pcs-rzn1-miic.c @@ -271,12 +271,20 @@ static void miic_link_up(struct phylink_pcs *pcs, unsigned int mode, static int miic_validate(struct phylink_pcs *pcs, unsigned long *supported, const struct phylink_link_state *state) { - if (phy_interface_mode_is_rgmii(state->interface) || - state->interface == PHY_INTERFACE_MODE_RMII || - state->interface == PHY_INTERFACE_MODE_MII) - return 1; + int ret = 1; - return -EINVAL; + if (!phy_interface_mode_is_rgmii(state->interface) && + state->interface != PHY_INTERFACE_MODE_RMII && + state->interface != PHY_INTERFACE_MODE_MII) + return -EINVAL; + + if (pcs->rxc_always_on) { + ret = miic_config(pcs, 0, state->interface, NULL, false); + if (ret) + pr_err("Error: Failed to init RX clock in RZN1 MIIC PCS!"); + } + + return ret; } static const struct phylink_pcs_ops miic_phylink_ops = {
The GMAC1 controller in the RZN1 IP requires the RX MII clock signal to be started before it initializes its own hardware, thus before it calls phylink_start. Check the rxc_always_on pcs flag and enable the clock signal during the link validation phase. Reported-by: Clément Léger <clement.leger@bootlin.com> Link: https://lore.kernel.org/linux-arm-kernel/20230116103926.276869-4-clement.leger@bootlin.com/ Signed-off-by: Romain Gantois <romain.gantois@bootlin.com> --- drivers/net/pcs/pcs-rzn1-miic.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)