diff mbox series

[v2,02/13] dt-bindings: serial: renesas,em-uart: Document r9a09g011 bindings

Message ID 20220330154024.112270-3-phil.edworthy@renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series Add new Renesas RZ/V2M SoC and Renesas RZ/V2M EVK support | expand

Commit Message

Phil Edworthy March 30, 2022, 3:40 p.m. UTC 
The Renesas RZ/V2M (r9a09g011) SoC uses a uart that is compatible with the
EMMA Mobile SoC.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2: Fix dtbs_check by adding missing alternative binding
---
 .../devicetree/bindings/serial/renesas,em-uart.yaml      | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Rob Herring (Arm) April 4, 2022, 7:24 p.m. UTC | #1 
On Wed, 30 Mar 2022 16:40:13 +0100, Phil Edworthy wrote:
> The Renesas RZ/V2M (r9a09g011) SoC uses a uart that is compatible with the
> EMMA Mobile SoC.
> 
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2: Fix dtbs_check by adding missing alternative binding
> ---
>  .../devicetree/bindings/serial/renesas,em-uart.yaml      | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>
Geert Uytterhoeven April 20, 2022, 9:26 p.m. UTC | #2 
Hi Phil,

On Wed, Mar 30, 2022 at 5:41 PM Phil Edworthy <phil.edworthy@renesas.com> wrote:
> The Renesas RZ/V2M (r9a09g011) SoC uses a uart that is compatible with the
> EMMA Mobile SoC.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2: Fix dtbs_check by adding missing alternative binding

