lkml.org 
[lkml]   [2022]   [Apr]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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!

    \
     
     \ /
      Last update: 2022-04-14 14:52    [W:5.795 / U:0.152 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site