Re: [PATCH net-next 8/8] net: mdio: realtek-rtl9300: Add support for RTL839x

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: mdio: realtek-rtl9300: Add support for RTL839x

This patch adds RTL839x (cypress) support to the realtek-rtl9300 MDIO
driver, reusing the multi-device infrastructure that was prepared for
this purpose. It introduces a new RTL8390 register set, four c22/c45
read/write helpers, and a new otto_emdio_8390_info compatible table
entry.

> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> index d9ff0b0aecbb..2ab1aeb85eed 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
> @@ -139,6 +139,29 @@
>  #define RTL8380_SMI_POLL_CTRL			0xa17c
>  #define RTL8380_SMI_PORT0_5_ADDR_CTRL		0xa1c8
>  
> +#define RTL8390_NUM_BUSES			2
> +#define RTL8390_NUM_PAGES			8192
> +#define RTL8390_NUM_PORTS			52
> +#define RTL8390_BCAST_PHYID_CTRL		0x03ec
> +#define RTL8390_PHYREG_ACCESS_CTRL		0x03dc
> +#define   RTL8390_PHY_CTRL_REG_ADDR		GENMASK(9, 5)
> +#define   RTL8390_PHY_CTRL_PARK_PAGE		GENMASK(27, 23)

[Low]
Is RTL8390_PHY_CTRL_PARK_PAGE used anywhere?  It does not appear to
be referenced in the rest of the patch, while the analogous masks for
the 8380 and 9300 variants are populated with 0x1f in their c22 helpers.

For example, otto_emdio_9300_read_c22() does:

	.c22_data	= FIELD_PREP(RTL9300_PHY_CTRL_REG_ADDR, regnum) |
			  FIELD_PREP(RTL9300_PHY_CTRL_PARK_PAGE, 0x1f) |
			  FIELD_PREP(RTL9300_PHY_CTRL_MAIN_PAGE, priv->page[port]),

while otto_emdio_8390_read_c22() below only sets REG_ADDR and MAIN_PAGE,
leaving bits 27..23 of c22_data at 0:

> +#define   RTL8390_PHY_CTRL_MAIN_PAGE		GENMASK(22, 10)
> +#define   RTL8390_PHY_CTRL_FAIL			BIT(1)
> +#define   RTL8390_PHY_CTRL_WRITE		BIT(3)
> +#define   RTL8390_PHY_CTRL_READ			0
> +#define   RTL8390_PHY_CTRL_TYPE_C45		BIT(2)
> +#define   RTL8390_PHY_CTRL_TYPE_C22		0

[ ... ]

> @@ -457,6 +480,62 @@ static int otto_emdio_8380_write_c45(struct mii_bus *bus, int port,
>  	return otto_emdio_write_cmd(bus, RTL8380_PHY_CTRL_TYPE_C45, &cmd_data);
>  }
>  
> +static int otto_emdio_8390_read_c22(struct mii_bus *bus, int port, int regnum, u32 *value)
> +{
> +	struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
> +	struct otto_emdio_cmd_regs cmd_data = {
> +		.c22_data	= FIELD_PREP(RTL8390_PHY_CTRL_REG_ADDR, regnum) |
> +				  FIELD_PREP(RTL8390_PHY_CTRL_MAIN_PAGE, priv->page[port]),
> +		.ext_page	= FIELD_PREP(RTL8390_PHY_CTRL_EXT_PAGE, 0x1ff),
> +		.io_data	= FIELD_PREP(RTL8390_PHY_CTRL_INDATA, port),
> +	};
> +
> +	return otto_emdio_read_cmd(bus, RTL8390_PHY_CTRL_TYPE_C22, &cmd_data,
> +				   RTL8390_PHY_CTRL_DATA, value);
> +}
> +
> +static int otto_emdio_8390_write_c22(struct mii_bus *bus, int port, int regnum, u16 value)
> +{
> +	struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus);
> +	struct otto_emdio_cmd_regs cmd_data = {
> +		.c22_data	= FIELD_PREP(RTL8390_PHY_CTRL_REG_ADDR, regnum) |
> +				  FIELD_PREP(RTL8390_PHY_CTRL_MAIN_PAGE, priv->page[port]),

Should these helpers also set RTL8390_PHY_CTRL_PARK_PAGE to 0x1f, matching
the 8380/9300 convention, or is the macro itself intended to be removed
because the 8390 hardware does not need a parking page?

> +		.ext_page	= FIELD_PREP(RTL8390_PHY_CTRL_EXT_PAGE, 0x1ff),
> +		.io_data	= FIELD_PREP(RTL8390_PHY_CTRL_INDATA, value),
> +		.port_mask_high	= (u32)(BIT_ULL(port) >> 32),
> +		.port_mask_low	= (u32)(BIT_ULL(port)),
> +	};
> +
> +	return otto_emdio_write_cmd(bus, RTL8390_PHY_CTRL_TYPE_C22, &cmd_data);
> +}

[ ... ]