Thanks for your patch, which is now commit 7bb301812b628099
("dt-bindings: serial: renesas,em-uart: Document r9a09g011
bindings") in tty/tty-next.

> --- a/Documentation/devicetree/bindings/serial/renesas,em-uart.yaml
> +++ b/Documentation/devicetree/bindings/serial/renesas,em-uart.yaml
> @@ -14,7 +14,14 @@ allOf:
>
>  properties:
>    compatible:
> -    const: renesas,em-uart
> +    oneOf:
> +      - items:
> +          - enum:
> +              - renesas,r9a09g011-uart    # RZ/V2M
> +          - const: renesas,em-uart        # generic EMMA Mobile compatible UART
> +
> +      - items:
> +          - const: renesas,em-uart        # generic EMMA Mobile compatible UART

The above looks good to me.

>
>    reg:
>      maxItems: 1

However, unlike EMEV2, RZ/V2M defines two clocks: pclk and sclk.
Hence please update the clocks section to reflect that.

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
Phil Edworthy April 22, 2022, 8:28 a.m. UTC | #3 
Hi Geert,

(updated Krzysztof's email)

On 20 April 2022 22:26 Geert Uytterhoeven wrote:
> On Wed, Mar 30, 2022 at 5:41 PM Phil Edworthy <phil.edworthy@renesas.com>
> wrote:
> > The Renesas RZ/V2M (r9a09g011) SoC uses a uart that is compatible with
> the
> > EMMA Mobile SoC.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2: Fix dtbs_check by adding missing alternative binding
> 
> Thanks for your patch, which is now commit 7bb301812b628099
> ("dt-bindings: serial: renesas,em-uart: Document r9a09g011
> bindings") in tty/tty-next.
> 
> > --- a/Documentation/devicetree/bindings/serial/renesas,em-uart.yaml
> > +++ b/Documentation/devicetree/bindings/serial/renesas,em-uart.yaml
> > @@ -14,7 +14,14 @@ allOf:
> >
> >  properties:
> >    compatible:
> > -    const: renesas,em-uart
> > +    oneOf:
> > +      - items:
> > +          - enum:
> > +              - renesas,r9a09g011-uart    # RZ/V2M
> > +          - const: renesas,em-uart        # generic EMMA Mobile
> compatible UART
> > +
> > +      - items:
> > +          - const: renesas,em-uart        # generic EMMA Mobile
> compatible UART
> 
> The above looks good to me.
> 
> >
> >    reg:
> >      maxItems: 1
> 
> However, unlike EMEV2, RZ/V2M defines two clocks: pclk and sclk.
> Hence please update the clocks section to reflect that.
You are right that the uart has two clocks.

Note though that pclk is shared by both uarts. The HW manual says:
"ch. 1 is for use with the ISP support package, so do not
use registers related to this channel.". Due to this, section
48.5.2.50 Clock ON/OFF Control Register 15 (CPG_CLK_ON15) says
that bit 20, CLK4_ONWEN (enable for URT_PCLK) should be written
as 0.

I took this to mean that the URT_PCLK is enabled by the ISP firmware
and software must not touch it. I am not sure if the DT bindings
should document a clock that is specified as do not touch in the
HW manual. This is a bit of a grey area.

Thanks
Phil
Geert Uytterhoeven April 22, 2022, 8:45 a.m. UTC | #4 
Hi Phil,

On Fri, Apr 22, 2022 at 10:28 AM Phil Edworthy
<phil.edworthy@renesas.com> wrote:
> On 20 April 2022 22:26 Geert Uytterhoeven wrote:
> > On Wed, Mar 30, 2022 at 5:41 PM Phil Edworthy <phil.edworthy@renesas.com>
> > wrote:
> > > The Renesas RZ/V2M (r9a09g011) SoC uses a uart that is compatible with
> > the
> > > EMMA Mobile SoC.
> > >
> > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > > v2: Fix dtbs_check by adding missing alternative binding
> >
> > Thanks for your patch, which is now commit 7bb301812b628099
> > ("dt-bindings: serial: renesas,em-uart: Document r9a09g011
> > bindings") in tty/tty-next.
> >
> > > --- a/Documentation/devicetree/bindings/serial/renesas,em-uart.yaml
> > > +++ b/Documentation/devicetree/bindings/serial/renesas,em-uart.yaml
> > > @@ -14,7 +14,14 @@ allOf:
> > >
> > >  properties:
> > >    compatible:
> > > -    const: renesas,em-uart
> > > +    oneOf:
> > > +      - items:
> > > +          - enum:
> > > +              - renesas,r9a09g011-uart    # RZ/V2M
> > > +          - const: renesas,em-uart        # generic EMMA Mobile
> > compatible UART
> > > +
> > > +      - items:
> > > +          - const: renesas,em-uart        # generic EMMA Mobile
> > compatible UART
> >
> > The above looks good to me.
> >
> > >
> > >    reg:
> > >      maxItems: 1
> >
> > However, unlike EMEV2, RZ/V2M defines two clocks: pclk and sclk.
> > Hence please update the clocks section to reflect that.
> You are right that the uart has two clocks.
>
> Note though that pclk is shared by both uarts. The HW manual says:
> "ch. 1 is for use with the ISP support package, so do not
> use registers related to this channel.". Due to this, section
> 48.5.2.50 Clock ON/OFF Control Register 15 (CPG_CLK_ON15) says
> that bit 20, CLK4_ONWEN (enable for URT_PCLK) should be written
> as 0.
>
> I took this to mean that the URT_PCLK is enabled by the ISP firmware
> and software must not touch it. I am not sure if the DT bindings
> should document a clock that is specified as do not touch in the
> HW manual. This is a bit of a grey area.

"DT describes hardware, not software policy".

But I agree this is a grey area.

One option would be to mark URT_PCLK critical, so it won't be disabled.
But that would still mean it's enabled by Linux, i.e. Linux would set
CLK4_ONWEN to 1 while enabling the clock.

Another option would be to create URT_PCLK as a non-gateable clock,
so Linux won't ever touch the register bits.

Or just ignore URT_PCLK and do nothing, like you did ;-)
Would it be possible for a user to not use the ISP firmware at all,
and go full Linux, hence using both UART channels and URT_PCLK?

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
Phil Edworthy April 22, 2022, 9:31 a.m. UTC | #5 
Hi Geert,

On 22 April 2022 09:45 Geert Uytterhoeven wrote:
> On Fri, Apr 22, 2022 at 10:28 AM Phil Edworthy wrote:
> > On 20 April 2022 22:26 Geert Uytterhoeven wrote:
> > > On Wed, Mar 30, 2022 at 5:41 PM Phil Edworthy wrote:
> > > > The Renesas RZ/V2M (r9a09g011) SoC uses a uart that is compatible
> with
> > > the
> > > > EMMA Mobile SoC.
> > > >
> > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > ---
> > > > v2: Fix dtbs_check by adding missing alternative binding
> > >
> > > Thanks for your patch, which is now commit 7bb301812b628099
> > > ("dt-bindings: serial: renesas,em-uart: Document r9a09g011
> > > bindings") in tty/tty-next.
> > >
> > > > --- a/Documentation/devicetree/bindings/serial/renesas,em-uart.yaml
> > > > +++ b/Documentation/devicetree/bindings/serial/renesas,em-uart.yaml
> > > > @@ -14,7 +14,14 @@ allOf:
> > > >
> > > >  properties:
> > > >    compatible:
> > > > -    const: renesas,em-uart
> > > > +    oneOf:
> > > > +      - items:
> > > > +          - enum:
> > > > +              - renesas,r9a09g011-uart    # RZ/V2M
> > > > +          - const: renesas,em-uart        # generic EMMA Mobile
> > > compatible UART
> > > > +
> > > > +      - items:
> > > > +          - const: renesas,em-uart        # generic EMMA Mobile
> > > compatible UART
> > >
> > > The above looks good to me.
> > >
> > > >
> > > >    reg:
> > > >      maxItems: 1
> > >
> > > However, unlike EMEV2, RZ/V2M defines two clocks: pclk and sclk.
> > > Hence please update the clocks section to reflect that.
> > You are right that the uart has two clocks.
> >
> > Note though that pclk is shared by both uarts. The HW manual says:
> > "ch. 1 is for use with the ISP support package, so do not
> > use registers related to this channel.". Due to this, section
> > 48.5.2.50 Clock ON/OFF Control Register 15 (CPG_CLK_ON15) says
> > that bit 20, CLK4_ONWEN (enable for URT_PCLK) should be written
> > as 0.
> >
> > I took this to mean that the URT_PCLK is enabled by the ISP firmware
> > and software must not touch it. I am not sure if the DT bindings
> > should document a clock that is specified as do not touch in the
> > HW manual. This is a bit of a grey area.
> 
> "DT describes hardware, not software policy".
> 
> But I agree this is a grey area.
I wish the HW manual either didn’t mention this clock that you should
not touch, or didn’t specify anything as "used by the ISP firmware" :)

> One option would be to mark URT_PCLK critical, so it won't be disabled.
> But that would still mean it's enabled by Linux, i.e. Linux would set
> CLK4_ONWEN to 1 while enabling the clock.
> 
> Another option would be to create URT_PCLK as a non-gateable clock,
> so Linux won't ever touch the register bits.
> 
> Or just ignore URT_PCLK and do nothing, like you did ;-)
> Would it be possible for a user to not use the ISP firmware at all,
> and go full Linux, hence using both UART channels and URT_PCLK?
It is possible to not use the ISP firmware, but them what do we do?
Ignore everything in the HW manual that says "ISP firmware"?

Ideally, we want to only enable a clock if it's not already enabled,
but not turn it off if it is enabled. Isn't that a critical clk?

Thanks
Phil
Geert Uytterhoeven April 22, 2022, 3:22 p.m. UTC | #6 
Hi Phil,

On Fri, Apr 22, 2022 at 11:31 AM Phil Edworthy
<phil.edworthy@renesas.com> wrote:
> On 22 April 2022 09:45 Geert Uytterhoeven wrote:
> > On Fri, Apr 22, 2022 at 10:28 AM Phil Edworthy wrote:
> > > On 20 April 2022 22:26 Geert Uytterhoeven wrote:
> > > > On Wed, Mar 30, 2022 at 5:41 PM Phil Edworthy wrote:
> > > > > The Renesas RZ/V2M (r9a09g011) SoC uses a uart that is compatible
> > with
> > > > the
> > > > > EMMA Mobile SoC.
> > > > >
> > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > ---
> > > > > v2: Fix dtbs_check by adding missing alternative binding
> > > >
> > > > Thanks for your patch, which is now commit 7bb301812b628099
> > > > ("dt-bindings: serial: renesas,em-uart: Document r9a09g011
> > > > bindings") in tty/tty-next.
> > > >
> > > > > --- a/Documentation/devicetree/bindings/serial/renesas,em-uart.yaml
> > > > > +++ b/Documentation/devicetree/bindings/serial/renesas,em-uart.yaml
> > > > However, unlike EMEV2, RZ/V2M defines two clocks: pclk and sclk.
> > > > Hence please update the clocks section to reflect that.
> > > You are right that the uart has two clocks.
> > >
> > > Note though that pclk is shared by both uarts. The HW manual says:
> > > "ch. 1 is for use with the ISP support package, so do not
> > > use registers related to this channel.". Due to this, section
> > > 48.5.2.50 Clock ON/OFF Control Register 15 (CPG_CLK_ON15) says
> > > that bit 20, CLK4_ONWEN (enable for URT_PCLK) should be written
> > > as 0.
> > >
> > > I took this to mean that the URT_PCLK is enabled by the ISP firmware
> > > and software must not touch it. I am not sure if the DT bindings
> > > should document a clock that is specified as do not touch in the
> > > HW manual. This is a bit of a grey area.
> >
> > "DT describes hardware, not software policy".
> >
> > But I agree this is a grey area.
> I wish the HW manual either didn’t mention this clock that you should
> not touch, or didn’t specify anything as "used by the ISP firmware" :)

Yeah, hardware manuals making too many assumptions about the software
that will run on it will lead to headaches...

> > One option would be to mark URT_PCLK critical, so it won't be disabled.
> > But that would still mean it's enabled by Linux, i.e. Linux would set
> > CLK4_ONWEN to 1 while enabling the clock.
> >
> > Another option would be to create URT_PCLK as a non-gateable clock,
> > so Linux won't ever touch the register bits.
> >
> > Or just ignore URT_PCLK and do nothing, like you did ;-)
> > Would it be possible for a user to not use the ISP firmware at all,
> > and go full Linux, hence using both UART channels and URT_PCLK?
> It is possible to not use the ISP firmware, but them what do we do?
> Ignore everything in the HW manual that says "ISP firmware"?
>
> Ideally, we want to only enable a clock if it's not already enabled,
> but not turn it off if it is enabled. Isn't that a critical clk?

__clk_core_init() explicitly enables clocks marked with
CLK_IS_CRITICAL.  I think it does so without checking the hardware
if the clock is already enabled or not, so probably it will access
the reserved hardware bits regardless.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/serial/renesas,em-uart.yaml b/Documentation/devicetree/bindings/serial/renesas,em-uart.yaml
index e98ec48fee46..332c385618e1 100644
--- a/Documentation/devicetree/bindings/serial/renesas,em-uart.yaml
+++ b/Documentation/devicetree/bindings/serial/renesas,em-uart.yaml
@@ -14,7 +14,14 @@  allOf:
 
 properties:
   compatible:
-    const: renesas,em-uart
+    oneOf:
+      - items:
+          - enum:
+              - renesas,r9a09g011-uart    # RZ/V2M
+          - const: renesas,em-uart        # generic EMMA Mobile compatible UART
+
+      - items:
+          - const: renesas,em-uart        # generic EMMA Mobile compatible UART
 
   reg:
     maxItems: 1