[v4,1/5,RFC] clk: shmobile: Add new Renesas CPG/MSSR DT bindings

On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse
Generator) and MSSR (Module Standby and Software Reset) blocks are
intimately connected, and share the same register block.

Hence it makes sense to describe these two blocks using a
single device node in DT, instead of using a hierarchical structure with
multiple nodes, using a mix of generic and SoC-specific bindings.

These new DT bindings are intended to replace the existing DT bindings
for CPG core clocks ("renesas,*-cpg-clocks", "renesas,cpg-div6-clock")
and module clocks ("renesas,*-mstp-clocks"), at least for new SoCs.

This will make it easier to add module reset support later, which is
currently not implemented, and difficult to achieve using the existing
bindings due to the intertwined register layout.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
  - No changes,

v3:
  - Integrate CPG and MSSR,

v2:
  - Switch from MSTP to MSSR.
---
 .../devicetree/bindings/clock/renesas,cpg-mssr.txt | 71 ++++++++++++++++++++++
 include/dt-bindings/clock/renesas-cpg-mssr.h       | 15 +++++
 2 files changed, 86 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt
 create mode 100644 include/dt-bindings/clock/renesas-cpg-mssr.h

Hi Geert,

Quoting Geert Uytterhoeven (2015-10-16 05:49:16)
> On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse
> Generator) and MSSR (Module Standby and Software Reset) blocks are
> intimately connected, and share the same register block.
> 
> Hence it makes sense to describe these two blocks using a
> single device node in DT, instead of using a hierarchical structure with
> multiple nodes, using a mix of generic and SoC-specific bindings.
> 
> These new DT bindings are intended to replace the existing DT bindings
> for CPG core clocks ("renesas,*-cpg-clocks", "renesas,cpg-div6-clock")
> and module clocks ("renesas,*-mstp-clocks"), at least for new SoCs.
> 
> This will make it easier to add module reset support later, which is
> currently not implemented, and difficult to achieve using the existing
> bindings due to the intertwined register layout.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks for re-working the binding per our discussion at ELC-E. Please
feel free to add my Ack to patches #1 and #2.

Regards,
Mike

> ---
> v4:
>   - No changes,
> 
> v3:
>   - Integrate CPG and MSSR,
> 
> v2:
>   - Switch from MSTP to MSSR.
> ---
>  .../devicetree/bindings/clock/renesas,cpg-mssr.txt | 71 ++++++++++++++++++++++
>  include/dt-bindings/clock/renesas-cpg-mssr.h       | 15 +++++
>  2 files changed, 86 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt
>  create mode 100644 include/dt-bindings/clock/renesas-cpg-mssr.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt b/Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt
> new file mode 100644
> index 0000000000000000..a56836aa2131a8db
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt
> @@ -0,0 +1,71 @@
> +* Renesas Clock Pulse Generator / Module Standby and Software Reset
> +
> +On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse Generator)
> +and MSSR (Module Standby and Software Reset) blocks are intimately connected,
> +and share the same register block.
> +
> +They provide the following functionalities:
> +  - The CPG block generates various core clocks,
> +  - The MSSR block provides two functions:
> +      1. Module Standby, providing a Clock Domain to control the clock supply
> +        to individual SoC devices,
> +      2. Reset Control, to perform a software reset of individual SoC devices.
> +
> +Required Properties:
> +  - compatible: Must be one of:
> +      - "renesas,r8a7791-cpg-mssr" for the r8a7791 SoC
> +      - "renesas,r8a7795-cpg-mssr" for the r8a7795 SoC
> +
> +  - reg: Base address and length of the memory resource used by the CPG/MSSR
> +    block
> +
> +  - clocks: References to external parent clocks, one entry for each entry in
> +    clock-names
> +  - clock-names: List of external parent clock names. Valid names are:
> +      - "extal" (r8a7791, r8a7795)
> +      - "extalr" (r8a7795)
> +      - "usb_extal" (r8a7791)
> +
> +  - #clock-cells: Must be 2
> +      - For CPG core clocks, the two clock specifier cells must be "CPG_CORE"
> +       and a core clock reference, as defined in
> +       <dt-bindings/clock/*-cpg-mssr.h>.
> +      - For module clocks, the two clock specifier cells must be "CPG_MOD" and
> +       a module number, as defined in the datasheet.
> +
> +  - #power-domain-cells: Must be 0
> +      - SoC devices that are part of the CPG/MSSR Clock Domain and can be
> +       power-managed through Module Standby should refer to the CPG device
> +       node in their "power-domains" property, as documented by the generic PM
> +       Domain bindings in
> +       Documentation/devicetree/bindings/power/power_domain.txt.
> +
> +
> +Examples
> +--------
> +
> +  - CPG device node:
> +
> +       cpg: clock-controller@e6150000 {
> +               compatible = "renesas,r8a7795-cpg-mssr";
> +               reg = <0 0xe6150000 0 0x1000>;
> +               clocks = <&extal_clk>, <&extalr_clk>;
> +               clock-names = "extal", "extalr";
> +               #clock-cells = <2>;
> +               #power-domain-cells = <0>;
> +       };
> +
> +
> +  - CPG/MSSR Clock Domain member device node:
> +
> +       scif2: serial@e6e88000 {
> +               compatible = "renesas,scif-r8a7795", "renesas,scif";
> +               reg = <0 0xe6e88000 0 64>;
> +               interrupts = <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>;
> +               clocks = <&cpg CPG_MOD 310>;
> +               clock-names = "sci_ick";
> +               dmas = <&dmac1 0x13>, <&dmac1 0x12>;
> +               dma-names = "tx", "rx";
> +               power-domains = <&cpg>;
> +               status = "disabled";
> +       };
> diff --git a/include/dt-bindings/clock/renesas-cpg-mssr.h b/include/dt-bindings/clock/renesas-cpg-mssr.h
> new file mode 100644
> index 0000000000000000..569a3cc33ffb5bc7
> --- /dev/null
> +++ b/include/dt-bindings/clock/renesas-cpg-mssr.h
> @@ -0,0 +1,15 @@
> +/*
> + * Copyright (C) 2015 Renesas Electronics Corp.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#ifndef __DT_BINDINGS_CLOCK_RENESAS_CPG_MSSR_H__
> +#define __DT_BINDINGS_CLOCK_RENESAS_CPG_MSSR_H__
> +
> +#define CPG_CORE                       0       /* Core Clock */
> +#define CPG_MOD                                1       /* Module Clock */
> +
> +#endif /* __DT_BINDINGS_CLOCK_RENESAS_CPG_MSSR_H__ */
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Oct 16, 2015 at 2:49 PM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt
> @@ -0,0 +1,71 @@
> +* Renesas Clock Pulse Generator / Module Standby and Software Reset
> +
> +On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse Generator)
> +and MSSR (Module Standby and Software Reset) blocks are intimately connected,
> +and share the same register block.
> +
> +They provide the following functionalities:
> +  - The CPG block generates various core clocks,
> +  - The MSSR block provides two functions:
> +      1. Module Standby, providing a Clock Domain to control the clock supply
> +        to individual SoC devices,
> +      2. Reset Control, to perform a software reset of individual SoC devices.
> +
> +Required Properties:
> +  - compatible: Must be one of:
> +      - "renesas,r8a7791-cpg-mssr" for the r8a7791 SoC

