diff mbox

[v8,2/5] dt-bindings: clock: renesas,r9a06g032-sysctrl: documentation

Message ID 1528187462-47093-3-git-send-email-michel.pollet@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Michel Pollet June 5, 2018, 8:29 a.m. UTC
The Renesas R9A06G032 SYSCTRL node description.

Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
---
 .../bindings/clock/renesas,r9a06g032-sysctrl.txt   | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.txt

Comments

Geert Uytterhoeven June 11, 2018, 10 a.m. UTC | #1
Hi Michel,

On Tue, Jun 5, 2018 at 10:36 AM Michel Pollet
<michel.pollet@bp.renesas.com> wrote:
> The Renesas R9A06G032 SYSCTRL node description.
>
> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.txt
> @@ -0,0 +1,32 @@
> +* Renesas R9A06G032 SYSCTRL
> +
> +Required Properties:
> +
> +  - compatible: Must be:
> +    - "renesas,r9a06g032-sysctrl"
> +  - reg: Base address and length of the SYSCTRL IO block.
> +  - #clock-cells: Must be 1

(repeating myself) No clocks/clock-names for the external clock inputs?

"RZ/N1 has 3 clock sources, 1 reference clock inputs for RGMII, and 2
 reference clock outputs for RMII/MII."

The rest looks fine to me.
Thanks!

Gr{oetje,eeting}s,

                        Geert
Michel Pollet June 13, 2018, 6:38 a.m. UTC | #2
Hi Geert,

On 11 June 2018 11:01, Geert wrote:
> Hi Michel,

>

> On Tue, Jun 5, 2018 at 10:36 AM Michel Pollet

> <michel.pollet@bp.renesas.com> wrote:

> > The Renesas R9A06G032 SYSCTRL node description.

> >

> > Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>

>

> > --- /dev/null

> > +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-

> sysctr

> > +++ l.txt

> > @@ -0,0 +1,32 @@

> > +* Renesas R9A06G032 SYSCTRL

> > +

> > +Required Properties:

> > +

> > +  - compatible: Must be:

> > +    - "renesas,r9a06g032-sysctrl"

> > +  - reg: Base address and length of the SYSCTRL IO block.

> > +  - #clock-cells: Must be 1

>

> (repeating myself) No clocks/clock-names for the external clock inputs?

>

> "RZ/N1 has 3 clock sources, 1 reference clock inputs for RGMII, and 2

> reference clock outputs for RMII/MII."


Well, I'm trying to keep the binding as simple as possible, to dodge any
further discussion. Adding these will be possible later, I don't need them
for the moment anyway.

>

> The rest looks fine to me.

> Thanks!


Did you have a chance to review the clock driver proper? I'm pondering
sending a v9 since it's been a week (with very minor changes) -- but I
don't want to interrupt if you were in the process of reviewing...

Cheers,
Michel

>

> 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




Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
Geert Uytterhoeven June 13, 2018, 7:17 a.m. UTC | #3
Hi Michel,

On Wed, Jun 13, 2018 at 8:38 AM Michel Pollet
<michel.pollet@bp.renesas.com> wrote:
> On 11 June 2018 11:01, Geert wrote:
> > On Tue, Jun 5, 2018 at 10:36 AM Michel Pollet
> > <michel.pollet@bp.renesas.com> wrote:
> > > The Renesas R9A06G032 SYSCTRL node description.
> > >
> > > Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
> >
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-
> > sysctr
> > > +++ l.txt
> > > @@ -0,0 +1,32 @@
> > > +* Renesas R9A06G032 SYSCTRL
> > > +
> > > +Required Properties:
> > > +
> > > +  - compatible: Must be:
> > > +    - "renesas,r9a06g032-sysctrl"
> > > +  - reg: Base address and length of the SYSCTRL IO block.
> > > +  - #clock-cells: Must be 1
> >
> > (repeating myself) No clocks/clock-names for the external clock inputs?
> >
> > "RZ/N1 has 3 clock sources, 1 reference clock inputs for RGMII, and 2
> > reference clock outputs for RMII/MII."
>
> Well, I'm trying to keep the binding as simple as possible, to dodge any
> further discussion. Adding these will be possible later, I don't need them
> for the moment anyway.

Don't you? The external clock inputs are at the root of the clock tree, so I'd
say you need them. Bolting them on later may become complicated, especially
if you care about DTB backwards compatibility.

The reset controller subsystem is optional anyway, and used only by a small
number of drivers, so support for resets can definitely be postponed.

> Did you have a chance to review the clock driver proper? I'm pondering
> sending a v9 since it's been a week (with very minor changes) -- but I
> don't want to interrupt if you were in the process of reviewing...

