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 |
Context | Check | Description |
---|---|---|
robh/checkpatch | success | |
robh/dt-meta-schema | success | |
robh/dtbs-check | success |
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
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
> @@ -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
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
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
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
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
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
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
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
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
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
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
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
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 --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