All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ARM: dts: r9a06g032: Correct UART and add all other UARTs
@ 2018-08-30  9:52 ` Phil Edworthy
  0 siblings, 0 replies; 8+ messages in thread
From: Phil Edworthy @ 2018-08-30  9:52 UTC (permalink / raw)
  To: Simon Horman
  Cc: Geert Uytterhoeven, linux-renesas-soc, linux-arm-kernel, Phil Edworthy

- UART0 was missing the bus clock ("apb_pclk").
- Now that the relevant rzn1 bindings have been added, replace the Synopsys
  compat string with the rzn1 strings.
- Add all the other UARTs.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
 arch/arm/boot/dts/r9a06g032.dtsi | 83 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 80 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
index 3e45375..1bc1f36 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -78,13 +78,90 @@
 		};
 
 		uart0: serial@40060000 {
-			compatible = "snps,dw-apb-uart";
+			compatible = "renesas,r9a06g032-uart", "renesas,rzn1-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";
+			clocks = <&sysctrl R9A06G032_CLK_UART0>, <&sysctrl R9A06G032_HCLK_UART0>;
+			clock-names = "baudclk", "apb_pclk";
+			status = "disabled";
+		};
+
+		uart1: serial@40061000 {
+			compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
+			reg = <0x40061000 0x400>;
+			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+			clocks = <&sysctrl R9A06G032_CLK_UART1>, <&sysctrl R9A06G032_HCLK_UART1>;
+			clock-names = "baudclk", "apb_pclk";
+			status = "disabled";
+		};
+
+		uart2: serial@40062000 {
+			compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
+			reg = <0x40062000 0x400>;
+			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+			clocks = <&sysctrl R9A06G032_CLK_UART2>, <&sysctrl R9A06G032_HCLK_UART2>;
+			clock-names = "baudclk", "apb_pclk";
+			status = "disabled";
+		};
+
+		uart3: serial@50000000 {
+			compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
+			reg = <0x50000000 0x400>;
+			interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+			clocks = <&sysctrl R9A06G032_CLK_UART3>, <&sysctrl R9A06G032_HCLK_UART3>;
+			clock-names = "baudclk", "apb_pclk";
+			status = "disabled";
+		};
+
+		uart4: serial@50001000 {
+			compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
+			reg = <0x50001000 0x400>;
+			interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+			clocks = <&sysctrl R9A06G032_CLK_UART4>, <&sysctrl R9A06G032_HCLK_UART4>;
+			clock-names = "baudclk", "apb_pclk";
+			status = "disabled";
+		};
+
+		uart5: serial@50002000 {
+			compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
+			reg = <0x50002000 0x400>;
+			interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+			clocks = <&sysctrl R9A06G032_CLK_UART5>, <&sysctrl R9A06G032_HCLK_UART5>;
+			clock-names = "baudclk", "apb_pclk";
+			status = "disabled";
+		};
+
+		uart6: serial@50003000 {
+			compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
+			reg = <0x50003000 0x400>;
+			interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+			clocks = <&sysctrl R9A06G032_CLK_UART6>, <&sysctrl R9A06G032_HCLK_UART6>;
+			clock-names = "baudclk", "apb_pclk";
+			status = "disabled";
+		};
+
+		uart7: serial@50004000 {
+			compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
+			reg = <0x50004000 0x400>;
+			interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+			clocks = <&sysctrl R9A06G032_CLK_UART7>, <&sysctrl R9A06G032_HCLK_UART7>;
+			clock-names = "baudclk", "apb_pclk";
 			status = "disabled";
 		};
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH] ARM: dts: r9a06g032: Correct UART and add all other UARTs
@ 2018-08-30  9:52 ` Phil Edworthy
  0 siblings, 0 replies; 8+ messages in thread
From: Phil Edworthy @ 2018-08-30  9:52 UTC (permalink / raw)
  To: linux-arm-kernel

- UART0 was missing the bus clock ("apb_pclk").
- Now that the relevant rzn1 bindings have been added, replace the Synopsys
  compat string with the rzn1 strings.
- Add all the other UARTs.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
 arch/arm/boot/dts/r9a06g032.dtsi | 83 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 80 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