I had a quick glance. Looks OK mostly, so it's definitely heading for the right
direction!
One remaining question: do you need CLK_OF_DECLARE()?
Is there any reason your clock driver cannot be a platform driver, which is
the recommended way?

Thanks!

Gr{oetje,eeting}s,

                        Geert
M P June 13, 2018, 8:53 a.m. UTC | #4
hi Geert,

On Wed, 13 Jun 2018 at 08:17, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Michel,
>
> On Wed, Jun 13, 2018 at 8:38 AM Michel Pollet
> <michel.pollet@bp.renesas.com> wrote:
> > On 11 June 2018 11:01, Geert wrote:
> > > On Tue, Jun 5, 2018 at 10:36 AM Michel Pollet
> > > <michel.pollet@bp.renesas.com> wrote:
> > > > The Renesas R9A06G032 SYSCTRL node description.
> > > >
> > > > Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
> > >
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-
> > > sysctr
> > > > +++ l.txt
> > > > @@ -0,0 +1,32 @@
> > > > +* Renesas R9A06G032 SYSCTRL
> > > > +
> > > > +Required Properties:
> > > > +
> > > > +  - compatible: Must be:
> > > > +    - "renesas,r9a06g032-sysctrl"
> > > > +  - reg: Base address and length of the SYSCTRL IO block.
> > > > +  - #clock-cells: Must be 1
> > >
> > > (repeating myself) No clocks/clock-names for the external clock inputs?
> > >
> > > "RZ/N1 has 3 clock sources, 1 reference clock inputs for RGMII, and 2
> > > reference clock outputs for RMII/MII."
> >
> > Well, I'm trying to keep the binding as simple as possible, to dodge any
> > further discussion. Adding these will be possible later, I don't need them
> > for the moment anyway.
>
> Don't you? The external clock inputs are at the root of the clock tree, so I'd
> say you need them. Bolting them on later may become complicated, especially
> if you care about DTB backwards compatibility.

Well the input is fixed frequency -- and there is a clock (gate)
created for it in the driver
(CLK_RGMII_REF) so you can turn it on/off if you like...

The only configurable bit is a *nightmare* as the selection bit for
whether it is coming
from CLKOUT_D8 or from an external input is in a *completely different
IP block*. I'd
need another iomap() and all that stuff, and a custom driver for it.

If I were to implement it, I'd have to add a custom driver for that
clock, use the current
gate as a parent, iomap the selection bit register etc -- basically.
All of that for a use
case of *none ever* as most people would probably tweak that bit in
the bootloader
anyway -- also, I'd have zero capability for testing it.

As far as the output, they also have their own gates (already
created). They are also fixed
frequency and the only thing that matters to them is the pinmux
settings. So (soon) with the
pinmux driver, you can enable that clock properly without a special
driver, just by adding
that gate to your ethernet, and add the pinmux config for it.

So what do I go with that?

>
> The reset controller subsystem is optional anyway, and used only by a small
> number of drivers, so support for resets can definitely be postponed.
>
> > Did you have a chance to review the clock driver proper? I'm pondering
> > sending a v9 since it's been a week (with very minor changes) -- but I
> > don't want to interrupt if you were in the process of reviewing...
>
> I had a quick glance. Looks OK mostly, so it's definitely heading for the right
> direction!
> One remaining question: do you need CLK_OF_DECLARE()?
> Is there any reason your clock driver cannot be a platform driver, which is
> the recommended way?

I copied it straight out of clk-rcar-gen2.c -- I don't 'need' anything more than
instantiating my driver; does it make a difference? ie a one liner macro vs
adding an extra struct, 2 functions to do the same thing?
Just curious, sometime I lose track of that the goalpost is -- for
years I understood
that having OF was simpler and better all around?

Thanks!
Michel
Geert Uytterhoeven June 13, 2018, 11:48 a.m. UTC | #5
Hi Michel,

