Re: [PATCH v2 1/3] net: add r9a06g032 A5PSW switch driver

[Marek Vasut via U-Boot <u-boot@lists.u-boot-project.org>]

On 8/14/26 8:30 PM, Ralph Siemsen wrote:

[...]

> +++ b/drivers/net/Kconfig
> @@ -880,6 +880,12 @@ config RENESAS_RAVB
>   	  This driver implements support for the Ethernet AVB block in
>   	  several Renesas R-Car and RZ SoCs.
>   
> +config RZN1_A5PSW

RENESAS_RZN1_A5PSW to be consistent with the other network hardware 
prefixes used here .

> +	bool "Renesas RZ/N1 A5PSW ethernet switch"
> +	help
> +	  Support the Advanced 5-Port ethernet switch (A5PSW) found in
> +	  the Renesas RZ/N1 SoC.
> +

[...]

> +++ b/drivers/net/rzn1_a5psw.c
> @@ -0,0 +1,377 @@

[...]

> +static int rzn1_phy_wait(struct udevice *dev)
> +{
> +	struct rzn1_a5psw_plat *plat = dev_get_plat(dev->parent);
> +	struct regmap *regmap = plat->regmap;
> +	u32 timeout = 100;
> +	int ret;
> +	u32 val;

regmap_read_poll_timeout()

> +	do {
> +		ret = regmap_read(regmap, MT5PT_MDIO_CFG_STATUS, &val);
> +		if (ret == 0 && (val & BIT(0)) == 0)
> +			return 0;
> +
> +		mdelay(1);
> +	} while (--timeout);
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int rzn1_a5psw_mdio_read(struct udevice *dev, int addr, int devad, int reg)
> +{
> +	struct rzn1_a5psw_plat *plat = dev_get_plat(dev->parent);
> +	struct regmap *regmap = plat->regmap;
> +	int ret;
> +	u32 val = BIT(15) | ((addr & 0x1f) << 5) | (reg & 0x1f);
> +
> +	/* Clause 22 only */
> +	rzn1_switch_write(regmap, MT5PT_MDIO_COMMAND, val);
> +
> +	ret = rzn1_phy_wait(dev);
> +	if (ret) {
> +		dev_warn(dev, "PHY read timeout\n");
> +		return ret;
> +	}
> +
> +	ret = regmap_read(regmap, MT5PT_MDIO_DATA, &val);
> +	if (ret) {
> +		dev_warn(dev, "PHY read error\n");
> +		return ret;
> +	}
> +	val &= 0xffff;
> +
> +	// TODO: check CFG_STATUS for READERR

This should be addressed it seems ?

> +	dev_dbg(dev, "addr=0x%x reg=0x%x read 0x%04x\n", addr, reg, val);
> +
> +	return val;
> +}
> +
> +static int rzn1_a5psw_mdio_write(struct udevice *dev, int addr, int devad,
> +				 int reg, u16 val)
> +{
> +	struct rzn1_a5psw_plat *plat = dev_get_plat(dev->parent);
> +	struct regmap *regmap = plat->regmap;
> +	int ret;
> +	u32 cmd = ((addr & 0x1f) << 5) | (reg & 0x1f);
> +
> +	dev_dbg(dev, "addr=0x%x reg=0x%x write 0x%04x\n", addr, reg, val);
> +
> +	/* Clause 22 only */
> +	rzn1_switch_write(regmap, MT5PT_MDIO_COMMAND, cmd);
> +	rzn1_switch_write(regmap, MT5PT_MDIO_DATA, val);
> +
> +	ret = rzn1_phy_wait(dev);
> +	if (ret) {
> +		dev_warn(dev, "PHY write timeout\n");

Include the error code in the message, it makes debugging easier.

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct mdio_ops rzn1_a5psw_mdio_ops = {
> +	.read = rzn1_a5psw_mdio_read,
> +	.write = rzn1_a5psw_mdio_write,
> +};
> +
> +static int rzn1_a5psw_mdio_bind(struct udevice *dev)
> +{
> +	char name[32];
> +	static int num_devices;

Reverse xmas tree, but don't you have ethernet-ports {} subnode in DT 
node for the switch, from which you could derive the MDIO bus address(es) ?

Example
dts/upstream/src/arm64/freescale/imx8mn-vhip4-evalboard-ksz8794-common.dtsi

15         ksz8794: ethernet-switch@1 {
16                 compatible = "microchip,ksz8794";
...
24                 ethernet-ports {
...
28                         port@0 {
29                                 reg = <0>;
30                                 label = "lan1";
31                         };

> +	sprintf(name, "rzn1-a5psw-mdio-%d", num_devices++);
> +	device_set_name(dev, name);
> +
> +	return 0;
> +}
> +
> +U_BOOT_DRIVER(rzn1_a5psw_mdio) = {
> +	.name		= "rzn1_a5psw_mdio",
> +	.id		= UCLASS_MDIO,
> +	.ops		= &rzn1_a5psw_mdio_ops,
> +	.bind		= rzn1_a5psw_mdio_bind,
> +	.plat_auto	= sizeof(struct mdio_perdev_priv),
> +};
> +
> +static int rzn1_a5psw_port_probe(struct udevice *dev, int port, struct phy_device *phy)
> +{
> +	/* Called once when switch is probed */
> +	return phy_config(phy);
> +}
> +
> +void rzn1_rgmii_rmii_conv_speed(struct udevice *dev, int phy,
> +				int full_duplex, int speed);
> +
> +static int rzn1_a5psw_port_enable(struct udevice *dev, int port, struct phy_device *phy)
> +{
> +	struct rzn1_a5psw_plat *plat = dev_get_plat(dev);
> +
> +	/* CPU port is already enabled */
> +	if (port == 4)

4 needs to be a macro, not a magic number directly in the code.

> +		return 0;
> +
> +	/* Get speed/duplex from PHY */
> +	genphy_update_link(phy);
> +	if (!phy->link) {
> +		dev_err(dev, "PHY %s no ethernet link\n", phy->dev->name);
> +		return 0;
> +	}
> +	genphy_parse_link(phy);
> +
> +	/* Program the converter accoringly */
> +	dev_info(dev, "MIIC = %p\n", plat->miic);
> +	rzn1_rgmii_rmii_conv_speed(plat->miic, 4 - port, phy->duplex, phy->speed);
> +
> +	/* Program the switch port accordingly */
> +	dev_info(dev, "%s port=%d using speed=%d\n", __func__, port, phy->speed);
> +	rzn1_switch_port_speed_duplex(dev, port, phy->speed, phy->duplex);
> +	rzn1_switch_port_enable(dev, port);
> +	return 0;
> +}
> +
> +static void rzn1_a5psw_port_disable(struct udevice *dev, int port, struct phy_device *phy)
> +{
> +	/* CPU port is already enabled */
> +	if (port == 4)
> +		return;
> +
> +	rzn1_switch_port_disable(dev, port);
> +}
> +
> +static const struct dsa_ops rzn1_a5psw_dsa_ops = {
> +	.port_probe = rzn1_a5psw_port_probe,
> +	.port_enable = rzn1_a5psw_port_enable,
> +	.port_disable = rzn1_a5psw_port_disable,
> +};
> +
> +static int rzn1_a5psw_probe_mdio(struct udevice *dev)
> +{
> +	struct udevice *mdev;
> +	const char *name;
> +	ofnode node;
> +	int ret;
> +
> +	/* bind phy ports of mdio child node to rzn1_a5psw_mdio device */
> +	node = dev_read_subnode(dev, "mdio");
> +	if (!ofnode_valid(node))
> +		return 0;
> +
> +	name = ofnode_get_name(node);
> +	ret = device_bind_driver_to_node(dev,
> +					 "rzn1_a5psw_mdio",
> +					 name, node, NULL);
> +	if (ret) {
> +		dev_err(dev, "failed to bind %s: %d\n", name, ret);

return ret;
}

> +	} else {

Drop the else {} .

> +		/* need to probe it as there is no compatible to do so */
> +		ret = uclass_get_device_by_ofnode(UCLASS_MDIO, node, &mdev);
> +		if (ret)
> +			dev_err(dev, "failed to probe %s: %d\n", name, ret);
> +	}
> +
> +	return ret;
> +}
> +
> +static int rzn1_a5psw_probe(struct udevice *dev)
> +{
> +	struct rzn1_a5psw_plat *plat = dev_get_plat(dev);
> +	int ret;
> +
> +	dev_dbg(dev, "%s:%d\n", __func__, __LINE__);
> +
> +	/* Enable clocks */
> +	ret = clk_get_bulk(dev, &plat->bulk);
> +	if (ret) {
> +		dev_err(dev, "Failed to get clocks\n");
> +		return ret;
> +	}
> +	ret = clk_enable_bulk(&plat->bulk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable clocks\n");

"Failed to enable clocks (%d)\n", ret);

Please fix globally.

> +		return ret;
> +	}
> +
> +	/* Access the registers */
> +	ret = regmap_init_mem(dev_ofnode(dev), &plat->regmap);
> +	if (ret) {
> +		dev_err(dev, "Failed to get regmap\n");
> +		return ret;
> +	}
> +
> +	/* Find the MIIC driver */
> +	ret = uclass_get_device_by_name(UCLASS_MISC, "eth-miic@44030000", &plat->miic);
> +	if (ret) {
> +		dev_err(dev, "Failed to get MIIC\n");
> +		return ret;
> +	}
> +	dev_dbg(dev, "got MIIC %p <%s>\n", plat->miic, plat->miic->name);
> +
> +	/* Upstream port is always 1Gbps */
> +	rzn1_switch_port_speed_duplex(dev, 4, SPEED_1000, DUPLEX_FULL);
> +	rzn1_switch_port_enable(dev, 4);
> +
> +	/* Some extra resets are located in system controller */
> +	plat->syscon = syscon_regmap_lookup_by_phandle(dev, "syscon");
> +	if (IS_ERR(plat->syscon)) {
> +		dev_err(dev, "No syscon node found\n");
> +		return PTR_ERR(plat->syscon);
> +	}
> +
> +	/* Clear ETH and CLK25 resets */
> +#define RZN1_SYSCTRL_PWRCTRL_SWITCH 0x188

This goes to the beginning of this file.

> +	regmap_update_bits(plat->syscon, RZN1_SYSCTRL_PWRCTRL_SWITCH,
> +			   BIT(4) | BIT(3), BIT(4) | BIT(3));
> +
> +	/* Reset all PHYs */
> +	struct gpio_desc phy_reset_gpio;
> +	u32 phy_reset_delay;

This goes to the beginning of the function ; didn't the compiler 
complain about this ?

> +	ret = gpio_request_by_name(dev, "phy-reset-gpios", 0,
> +				   &phy_reset_gpio, GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
> +	if (ret) {
> +		dev_err(dev, "failed to get phy-reset-gpios: %d\n", ret);
> +		return ret;
> +	}
> +
> +	phy_reset_delay = dev_read_u32_default(dev, "phy-reset-duration", 15);
[...]