index 3e45375..1bc1f36 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -78,13 +78,90 @@
 		};
 
 		uart0: serial at 40060000 {
-			compatible = "snps,dw-apb-uart";
+			compatible = "renesas,r9a06g032-uart", "renesas,rzn1-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";
+			clocks = <&sysctrl R9A06G032_CLK_UART0>, <&sysctrl R9A06G032_HCLK_UART0>;
+			clock-names = "baudclk", "apb_pclk";
+			status = "disabled";
+		};
+
+		uart1: serial at 40061000 {
+			compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
+			reg = <0x40061000 0x400>;
+			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+			clocks = <&sysctrl R9A06G032_CLK_UART1>, <&sysctrl R9A06G032_HCLK_UART1>;
+			clock-names = "baudclk", "apb_pclk";
+			status = "disabled";
+		};
+
+		uart2: serial at 40062000 {
+			compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
+			reg = <0x40062000 0x400>;
+			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+			clocks = <&sysctrl R9A06G032_CLK_UART2>, <&sysctrl R9A06G032_HCLK_UART2>;
+			clock-names = "baudclk", "apb_pclk";
+			status = "disabled";
+		};
+
+		uart3: serial at 50000000 {
+			compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
+			reg = <0x50000000 0x400>;
+			interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+			clocks = <&sysctrl R9A06G032_CLK_UART3>, <&sysctrl R9A06G032_HCLK_UART3>;
+			clock-names = "baudclk", "apb_pclk";
+			status = "disabled";
+		};
+
+		uart4: serial at 50001000 {
+			compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
+			reg = <0x50001000 0x400>;
+			interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+			clocks = <&sysctrl R9A06G032_CLK_UART4>, <&sysctrl R9A06G032_HCLK_UART4>;
+			clock-names = "baudclk", "apb_pclk";
+			status = "disabled";
+		};
+
+		uart5: serial at 50002000 {
+			compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
+			reg = <0x50002000 0x400>;
+			interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+			clocks = <&sysctrl R9A06G032_CLK_UART5>, <&sysctrl R9A06G032_HCLK_UART5>;
+			clock-names = "baudclk", "apb_pclk";
+			status = "disabled";
+		};
+
+		uart6: serial at 50003000 {
+			compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
+			reg = <0x50003000 0x400>;
+			interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+			clocks = <&sysctrl R9A06G032_CLK_UART6>, <&sysctrl R9A06G032_HCLK_UART6>;
+			clock-names = "baudclk", "apb_pclk";
+			status = "disabled";
+		};
+
+		uart7: serial at 50004000 {
+			compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
+			reg = <0x50004000 0x400>;
+			interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+			clocks = <&sysctrl R9A06G032_CLK_UART7>, <&sysctrl R9A06G032_HCLK_UART7>;
+			clock-names = "baudclk", "apb_pclk";
 			status = "disabled";
 		};
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] ARM: dts: r9a06g032: Correct UART and add all other UARTs
  2018-08-30  9:52 ` Phil Edworthy
  (?)
@ 2018-09-06 14:38   ` Geert Uytterhoeven
  -1 siblings, 0 replies; 8+ messages in thread
From: Geert Uytterhoeven @ 2018-09-06 14:38 UTC (permalink / raw)
  To: Phil Edworthy
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Simon Horman, Linux-Renesas, Rob Herring, Linux ARM

Hi Phil,

On Thu, Aug 30, 2018 at 11:52 AM Phil Edworthy
<phil.edworthy@renesas.com> wrote:
> - UART0 was missing the bus clock ("apb_pclk").
> - Now that the relevant rzn1 bindings have been added, replace the Synopsys
>   compat string with the rzn1 strings.
> - Add all the other UARTs.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>

Thanks for your patch!

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

But a few notes/questions below.

