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

[Nicolai Buchwitz <nb@tipi-net.de>]

On 21.10.2026 16:58, sashiko-bot@kernel.org wrote:
> 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?

Known, see the cover letter. Today the pdata branch never looks at the
mdio node, so the property is unused until James's series [1] adds the
node-first devm_clk_get_optional() there (planned for v2), with the
unimac_mdio_clk_set() change as a follow-up. The DT describes the
hardware either way.

[1] 
https://lore.kernel.org/netdev/20261009143506.2507607-1-jjc@jclark.com/