[PATCH 1/2] ARM: shmobile: r8a7791: add GyroADC clock

[PATCH 1/2] ARM: shmobile: r8a7791: add GyroADC clock

[Marek Vasut]

Add the GyroADC clock to the R8A7791 device tree.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Simon Horman <horms+renesas@verge.net.au>
Cc: linux-renesas-soc@vger.kernel.org
---
 include/dt-bindings/clock/r8a7791-clock.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/dt-bindings/clock/r8a7791-clock.h b/include/dt-bindings/clock/r8a7791-clock.h
index adc50dc31ab3..ef692134146b 100644
--- a/include/dt-bindings/clock/r8a7791-clock.h
+++ b/include/dt-bindings/clock/r8a7791-clock.h
@@ -109,6 +109,7 @@
 #define R8A7791_CLK_SATA0		15
 
 /* MSTP9 */
+#define R8A7791_CLK_GYROADC		1
 #define R8A7791_CLK_GPIO7		4
 #define R8A7791_CLK_GPIO6		5
 #define R8A7791_CLK_GPIO5		7
-- 
2.11.0

[PATCH 2/2] ARM: dts: r8a7791: Add GyroADC bindings

[Marek Vasut]

Add bindings for the GyroADC block and it's associated clock.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Simon Horman <horms+renesas@verge.net.au>
Cc: linux-renesas-soc@vger.kernel.org
---
 arch/arm/boot/dts/r8a7791.dtsi | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
index 4d0c2ce59900..1b099dbc9eef 100644
--- a/arch/arm/boot/dts/r8a7791.dtsi
+++ b/arch/arm/boot/dts/r8a7791.dtsi
@@ -776,6 +776,15 @@
 		status = "disabled";
 	};
 
+	adc: adc@e6e54000 {
+		compatible = "renesas,r8a7791-gyroadc", "renesas,rcar-gyroadc";
+		reg = <0 0xe6e54000 0 64>;
+		clocks = <&mstp9_clks R8A7791_CLK_GYROADC>, <&adc_clk>;
+		clock-names = "fck", "if";
+		power-domains = <&sysc R8A7791_PD_ALWAYS_ON>;
+		status = "disabled";
+	};
+
 	scif2: serial@e6e58000 {
 		compatible = "renesas,scif-r8a7791", "renesas,rcar-gen2-scif",
 			     "renesas,scif";
@@ -1133,6 +1142,13 @@
 			clock-frequency = <0>;
 		};
 