On Wed, Jun 13, 2018 at 10:53 AM M P <buserror@gmail.com> wrote:
> On Wed, 13 Jun 2018 at 08:17, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Jun 13, 2018 at 8:38 AM Michel Pollet
> > <michel.pollet@bp.renesas.com> wrote:
> > > On 11 June 2018 11:01, Geert wrote:
> > > > On Tue, Jun 5, 2018 at 10:36 AM Michel Pollet
> > > > <michel.pollet@bp.renesas.com> wrote:
> > > > > The Renesas R9A06G032 SYSCTRL node description.
> > > > >
> > > > > Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
> > > >
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-
> > > > sysctr
> > > > > +++ l.txt
> > > > > @@ -0,0 +1,32 @@
> > > > > +* Renesas R9A06G032 SYSCTRL
> > > > > +
> > > > > +Required Properties:
> > > > > +
> > > > > +  - compatible: Must be:
> > > > > +    - "renesas,r9a06g032-sysctrl"
> > > > > +  - reg: Base address and length of the SYSCTRL IO block.
> > > > > +  - #clock-cells: Must be 1
> > > >
> > > > (repeating myself) No clocks/clock-names for the external clock inputs?
> > > >
> > > > "RZ/N1 has 3 clock sources, 1 reference clock inputs for RGMII, and 2
> > > > reference clock outputs for RMII/MII."
> > >
> > > Well, I'm trying to keep the binding as simple as possible, to dodge any
> > > further discussion. Adding these will be possible later, I don't need them
> > > for the moment anyway.
> >
> > Don't you? The external clock inputs are at the root of the clock tree, so I'd
> > say you need them. Bolting them on later may become complicated, especially
> > if you care about DTB backwards compatibility.
>
> Well the input is fixed frequency -- and there is a clock (gate)
> created for it in the driver
> (CLK_RGMII_REF) so you can turn it on/off if you like...
>
> The only configurable bit is a *nightmare* as the selection bit for
> whether it is coming
> from CLKOUT_D8 or from an external input is in a *completely different
> IP block*. I'd
> need another iomap() and all that stuff, and a custom driver for it.
>
> If I were to implement it, I'd have to add a custom driver for that
> clock, use the current
> gate as a parent, iomap the selection bit register etc -- basically.
> All of that for a use
> case of *none ever* as most people would probably tweak that bit in
> the bootloader
> anyway -- also, I'd have zero capability for testing it.
>
> As far as the output, they also have their own gates (already
> created). They are also fixed
> frequency and the only thing that matters to them is the pinmux
> settings. So (soon) with the
> pinmux driver, you can enable that clock properly without a special
> driver, just by adding
> that gate to your ethernet, and add the pinmux config for it.
>
> So what do I go with that?

IC.

Still, DT describes the hardware, not (the limitations of) the software
implementation.  So you can have the external clock inputs in the DT
bindings and in the DTS, but ignore complicated routing and source
selection in the driver.

> > > Did you have a chance to review the clock driver proper? I'm pondering
> > > sending a v9 since it's been a week (with very minor changes) -- but I
> > > don't want to interrupt if you were in the process of reviewing...
> >
> > I had a quick glance. Looks OK mostly, so it's definitely heading for the right
> > direction!
> > One remaining question: do you need CLK_OF_DECLARE()?
> > Is there any reason your clock driver cannot be a platform driver, which is
> > the recommended way?
>
> I copied it straight out of clk-rcar-gen2.c -- I don't 'need' anything more than
> instantiating my driver; does it make a difference? ie a one liner macro vs
> adding an extra struct, 2 functions to do the same thing?

clk-rcar-gen2.c is an old (early DT CCF) driver.
Please check out renesas-cpg-mssr.c for something newer.
Having a platform_driver means you're also having a struct device,
and can do power management, if needed.

> Just curious, sometime I lose track of that the goalpost is -- for
> years I understood
> that having OF was simpler and better all around?

"having OF" does not mean CLK_OF_DECLARE().
Those *_OF_DECLARE() macros impose a very strict initialization
order, which is not always suitable. Hence the trend to move to plain
platform drivers.

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

Patch

diff --git a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.txt b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.txt
new file mode 100644
index 0000000..6aee360
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.txt
@@ -0,0 +1,32 @@ 
+* Renesas R9A06G032 SYSCTRL
+
+Required Properties:
+
+  - compatible: Must be:
+    - "renesas,r9a06g032-sysctrl"
+  - reg: Base address and length of the SYSCTRL IO block.
+  - #clock-cells: Must be 1
+
+Examples
+--------
+
+  - SYSCTRL node:
+
+	sysctrl: system-controller@4000c000 {
+		compatible = "renesas,r9a06g032-sysctrl";
+		reg = <0x4000c000 0x1000>;
+		#clock-cells = <1>;
+	};
+
+  - Other nodes can use the clocks provided by SYSCTRL as in:
+
+	#include <dt-bindings/clock/r9a06g032-sysctrl.h>
+	uart0: serial@40060000 {
+		compatible = "snps,dw-apb-uart";
+		reg = <0x40060000 0x400>;
+		interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
+		reg-shift = <2>;
+		reg-io-width = <4>;
+		clocks = <&sysctrl R9A06G032_CLK_UART0>;
+		clock-names = "baudclk";
+	};