Re: [PATCH v6 3/6] dt-bindings: clock: renesas,rzn1-clocks: document RZ/N1 clock driver

[Rob Herring]

On Wed, May 23, 2018 at 1:52 AM, M P  wrote:
> Hi Rob,
>
> On Tue, 22 May 2018 at 17:09, Rob Herring  wrote:
>
>> On Tue, May 22, 2018 at 11:01:23AM +0100, Michel Pollet wrote:
>> > The Renesas RZ/N1 Family (Part #R9A06G0xx) requires a driver
>> > to provide the SoC clock infrastructure for Linux.
>> >
>> > This documents the driver bindings.
>> >
>> > Signed-off-by: Michel Pollet 
>> > ---
>> >  .../bindings/clock/renesas,rzn1-clocks.txt | 44
> ++
>> >  1 file changed, 44 insertions(+)
>> >  create mode 100644
> Documentation/devicetree/bindings/clock/renesas,rzn1-clocks.txt
>> >
>> > diff --git
> a/Documentation/devicetree/bindings/clock/renesas,rzn1-clocks.txt
> b/Documentation/devicetree/bindings/clock/renesas,rzn1-clocks.txt
>> > new file mode 100644
>> > index 000..0c41137
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/clock/renesas,rzn1-clocks.txt
>> > @@ -0,0 +1,44 @@
>> > +* Renesas RZ/N1 Clock Driver
>> > +
>> > +This driver provides the clock infrastructure used by all the other
> drivers.
>
>> Bindings document h/w not drivers.
>
>> > +
>> > +One of the 'special' feature of this infrastructure is that Linux
> doesn't
>
>> Bindings are not just for Linux.
>
>> > +necessary 'own' all the clocks on the SoC, some other OS runs on
>> > +the Cortex-M3 core and that OS can access and claim it's own clocks.
>
>> How is this relevant to the binding?
>
> Well you just said it, if the binding is not just for linux, it's probably
> a good idea to mention it somewhere since I can promise you it's *not*
> documented in the hardware manual. Whatever this binding is for, it's
> relevant to point out it is different from the other clock drivers in the
> same directory... ?

That's not an uncommon issue (sometimes it's secure world that owns
the clocks, not even a different processor). I'm just not sure what I
do with this information. It doesn't tell me which clocks are owned by
the M3 or not.

>> > +
>> > +Required Properties:
>> > +
>> > +  - compatible: Must be
>> > +- "renesas,r9a06g032-clocks" for the RZ/N1D
>> > +and "renesas,rzn1-clocks" as a fallback.
>
>> Is "clocks" how the chip reference manual refers to this block?
>
> No, the block is called 'sysctrl' and has tons of other stuff in there.
> I've tried multiple times to get a 'sysctrl' driver in the previous
> versions of the patch, in various shape or form, and I can't because I'd be
> supposed to 'document' binding for stuff that has no code, no purpose, no
> testing, and have all wildly different topics. So, after some more
> lengthily discussion with Geert, we've decided to make the 'primary'
> feature of that block (clocks) as a driver, and see from there onward.

If you are referring to Geert's reply on v4, then this is not how I
interpreted it. I understood it as make the clock driver bind to the
sysctrl node and the clock driver can register other functions like
reset. Then later if you need multiple drivers, you can make an MFD
driver that binds to the sysctrl node and creates child devices to
bind sub-drivers (like clocks) to. But the DT doesn't change in the
process wrt clocks.

You don't have to have a driver for everything, but the binding should
be as complete as possible and done in an extendable way. The way you
have done it now is not. I can already see that at some point you will
want to break things and do something like this:

{
  compatible = "renesas,r9a06g032-sysctrl";
  ...
  {
compatible = "renesas,r9a06g032-clocks";
...
  };
};

Which is likely wrong because there's no need to have a child node
like that. The parent node can be the clock provider (and any other
provider). There are cases when child nodes do make sense, but we need
a more complete binding to make that decision. Evolving it one feature
at a time doesn't work.

Rob

Re: [PATCH v6 3/6] dt-bindings: clock: renesas,rzn1-clocks: document RZ/N1 clock driver

[Rob Herring]