I'll drop the reference to "r8a7791", as we won't convert r8a7791 (yet).

> +      - "renesas,r8a7795-cpg-mssr" for the r8a7795 SoC
> +
> +  - reg: Base address and length of the memory resource used by the CPG/MSSR
> +    block
> +
> +  - clocks: References to external parent clocks, one entry for each entry in
> +    clock-names
> +  - clock-names: List of external parent clock names. Valid names are:
> +      - "extal" (r8a7791, r8a7795)
> +      - "extalr" (r8a7795)
> +      - "usb_extal" (r8a7791)

Likewise.

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Geert,

On Tue, Oct 20, 2015 at 9:16 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Fri, Oct 16, 2015 at 2:49 PM, Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt
>> @@ -0,0 +1,71 @@
>> +* Renesas Clock Pulse Generator / Module Standby and Software Reset
>> +
>> +On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse Generator)
>> +and MSSR (Module Standby and Software Reset) blocks are intimately connected,
>> +and share the same register block.
>> +
>> +They provide the following functionalities:
>> +  - The CPG block generates various core clocks,
>> +  - The MSSR block provides two functions:
>> +      1. Module Standby, providing a Clock Domain to control the clock supply
>> +        to individual SoC devices,
>> +      2. Reset Control, to perform a software reset of individual SoC devices.
>> +
>> +Required Properties:
>> +  - compatible: Must be one of:
>> +      - "renesas,r8a7791-cpg-mssr" for the r8a7791 SoC
>
> I'll drop the reference to "r8a7791", as we won't convert r8a7791 (yet).
>
>> +      - "renesas,r8a7795-cpg-mssr" for the r8a7795 SoC
>> +
>> +  - reg: Base address and length of the memory resource used by the CPG/MSSR
>> +    block
>> +
>> +  - clocks: References to external parent clocks, one entry for each entry in
>> +    clock-names
>> +  - clock-names: List of external parent clock names. Valid names are:
>> +      - "extal" (r8a7791, r8a7795)
>> +      - "extalr" (r8a7795)
>> +      - "usb_extal" (r8a7791)
>
> Likewise.

Yeah, dropping r8a7791 from the initial version of this DT binding
seems like a good idea to me as well.

All looks good to me. Thanks for preparing this nicely written DT
binding document!

Reviewed-by: Magnus Damm <damm+renesas@opensource.se>

Cheers,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello,

On Wednesday 21 October 2015 01:01:03 Magnus Damm wrote:
> On Tue, Oct 20, 2015 at 9:16 PM, Geert Uytterhoeven wrote:
> > On Fri, Oct 16, 2015 at 2:49 PM, Geert Uytterhoeven wrote:
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt
> >> @@ -0,0 +1,71 @@
> >> +* Renesas Clock Pulse Generator / Module Standby and Software Reset
> >> +
> >> +On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse
> >> Generator)
> >> +and MSSR (Module Standby and Software Reset) blocks are intimately
> >> connected,
> >> +and share the same register block.
> >> +
> >> +They provide the following functionalities:
> >> +  - The CPG block generates various core clocks,
> >> +  - The MSSR block provides two functions:
> >> +      1. Module Standby, providing a Clock Domain to control the clock
> >> supply
> >> +        to individual SoC devices,
> >> +      2. Reset Control, to perform a software reset of individual SoC
> >> devices.
> >> +
> >> +Required Properties:
> >> +  - compatible: Must be one of:
> >> +      - "renesas,r8a7791-cpg-mssr" for the r8a7791 SoC
> > 
> > I'll drop the reference to "r8a7791", as we won't convert r8a7791 (yet).

