On Sun, 2018-01-14 at 22:48 +0100, Martin Blumenstingl wrote:
>
[...]
> @@ -204,12 +200,24 @@ static int meson8b_init_prg_eth(struct meson8b_dwmac
> *dwmac)
>
> meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_TXDLY_MASK,
> tx_dly_val << PRG_ETH0_TXDLY_SHIFT);
> +
> + ret = clk_prepare_enable(dwmac->m25_div_clk);
> + if (ret) {
> + dev_err(&dwmac->pdev->dev, "failed to enable the PHY
> clock\n");
> + return ret;
> + }
> +
> + /* Generate the 25MHz RGMII clock for the PHY */
> + ret = clk_set_rate(dwmac->m25_div_clk, 25 * 1000 * 1000);
> + if (ret) {
> + clk_disable_unprepare(dwmac->m25_div_clk);
> +
> + dev_err(&dwmac->pdev->dev, "failed to set PHY clock\n");
> + return ret;
> + }
> break;
>
> case PHY_INTERFACE_MODE_RMII:
> - /* Use the rate of the mux clock for the internal RMII PHY */
> - clk_rate = clk_get_rate(dwmac->m250_mux_clk);
> -
> /* disable RGMII mode -> enables RMII mode */
> meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_RGMII_MODE,
> 0);
> @@ -231,20 +239,6 @@ static int meson8b_init_prg_eth(struct meson8b_dwmac
> *dwmac)
> return -EINVAL;
> }
>
> - ret = clk_prepare_enable(dwmac->m25_div_clk);
> - if (ret) {
> - dev_err(&dwmac->pdev->dev, "failed to enable the PHY clock\n");
> - return ret;
> - }
> -
> - ret = clk_set_rate(dwmac->m25_div_clk, clk_rate);
> - if (ret) {
> - clk_disable_unprepare(dwmac->m25_div_clk);
> -
> - dev_err(&dwmac->pdev->dev, "failed to set PHY clock\n");
> - return ret;
> - }
> -
I would set the rate first then enable. Less chances of glitches and no need to
call clk_disable_unprepare if it fails
> /* enable TX_CLK and PHY_REF_CLK generator */
> meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_TX_AND_PHY_REF_CLK,
> PRG_ETH0_TX_AND_PHY_REF_CLK);
> @@ -294,7 +288,7 @@ static int meson8b_dwmac_probe(struct platform_device
> *pdev)
> &dwmac->tx_delay_ns))
> dwmac->tx_delay_ns = 2;
>
> - ret = meson8b_init_clk(dwmac);
> + ret = meson8b_init_rgmii_tx_clk(dwmac);
> if (ret)
> goto err_remove_config_dt;
>
> @@ -311,7 +305,8 @@ static int meson8b_dwmac_probe(struct platform_device
> *pdev)
> return 0;
>
> err_clk_disable:
> - clk_disable_unprepare(dwmac->m25_div_clk);
> + if (phy_interface_mode_is_rgmii(dwmac->phy_mode))
> + clk_disable_unprepare(dwmac->m25_div_clk);
> err_remove_config_dt:
> stmmac_remove_config_dt(pdev, plat_dat);
>
> @@ -322,7 +317,8 @@ static int meson8b_dwmac_remove(struct platform_device
> *pdev)
> {
> struct meson8b_dwmac *dwmac = get_stmmac_bsp_priv(&pdev->dev);
>
> - clk_disable_unprepare(dwmac->m25_div_clk);
> + if (phy_interface_mode_is_rgmii(dwmac->phy_mode))
> + clk_disable_unprepare(dwmac->m25_div_clk);
if you use
devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare,
 YOUR-CLK)
after enabling the clock, your can discard these conditional hunks.
>
> return stmmac_pltfr_remove(pdev);
> }