On Tue, May 22, 2018 at 11:01:23AM +0100, Michel Pollet wrote:
> The Renesas RZ/N1 Family (Part #R9A06G0xx) requires a driver
> to provide the SoC clock infrastructure for Linux.
> 
> This documents the driver bindings.
> 
> Signed-off-by: Michel Pollet 
> ---
>  .../bindings/clock/renesas,rzn1-clocks.txt | 44 
> ++
>  1 file changed, 44 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/clock/renesas,rzn1-clocks.txt
> 
> diff --git a/Documentation/devicetree/bindings/clock/renesas,rzn1-clocks.txt 
> b/Documentation/devicetree/bindings/clock/renesas,rzn1-clocks.txt
> new file mode 100644
> index 000..0c41137
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,rzn1-clocks.txt
> @@ -0,0 +1,44 @@
> +* Renesas RZ/N1 Clock Driver
> +
> +This driver provides the clock infrastructure used by all the other drivers.

Bindings document h/w not drivers.

> +
> +One of the 'special' feature of this infrastructure is that Linux doesn't

Bindings are not just for Linux.

> +necessary 'own' all the clocks on the SoC, some other OS runs on
> +the Cortex-M3 core and that OS can access and claim it's own clocks.

How is this relevant to the binding?

> +
> +Required Properties:
> +
> +  - compatible: Must be
> +- "renesas,r9a06g032-clocks" for the RZ/N1D
> +and "renesas,rzn1-clocks" as a fallback.

Is "clocks" how the chip reference manual refers to this block?

> +  - reg: Base address and length of the memory resource used by the driver
> +  - #clock-cells: Must be 1
> +
> +Examples
> +
> +
> +  - Clock driver device node:
> +
> + clock: clocks@4000c000 {

clock-controller@...

> + compatible = "renesas,r9a06g032-clocks",
> + "renesas,rzn1-clocks";
> + reg = <0x4000c000 0x1000>;
> + status = "okay";

Don't show status in examples. (Plus, I doubt you ever want to have this 
disabled, so you don't need the property in your dts files either).

> + #clock-cells = <1>;
> + };
> +
> +
> +  - Other drivers can use the clocks as in:

s/drivers/nodes/

> +
> + uart0: serial@4006 {
> + compatible = "snps,dw-apb-uart";
> + reg = <0x4006 0x400>;
> + interrupts = ;
> + reg-shift = <2>;
> + reg-io-width = <4>;
> + clocks = < RZN1_CLK_UART0>;
> + clock-names = "baudclk";
> + };
> +Note the use of RZN1_CLK_UART0 -- these constants are declared in
> +the rzn1-clocks.h header file. These are not hardware based constants
> +and are Linux specific.

No, they are not Linux specific. They are part of the DT ABI. 

While it is not a requirement to base them on some h/w numbering, it is 
preferred if you can. That usually only works if you can base them on 
bit positions or register offsets.

Rob

[PATCH v6 3/6] dt-bindings: clock: renesas,rzn1-clocks: document RZ/N1 clock driver

[Michel Pollet]

The Renesas RZ/N1 Family (Part #R9A06G0xx) requires a driver
to provide the SoC clock infrastructure for Linux.

This documents the driver bindings.

Signed-off-by: Michel Pollet 
---
 .../bindings/clock/renesas,rzn1-clocks.txt | 44 ++
 1 file changed, 44 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/clock/renesas,rzn1-clocks.txt

diff --git a/Documentation/devicetree/bindings/clock/renesas,rzn1-clocks.txt 
b/Documentation/devicetree/bindings/clock/renesas,rzn1-clocks.txt
new file mode 100644
index 000..0c41137
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,rzn1-clocks.txt
@@ -0,0 +1,44 @@
+* Renesas RZ/N1 Clock Driver
+
+This driver provides the clock infrastructure used by all the other drivers.
+
+One of the 'special' feature of this infrastructure is that Linux doesn't
+necessary 'own' all the clocks on the SoC, some other OS runs on
+the Cortex-M3 core and that OS can access and claim it's own clocks.
+
+Required Properties:
+
+  - compatible: Must be
+- "renesas,r9a06g032-clocks" for the RZ/N1D
+and "renesas,rzn1-clocks" as a fallback.
+  - reg: Base address and length of the memory resource used by the driver
+  - #clock-cells: Must be 1
+
+Examples
+
+
+  - Clock driver device node:
+
+   clock: clocks@4000c000 {
+   compatible = "renesas,r9a06g032-clocks",
+   "renesas,rzn1-clocks";
+   reg = <0x4000c000 0x1000>;
+   status = "okay";
+   #clock-cells = <1>;
+   };
+
+
+  - Other drivers can use the clocks as in:
+
+   uart0: serial@4006 {
+   compatible = "snps,dw-apb-uart";
+   reg = <0x4006 0x400>;
+   interrupts = ;
+   reg-shift = <2>;
+   reg-io-width = <4>;
+   clocks = < RZN1_CLK_UART0>;
+   clock-names = "baudclk";
+   };
+Note the use of RZN1_CLK_UART0 -- these constants are declared in
+the rzn1-clocks.h header file. These are not hardware based constants
+and are Linux specific.
-- 
2.7.4