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,
--
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