+		/* GyroADC clock */
+		adc_clk: adc_clk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <65000000>;
+		};
+
 		/* Special CPG clocks */
 		cpg_clocks: cpg_clocks@e6150000 {
 			compatible = "renesas,r8a7791-cpg-clocks",
@@ -1432,6 +1448,7 @@
 				 <&hp_clk>, <&hp_clk>;
 			#clock-cells = <1>;
 			clock-indices = <
+				R8A7791_CLK_GYROADC
 				R8A7791_CLK_GPIO7 R8A7791_CLK_GPIO6 R8A7791_CLK_GPIO5 R8A7791_CLK_GPIO4
 				R8A7791_CLK_GPIO3 R8A7791_CLK_GPIO2 R8A7791_CLK_GPIO1 R8A7791_CLK_GPIO0
 				R8A7791_CLK_RCAN1 R8A7791_CLK_RCAN0 R8A7791_CLK_QSPI_MOD R8A7791_CLK_I2C5
@@ -1439,6 +1456,7 @@
 				R8A7791_CLK_I2C1 R8A7791_CLK_I2C0
 			>;
 			clock-output-names =
+				"gyroadc",
 				"gpio7", "gpio6", "gpio5", "gpio4", "gpio3", "gpio2", "gpio1", "gpio0",
 				"rcan1", "rcan0", "qspi_mod", "i2c5", "i2c6", "i2c4", "i2c3", "i2c2",
 				"i2c1", "i2c0";
-- 
2.11.0

Re: [PATCH 2/2] ARM: dts: r8a7791: Add GyroADC bindings

[Sergei Shtylyov]

Hello!

On 4/16/2017 7:57 PM, Marek Vasut wrote:

> Add bindings for the GyroADC block and it's associated clock.

    Well, I already spoke to you about the bindings on IRC...

> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> Cc: linux-renesas-soc@vger.kernel.org
> ---
>  arch/arm/boot/dts/r8a7791.dtsi | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
> index 4d0c2ce59900..1b099dbc9eef 100644
> --- a/arch/arm/boot/dts/r8a7791.dtsi
> +++ b/arch/arm/boot/dts/r8a7791.dtsi
> @@ -776,6 +776,15 @@
>  		status = "disabled";
>  	};
>
> +	adc: adc@e6e54000 {

    Why not label it "gyroadc:"?

> +		compatible = "renesas,r8a7791-gyroadc", "renesas,rcar-gyroadc";
> +		reg = <0 0xe6e54000 0 64>;

    s/64/0x40/.

[...]
> @@ -1133,6 +1142,13 @@
>  			clock-frequency = <0>;
>  		};
>
> +		/* GyroADC clock */
> +		adc_clk: adc_clk {

    We geberally skip the "_clk" suffix in the clock node names, so that the 
clock name generated from the node name doesn't have this suffix.

[...]

MBR, Sergei

Re: [PATCH 1/2] ARM: shmobile: r8a7791: add GyroADC clock

[Geert Uytterhoeven]

On Sun, Apr 16, 2017 at 6:57 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Add the GyroADC clock to the R8A7791 device tree.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [PATCH 2/2] ARM: dts: r8a7791: Add GyroADC bindings

[Geert Uytterhoeven]

Hi Marek,

On Sun, Apr 16, 2017 at 6:57 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Add bindings for the GyroADC block and it's associated clock.

bindings??

> --- a/arch/arm/boot/dts/r8a7791.dtsi
> +++ b/arch/arm/boot/dts/r8a7791.dtsi
> @@ -776,6 +776,15 @@
>                 status = "disabled";
>         };
>
> +       adc: adc@e6e54000 {
> +               compatible = "renesas,r8a7791-gyroadc", "renesas,rcar-gyroadc";
> +               reg = <0 0xe6e54000 0 64>;
> +               clocks = <&mstp9_clks R8A7791_CLK_GYROADC>, <&adc_clk>;
> +               clock-names = "fck", "if";
> +               power-domains = <&sysc R8A7791_PD_ALWAYS_ON>;
> +               status = "disabled";
> +       };
> +
>         scif2: serial@e6e58000 {
>                 compatible = "renesas,scif-r8a7791", "renesas,rcar-gen2-scif",
>                              "renesas,scif";
> @@ -1133,6 +1142,13 @@
>                         clock-frequency = <0>;
>                 };
>
> +               /* GyroADC clock */
> +               adc_clk: adc_clk {
> +                       compatible = "fixed-clock";
> +                       #clock-cells = <0>;
> +                       clock-frequency = <65000000>;
> +               };

Why do you have to add a clock?
I think you should just refer to the on-SoC peripheral clock: &cp_clk.