I'm fine with supporting r8a7795 only in the initial patch set, but when do we 
plan to convert r8a7791 ? I think it would be a good idea to validate these 
bindings on r8a7791 before considering them as stable.

> >> +      - "renesas,r8a7795-cpg-mssr" for the r8a7795 SoC
> >> +
> >> +  - reg: Base address and length of the memory resource used by the
> >> CPG/MSSR
> >> +    block
> >> +
> >> +  - clocks: References to external parent clocks, one entry for each
> >> entry in
> >> +    clock-names
> >> +  - clock-names: List of external parent clock names. Valid names are:
> >> +      - "extal" (r8a7791, r8a7795)
> >> +      - "extalr" (r8a7795)
> >> +      - "usb_extal" (r8a7791)
> > 
> > Likewise.
> 
> Yeah, dropping r8a7791 from the initial version of this DT binding
> seems like a good idea to me as well.
> 
> All looks good to me. Thanks for preparing this nicely written DT
> binding document!
> 
> Reviewed-by: Magnus Damm <damm+renesas@opensource.se>

Hi Laurent,

On Fri, Oct 23, 2015 at 1:05 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wednesday 21 October 2015 01:01:03 Magnus Damm wrote:
>> On Tue, Oct 20, 2015 at 9:16 PM, Geert Uytterhoeven wrote:
>> > On Fri, Oct 16, 2015 at 2:49 PM, Geert Uytterhoeven wrote:
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt
>> >> @@ -0,0 +1,71 @@
>> >> +* Renesas Clock Pulse Generator / Module Standby and Software Reset
>> >> +
>> >> +On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse
>> >> Generator)
>> >> +and MSSR (Module Standby and Software Reset) blocks are intimately
>> >> connected,
>> >> +and share the same register block.
>> >> +
>> >> +They provide the following functionalities:
>> >> +  - The CPG block generates various core clocks,
>> >> +  - The MSSR block provides two functions:
>> >> +      1. Module Standby, providing a Clock Domain to control the clock
>> >> supply
>> >> +        to individual SoC devices,
>> >> +      2. Reset Control, to perform a software reset of individual SoC
>> >> devices.
>> >> +
>> >> +Required Properties:
>> >> +  - compatible: Must be one of:
>> >> +      - "renesas,r8a7791-cpg-mssr" for the r8a7791 SoC
>> >
>> > I'll drop the reference to "r8a7791", as we won't convert r8a7791 (yet).
>
> I'm fine with supporting r8a7795 only in the initial patch set, but when do we
> plan to convert r8a7791 ? I think it would be a good idea to validate these

That's a political question...

> bindings on r8a7791 before considering them as stable.

I did the validation on both r8a7795/salvator-x and r8a7791/koelsch.
Please try branch topic/cpg-mssr-v4 of renesas-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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Geert,

Thank you for the patch.

