diff mbox series

[V3,1/5] dt-bindings: net: renesas,etheravb: Add additional clocks

Message ID 20210224115146.9131-1-aford173@gmail.com
State Not Applicable, archived
Headers show
Series [V3,1/5] dt-bindings: net: renesas,etheravb: Add additional clocks | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema success
robh/dtbs-check success

Commit Message

Adam Ford Feb. 24, 2021, 11:51 a.m. UTC 
The AVB driver assumes there is an external crystal, but it could
be clocked by other means.  In order to enable a programmable
clock, it needs to be added to the clocks list and enabled in the
driver.  Since there currently only one clock, there is no
clock-names list either.

Update bindings to add the additional optional clock, and explicitly
name both of them.

Signed-off-by: Adam Ford <aford173@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Rob Herring <robh@kernel.org>
---
V3:  No Change
V2:  No Change

Comments

Andrew Lunn Feb. 24, 2021, 1:45 p.m. UTC | #1 
On Wed, Feb 24, 2021 at 05:51:42AM -0600, Adam Ford wrote:
> The bindings have been updated to support two clocks, but the
> original clock now requires the name fck.  Add a clock-names
> list in the device tree with fck in it.

Hi Adam

I think requires is too strong. As far as i can see, you don't
introduce a change using the name 'fck'. So the name is optional,
which is good, because otherwise you would break backwards
compatibility with DT blobs.

Is the plan to merge this whole patchset via netdev? If so, you need
to repost anyway, once netdev reopens. So maybe you can change the
wording?

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Andrew Lunn Feb. 24, 2021, 1:46 p.m. UTC | #2 
On Wed, Feb 24, 2021 at 05:51:43AM -0600, Adam Ford wrote:
> The bindings have been updated to support two clocks, but the
> original clock now requires the name fck.  Add a clock-names
> list in the device tree with fck in it.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Andrew Lunn Feb. 24, 2021, 1:47 p.m. UTC | #3 
> @@ -2260,6 +2267,9 @@ static int ravb_remove(struct platform_device *pdev)
>  	if (priv->chip_id != RCAR_GEN2)
>  		ravb_ptp_stop(ndev);
>  
> +	if (priv->refclk)
> +		clk_disable_unprepare(priv->refclk);
> +

Hi Adam

You don't need the if (). The clk API is happy with a NULL pointer and
will do the right thing. Otherwise:

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Geert Uytterhoeven Feb. 25, 2021, 7:51 a.m. UTC | #4 
Hi Andrew,

On Wed, Feb 24, 2021 at 2:45 PM Andrew Lunn <andrew@lunn.ch> wrote:
> On Wed, Feb 24, 2021 at 05:51:42AM -0600, Adam Ford wrote:
> > The bindings have been updated to support two clocks, but the
> > original clock now requires the name fck.  Add a clock-names
> > list in the device tree with fck in it.
>
> I think requires is too strong. As far as i can see, you don't
> introduce a change using the name 'fck'. So the name is optional,
> which is good, because otherwise you would break backwards
> compatibility with DT blobs.
>
> Is the plan to merge this whole patchset via netdev? If so, you need
> to repost anyway, once netdev reopens. So maybe you can change the
> wording?

The DTS patches should go in through the renesas and soc trees.
I can apply them as soon as the DT binding patch has been accepted.

Thanks!

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven March 4, 2021, 8:03 a.m. UTC | #5 
On Wed, Feb 24, 2021 at 12:52 PM Adam Ford <aford173@gmail.com> wrote:
> The AVB refererence clock assumes an external clock that runs

reference

> automatically.  Because the Versaclock is wired to provide the
> AVB refclock, the device tree needs to reference it in order for the
> driver to start the clock.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel (with the typo fixed) once the DT
bindings have been accepted.

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven March 4, 2021, 8:08 a.m. UTC | #6 
Hi Adam,

On Wed, Feb 24, 2021 at 12:52 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>