> --- a/arch/arm/boot/dts/r9a06g032.dtsi
> +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> @@ -78,13 +78,90 @@
>                 };
>
>                 uart0: serial@40060000 {
> -                       compatible = "snps,dw-apb-uart";
> +                       compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";

Apparently not all 8 UARTs are identical: the first 3 don't have DMA, the
last 5 do, and they have more registers.

Are you sure no different compatible values are needed to distinguish
between them?
Can you handle them purely based on the presence or absence of
"dmas" properties (which are not yet present)?

According to commit 72b0505f0830df95 ("dt: serial: Add Renesas RZ/N1
binding documentation"), the Synopsis compatible string would work if you
are not using DMA, so perhaps it should be added for the ports that cannot
do DMA?

>                         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";
> +                       clocks = <&sysctrl R9A06G032_CLK_UART0>, <&sysctrl R9A06G032_HCLK_UART0>;

It's a pity the clock names don't match the datasheet, but the output from
the internal Renesas tools. So I have to trust you to not list them
in the wrong order.

> +                       clock-names = "baudclk", "apb_pclk";
> +                       status = "disabled";
> +               };

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ARM: dts: r9a06g032: Correct UART and add all other UARTs
@ 2018-09-06 14:38   ` Geert Uytterhoeven
  0 siblings, 0 replies; 8+ messages in thread
From: Geert Uytterhoeven @ 2018-09-06 14:38 UTC (permalink / raw)
  To: Phil Edworthy
  Cc: Simon Horman, Linux-Renesas, Linux ARM, Rob Herring,
	Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Hi Phil,

On Thu, Aug 30, 2018 at 11:52 AM Phil Edworthy
<phil.edworthy@renesas.com> wrote:
> - UART0 was missing the bus clock ("apb_pclk").
> - Now that the relevant rzn1 bindings have been added, replace the Synopsys
>   compat string with the rzn1 strings.
> - Add all the other UARTs.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>

Thanks for your patch!

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

But a few notes/questions below.

> --- a/arch/arm/boot/dts/r9a06g032.dtsi
> +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> @@ -78,13 +78,90 @@
>                 };
>
>                 uart0: serial@40060000 {
> -                       compatible = "snps,dw-apb-uart";
> +                       compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";

Apparently not all 8 UARTs are identical: the first 3 don't have DMA, the
last 5 do, and they have more registers.

Are you sure no different compatible values are needed to distinguish
between them?
Can you handle them purely based on the presence or absence of
"dmas" properties (which are not yet present)?

According to commit 72b0505f0830df95 ("dt: serial: Add Renesas RZ/N1
binding documentation"), the Synopsis compatible string would work if you
are not using DMA, so perhaps it should be added for the ports that cannot
do DMA?

>                         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";
> +                       clocks = <&sysctrl R9A06G032_CLK_UART0>, <&sysctrl R9A06G032_HCLK_UART0>;

It's a pity the clock names don't match the datasheet, but the output from
the internal Renesas tools. So I have to trust you to not list them
in the wrong order.

> +                       clock-names = "baudclk", "apb_pclk";
> +                       status = "disabled";
> +               };

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] ARM: dts: r9a06g032: Correct UART and add all other UARTs
@ 2018-09-06 14:38   ` Geert Uytterhoeven
  0 siblings, 0 replies; 8+ messages in thread
From: Geert Uytterhoeven @ 2018-09-06 14:38 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Phil,

On Thu, Aug 30, 2018 at 11:52 AM Phil Edworthy
<phil.edworthy@renesas.com> wrote:
> - UART0 was missing the bus clock ("apb_pclk").
> - Now that the relevant rzn1 bindings have been added, replace the Synopsys
>   compat string with the rzn1 strings.
> - Add all the other UARTs.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>

Thanks for your patch!

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

But a few notes/questions below.

> --- a/arch/arm/boot/dts/r9a06g032.dtsi
> +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> @@ -78,13 +78,90 @@
>                 };
>
>                 uart0: serial at 40060000 {
> -                       compatible = "snps,dw-apb-uart";
> +                       compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";

Apparently not all 8 UARTs are identical: the first 3 don't have DMA, the
last 5 do, and they have more registers.

Are you sure no different compatible values are needed to distinguish
between them?
Can you handle them purely based on the presence or absence of
"dmas" properties (which are not yet present)?

According to commit 72b0505f0830df95 ("dt: serial: Add Renesas RZ/N1
binding documentation"), the Synopsis compatible string would work if you
are not using DMA, so perhaps it should be added for the ports that cannot
do DMA?

>                         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";
> +                       clocks = <&sysctrl R9A06G032_CLK_UART0>, <&sysctrl R9A06G032_HCLK_UART0>;

It's a pity the clock names don't match the datasheet, but the output from
the internal Renesas tools. So I have to trust you to not list them
in the wrong order.

> +                       clock-names = "baudclk", "apb_pclk";
> +                       status = "disabled";
> +               };

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] ARM: dts: r9a06g032: Correct UART and add all other UARTs
  2018-09-06 14:38   ` Geert Uytterhoeven
  (?)
@ 2018-09-06 15:34     ` Phil Edworthy
  -1 siblings, 0 replies; 8+ messages in thread