On Friday 16 October 2015 14:49:16 Geert Uytterhoeven wrote:
> On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse
> Generator) and MSSR (Module Standby and Software Reset) blocks are
> intimately connected, and share the same register block.
> 
> Hence it makes sense to describe these two blocks using a
> single device node in DT, instead of using a hierarchical structure with
> multiple nodes, using a mix of generic and SoC-specific bindings.
> 
> These new DT bindings are intended to replace the existing DT bindings
> for CPG core clocks ("renesas,*-cpg-clocks", "renesas,cpg-div6-clock")
> and module clocks ("renesas,*-mstp-clocks"), at least for new SoCs.
> 
> This will make it easier to add module reset support later, which is
> currently not implemented, and difficult to achieve using the existing
> bindings due to the intertwined register layout.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v4:
>   - No changes,
> 
> v3:
>   - Integrate CPG and MSSR,
> 
> v2:
>   - Switch from MSTP to MSSR.
> ---
>  .../devicetree/bindings/clock/renesas,cpg-mssr.txt | 71 +++++++++++++++++++
>  include/dt-bindings/clock/renesas-cpg-mssr.h       | 15 +++++
>  2 files changed, 86 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt create mode
> 100644 include/dt-bindings/clock/renesas-cpg-mssr.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt
> b/Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt new file
> mode 100644
> index 0000000000000000..a56836aa2131a8db
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt
> @@ -0,0 +1,71 @@
> +* Renesas Clock Pulse Generator / Module Standby and Software Reset
> +
> +On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse
> Generator)
> +and MSSR (Module Standby and Software Reset) blocks are intimately
> connected,
> +and share the same register block.
> +
> +They provide the following functionalities:
> +  - The CPG block generates various core clocks,
> +  - The MSSR block provides two functions:
> +      1. Module Standby, providing a Clock Domain to control the clock
> supply
> +	 to individual SoC devices,
> +      2. Reset Control, to perform a software reset of individual SoC
> devices.
> +
> +Required Properties:
> +  - compatible: Must be one of:
> +      - "renesas,r8a7791-cpg-mssr" for the r8a7791 SoC
> +      - "renesas,r8a7795-cpg-mssr" for the r8a7795 SoC
> +
> +  - reg: Base address and length of the memory resource used by the
> CPG/MSSR
> +    block
> +
> +  - clocks: References to external parent clocks, one entry for each entry
> in
> +    clock-names
> +  - clock-names: List of external parent clock names. Valid names are:
> +      - "extal" (r8a7791, r8a7795)
> +      - "extalr" (r8a7795)
> +      - "usb_extal" (r8a7791)
> +
> +  - #clock-cells: Must be 2
> +      - For CPG core clocks, the two clock specifier cells must be
> "CPG_CORE"
> +	and a core clock reference, as defined in
> +	<dt-bindings/clock/*-cpg-mssr.h>.
> +      - For module clocks, the two clock specifier cells must be "CPG_MOD"
> and
> +	a module number, as defined in the datasheet.
> +
> +  - #power-domain-cells: Must be 0
> +      - SoC devices that are part of the CPG/MSSR Clock Domain and can be
> +	power-managed through Module Standby should refer to the CPG device
> +	node in their "power-domains" property, as documented by the generic PM
> +	Domain bindings in
> +	Documentation/devicetree/bindings/power/power_domain.txt.
> +
> +
> +Examples
> +--------
> +
> +  - CPG device node:
> +
> +	cpg: clock-controller@e6150000 {
> +		compatible = "renesas,r8a7795-cpg-mssr";
> +		reg = <0 0xe6150000 0 0x1000>;
> +		clocks = <&extal_clk>, <&extalr_clk>;
> +		clock-names = "extal", "extalr";
> +		#clock-cells = <2>;
> +		#power-domain-cells = <0>;
> +	};
> +
> +
> +  - CPG/MSSR Clock Domain member device node:

Would it make sense to show two examples, one for a device whose driver 
manages the MSTP clock manually, and another one for a device whose driver 
delegates that to the power domain ?

I hate using the word driver in DT bindings, but unfortunately that's 
essentially what it boils down to here as the decision to specify the power 
domain is really based on the driver, not on the hardware.

Apart from that the rest looks good to me.

> +	scif2: serial@e6e88000 {
> +		compatible = "renesas,scif-r8a7795", "renesas,scif";
> +		reg = <0 0xe6e88000 0 64>;
> +		interrupts = <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cpg CPG_MOD 310>;
> +		clock-names = "sci_ick";
> +		dmas = <&dmac1 0x13>, <&dmac1 0x12>;
> +		dma-names = "tx", "rx";
> +		power-domains = <&cpg>;
> +		status = "disabled";
> +	};
> diff --git a/include/dt-bindings/clock/renesas-cpg-mssr.h
> b/include/dt-bindings/clock/renesas-cpg-mssr.h new file mode 100644
> index 0000000000000000..569a3cc33ffb5bc7
> --- /dev/null
> +++ b/include/dt-bindings/clock/renesas-cpg-mssr.h
> @@ -0,0 +1,15 @@
> +/*
> + * Copyright (C) 2015 Renesas Electronics Corp.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#ifndef __DT_BINDINGS_CLOCK_RENESAS_CPG_MSSR_H__
> +#define __DT_BINDINGS_CLOCK_RENESAS_CPG_MSSR_H__
> +
> +#define CPG_CORE			0	/* Core Clock */
> +#define CPG_MOD				1	/* Module Clock */
> +
> +#endif /* __DT_BINDINGS_CLOCK_RENESAS_CPG_MSSR_H__ */

Hi Geert,

On Friday 23 October 2015 13:09:22 Geert Uytterhoeven wrote:
> On Fri, Oct 23, 2015 at 1:05 PM, Laurent Pinchart wrote:
> > On Wednesday 21 October 2015 01:01:03 Magnus Damm wrote:
> >> On Tue, Oct 20, 2015 at 9:16 PM, Geert Uytterhoeven wrote:
> >>> On Fri, Oct 16, 2015 at 2:49 PM, Geert Uytterhoeven wrote:
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt
> >>>> @@ -0,0 +1,71 @@
> >>>> +* Renesas Clock Pulse Generator / Module Standby and Software Reset
> >>>> +
> >>>> +On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse
> >>>> Generator)
> >>>> +and MSSR (Module Standby and Software Reset) blocks are intimately
> >>>> connected,
> >>>> +and share the same register block.
> >>>> +
> >>>> +They provide the following functionalities:
> >>>> +  - The CPG block generates various core clocks,
> >>>> +  - The MSSR block provides two functions:
> >>>> +      1. Module Standby, providing a Clock Domain to control the
> >>>> clock supply
> >>>> +        to individual SoC devices,
> >>>> +      2. Reset Control, to perform a software reset of individual SoC
> >>>> devices.
> >>>> +
> >>>> +Required Properties:
> >>>> +  - compatible: Must be one of:
> >>>> +      - "renesas,r8a7791-cpg-mssr" for the r8a7791 SoC
> >>> 
> >>> I'll drop the reference to "r8a7791", as we won't convert r8a7791
> >>> (yet).
> > 
> > I'm fine with supporting r8a7795 only in the initial patch set, but when
> > do we plan to convert r8a7791 ? I think it would be a good idea to
> > validate these
>
> That's a political question...
> 
> > bindings on r8a7791 before considering them as stable.
> 
> I did the validation on both r8a7795/salvator-x and r8a7791/koelsch.
> Please try branch topic/cpg-mssr-v4 of renesas-drivers.

If it works what's blocking r8a7791 support from being upstreamed ? :-)

