| Message ID | 20201228213121.2331449-1-aford173@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/4] dt-bindings: net: renesas,etheravb: Add additional clocks | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | success | |
| robh/dtbs-check | fail | build log |
Hello! On 29.12.2020 0:31, Adam Ford wrote: > The AVB driver assumes there is an external clock, but it could > be driven by an external clock. Driver can be driven by external clock? :-) > 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 ^ is > 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> [...] MBR, Sergei
On 29.12.2020 0:31, Adam Ford wrote: > The bindings have been updated to support two clocks, but the > original clock now requires the name fck to distinguish it > from the other. > > Signed-off-by: Adam Ford <aford173@gmail.com> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@gmail.com> MBR, Sergei
On 29.12.2020 0:31, 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. Hopefully this won't break RPM... > Signed-off-by: Adam Ford <aford173@gmail.com> [...] MBR, Sergei
Hi Adam, On Mon, Dec 28, 2020 at 10:32 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 to distinguish it > from the other. > > 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 > @@ -2142,7 +2142,7 @@ static int ravb_probe(struct platform_device *pdev) > > priv->chip_id = chip_id; > > - priv->clk = devm_clk_get(&pdev->dev, NULL); > + priv->clk = devm_clk_get(&pdev->dev, "fck"); This change is not backwards compatible, as existing DTB files do not have the "fck" clock. So the driver has to keep on assuming the first clock is the functional clock, and this patch is thus not needed nor desired. > if (IS_ERR(priv->clk)) { > error = PTR_ERR(priv->clk); > goto out_release; Gr{oetje,eeting}s, Geert
On Mon, Jan 4, 2021 at 4:41 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Adam, > > On Mon, Dec 28, 2020 at 10:32 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 to distinguish it > > from the other. > > > > 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 > > @@ -2142,7 +2142,7 @@ static int ravb_probe(struct platform_device *pdev) > > > > priv->chip_id = chip_id; > > > > - priv->clk = devm_clk_get(&pdev->dev, NULL); > > + priv->clk = devm_clk_get(&pdev->dev, "fck"); > > This change is not backwards compatible, as existing DTB files do not > have the "fck" clock. So the driver has to keep on assuming the first > clock is the functional clock, and this patch is thus not needed nor > desired. Should I post a V2 with this removed, or can this patch just be excluded? adam > > > if (IS_ERR(priv->clk)) { > > error = PTR_ERR(priv->clk); > > goto out_release; > > 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 Mon, Dec 28, 2020 at 10:32 PM Adam Ford <aford173@gmail.com> wrote: > The AVB driver assumes there is an external clock, but it could > be driven by an external clock. 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> Gr{oetje,eeting}s, Geert
Hi Adam, On Tue, Jan 5, 2021 at 1:53 PM Adam Ford <aford173@gmail.com> wrote: > On Mon, Jan 4, 2021 at 4:41 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, Dec 28, 2020 at 10:32 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 to distinguish it > > > from the other. > > > > > > 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 > > > @@ -2142,7 +2142,7 @@ static int ravb_probe(struct platform_device *pdev) > > > > > > priv->chip_id = chip_id; > > > > > > - priv->clk = devm_clk_get(&pdev->dev, NULL); > > > + priv->clk = devm_clk_get(&pdev->dev, "fck"); > > > > This change is not backwards compatible, as existing DTB files do not > > have the "fck" clock. So the driver has to keep on assuming the first > > clock is the functional clock, and this patch is thus not needed nor > > desired. > > Should I post a V2 with this removed, or can this patch just be excluded? As far as I am concerned, it can just be excluded. Patches 1 and 2+3 have to follow different maintainer paths anyway. Gr{oetje,eeting}s, Geert
On Mon, Dec 28, 2020 at 10:32 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> i.e. will queue in renesas-devel once PATCH 1/4 has been accepted. Gr{oetje,eeting}s, Geert
Hi Adam, On Fri, Jan 8, 2021 at 3:11 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, Dec 28, 2020 at 10:32 PM Adam Ford <aford173@gmail.com> wrote: > > The AVB driver assumes there is an external clock, but it could > > be driven by an external clock. 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> Upon second look... >> --- 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: txc_refclk On R-Car Gen3 and RZ/G2, RGMII is used, and this clock is called "txcrefclk", i.e. without underscore. On R-Car Gen2, GMII is used, and this clock is called "gtxrefclk". Perhaps it should just be called "refclk"? Gr{oetje,eeting}s, Geert
On Mon, Dec 28, 2020 at 10:32 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> i.e. will queue in renesas-devel once PATCH 1/4 has been accepted. Gr{oetje,eeting}s, Geert
On Mon, 28 Dec 2020 15:31:17 -0600, Adam Ford wrote: > The AVB driver assumes there is an external clock, but it could > be driven by an external clock. 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> > --- > .../devicetree/bindings/net/renesas,etheravb.yaml | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > Acked-by: Rob Herring <robh@kernel.org>
diff --git a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml index 244befb6402a..c1a06510f056 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: txc_refclk iommus: maxItems: 1
The AVB driver assumes there is an external clock, but it could be driven by an external clock. 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> --- .../devicetree/bindings/net/renesas,etheravb.yaml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)