> +
>                 /* Special CPG clocks */
>                 cpg_clocks: cpg_clocks@e6150000 {
>                         compatible = "renesas,r8a7791-cpg-clocks",
> @@ -1432,6 +1448,7 @@
>                                  <&hp_clk>, <&hp_clk>;

Missing addition of the parent clock for the newly added module clock.
Perhaps this should be the peripheral clock (<&cp_clk>)?

Oops, that means there's no need to have two clock inputs in the adc device
node, and thus we screwed up when reviewing the GyroADC bindings :-(

>                         #clock-cells = <1>;
>                         clock-indices = <
> +                               R8A7791_CLK_GYROADC
>                                 R8A7791_CLK_GPIO7 R8A7791_CLK_GPIO6 R8A7791_CLK_GPIO5 R8A7791_CLK_GPIO4
>                                 R8A7791_CLK_GPIO3 R8A7791_CLK_GPIO2 R8A7791_CLK_GPIO1 R8A7791_CLK_GPIO0
>                                 R8A7791_CLK_RCAN1 R8A7791_CLK_RCAN0 R8A7791_CLK_QSPI_MOD R8A7791_CLK_I2C5
> @@ -1439,6 +1456,7 @@
>                                 R8A7791_CLK_I2C1 R8A7791_CLK_I2C0
>                         >;
>                         clock-output-names =
> +                               "gyroadc",
>                                 "gpio7", "gpio6", "gpio5", "gpio4", "gpio3", "gpio2", "gpio1", "gpio0",
>                                 "rcan1", "rcan0", "qspi_mod", "i2c5", "i2c6", "i2c4", "i2c3", "i2c2",
>                                 "i2c1", "i2c0";

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

Re: [PATCH 2/2] ARM: dts: r8a7791: Add GyroADC bindings

[Marek Vasut]

On 04/17/2017 11:19 AM, Sergei Shtylyov wrote:
> Hello!

Hi!

> On 4/16/2017 7:57 PM, Marek Vasut wrote:
> 
>> Add bindings for the GyroADC block and it's associated clock.
> 
>    Well, I already spoke to you about the bindings on IRC...

That's fixed.

>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>> Cc: Simon Horman <horms+renesas@verge.net.au>
>> Cc: linux-renesas-soc@vger.kernel.org
>> ---
>>  arch/arm/boot/dts/r8a7791.dtsi | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/r8a7791.dtsi
>> b/arch/arm/boot/dts/r8a7791.dtsi
>> index 4d0c2ce59900..1b099dbc9eef 100644
>> --- a/arch/arm/boot/dts/r8a7791.dtsi
>> +++ b/arch/arm/boot/dts/r8a7791.dtsi
>> @@ -776,6 +776,15 @@
>>          status = "disabled";
>>      };
>>
>> +    adc: adc@e6e54000 {
> 
>    Why not label it "gyroadc:"?

We can ... I don't mind either way. Shall we ?

>> +        compatible = "renesas,r8a7791-gyroadc", "renesas,rcar-gyroadc";
>> +        reg = <0 0xe6e54000 0 64>;
> 
>    s/64/0x40/.

The surrounding code uses 64, so I kept it consistent.

> [...]
>> @@ -1133,6 +1142,13 @@
>>              clock-frequency = <0>;
>>          };
>>
>> +        /* GyroADC clock */
>> +        adc_clk: adc_clk {
> 
>    We geberally skip the "_clk" suffix in the clock node names, so that
> the clock name generated from the node name doesn't have this suffix.

Fixed

> [...]
> 
> MBR, Sergei
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH 2/2] ARM: dts: r8a7791: Add GyroADC bindings

[Marek Vasut]

On 04/18/2017 03:59 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi Geert,

> On Sun, Apr 16, 2017 at 6:57 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> Add bindings for the GyroADC block and it's associated clock.
> 
> bindings??

That's fixed...

>> --- a/arch/arm/boot/dts/r8a7791.dtsi
>> +++ b/arch/arm/boot/dts/r8a7791.dtsi
>> @@ -776,6 +776,15 @@
>>                 status = "disabled";
>>         };
>>
>> +       adc: adc@e6e54000 {
>> +               compatible = "renesas,r8a7791-gyroadc", "renesas,rcar-gyroadc";
>> +               reg = <0 0xe6e54000 0 64>;
>> +               clocks = <&mstp9_clks R8A7791_CLK_GYROADC>, <&adc_clk>;
>> +               clock-names = "fck", "if";
>> +               power-domains = <&sysc R8A7791_PD_ALWAYS_ON>;
>> +               status = "disabled";
>> +       };
>> +
>>         scif2: serial@e6e58000 {
>>                 compatible = "renesas,scif-r8a7791", "renesas,rcar-gen2-scif",
>>                              "renesas,scif";
>> @@ -1133,6 +1142,13 @@
>>                         clock-frequency = <0>;
>>                 };
>>
>> +               /* GyroADC clock */
>> +               adc_clk: adc_clk {
>> +                       compatible = "fixed-clock";
>> +                       #clock-cells = <0>;
>> +                       clock-frequency = <65000000>;
>> +               };
> 
> Why do you have to add a clock?
> I think you should just refer to the on-SoC peripheral clock: &cp_clk.

I think you are right, in fact, see below ...