Hi Laurent,

On Fri, Oct 23, 2015 at 1:10 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Friday 16 October 2015 14:49:16 Geert Uytterhoeven wrote:
>> On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse
>> Generator) and MSSR (Module Standby and Software Reset) blocks are
>> intimately connected, and share the same register block.
>>
>> Hence it makes sense to describe these two blocks using a
>> single device node in DT, instead of using a hierarchical structure with
>> multiple nodes, using a mix of generic and SoC-specific bindings.
>>
>> These new DT bindings are intended to replace the existing DT bindings
>> for CPG core clocks ("renesas,*-cpg-clocks", "renesas,cpg-div6-clock")
>> and module clocks ("renesas,*-mstp-clocks"), at least for new SoCs.
>>
>> This will make it easier to add module reset support later, which is
>> currently not implemented, and difficult to achieve using the existing
>> bindings due to the intertwined register layout.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt
>> @@ -0,0 +1,71 @@
>> +* Renesas Clock Pulse Generator / Module Standby and Software Reset
>> +
>> +On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse
>> Generator)
>> +and MSSR (Module Standby and Software Reset) blocks are intimately
>> connected,
>> +and share the same register block.
>> +
>> +They provide the following functionalities:
>> +  - The CPG block generates various core clocks,
>> +  - The MSSR block provides two functions:
>> +      1. Module Standby, providing a Clock Domain to control the clock
>> supply
>> +      to individual SoC devices,
>> +      2. Reset Control, to perform a software reset of individual SoC
>> devices.

[...]

>> +  - #power-domain-cells: Must be 0
>> +      - SoC devices that are part of the CPG/MSSR Clock Domain and can be
>> +     power-managed through Module Standby should refer to the CPG device
>> +     node in their "power-domains" property, as documented by the generic PM
>> +     Domain bindings in
>> +     Documentation/devicetree/bindings/power/power_domain.txt.
>> +
>> +
>> +Examples
>> +--------
>> +
>> +  - CPG device node:
>> +
>> +     cpg: clock-controller@e6150000 {
>> +             compatible = "renesas,r8a7795-cpg-mssr";
>> +             reg = <0 0xe6150000 0 0x1000>;
>> +             clocks = <&extal_clk>, <&extalr_clk>;
>> +             clock-names = "extal", "extalr";
>> +             #clock-cells = <2>;
>> +             #power-domain-cells = <0>;
>> +     };
>> +
>> +
>> +  - CPG/MSSR Clock Domain member device node:
>
> Would it make sense to show two examples, one for a device whose driver
> manages the MSTP clock manually, and another one for a device whose driver
> delegates that to the power domain ?
>
> I hate using the word driver in DT bindings, but unfortunately that's
> essentially what it boils down to here as the decision to specify the power
> domain is really based on the driver, not on the hardware.

IMHO it's not the driver, but the on-SoC module and its representation in DT.
Cfr. commit 797a0626e08ca4af ("ARM: shmobile: r8a7791 dtsi: Add CPG/MSTP Clock
Domain", which states:

|   Add "power-domains" properties to all device nodes for devices that are
|   part of the CPG/MSTP Clock Domain and can be power-managed through an
|   MSTP clock.  This applies to most on-SoC devices, which have a
|   one-to-one mapping from SoC device to DT device node.  Notable
|   exceptions are the "display" and "sound" nodes, which represent multiple
|   SoC devices, each having their own MSTP clocks.

If a single device block (sharing the same register space), represented in
DT as a single device node, actually represents multiple modules, there's no
one-to-one mapping from SoC device to DT device node.
Hence the device node will have multiple module clocks, and the driver will
have to take care of managing them explicitly.

The register space sharing is an SoC-specific issue.
The single device node is a DT binding issue.

Perhaps such devices should use subnodes, so each of them can represent a
module, each with its own module clock.

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Geert,

On Monday 26 October 2015 20:02:45 Geert Uytterhoeven wrote:
> On Fri, Oct 23, 2015 at 1:10 PM, Laurent Pinchart wrote:
> > On Friday 16 October 2015 14:49:16 Geert Uytterhoeven wrote:
> >> On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse
> >> Generator) and MSSR (Module Standby and Software Reset) blocks are
> >> intimately connected, and share the same register block.
> >> 
> >> Hence it makes sense to describe these two blocks using a
> >> single device node in DT, instead of using a hierarchical structure with
> >> multiple nodes, using a mix of generic and SoC-specific bindings.
> >> 
> >> These new DT bindings are intended to replace the existing DT bindings
> >> for CPG core clocks ("renesas,*-cpg-clocks", "renesas,cpg-div6-clock")
> >> and module clocks ("renesas,*-mstp-clocks"), at least for new SoCs.
> >> 
> >> This will make it easier to add module reset support later, which is
> >> currently not implemented, and difficult to achieve using the existing
> >> bindings due to the intertwined register layout.
> >> 
> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >> 
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt
> >> @@ -0,0 +1,71 @@
> >> +* Renesas Clock Pulse Generator / Module Standby and Software Reset
> >> +
> >> +On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse
> >> Generator)
> >> +and MSSR (Module Standby and Software Reset) blocks are intimately
> >> connected,
> >> +and share the same register block.
> >> +
> >> +They provide the following functionalities:
> >> +  - The CPG block generates various core clocks,
> >> +  - The MSSR block provides two functions:
> >> +      1. Module Standby, providing a Clock Domain to control the clock
> >> supply
> >> +      to individual SoC devices,
> >> +      2. Reset Control, to perform a software reset of individual SoC
> >> devices.
> 
> [...]
> 
> >> +  - #power-domain-cells: Must be 0
> >> +      - SoC devices that are part of the CPG/MSSR Clock Domain and can
> >> be
> >> +     power-managed through Module Standby should refer to the CPG device
> >> +     node in their "power-domains" property, as documented by the
> >> generic PM +     Domain bindings in
> >> +     Documentation/devicetree/bindings/power/power_domain.txt.
> >> +
> >> +
> >> +Examples
> >> +--------
> >> +
> >> +  - CPG device node:
> >> +
> >> +     cpg: clock-controller@e6150000 {
> >> +             compatible = "renesas,r8a7795-cpg-mssr";
> >> +             reg = <0 0xe6150000 0 0x1000>;
> >> +             clocks = <&extal_clk>, <&extalr_clk>;
> >> +             clock-names = "extal", "extalr";
> >> +             #clock-cells = <2>;
> >> +             #power-domain-cells = <0>;
> >> +     };
> >> +
> >> +
> > 
> >> +  - CPG/MSSR Clock Domain member device node:
> > Would it make sense to show two examples, one for a device whose driver
> > manages the MSTP clock manually, and another one for a device whose driver
> > delegates that to the power domain ?
> > 
> > I hate using the word driver in DT bindings, but unfortunately that's
> > essentially what it boils down to here as the decision to specify the
> > power domain is really based on the driver, not on the hardware.
> 
> IMHO it's not the driver, but the on-SoC module and its representation in
> DT. Cfr. commit 797a0626e08ca4af ("ARM: shmobile: r8a7791 dtsi: Add
> CPG/MSTP Clock Domain", which states:
>
> |   Add "power-domains" properties to all device nodes for devices that are
> |   part of the CPG/MSTP Clock Domain and can be power-managed through an
> |   MSTP clock.  This applies to most on-SoC devices, which have a
> |   one-to-one mapping from SoC device to DT device node.  Notable
> |   exceptions are the "display" and "sound" nodes, which represent multiple
> |   SoC devices, each having their own MSTP clocks.

You're quoting your own documentation to support your point, that's not fair 
:-)

