[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220621114415.zj37zmgkwkon6u7e@skbuf>
Date: Tue, 21 Jun 2022 14:44:15 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Clément Léger <clement.leger@...tlin.com>
Cc: Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
"David S . Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Geert Uytterhoeven <geert+renesas@...der.be>,
Magnus Damm <magnus.damm@...il.com>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
Giuseppe Cavallaro <peppe.cavallaro@...com>,
Jose Abreu <joabreu@...opsys.com>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
Herve Codina <herve.codina@...tlin.com>,
Miquèl Raynal <miquel.raynal@...tlin.com>,
Milan Stevanovic <milan.stevanovic@...com>,
Jimmy Lalande <jimmy.lalande@...com>,
Pascal Eberhard <pascal.eberhard@...com>,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-renesas-soc@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v8 05/16] net: pcs: add Renesas MII converter
driver
On Mon, Jun 20, 2022 at 01:08:35PM +0200, Clément Léger wrote:
> Add a PCS driver for the MII converter that is present on the Renesas
> RZ/N1 SoC. This MII converter is reponsible for converting MII to
> RMII/RGMII or act as a MII pass-trough. 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.
>
> Signed-off-by: Clément Léger <clement.leger@...tlin.com>
> ---
Pretty cool driver. I understand this to be more or less the same thing
as drivers/net/phy/xilinx_gmii2rgmii.c in principle, but this appears
nicer done and I'm glad you didn't follow the same model (the
phylink_pcs seems to be a much better fit than a chained PHY).
If PHY library maintainers don't have any objections you can add my:
Reviewed-by: Vladimir Oltean <olteanv@...il.com>
> +static void miic_reg_rmw(struct miic *miic, int offset, u32 mask, u32 val)
> +{
> + u32 reg;
> +
> + spin_lock(&miic->lock);
> +
> + reg = miic_reg_readl(miic, offset);
> + reg &= ~mask;
> + reg |= val;
> + miic_reg_writel(miic, offset, reg);
> +
> + spin_unlock(&miic->lock);
> +}
Just a small comment: I don't think pcs_config and pcs_link_up need
serialization with respect to each other, so this read-modify-write
spinlock doesn't do much. But it doesn't really hurt either.
> +
> +static void miic_converter_enable(struct miic *miic, int port, int enable)
> +{
> + u32 val = 0;
> +
> + if (enable)
> + val = MIIC_CONVRST_PHYIF_RST(port);
> +
> + miic_reg_rmw(miic, MIIC_CONVRST, MIIC_CONVRST_PHYIF_RST(port), val);
> +}
> +
> +static int miic_config(struct phylink_pcs *pcs, unsigned int mode,
> + phy_interface_t interface,
> + const unsigned long *advertising, bool permit)
> +{
> + struct miic_port *miic_port = phylink_pcs_to_miic_port(pcs);
> + struct miic *miic = miic_port->miic;
> + int port = miic_port->port;
> + u32 speed, conv_mode, val;
> +
> + switch (interface) {
> + case PHY_INTERFACE_MODE_RMII:
> + conv_mode = CONV_MODE_RMII;
> + speed = CONV_MODE_100MBPS;
> + break;
> + case PHY_INTERFACE_MODE_RGMII:
> + case PHY_INTERFACE_MODE_RGMII_ID:
> + case PHY_INTERFACE_MODE_RGMII_TXID:
> + case PHY_INTERFACE_MODE_RGMII_RXID:
> + conv_mode = CONV_MODE_RGMII;
> + speed = CONV_MODE_1000MBPS;
> + break;
> + case PHY_INTERFACE_MODE_MII:
> + conv_mode = CONV_MODE_MII;
> + /* When in MII mode, speed should be set to 0 (which is actually
> + * CONV_MODE_10MBPS)
> + */
> + speed = CONV_MODE_10MBPS;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + val = FIELD_PREP(MIIC_CONVCTRL_CONV_MODE, conv_mode) |
> + FIELD_PREP(MIIC_CONVCTRL_CONV_SPEED, speed);
> +
> + miic_reg_rmw(miic, MIIC_CONVCTRL(port),
> + MIIC_CONVCTRL_CONV_MODE | MIIC_CONVCTRL_CONV_SPEED, val);
> + miic_converter_enable(miic_port->miic, miic_port->port, 1);
> +
> + return 0;
> +}
> +
> +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;
> + u32 conv_speed = 0, val = 0;
> + int port = miic_port->port;
> +
> + if (duplex == DUPLEX_FULL)
> + val |= MIIC_CONVCTRL_FULLD;
> +
> + /* No speed in MII through-mode */
> + if (interface != PHY_INTERFACE_MODE_MII) {
> + switch (speed) {
> + case SPEED_1000:
> + conv_speed = CONV_MODE_1000MBPS;
> + break;
> + case SPEED_100:
> + conv_speed = CONV_MODE_100MBPS;
> + break;
> + case SPEED_10:
> + conv_speed = CONV_MODE_10MBPS;
> + break;
> + default:
> + return;
> + }
> + }
> +
> + val |= FIELD_PREP(MIIC_CONVCTRL_CONV_SPEED, conv_speed);
> +
> + miic_reg_rmw(miic, MIIC_CONVCTRL(port),
> + (MIIC_CONVCTRL_CONV_SPEED | MIIC_CONVCTRL_FULLD), val);
> +}
Powered by blists - more mailing lists