| Message ID | 20180114214858.7217-2-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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); > }
Hi Jerome, On Mon, Jan 15, 2018 at 12:46 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > 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 I did swap the calls in PATCH #3 of this series with this patch I wanted to make sure that all of the current logic is only executed in RGMII mode >> /* 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. noted, I'll also make this part of the clock cleanup series >> >> return stmmac_pltfr_remove(pdev); >> } >
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c index 4404650b32c5..c6f87e9c4ccb 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c @@ -81,7 +81,7 @@ static void meson8b_dwmac_mask_bits(struct meson8b_dwmac *dwmac, u32 reg, writel(data, dwmac->regs + reg); } -static int meson8b_init_clk(struct meson8b_dwmac *dwmac) +static int meson8b_init_rgmii_tx_clk(struct meson8b_dwmac *dwmac) { struct clk_init_data init; int i, ret; @@ -176,7 +176,6 @@ static int meson8b_init_clk(struct meson8b_dwmac *dwmac) static int meson8b_init_prg_eth(struct meson8b_dwmac *dwmac) { int ret; - unsigned long clk_rate; u8 tx_dly_val = 0; switch (dwmac->phy_mode) { @@ -191,9 +190,6 @@ static int meson8b_init_prg_eth(struct meson8b_dwmac *dwmac) case PHY_INTERFACE_MODE_RGMII_ID: case PHY_INTERFACE_MODE_RGMII_TXID: - /* Generate a 25MHz clock for the PHY */ - clk_rate = 25 * 1000 * 1000; - /* enable RGMII mode */ meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_RGMII_MODE, PRG_ETH0_RGMII_MODE); @@ -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; - } - /* 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); return stmmac_pltfr_remove(pdev); }
Neither the m25_div_clk nor the m250_div_clk or m250_mux_clk are used in RMII mode. The m25_div_clk output is routed to the RGMII PHY's "RGMII clock". This means that we don't need to configure the clocks in RMII mode. The driver however did this - with no effect since the clocks are not routed to the PHY in RMII mode. While here also rename meson8b_init_clk to meson8b_init_rgmii_tx_clk to make it easier to understand the code. Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC") Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- .../net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 46 ++++++++++------------ 1 file changed, 21 insertions(+), 25 deletions(-)