We're using power domains to gate clocks. The fact that it's not related to 
power supplies can already be borderline, but I can buy the argument that 
clocks relate to power consumption here. However, where the power domain 
abstraction is really abused is that we're adding all kind of devices to a 
single power domain while they're controlled by one clock gate each. That's a 
software hack, and we're using DT to tell whether our core code should control 
clock gating or not. That's not a hardware description, sorry.

> If a single device block (sharing the same register space), represented in
> DT as a single device node, actually represents multiple modules, there's no
> one-to-one mapping from SoC device to DT device node.
>
> Hence the device node will have multiple module clocks, and the driver will
> have to take care of managing them explicitly.

There can be other reasons why the clocks need to be controlled explicitly by 
the driver. At the end of the end it's a per-driver decision whether it wants 
to delegate clock management to runtime PM (which will be the default cause) 
or handle it itself.

> The register space sharing is an SoC-specific issue.
> The single device node is a DT binding issue.
> 
> Perhaps such devices should use subnodes, so each of them can represent a
> module, each with its own module clock.

I'm not sure to see how that would help, as we'd have a single struct device 
in that case (associated with the top-level DT node), so runtime PM couldn't 
be used to manage clocks independently.

Hi Laurent,

CC linux-pm

On Tue, Oct 27, 2015 at 2:34 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 26 October 2015 20:02:45 Geert Uytterhoeven wrote:
>> On Fri, Oct 23, 2015 at 1:10 PM, Laurent Pinchart wrote:
>> > On Friday 16 October 2015 14:49:16 Geert Uytterhoeven wrote:
>> >> On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse
>> >> Generator) and MSSR (Module Standby and Software Reset) blocks are
>> >> intimately connected, and share the same register block.
>> >>
>> >> Hence it makes sense to describe these two blocks using a
>> >> single device node in DT, instead of using a hierarchical structure with
>> >> multiple nodes, using a mix of generic and SoC-specific bindings.
>> >>
>> >> These new DT bindings are intended to replace the existing DT bindings
>> >> for CPG core clocks ("renesas,*-cpg-clocks", "renesas,cpg-div6-clock")
>> >> and module clocks ("renesas,*-mstp-clocks"), at least for new SoCs.
>> >>
>> >> This will make it easier to add module reset support later, which is
>> >> currently not implemented, and difficult to achieve using the existing
>> >> bindings due to the intertwined register layout.
>> >>
>> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> >>
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt
>> >> @@ -0,0 +1,71 @@
>> >> +* Renesas Clock Pulse Generator / Module Standby and Software Reset
>> >> +
>> >> +On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse
>> >> Generator)
>> >> +and MSSR (Module Standby and Software Reset) blocks are intimately
>> >> connected,
>> >> +and share the same register block.
>> >> +
>> >> +They provide the following functionalities:
>> >> +  - The CPG block generates various core clocks,
>> >> +  - The MSSR block provides two functions:
>> >> +      1. Module Standby, providing a Clock Domain to control the clock
>> >> supply
>> >> +      to individual SoC devices,
>> >> +      2. Reset Control, to perform a software reset of individual SoC
>> >> devices.
>>
>> [...]
>>
>> >> +  - #power-domain-cells: Must be 0
>> >> +      - SoC devices that are part of the CPG/MSSR Clock Domain and can
>> >> be
>> >> +     power-managed through Module Standby should refer to the CPG device
>> >> +     node in their "power-domains" property, as documented by the
>> >> generic PM +     Domain bindings in
>> >> +     Documentation/devicetree/bindings/power/power_domain.txt.
>> >> +
>> >> +
>> >> +Examples
>> >> +--------
>> >> +
>> >> +  - CPG device node:
>> >> +
>> >> +     cpg: clock-controller@e6150000 {
>> >> +             compatible = "renesas,r8a7795-cpg-mssr";
>> >> +             reg = <0 0xe6150000 0 0x1000>;
>> >> +             clocks = <&extal_clk>, <&extalr_clk>;
>> >> +             clock-names = "extal", "extalr";
>> >> +             #clock-cells = <2>;
>> >> +             #power-domain-cells = <0>;
>> >> +     };
>> >> +
>> >> +
>> >
>> >> +  - CPG/MSSR Clock Domain member device node:
>> > Would it make sense to show two examples, one for a device whose driver
>> > manages the MSTP clock manually, and another one for a device whose driver
>> > delegates that to the power domain ?
>> >
>> > I hate using the word driver in DT bindings, but unfortunately that's
>> > essentially what it boils down to here as the decision to specify the
>> > power domain is really based on the driver, not on the hardware.
>>
>> IMHO it's not the driver, but the on-SoC module and its representation in
>> DT. Cfr. commit 797a0626e08ca4af ("ARM: shmobile: r8a7791 dtsi: Add
>> CPG/MSTP Clock Domain", which states:
>>
>> |   Add "power-domains" properties to all device nodes for devices that are
>> |   part of the CPG/MSTP Clock Domain and can be power-managed through an
>> |   MSTP clock.  This applies to most on-SoC devices, which have a
>> |   one-to-one mapping from SoC device to DT device node.  Notable
>> |   exceptions are the "display" and "sound" nodes, which represent multiple
>> |   SoC devices, each having their own MSTP clocks.
>
> You're quoting your own documentation to support your point, that's not fair
> :-)

