diff mbox series

[V4,2/2] net: ethernet: ravb: Enable optional refclk

Message ID 20210412132619.7896-2-aford173@gmail.com
State Accepted
Commit 8ef7adc6beb2ef0bce83513dc9e4505e7b21e8c2
Headers show
Series None | expand

Commit Message

Adam Ford April 12, 2021, 1:26 p.m. UTC 
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.

V3:  Change 'avb' to 'AVB'
     Remove unnessary else statement and pointer maniupluation when
     enabling the refclock.
     Add disable_unprepare call in remove funtion.

V2:  The previous patch to fetch the fclk was dropped.  In its place
     is code to enable the refclk

Comments

Geert Uytterhoeven April 13, 2021, 7:33 a.m. UTC | #1 
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

-- 
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
Adam Ford April 14, 2021, 1:07 p.m. UTC | #2 
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
Geert Uytterhoeven April 14, 2021, 5:59 p.m. UTC | #3 
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

-- 
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
diff mbox series

Patch

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 */