Re: [PATCH v2 2/5] net: mdio: ipq4019: enable the SoC uniphy clocks for ipq5332 platform

[Maxime Chevallier]

Hello,

On Tue, 12 Dec 2023 19:51:47 +0800
Luo Jie <quic_luoj@xxxxxxxxxxx> wrote:

> On the platform ipq5332, the related SoC uniphy GCC clocks need
> to be enabled for making the MDIO slave devices accessible.
> 
> These UNIPHY clocks are from the SoC platform GCC clock provider,
> which are enabled for the connected PHY devices working.
> 
> Signed-off-by: Luo Jie <quic_luoj@xxxxxxxxxxx>

[...]

>  static int ipq4019_mdio_wait_busy(struct mii_bus *bus)
> @@ -209,14 +230,43 @@ static int ipq4019_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
>  static int ipq_mdio_reset(struct mii_bus *bus)
>  {
>  	struct ipq4019_mdio_data *priv = bus->priv;
> -	int ret;
> +	int ret, index;
> +	unsigned long rate;

Please remember to use reverse christmas-tree ordering, meaning longer
declaration lines go first :

	struct ipq4019_mdio_data *priv = bus->priv;
	unsigned long rate;
	int ret, index;

> +
> +	/* For the platform ipq5332, there are two SoC uniphies available
> +	 * for connecting with ethernet PHY, the SoC uniphy gcc clock
> +	 * should be enabled for resetting the connected device such
> +	 * as qca8386 switch, qca8081 PHY or other PHYs effectively.
> +	 *
> +	 * Configure MDIO/UNIPHY clock source frequency if clock instance
> +	 * is specified in the device tree.
> +	 */
> +	for (index = MDIO_CLK_MDIO_AHB; index < MDIO_CLK_CNT; index++) {
> +		switch (index) {
> +		case MDIO_CLK_MDIO_AHB:
> +			rate = IPQ_MDIO_CLK_RATE;
> +			break;
> +		case MDIO_CLK_UNIPHY0_AHB:
> +		case MDIO_CLK_UNIPHY1_AHB:
> +			rate = IPQ_UNIPHY_AHB_CLK_RATE;
> +			break;
> +		case MDIO_CLK_UNIPHY0_SYS:
> +		case MDIO_CLK_UNIPHY1_SYS:
> +			rate = IPQ_UNIPHY_SYS_CLK_RATE;
> +			break;
> +		default:
> +			break;
> +		}
>  
> -	/* Configure MDIO clock source frequency if clock is specified in the device tree */
> -	ret = clk_set_rate(priv->mdio_clk, IPQ_MDIO_CLK_RATE);
> -	if (ret)
> -		return ret;
> +		ret = clk_set_rate(priv->clk[index], rate);
> +		if (ret)
> +			return ret;
> +
> +		ret = clk_prepare_enable(priv->clk[index]);
> +		if (ret)
> +			return ret;
> +	}
>  
> -	ret = clk_prepare_enable(priv->mdio_clk);
>  	if (ret == 0)
>  		mdelay(10);
>  
> @@ -240,10 +290,6 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->membase))
>  		return PTR_ERR(priv->membase);
>  
> -	priv->mdio_clk = devm_clk_get_optional(&pdev->dev, "gcc_mdio_ahb_clk");
> -	if (IS_ERR(priv->mdio_clk))
> -		return PTR_ERR(priv->mdio_clk);
> -
>  	/* These platform resources are provided on the chipset IPQ5018 or
>  	 * IPQ5332.
>  	 */
> @@ -271,6 +317,13 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	for (index = 0; index < MDIO_CLK_CNT; index++) {
> +		priv->clk[index] = devm_clk_get_optional(&pdev->dev,
> +							 mdio_clk_name[index]);
> +		if (IS_ERR(priv->clk[index]))
> +			return PTR_ERR(priv->clk[index]);
> +	}

You should be able to use devm_clk_bulk_get_optional(), to avoid that
loop.

Thanks,

Maxime