I quoted it because it explains what was done.

> We're using power domains to gate clocks. The fact that it's not related to
> power supplies can already be borderline, but I can buy the argument that
> clocks relate to power consumption here. However, where the power domain
> abstraction is really abused is that we're adding all kind of devices to a
> single power domain while they're controlled by one clock gate each. That's a
> software hack, and we're using DT to tell whether our core code should control
> clock gating or not. That's not a hardware description, sorry.

IMHO the "power-domains" property naming is wrong: it should have been
"pm-domains", as it's following the spirit of the Linux PM Domain abstraction.

PM Domain = Collection of devices treated similarly w.r.t. power management

"similarly" here means "using module clocks". And yes the datasheet states
that's what the module clocks do: control supply of the clock signal to the
module.

>> If a single device block (sharing the same register space), represented in
>> DT as a single device node, actually represents multiple modules, there's no
>> one-to-one mapping from SoC device to DT device node.
>>
>> Hence the device node will have multiple module clocks, and the driver will
>> have to take care of managing them explicitly.
>
> There can be other reasons why the clocks need to be controlled explicitly by
> the driver. At the end of the end it's a per-driver decision whether it wants
> to delegate clock management to runtime PM (which will be the default cause)
> or handle it itself.

The driver can still override if it feels the need.

>> The register space sharing is an SoC-specific issue.
>> The single device node is a DT binding issue.
>>
>> Perhaps such devices should use subnodes, so each of them can represent a
>> module, each with its own module clock.
>
> I'm not sure to see how that would help, as we'd have a single struct device
> in that case (associated with the top-level DT node), so runtime PM couldn't
> be used to manage clocks independently.

The driver for the subnode device can still create child devices, can't it?

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Geert,

