| Message ID | 20210412132619.7896-2-aford173@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [V4,1/2] dt-bindings: net: renesas,etheravb: Add additional clocks | expand |
Hi Adam, On Mon, Apr 12, 2021 at 3:27 PM Adam Ford <aford173@gmail.com> wrote: > For devices that use a programmable clock for the AVB reference clock, > the driver may need to enable them. Add code to find the optional clock > and enable it when available. > > Signed-off-by: Adam Ford <aford173@gmail.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > --- > V4: Eliminate the NULL check when disabling refclk, and add a line > to disable the refclk if there is a failure after it's been > initialized. Thanks for the update! > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -2148,6 +2148,13 @@ static int ravb_probe(struct platform_device *pdev) > goto out_release; > } > > + priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk"); > + if (IS_ERR(priv->refclk)) { > + error = PTR_ERR(priv->refclk); > + goto out_release; Note that this will call clk_disable_unprepare() in case of failure, which is fine, as that function is a no-op in case of a failed clock. > + } > + clk_prepare_enable(priv->refclk); > + > ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN); > ndev->min_mtu = ETH_MIN_MTU; > > @@ -2244,6 +2251,7 @@ static int ravb_probe(struct platform_device *pdev) > if (chip_id != RCAR_GEN2) > ravb_ptp_stop(ndev); > out_release: > + clk_disable_unprepare(priv->refclk); > free_netdev(ndev); > > pm_runtime_put(&pdev->dev); Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
On Tue, Apr 13, 2021 at 2:33 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Adam, > > On Mon, Apr 12, 2021 at 3:27 PM Adam Ford <aford173@gmail.com> wrote: > > For devices that use a programmable clock for the AVB reference clock, > > the driver may need to enable them. Add code to find the optional clock > > and enable it when available. > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > > --- > > V4: Eliminate the NULL check when disabling refclk, and add a line > > to disable the refclk if there is a failure after it's been > > initialized. > > Thanks for the update! > > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -2148,6 +2148,13 @@ static int ravb_probe(struct platform_device *pdev) > > goto out_release; > > } > > > > + priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk"); > > + if (IS_ERR(priv->refclk)) { > > + error = PTR_ERR(priv->refclk); > > + goto out_release; > > Note that this will call clk_disable_unprepare() in case of failure, which is > fine, as that function is a no-op in case of a failed clock. Geert, A bot reported that if I jump to out_release may try to free a clock if some instances where priv isn't defined. Currently, the priv->clk isn't freed either. I have heard some back-and-forth discussions in other threads on whether or not devm functions auto free or not. I'm fine with sending a V5 to make the free for the refclock happen only when the priv has successfully initialized. Should I also add one for freeing priv->clk and change all the other goto out_release commands to point to this new section? I am thinking it would like something like: free_refclk: clk_disable_unprepare(priv->refclk); free_clk; clk_disable_unprepare(priv->clk); out_release: free_netdev(ndev); .... adam > > > + } > > + clk_prepare_enable(priv->refclk); > > + > > ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN); > > ndev->min_mtu = ETH_MIN_MTU; > > > > @@ -2244,6 +2251,7 @@ static int ravb_probe(struct platform_device *pdev) > > if (chip_id != RCAR_GEN2) > > ravb_ptp_stop(ndev); > > out_release: > > + clk_disable_unprepare(priv->refclk); > > free_netdev(ndev); > > > > pm_runtime_put(&pdev->dev); > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Adam, On Wed, Apr 14, 2021 at 3:08 PM Adam Ford <aford173@gmail.com> wrote: > On Tue, Apr 13, 2021 at 2:33 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, Apr 12, 2021 at 3:27 PM Adam Ford <aford173@gmail.com> wrote: > > > For devices that use a programmable clock for the AVB reference clock, > > > the driver may need to enable them. Add code to find the optional clock > > > and enable it when available. > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > > > > --- > > > V4: Eliminate the NULL check when disabling refclk, and add a line > > > to disable the refclk if there is a failure after it's been > > > initialized. > > > > Thanks for the update! > > > > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > > @@ -2148,6 +2148,13 @@ static int ravb_probe(struct platform_device *pdev) > > > goto out_release; > > > } > > > > > > + priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk"); > > > + if (IS_ERR(priv->refclk)) { > > > + error = PTR_ERR(priv->refclk); > > > + goto out_release; > > > > Note that this will call clk_disable_unprepare() in case of failure, which is > > fine, as that function is a no-op in case of a failed clock. > > Geert, > > A bot reported that if I jump to out_release may try to free a clock > if some instances where priv isn't defined. As priv is allocated using alloc_etherdev_mqs(), priv->refclk is NULL initially, but priv itself may indeed not be initialized when the first "goto out_release" is taken. Sorry for missing that. > Currently, the priv->clk isn't freed either. I have heard some > back-and-forth discussions in other threads on whether or not devm > functions auto free or not. The devm_clk_get_optional() will be undone automatically, so there is no need to handle that explicitly. > I'm fine with sending a V5 to make the free for the refclock happen > only when the priv has successfully initialized. Should I also add As this patch has been applied to net-next, you''ll have to send a follow-up fix patch, not a v5. > one for freeing priv->clk and change all the other goto out_release > commands to point to this new section? No, not for priv->clk, due to devm_*(). > I am thinking it would like something like: > > free_refclk: > clk_disable_unprepare(priv->refclk); OK. > free_clk; > clk_disable_unprepare(priv->clk); NAK, as priv->clk is not enabled in ravb_probe(). > out_release: > free_netdev(ndev); > .... OK. Thanks! Gr{oetje,eeting}s, Geert
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index cb47e68c1a3e..86a1eb0634e8 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -993,6 +993,7 @@ struct ravb_private { struct platform_device *pdev; void __iomem *addr; struct clk *clk; + struct clk *refclk; struct mdiobb_ctrl mdiobb; u32 num_rx_ring[NUM_RX_QUEUE]; u32 num_tx_ring[NUM_TX_QUEUE]; diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index eb0c03bdb12d..1409ae986aa2 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -2148,6 +2148,13 @@ static int ravb_probe(struct platform_device *pdev) goto out_release; } + priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk"); + if (IS_ERR(priv->refclk)) { + error = PTR_ERR(priv->refclk); + goto out_release; + } + clk_prepare_enable(priv->refclk); + ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN); ndev->min_mtu = ETH_MIN_MTU; @@ -2244,6 +2251,7 @@ static int ravb_probe(struct platform_device *pdev) if (chip_id != RCAR_GEN2) ravb_ptp_stop(ndev); out_release: + clk_disable_unprepare(priv->refclk); free_netdev(ndev); pm_runtime_put(&pdev->dev); @@ -2260,6 +2268,8 @@ static int ravb_remove(struct platform_device *pdev) if (priv->chip_id != RCAR_GEN2) ravb_ptp_stop(ndev); + clk_disable_unprepare(priv->refclk); + dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat, priv->desc_bat_dma); /* Set reset mode */