From: Phil Edworthy @ 2018-09-06 15:34 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Simon Horman, Linux-Renesas, Rob Herring, Linux ARM

Hi Geert,

On 06 September 2018 15:38 Geert Uytterhoeven wrote:
> On Thu, Aug 30, 2018 at 11:52 AM Phil Edworthy wrote:
> > - UART0 was missing the bus clock ("apb_pclk").
> > - Now that the relevant rzn1 bindings have been added, replace the
> Synopsys
> >   compat string with the rzn1 strings.
> > - Add all the other UARTs.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> 
> Thanks for your patch!
And thanks for your review!
 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> But a few notes/questions below.
> 
> > --- a/arch/arm/boot/dts/r9a06g032.dtsi
> > +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> > @@ -78,13 +78,90 @@
> >                 };
> >
> >                 uart0: serial@40060000 {
> > -                       compatible = "snps,dw-apb-uart";
> > +                       compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
> 
> Apparently not all 8 UARTs are identical: the first 3 don't have DMA, the
> last 5 do, and they have more registers.
> 
> Are you sure no different compatible values are needed to distinguish
> between them?
> Can you handle them purely based on the presence or absence of
> "dmas" properties (which are not yet present)?
Yes, I am pretty sure... the extra registers for the last 5 uarts are only dma
related and we have been using the standard Synopsys driver on the other
3 uarts without issue for some time.

> According to commit 72b0505f0830df95 ("dt: serial: Add Renesas RZ/N1
> binding documentation"), the Synopsis compatible string would work if you
> are not using DMA, so perhaps it should be added for the ports that cannot
> do DMA?
Makes sense, I will make them:
compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart", "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";
> > +                       clocks = <&sysctrl R9A06G032_CLK_UART0>, <&sysctrl
> R9A06G032_HCLK_UART0>;
> 
> It's a pity the clock names don't match the datasheet, but the output from
> the internal Renesas tools. So I have to trust you to not list them
> in the wrong order.
True... Note that all bus clocks required to access the peripheral's registers
are 'HCLK', other clocks such as baud clks are 'CLK'.

> > +                       clock-names = "baudclk", "apb_pclk";
> > +                       status = "disabled";
> > +               };

Thanks
Phil

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] ARM: dts: r9a06g032: Correct UART and add all other UARTs
@ 2018-09-06 15:34     ` Phil Edworthy
  0 siblings, 0 replies; 8+ messages in thread
From: Phil Edworthy @ 2018-09-06 15:34 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Simon Horman, Linux-Renesas, Linux ARM, Rob Herring,
	Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Hi Geert,

On 06 September 2018 15:38 Geert Uytterhoeven wrote:
> On Thu, Aug 30, 2018 at 11:52 AM Phil Edworthy wrote:
> > - UART0 was missing the bus clock ("apb_pclk").
> > - Now that the relevant rzn1 bindings have been added, replace the
> Synopsys
> >   compat string with the rzn1 strings.
> > - Add all the other UARTs.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> 
> Thanks for your patch!
And thanks for your review!
 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> But a few notes/questions below.
> 
> > --- a/arch/arm/boot/dts/r9a06g032.dtsi
> > +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> > @@ -78,13 +78,90 @@
> >                 };
> >
> >                 uart0: serial@40060000 {
> > -                       compatible = "snps,dw-apb-uart";
> > +                       compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
> 
> Apparently not all 8 UARTs are identical: the first 3 don't have DMA, the
> last 5 do, and they have more registers.
> 
> Are you sure no different compatible values are needed to distinguish
> between them?
> Can you handle them purely based on the presence or absence of
> "dmas" properties (which are not yet present)?
Yes, I am pretty sure... the extra registers for the last 5 uarts are only dma
related and we have been using the standard Synopsys driver on the other
3 uarts without issue for some time.

> According to commit 72b0505f0830df95 ("dt: serial: Add Renesas RZ/N1
> binding documentation"), the Synopsis compatible string would work if you
> are not using DMA, so perhaps it should be added for the ports that cannot
> do DMA?
Makes sense, I will make them:
compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart", "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";
> > +                       clocks = <&sysctrl R9A06G032_CLK_UART0>, <&sysctrl
> R9A06G032_HCLK_UART0>;
> 
> It's a pity the clock names don't match the datasheet, but the output from
> the internal Renesas tools. So I have to trust you to not list them
> in the wrong order.
True... Note that all bus clocks required to access the peripheral's registers
are 'HCLK', other clocks such as baud clks are 'CLK'.

> > +                       clock-names = "baudclk", "apb_pclk";
> > +                       status = "disabled";
> > +               };

Thanks
Phil

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] ARM: dts: r9a06g032: Correct UART and add all other UARTs
@ 2018-09-06 15:34     ` Phil Edworthy
  0 siblings, 0 replies; 8+ messages in thread