On Tuesday 27 October 2015 09:14:15 Geert Uytterhoeven wrote:
> On Tue, Oct 27, 2015 at 2:34 AM, Laurent Pinchart wrote:
> > On Monday 26 October 2015 20:02:45 Geert Uytterhoeven wrote:
> >> On Fri, Oct 23, 2015 at 1:10 PM, Laurent Pinchart wrote:
> >>> On Friday 16 October 2015 14:49:16 Geert Uytterhoeven wrote:
> >>>> On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse
> >>>> Generator) and MSSR (Module Standby and Software Reset) blocks are
> >>>> intimately connected, and share the same register block.
> >>>> 
> >>>> Hence it makes sense to describe these two blocks using a
> >>>> single device node in DT, instead of using a hierarchical structure
> >>>> with multiple nodes, using a mix of generic and SoC-specific bindings.
> >>>> 
> >>>> These new DT bindings are intended to replace the existing DT bindings
> >>>> for CPG core clocks ("renesas,*-cpg-clocks", "renesas,cpg-div6-clock")
> >>>> and module clocks ("renesas,*-mstp-clocks"), at least for new SoCs.
> >>>> 
> >>>> This will make it easier to add module reset support later, which is
> >>>> currently not implemented, and difficult to achieve using the existing
> >>>> bindings due to the intertwined register layout.
> >>>> 
> >>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >>>> 
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mssr.txt
> >>>> @@ -0,0 +1,71 @@
> >>>> +* Renesas Clock Pulse Generator / Module Standby and Software Reset
> >>>> +
> >>>> +On Renesas ARM SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse
> >>>> Generator)
> >>>> +and MSSR (Module Standby and Software Reset) blocks are intimately
> >>>> connected,
> >>>> +and share the same register block.
> >>>> +
> >>>> +They provide the following functionalities:
> >>>> +  - The CPG block generates various core clocks,
> >>>> +  - The MSSR block provides two functions:
> >>>> +      1. Module Standby, providing a Clock Domain to control the
> >>>> clock supply
> >>>> +      to individual SoC devices,
> >>>> +      2. Reset Control, to perform a software reset of individual SoC
> >>>> devices.
> >> 
> >> [...]
> >> 
> >>>> +  - #power-domain-cells: Must be 0
> >>>> +      - SoC devices that are part of the CPG/MSSR Clock Domain and
> >>>> can be
> >>>> +     power-managed through Module Standby should refer to the CPG
> >>>> device
> >>>> +     node in their "power-domains" property, as documented by the
> >>>> generic PM
> >>>> +     Domain bindings in
> >>>> +     Documentation/devicetree/bindings/power/power_domain.txt.
> >>>> +
> >>>> +
> >>>> +Examples
> >>>> +--------
> >>>> +
> >>>> +  - CPG device node:
> >>>> +
> >>>> +     cpg: clock-controller@e6150000 {
> >>>> +             compatible = "renesas,r8a7795-cpg-mssr";
> >>>> +             reg = <0 0xe6150000 0 0x1000>;
> >>>> +             clocks = <&extal_clk>, <&extalr_clk>;
> >>>> +             clock-names = "extal", "extalr";
> >>>> +             #clock-cells = <2>;
> >>>> +             #power-domain-cells = <0>;
> >>>> +     };
> >> >> +
> >> >> +
> >> > 
> >> >> +  - CPG/MSSR Clock Domain member device node:
> >>>
> >>> Would it make sense to show two examples, one for a device whose driver
> >>> manages the MSTP clock manually, and another one for a device whose
> >>> driver delegates that to the power domain ?
> >>> 
> >>> I hate using the word driver in DT bindings, but unfortunately that's
> >>> essentially what it boils down to here as the decision to specify the
> >>> power domain is really based on the driver, not on the hardware.
> >> 
> >> IMHO it's not the driver, but the on-SoC module and its representation in
> >> DT. Cfr. commit 797a0626e08ca4af ("ARM: shmobile: r8a7791 dtsi: Add
> >> 
> >> CPG/MSTP Clock Domain", which states:
> >> |   Add "power-domains" properties to all device nodes for devices that
> >> |   are part of the CPG/MSTP Clock Domain and can be power-managed
> >> |   through an MSTP clock.  This applies to most on-SoC devices, which
> >> |   have a one-to-one mapping from SoC device to DT device node.  Notable
> >> |   exceptions are the "display" and "sound" nodes, which represent
> >> |   multiple SoC devices, each having their own MSTP clocks.
> > 
> > You're quoting your own documentation to support your point, that's not
> > fair :-)
> 
> I quoted it because it explains what was done.
> 
> > We're using power domains to gate clocks. The fact that it's not related
> > to power supplies can already be borderline, but I can buy the argument
> > that clocks relate to power consumption here. However, where the power
> > domain abstraction is really abused is that we're adding all kind of
> > devices to a single power domain while they're controlled by one clock
> > gate each. That's a software hack, and we're using DT to tell whether our
> > core code should control clock gating or not. That's not a hardware
> > description, sorry.
> 
> IMHO the "power-domains" property naming is wrong: it should have been
> "pm-domains", as it's following the spirit of the Linux PM Domain
> abstraction.
> 
> PM Domain = Collection of devices treated similarly w.r.t. power management

Doesn't treated refer to how the Linux kernel software implementation treats 
the devices ?

> "similarly" here means "using module clocks". And yes the datasheet states
> that's what the module clocks do: control supply of the clock signal to the
> module.
> 
> >> If a single device block (sharing the same register space), represented
> >> in DT as a single device node, actually represents multiple modules,
> >> there's no one-to-one mapping from SoC device to DT device node.
> >> 
> >> Hence the device node will have multiple module clocks, and the driver
> >> will have to take care of managing them explicitly.
> > 
> > There can be other reasons why the clocks need to be controlled explicitly
> > by the driver. At the end of the end it's a per-driver decision whether
> > it wants to delegate clock management to runtime PM (which will be the
> > default cause) or handle it itself.
> 
> The driver can still override if it feels the need.

How should it do so by the way ?

> >> The register space sharing is an SoC-specific issue.
> >> The single device node is a DT binding issue.
> >> 
> >> Perhaps such devices should use subnodes, so each of them can represent a
> >> module, each with its own module clock.
> > 
> > I'm not sure to see how that would help, as we'd have a single struct
> > device in that case (associated with the top-level DT node), so runtime
> > PM couldn't be used to manage clocks independently.
> 
> The driver for the subnode device can still create child devices, can't it?

I think it could, yes, but I'm not sure if that would really simplify much 
compared to having three independent DT nodes. It could be experimented 
though. The LVDS transmitter in particular should probably be a separate DT 
node, linked to the DU using phandles.