Re: [PATCH 3/3] ARM: dts: bcm2711: give the GENET MDIO controller its clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ARM: dts: bcm2711: genet_mdio clocks property ignored in unimac_mdio_probe()
--

commit abe7b1908b1ca3d642c93b8cea72ce17c73c4c14
Author: Nicolai Buchwitz <nb@tipi-net.de>

ARM: dts: bcm2711: give the GENET MDIO controller its clock

This commit adds the BCM2711_CLOCK_GISB clock phandle to the genet_mdio
device tree node in bcm2711.dtsi so the mdio-bcm-unimac driver can query
the 200 MHz clock rate.

> diff --git a/arch/arm/boot/dts/broadcom/bcm2711.dtsi b/arch/arm/boot/dts/broadcom/bcm2711.dtsi
> index 5e3b4bb393967..82b38551d55cf 100644
> --- a/arch/arm/boot/dts/broadcom/bcm2711.dtsi
> +++ b/arch/arm/boot/dts/broadcom/bcm2711.dtsi
> @@ -597,6 +597,7 @@ genet_mdio: mdio@e14 {
>  				compatible = "brcm,genet-mdio-v5";
>  				reg = <0xe14 0x8>;
>  				reg-names = "mdio";
> +				clocks = <&clocks BCM2711_CLOCK_GISB>;
>  				#address-cells = <0x1>;
>  				#size-cells = <0x0>;
>  			};

[Severity: Medium]
Will mdio-bcm-unimac actually read the clocks property added to the
genet_mdio child node here?

When bcmgenet_probe() initializes the controller, it requests the parent
genet node's "enet" clock via devm_clk_get_optional(&priv->pdev->dev,
"enet"), which evaluates to NULL on BCM2711 because the parent
ethernet@7d580000 node has no clocks property.

Then bcmgenet_probe() -> bcmgenet_mii_init() -> bcmgenet_mii_register()
registers the unimac-mdio platform device with platform_data attached:

drivers/net/ethernet/broadcom/genet/bcmmii.c:bcmgenet_mii_register() {
	...
	ppd.clk = priv->clk;
	...
	if (dn)
		platform_device_set_of_node(ppdev, bcmgenet_mii_of_find_mdio(priv));
	else
		ppd.phy_mask = ~0;

	ret = platform_device_add_resources(ppdev, &res, 1);
	if (ret)
		goto out;

	ret = platform_device_add_data(ppdev, &ppd, sizeof(ppd));
	...
}

When unimac_mdio_probe() runs for that device, pdata is non-NULL, so it
takes the if (pdata) branch and never calls devm_clk_get_optional() on the
genet_mdio OF node:

drivers/net/mdio/mdio-bcm-unimac.c:unimac_mdio_probe() {
	...
	if (pdata) {
		bus->name = pdata->bus_name;
		priv->wait_func = pdata->wait_func;
		priv->wait_func_data = pdata->wait_func_data;
		bus->phy_mask = ~pdata->phy_mask;
		priv->clk = pdata->clk;
	} else {
		bus->name = "unimac MII bus";
		priv->wait_func_data = priv;
		priv->wait_func = unimac_mdio_poll;
		priv->clk = devm_clk_get_optional(&pdev->dev, NULL);
	}
	...
}

As a result, priv->clk remains NULL and clk_get_rate(priv->clk) in
unimac_mdio_clk_set() still returns 0 (and unimac_mdio_clk_set() also
returns early unless clock-frequency is set).

Should unimac_mdio_probe() fall back to devm_clk_get_optional() when
pdata->clk is NULL, or should the clock be wired up through the parent
genet node?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-devel-bcm2711-gisb-clk-v1-0-a1728724d0f8@tipi-net.de?part=3