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

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: "Clément Léger" <clement.leger@bootlin.com>
Cc: "Andrew Lunn" <andrew@lunn.ch>,
	"Vivien Didelot" <vivien.didelot@gmail.com>,
	"Florian Fainelli" <f.fainelli@gmail.com>,
	"Vladimir Oltean" <olteanv@gmail.com>,
	"David S . Miller" <davem@davemloft.net>,
	"Eric Dumazet" <edumazet@google.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Paolo Abeni" <pabeni@redhat.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzk+dt@kernel.org>,
	"Geert Uytterhoeven" <geert+renesas@glider.be>,
	"Magnus Damm" <magnus.damm@gmail.com>,
	"Heiner Kallweit" <hkallweit1@gmail.com>,
	"Alexandre Torgue" <alexandre.torgue@foss.st.com>,
	"Giuseppe Cavallaro" <peppe.cavallaro@st.com>,
	"Jose Abreu" <joabreu@synopsys.com>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Herve Codina" <herve.codina@bootlin.com>,
	"Miquèl Raynal" <miquel.raynal@bootlin.com>,
	"Milan Stevanovic" <milan.stevanovic@se.com>,
	"Jimmy Lalande" <jimmy.lalande@se.com>,
	"Pascal Eberhard" <pascal.eberhard@se.com>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH net-next v9 05/16] net: pcs: add Renesas MII converter driver
Date: Tue, 28 Jun 2022 17:42:58 +0100	[thread overview]
Message-ID: <YrsvkqBbzUvTYOeI@shell.armlinux.org.uk> (raw)
In-Reply-To: <20220624144001.95518-6-clement.leger@bootlin.com>

On Fri, Jun 24, 2022 at 04:39:50PM +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@bootlin.com>
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Looks good to me, thanks.

The only issue I haven't brought up is:

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

the stting of the speed here. As this function can be called as a result
of ethtool setting the configuration while the link is up, this could
have disasterous effects on the link. This will only happen if there is
no PHY present and we aren't using fixed-link mode.

Therefore, I'm willing to get this pass, but I think it would be better
if the speed was only updated if the interface setting is actually
being changed. So:

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!