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
    Le Thu, 14 Apr 2022 13:49:08 +0100,
    "Russell King (Oracle)" <linux@armlinux.org.uk> a écrit :

    > 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.

    You are right, the datasheet presents that as a single bitfield but
    that can be split.

    >
    > > +#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.

    Acked.

    >
    > > +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.

    Indeed, seems stupid, I will simply keep the mode bits once split from
    speed.

    [...]

    > > +};
    > > +
    > > +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.

    Yep, sounds like a good idea.

    >
    > Thanks!
    >

    Thanks for the review,

    --
    Clément Léger,
    Embedded Linux and Kernel engineer at Bootlin
    https://bootlin.com

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