netdev - Re: [PATCH net-next v8 05/16] net: pcs: add Renesas MII converter driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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);
> +}