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

[Vladimir Oltean <olteanv@gmail.com>]

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@bootlin.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@gmail.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);
> +}