>> +
>>                 /* Special CPG clocks */
>>                 cpg_clocks: cpg_clocks@e6150000 {
>>                         compatible = "renesas,r8a7791-cpg-clocks",
>> @@ -1432,6 +1448,7 @@
>>                                  <&hp_clk>, <&hp_clk>;
> 
> Missing addition of the parent clock for the newly added module clock.
> Perhaps this should be the peripheral clock (<&cp_clk>)?
> 
> Oops, that means there's no need to have two clock inputs in the adc device
> node, and thus we screwed up when reviewing the GyroADC bindings :-(

I think you're right. I should be just getting the clk rate of the fck
and derive the gyroadc timings from that, correct ? I can send a patch
for the driver to just ignore the second clock entry and update the DT
binding document to drop the "if" clock (?) .

>>                         #clock-cells = <1>;
>>                         clock-indices = <
>> +                               R8A7791_CLK_GYROADC
>>                                 R8A7791_CLK_GPIO7 R8A7791_CLK_GPIO6 R8A7791_CLK_GPIO5 R8A7791_CLK_GPIO4
>>                                 R8A7791_CLK_GPIO3 R8A7791_CLK_GPIO2 R8A7791_CLK_GPIO1 R8A7791_CLK_GPIO0
>>                                 R8A7791_CLK_RCAN1 R8A7791_CLK_RCAN0 R8A7791_CLK_QSPI_MOD R8A7791_CLK_I2C5

[...]

-- 
Best regards,
Marek Vasut

Re: [PATCH 2/2] ARM: dts: r8a7791: Add GyroADC bindings

[Geert Uytterhoeven]

Hi Marek,

On Tue, Apr 18, 2017 at 9:05 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 04/18/2017 03:59 PM, Geert Uytterhoeven wrote:
>> On Sun, Apr 16, 2017 at 6:57 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> --- a/arch/arm/boot/dts/r8a7791.dtsi
>>> +++ b/arch/arm/boot/dts/r8a7791.dtsi
>>> @@ -776,6 +776,15 @@
>>>                 status = "disabled";
>>>         };
>>>
>>> +       adc: adc@e6e54000 {
>>> +               compatible = "renesas,r8a7791-gyroadc", "renesas,rcar-gyroadc";
>>> +               reg = <0 0xe6e54000 0 64>;
>>> +               clocks = <&mstp9_clks R8A7791_CLK_GYROADC>, <&adc_clk>;
>>> +               clock-names = "fck", "if";
>>> +               power-domains = <&sysc R8A7791_PD_ALWAYS_ON>;
>>> +               status = "disabled";
>>> +       };
>>> +
>>>         scif2: serial@e6e58000 {
>>>                 compatible = "renesas,scif-r8a7791", "renesas,rcar-gen2-scif",
>>>                              "renesas,scif";
>>> @@ -1133,6 +1142,13 @@
>>>                         clock-frequency = <0>;
>>>                 };
>>>
>>> +               /* GyroADC clock */
>>> +               adc_clk: adc_clk {
>>> +                       compatible = "fixed-clock";
>>> +                       #clock-cells = <0>;
>>> +                       clock-frequency = <65000000>;
>>> +               };
>>
>> Why do you have to add a clock?
>> I think you should just refer to the on-SoC peripheral clock: &cp_clk.
>
> I think you are right, in fact, see below ...
>
>>> +
>>>                 /* Special CPG clocks */
>>>                 cpg_clocks: cpg_clocks@e6150000 {
>>>                         compatible = "renesas,r8a7791-cpg-clocks",
>>> @@ -1432,6 +1448,7 @@
>>>                                  <&hp_clk>, <&hp_clk>;
>>
>> Missing addition of the parent clock for the newly added module clock.
>> Perhaps this should be the peripheral clock (<&cp_clk>)?
>>
>> Oops, that means there's no need to have two clock inputs in the adc device
>> node, and thus we screwed up when reviewing the GyroADC bindings :-(
>
> I think you're right. I should be just getting the clk rate of the fck
> and derive the gyroadc timings from that, correct ? I can send a patch
> for the driver to just ignore the second clock entry and update the DT
> binding document to drop the "if" clock (?) .

Fine for me. Thanks!

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

Re: [PATCH 2/2] ARM: dts: r8a7791: Add GyroADC bindings

[Marek Vasut]

On 04/18/2017 09:09 PM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi!

> On Tue, Apr 18, 2017 at 9:05 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 04/18/2017 03:59 PM, Geert Uytterhoeven wrote:
>>> On Sun, Apr 16, 2017 at 6:57 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> --- a/arch/arm/boot/dts/r8a7791.dtsi
>>>> +++ b/arch/arm/boot/dts/r8a7791.dtsi
>>>> @@ -776,6 +776,15 @@
>>>>                 status = "disabled";
>>>>         };
>>>>
>>>> +       adc: adc@e6e54000 {
>>>> +               compatible = "renesas,r8a7791-gyroadc", "renesas,rcar-gyroadc";
>>>> +               reg = <0 0xe6e54000 0 64>;
>>>> +               clocks = <&mstp9_clks R8A7791_CLK_GYROADC>, <&adc_clk>;
>>>> +               clock-names = "fck", "if";
>>>> +               power-domains = <&sysc R8A7791_PD_ALWAYS_ON>;
>>>> +               status = "disabled";
>>>> +       };
>>>> +
>>>>         scif2: serial@e6e58000 {
>>>>                 compatible = "renesas,scif-r8a7791", "renesas,rcar-gen2-scif",
>>>>                              "renesas,scif";
>>>> @@ -1133,6 +1142,13 @@
>>>>                         clock-frequency = <0>;
>>>>                 };
>>>>
>>>> +               /* GyroADC clock */
>>>> +               adc_clk: adc_clk {
>>>> +                       compatible = "fixed-clock";
>>>> +                       #clock-cells = <0>;
>>>> +                       clock-frequency = <65000000>;
>>>> +               };
>>>
>>> Why do you have to add a clock?
>>> I think you should just refer to the on-SoC peripheral clock: &cp_clk.
>>
>> I think you are right, in fact, see below ...
>>
>>>> +
>>>>                 /* Special CPG clocks */
>>>>                 cpg_clocks: cpg_clocks@e6150000 {
>>>>                         compatible = "renesas,r8a7791-cpg-clocks",
>>>> @@ -1432,6 +1448,7 @@
>>>>                                  <&hp_clk>, <&hp_clk>;
>>>
>>> Missing addition of the parent clock for the newly added module clock.
>>> Perhaps this should be the peripheral clock (<&cp_clk>)?
>>>
>>> Oops, that means there's no need to have two clock inputs in the adc device
>>> node, and thus we screwed up when reviewing the GyroADC bindings :-(
>>
>> I think you're right. I should be just getting the clk rate of the fck
>> and derive the gyroadc timings from that, correct ? I can send a patch
>> for the driver to just ignore the second clock entry and update the DT
>> binding document to drop the "if" clock (?) .
> 
> Fine for me. Thanks!

OK, patches are ready. I need to find koelsch to test on ..

-- 
Best regards,
Marek Vasut

Re: [PATCH 1/2] ARM: shmobile: r8a7791: add GyroADC clock

[Simon Horman]

On Tue, Apr 18, 2017 at 03:42:25PM +0200, Geert Uytterhoeven wrote:
> On Sun, Apr 16, 2017 at 6:57 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > Add the GyroADC clock to the R8A7791 device tree.
> >
> > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks I have queued this patch (1/2) up for v4.13.
I am expecting a v2 of patch 2/2 given the review it received.

Re: [PATCH 1/2] ARM: shmobile: r8a7791: add GyroADC clock

[Marek Vasut]

On 04/20/2017 11:00 AM, Simon Horman wrote:
> On Tue, Apr 18, 2017 at 03:42:25PM +0200, Geert Uytterhoeven wrote:
>> On Sun, Apr 16, 2017 at 6:57 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> Add the GyroADC clock to the R8A7791 device tree.
>>>
>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Thanks I have queued this patch (1/2) up for v4.13.
> I am expecting a v2 of patch 2/2 given the review it received.
> 
V2 is coming now.

-- 
Best regards,
Marek Vasut