Thanks for your patch!

> --- 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);
> +

Shouldn't the reference clock be disabled in case of any failure below?

>         ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
>         ndev->min_mtu = ETH_MIN_MTU;
>
> @@ -2260,6 +2267,9 @@ static int ravb_remove(struct platform_device *pdev)
>         if (priv->chip_id != RCAR_GEN2)
>                 ravb_ptp_stop(ndev);
>
> +       if (priv->refclk)
> +               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 */

Gr{oetje,eeting}s,

                        Geert
Adam Ford March 18, 2021, 12:44 p.m. UTC | #7 
On Thu, Mar 4, 2021 at 2:04 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> On Wed, Feb 24, 2021 at 12:52 PM Adam Ford <aford173@gmail.com> wrote:
> > The AVB refererence clock assumes an external clock that runs
>
> reference
>
> > automatically.  Because the Versaclock is wired to provide the
> > AVB refclock, the device tree needs to reference it in order for the
> > driver to start the clock.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> i.e. will queue in renesas-devel (with the typo fixed) once the DT
> bindings have been accepted.
>

Who do I need to ping to get the DT bindings accepted?  They have an
acked-by from Rob.

adam

> 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 March 18, 2021, 1:08 p.m. UTC | #8 
Hi Adam,

On Thu, Mar 18, 2021 at 1:44 PM Adam Ford <aford173@gmail.com> wrote:
> On Thu, Mar 4, 2021 at 2:04 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Feb 24, 2021 at 12:52 PM Adam Ford <aford173@gmail.com> wrote:
> > > The AVB refererence clock assumes an external clock that runs
> >
> > reference
> >
> > > automatically.  Because the Versaclock is wired to provide the
> > > AVB refclock, the device tree needs to reference it in order for the
> > > driver to start the clock.
> > >
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > i.e. will queue in renesas-devel (with the typo fixed) once the DT
> > bindings have been accepted.
> >
>
> Who do I need to ping to get the DT bindings accepted?  They have an
> acked-by from Rob.

Sergei, can you please have a look at the DT binding change?

Thanks!

Gr{oetje,eeting}s,

                        Geert
Sergei Shtylyov March 18, 2021, 8:18 p.m. UTC | #9 
Hi!

On 2/24/21 2:51 PM, Adam Ford wrote:

> The AVB driver assumes there is an external crystal, but it could
> be clocked by other means.  In order to enable a programmable
> clock, it needs to be added to the clocks list and enabled in the
> driver.  Since there currently only one clock, there is no
> clock-names list either.
> 
> Update bindings to add the additional optional clock, and explicitly
> name both of them.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Rob Herring <robh@kernel.org>

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@gmail.com>

[...]

PS: Sorry for the dalay reviewing...

MBR, Sergei
Sergei Shtylyov March 19, 2021, 8:40 a.m. UTC | #10 
On 18.03.2021 15:44, Adam Ford wrote:

>>> The AVB refererence clock assumes an external clock that runs
>>
>> reference
>>
>>> automatically.  Because the Versaclock is wired to provide the
>>> AVB refclock, the device tree needs to reference it in order for the
>>> driver to start the clock.
>>>
>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> i.e. will queue in renesas-devel (with the typo fixed) once the DT
>> bindings have been accepted.
>>
> 
> Who do I need to ping to get the DT bindings accepted?  They have an
> acked-by from Rob.

    Normally, the bindings get picked up by a subsystem maintainer... or Rob :-)

[...]

MBR, Sergei
Adam Ford March 29, 2021, 12:45 p.m. UTC | #11 
On Thu, Mar 4, 2021 at 2:08 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Adam,
>
> On Wed, Feb 24, 2021 at 12:52 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>
>
> Thanks for your patch!
>
> > --- 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);
> > +
>
> Shouldn't the reference clock be disabled in case of any failure below?
>
I'll generate a V4.