From: Phil Edworthy @ 2018-09-06 15:34 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Geert,

On 06 September 2018 15:38 Geert Uytterhoeven wrote:
> On Thu, Aug 30, 2018 at 11:52 AM Phil Edworthy wrote:
> > - UART0 was missing the bus clock ("apb_pclk").
> > - Now that the relevant rzn1 bindings have been added, replace the
> Synopsys
> >   compat string with the rzn1 strings.
> > - Add all the other UARTs.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> 
> Thanks for your patch!
And thanks for your review!
 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> But a few notes/questions below.
> 
> > --- a/arch/arm/boot/dts/r9a06g032.dtsi
> > +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> > @@ -78,13 +78,90 @@
> >                 };
> >
> >                 uart0: serial at 40060000 {
> > -                       compatible = "snps,dw-apb-uart";
> > +                       compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart";
> 
> Apparently not all 8 UARTs are identical: the first 3 don't have DMA, the
> last 5 do, and they have more registers.
> 
> Are you sure no different compatible values are needed to distinguish
> between them?
> Can you handle them purely based on the presence or absence of
> "dmas" properties (which are not yet present)?
Yes, I am pretty sure... the extra registers for the last 5 uarts are only dma
related and we have been using the standard Synopsys driver on the other
3 uarts without issue for some time.

> According to commit 72b0505f0830df95 ("dt: serial: Add Renesas RZ/N1
> binding documentation"), the Synopsis compatible string would work if you
> are not using DMA, so perhaps it should be added for the ports that cannot
> do DMA?
Makes sense, I will make them:
compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart", "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";
> > +                       clocks = <&sysctrl R9A06G032_CLK_UART0>, <&sysctrl
> R9A06G032_HCLK_UART0>;
> 
> It's a pity the clock names don't match the datasheet, but the output from
> the internal Renesas tools. So I have to trust you to not list them
> in the wrong order.
True... Note that all bus clocks required to access the peripheral's registers
are 'HCLK', other clocks such as baud clks are 'CLK'.

> > +                       clock-names = "baudclk", "apb_pclk";
> > +                       status = "disabled";
> > +               };

Thanks
Phil

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-09-06 20:10 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-30  9:52 [PATCH] ARM: dts: r9a06g032: Correct UART and add all other UARTs Phil Edworthy
2018-08-30  9:52 ` Phil Edworthy
2018-09-06 14:38 ` Geert Uytterhoeven
2018-09-06 14:38   ` Geert Uytterhoeven
2018-09-06 14:38   ` Geert Uytterhoeven
2018-09-06 15:34   ` Phil Edworthy
2018-09-06 15:34     ` Phil Edworthy
2018-09-06 15:34     ` Phil Edworthy

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.