Messages in this thread | | | Date | Thu, 14 Apr 2022 13:49:08 +0100 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH net-next 04/12] net: pcs: add Renesas MII converter driver |
| |
On Thu, Apr 14, 2022 at 02:22:42PM +0200, Clément Léger wrote: > Add PCS driver for the MII converter that is present on Renesas RZ/N1 > SoC. This MII converter is reponsible of converting MII to RMII/RGMII > or act as a MII passtrough. Exposing it as a PCS allows to reuse it > in both the switch driver and the stmmac driver. Currently, this driver > only allows the PCS to be used by the dual Cortex-A7 subsystem since > the register locking system is not used.
Hi,
> +#define MIIC_CONVCTRL_CONV_MODE GENMASK(4, 0) > +#define CONV_MODE_MII 0 > +#define CONV_MODE_RMII BIT(2) > +#define CONV_MODE_RGMII BIT(3) > +#define CONV_MODE_10MBPS 0 > +#define CONV_MODE_100MBPS BIT(0) > +#define CONV_MODE_1000MBPS BIT(1)
Is this really a single 4-bit wide field? It looks like two 2-bit fields to me.
> +#define phylink_pcs_to_miic_port(pcs) container_of((pcs), struct miic_port, pcs)
I prefer a helper function to a preprocessor macro for that, but I'm not going to insist on that point.
> +static void miic_link_up(struct phylink_pcs *pcs, unsigned int mode, > + phy_interface_t interface, int speed, int duplex) > +{ > + struct miic_port *miic_port = phylink_pcs_to_miic_port(pcs); > + struct miic *miic = miic_port->miic; > + int port = miic_port->port; > + u32 val = 0; > + > + if (duplex == DUPLEX_FULL) > + val |= MIIC_CONVCTRL_FULLD; > + > + switch (interface) { > + case PHY_INTERFACE_MODE_RMII: > + val |= CONV_MODE_RMII; > + break; > + case PHY_INTERFACE_MODE_RGMII: > + val |= CONV_MODE_RGMII; > + break; > + case PHY_INTERFACE_MODE_MII: > + val |= CONV_MODE_MII; > + break; > + default: > + dev_err(miic->dev, "Unsupported interface %s\n", > + phy_modes(interface)); > + return; > + }
Why are you re-decoding the interface mode? The interface mode won't change as a result of a call to link-up. Changing the interface mode is a major configuration event that will always see a call to your miic_config() function first.
> + > + /* No speed in MII through-mode */ > + if (interface != PHY_INTERFACE_MODE_MII) { > + switch (speed) { > + case SPEED_1000: > + val |= CONV_MODE_1000MBPS; > + break; > + case SPEED_100: > + val |= CONV_MODE_100MBPS; > + break; > + case SPEED_10: > + val |= CONV_MODE_10MBPS; > + break; > + case SPEED_UNKNOWN: > + pr_err("Invalid speed\n"); > + /* Silently don't do anything */ > + return;
You shouldn't need to consider SPEED_UNKNOWN - if that's something we really want to print a warning for, that should be done by phylink and not by drivers.
> + default: > + dev_err(miic->dev, "Invalid PCS speed %d\n", speed); > + return; > + } > + } > + > + miic_reg_rmw(miic, MIIC_CONVCTRL(port), > + (MIIC_CONVCTRL_CONV_MODE | MIIC_CONVCTRL_FULLD), val); > +} > + > +static bool miic_mode_supported(phy_interface_t interface) > +{ > + return (interface == PHY_INTERFACE_MODE_RGMII || > + interface == PHY_INTERFACE_MODE_RMII || > + interface == PHY_INTERFACE_MODE_MII); > +} > + > +static int miic_validate(struct phylink_pcs *pcs, unsigned long *supported, > + const struct phylink_link_state *state) > +{ > + struct miic_port *miic_port = phylink_pcs_to_miic_port(pcs); > + struct miic *miic = miic_port->miic; > + > + if (state->interface != PHY_INTERFACE_MODE_NA &&
PHY_INTERFACE_MODE_NA is no longer a "thing" with phylink with PCS support, you no longer need to test for it.
> + !miic_mode_supported(state->interface)) { > + dev_err(miic->dev, "phy mode %s is unsupported on port %d\n", > + phy_modes(state->interface), miic_port->port);
Please don't print an error if the interface mode is not supported.
> + linkmode_zero(supported);
There is no need to zero the support mask if you return an error.
> + return -EOPNOTSUPP;
From the method documentation:
* Returns -EINVAL if the interface mode/autoneg mode is not supported. * Returns non-zero positive if the link state can be supported.
Also, really, the MAC layer should ensure that the PCS isn't used for interface modes that it doesn't support. I might introduce a bitmap of interface modes for PCS later if there's a benefit to doing so.
> + } > + > + return 0; > +} > + > +static const struct phylink_pcs_ops miic_phylink_ops = { > + .pcs_config = miic_config, > + .pcs_link_up = miic_link_up, > + .pcs_validate = miic_validate,
I'd prefer to have them in the order that they are in the structure.
> +}; > + > +struct phylink_pcs *miic_create(struct device_node *np) > +{ > + struct platform_device *pdev; > + struct miic_port *miic_port; > + struct device_node *pcs_np; > + u32 port; > + > + if (of_property_read_u32(np, "reg", &port)) > + return ERR_PTR(-EINVAL); > + > + if (port >= MIIC_MAX_NR_PORTS) > + return ERR_PTR(-EINVAL); > + > + /* The PCS pdev is attached to the parent node */ > + pcs_np = of_get_parent(np); > + if (!pcs_np) > + return ERR_PTR(-EINVAL); > + > + pdev = of_find_device_by_node(pcs_np); > + if (!pdev || !platform_get_drvdata(pdev)) > + return ERR_PTR(-EPROBE_DEFER);
It would be a good idea to have a comment in the probe function to say that this relies on platform_set_drvdata() being the very last thing after a point where initialisation is complete and we won't fail.
Thanks!
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
| |