Should I just regenerate this patch since it seems like the rest are
OK, or should I regenerate the whole series?

adam
> >         ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
> >         ndev->min_mtu = ETH_MIN_MTU;
> >
> > @@ -2260,6 +2267,9 @@ static int ravb_remove(struct platform_device *pdev)
> >         if (priv->chip_id != RCAR_GEN2)
> >                 ravb_ptp_stop(ndev);
> >
> > +       if (priv->refclk)
> > +               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 */
>
> 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 March 29, 2021, 1:08 p.m. UTC | #12 
Hi Adam,

On Mon, Mar 29, 2021 at 2:45 PM Adam Ford <aford173@gmail.com> wrote:
> On Thu, Mar 4, 2021 at 2:08 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Feb 24, 2021 at 12:52 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>
> >
> > Thanks for your patch!
> >
> > > --- 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);
> > > +
> >
> > Shouldn't the reference clock be disabled in case of any failure below?
> >
> I'll generate a V4.
>
> Should I just regenerate this patch since it seems like the rest are
> OK, or should I regenerate the whole series?

As the DT bindings haven't been applied yet, I think it would be
best if you would send a v4 with just the patches for the netdev
tree (i.e. DT bindings patch 1 and driver patch 4).

I will take the DT patches from this series, once the bindings have been
accepted.

Thank you!

Gr{oetje,eeting}s,

                        Geert
Adam Ford April 17, 2021, 1:54 p.m. UTC | #13 
On Thu, Mar 4, 2021 at 2:04 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> On Wed, Feb 24, 2021 at 12:52 PM Adam Ford <aford173@gmail.com> wrote:
> > The AVB refererence clock assumes an external clock that runs
>
> reference
>
> > automatically.  Because the Versaclock is wired to provide the
> > AVB refclock, the device tree needs to reference it in order for the
> > driver to start the clock.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> i.e. will queue in renesas-devel (with the typo fixed) once the DT
> bindings have been accepted.

Geert,

Since the refclk update and corresponding dt-bindings are in net-next,
are you OK applying the rest of the DT changes so they can get into
5.13?

adam
>
> 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 19, 2021, 9:38 a.m. UTC | #14 
On Wed, Feb 24, 2021 at 12:52 PM Adam Ford <aford173@gmail.com> wrote:
> The bindings have been updated to support two clocks, but the
> original clock now requires the name fck.  Add a clock-names
> list in the device tree with fck in it.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

queueing in renesas-devel for v5.14, with an additional update for
the recently added r8a779a0.dtsi.

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven April 19, 2021, 9:39 a.m. UTC | #15 
Hi Adam,

On Sat, Apr 17, 2021 at 3:54 PM Adam Ford <aford173@gmail.com> wrote:
> On Thu, Mar 4, 2021 at 2:04 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Feb 24, 2021 at 12:52 PM Adam Ford <aford173@gmail.com> wrote:
> > > The AVB refererence clock assumes an external clock that runs
> >
> > reference
> >
> > > automatically.  Because the Versaclock is wired to provide the
> > > AVB refclock, the device tree needs to reference it in order for the
> > > driver to start the clock.
> > >
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > i.e. will queue in renesas-devel (with the typo fixed) once the DT
> > bindings have been accepted.
>
> Geert,
>
> Since the refclk update and corresponding dt-bindings are in net-next,
> are you OK applying the rest of the DT changes so they can get into
> 5.13?

Queueing in renesas-devel for v5.14, as the soc deadline for v5.13
has already passed two weeks ago.

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
index de9dd574a2f9..7b32363ad8b4 100644
--- a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
+++ b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml
@@ -49,7 +49,16 @@  properties:
   interrupt-names: true
 
   clocks:
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
+    items:
+      - description: AVB functional clock
+      - description: Optional TXC reference clock
+
+  clock-names:
+    items:
+      - const: fck
+      - const: refclk
 
   iommus:
     maxItems: 1