| Message ID | 20240212-rxc_bugfix-v3-7-e9f2eb6b3b05@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Fix missing PHY-to-MAC RX clock | expand |
Hi Romain, On Mon, 12 Feb 2024 17:42:14 +0100 Romain Gantois <romain.gantois@bootlin.com> 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. It looks like this commit log doesn't match the content of the commit. > 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 | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c > index d93f84fbb1fd..b0d9736c678e 100644 > --- a/drivers/net/pcs/pcs-rzn1-miic.c > +++ b/drivers/net/pcs/pcs-rzn1-miic.c > @@ -279,10 +279,37 @@ static int miic_validate(struct phylink_pcs *pcs, unsigned long *supported, > return -EINVAL; > } > > +static int miic_pre_init(struct phylink_pcs *pcs) > +{ > + struct miic_port *miic_port = phylink_pcs_to_miic_port(pcs); > + struct miic *miic = miic_port->miic; > + u32 val; > + > + /* Start RX clock if required */ > + if (pcs->rxc_always_on) { > + /* In MII through mode, the clock signals will be driven by the > + * external PHY, which might not be initialized yet. Set RMII > + * as default mode to ensure that a reference clock signal is > + * generated. > + */ > + miic_port->interface = PHY_INTERFACE_MODE_RMII; There's this check in miic_config : if (interface != miic_port->interface) { val |= FIELD_PREP(MIIC_CONVCTRL_CONV_SPEED, speed); mask |= MIIC_CONVCTRL_CONV_SPEED; miic_port->interface = interface; } As you set the interface to RMII and set the CONV_MODE below without really looking at the speed, is there any risk of a mismatch between the configured mode and the speed ? Thanks, Maxime
Hi Maxime, On Mon, 12 Feb 2024, Maxime Chevallier wrote: > > +static int miic_pre_init(struct phylink_pcs *pcs) > > +{ > > + struct miic_port *miic_port = phylink_pcs_to_miic_port(pcs); > > + struct miic *miic = miic_port->miic; > > + u32 val; > > + > > + /* Start RX clock if required */ > > + if (pcs->rxc_always_on) { > > + /* In MII through mode, the clock signals will be driven by the > > + * external PHY, which might not be initialized yet. Set RMII > > + * as default mode to ensure that a reference clock signal is > > + * generated. > > + */ > > + miic_port->interface = PHY_INTERFACE_MODE_RMII; > > There's this check in miic_config : > > if (interface != miic_port->interface) { > val |= FIELD_PREP(MIIC_CONVCTRL_CONV_SPEED, speed); > mask |= MIIC_CONVCTRL_CONV_SPEED; > miic_port->interface = interface; > } > > As you set the interface to RMII and set the CONV_MODE below without > really looking at the speed, is there any risk of a mismatch between > the configured mode and the speed ? Good point, it is probably necessary to set the default RMII speed in miic_pre_init(), since miic_config will not do it if the link mode hasn't changed in the meantime. However, this is only an issue if the link isn't already up when miic_config() is called. If the link is up, then that means that miic_link_up() has already been called and has set the appropriate speed anyway. Thanks,
diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c index d93f84fbb1fd..b0d9736c678e 100644 --- a/drivers/net/pcs/pcs-rzn1-miic.c +++ b/drivers/net/pcs/pcs-rzn1-miic.c @@ -279,10 +279,37 @@ static int miic_validate(struct phylink_pcs *pcs, unsigned long *supported, return -EINVAL; } +static int miic_pre_init(struct phylink_pcs *pcs) +{ + struct miic_port *miic_port = phylink_pcs_to_miic_port(pcs); + struct miic *miic = miic_port->miic; + u32 val; + + /* Start RX clock if required */ + if (pcs->rxc_always_on) { + /* In MII through mode, the clock signals will be driven by the + * external PHY, which might not be initialized yet. Set RMII + * as default mode to ensure that a reference clock signal is + * generated. + */ + miic_port->interface = PHY_INTERFACE_MODE_RMII; + + val = FIELD_PREP(MIIC_CONVCTRL_CONV_MODE, CONV_MODE_RMII); + miic_reg_rmw(miic, MIIC_CONVCTRL(miic_port->port), + MIIC_CONVCTRL_CONV_MODE, + val); + + miic_converter_enable(miic, miic_port->port, 1); + } + + return 0; +} + static const struct phylink_pcs_ops miic_phylink_ops = { .pcs_validate = miic_validate, .pcs_config = miic_config, .pcs_link_up = miic_link_up, + .pcs_pre_init = miic_pre_init, }; struct phylink_pcs *miic_create(struct device *dev, struct device_node *np)
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 | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)