diff mbox series

[v7,3/6] dt-bindings: mvebu-uart: document DT bindings for marvell, armada-3700-uart-clock

Message ID 20210930095838.28145-4-pali@kernel.org (mailing list archive)
State New, archived
Headers show
Series serial: mvebu-uart: Support for higher baudrates | expand

Commit Message

Pali Rohár Sept. 30, 2021, 9:58 a.m. UTC
This change adds DT bindings documentation for device nodes with compatible
string "marvell,armada-3700-uart-clock".

Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v7
* Fix errors

Changes in v6
* Fix license
* Rename node to clock-controller@12010
* Remove maxItems
---
 .../clock/marvell,armada-3700-uart-clock.yaml | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml

Comments

Rob Herring Oct. 6, 2021, 9:07 p.m. UTC | #1
On Thu, 30 Sep 2021 11:58:35 +0200, Pali Rohár wrote:
> This change adds DT bindings documentation for device nodes with compatible
> string "marvell,armada-3700-uart-clock".
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> ---
> Changes in v7
> * Fix errors
> 
> Changes in v6
> * Fix license
> * Rename node to clock-controller@12010
> * Remove maxItems
> ---
>  .../clock/marvell,armada-3700-uart-clock.yaml | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Stephen Boyd Oct. 15, 2021, 12:13 a.m. UTC | #2
Quoting Pali Rohár (2021-09-30 02:58:35)
> diff --git a/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> new file mode 100644
> index 000000000000..175f5c8f2bc5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> @@ -0,0 +1,59 @@
[..]
> +  '#clock-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    uartclk: clock-controller@12010 {

The uart device is at 0x12000 and the clock-controller is at 0x12010?
This looks like a node is being put into DT to represent a clk driver.
Why can't we register a clk from the uart device driver itself? I think
we talked about this a month or two ago but it still isn't clear to me.

> +      compatible = "marvell,armada-3700-uart-clock";
> +      reg = <0x12010 0x4>, <0x12210 0x4>;
> +      clocks = <&tbg 0>, <&tbg 1>, <&tbg 2>, <&tbg 3>, <&xtalclk>;
> +      clock-names = "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal";
> +      #clock-cells = <1>;
> +    };
> -- 
> 2.20.1
>
Pali Rohár Oct. 15, 2021, 9:09 a.m. UTC | #3
On Thursday 14 October 2021 17:13:03 Stephen Boyd wrote:
> Quoting Pali Rohár (2021-09-30 02:58:35)
> > diff --git a/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > new file mode 100644
> > index 000000000000..175f5c8f2bc5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > @@ -0,0 +1,59 @@
> [..]
> > +  '#clock-cells':
> > +    const: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - '#clock-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    uartclk: clock-controller@12010 {
> 
> The uart device is at 0x12000 and the clock-controller is at 0x12010?
> This looks like a node is being put into DT to represent a clk driver.
> Why can't we register a clk from the uart device driver itself? I think
> we talked about this a month or two ago but it still isn't clear to me.

We have already talked about it and I have already wrote reasons. UART
clk is shared for both UART1 and UART2. And UART clk regs are in both
address spaces of UART1 and UART2. UART1 or UART2 can be independently
disabled on particular board (as pins are MPP which may be configured to
different function). So you have a board only with UART2, you have to
disable UART1 node, but at the same time you have to access UART clk to
drive UART2. And UART clk bits are in UART1 address space.

> > +      compatible = "marvell,armada-3700-uart-clock";
> > +      reg = <0x12010 0x4>, <0x12210 0x4>;
> > +      clocks = <&tbg 0>, <&tbg 1>, <&tbg 2>, <&tbg 3>, <&xtalclk>;
> > +      clock-names = "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal";
> > +      #clock-cells = <1>;
> > +    };
> > -- 
> > 2.20.1
> >
Pali Rohár Oct. 15, 2021, 9:37 a.m. UTC | #4
On Friday 15 October 2021 11:09:37 Pali Rohár wrote:
> On Thursday 14 October 2021 17:13:03 Stephen Boyd wrote:
> > Quoting Pali Rohár (2021-09-30 02:58:35)
> > > diff --git a/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > > new file mode 100644
> > > index 000000000000..175f5c8f2bc5
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > > @@ -0,0 +1,59 @@
> > [..]
> > > +  '#clock-cells':
> > > +    const: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - clock-names
> > > +  - '#clock-cells'
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    uartclk: clock-controller@12010 {
> > 
> > The uart device is at 0x12000 and the clock-controller is at 0x12010?
> > This looks like a node is being put into DT to represent a clk driver.
> > Why can't we register a clk from the uart device driver itself? I think
> > we talked about this a month or two ago but it still isn't clear to me.
> 
> We have already talked about it and I have already wrote reasons. UART
> clk is shared for both UART1 and UART2. And UART clk regs are in both
> address spaces of UART1 and UART2. UART1 or UART2 can be independently
> disabled on particular board (as pins are MPP which may be configured to
> different function). So you have a board only with UART2, you have to
> disable UART1 node, but at the same time you have to access UART clk to
> drive UART2. And UART clk bits are in UART1 address space.

It is explained also in commit message of patch 2/6.

> > > +      compatible = "marvell,armada-3700-uart-clock";
> > > +      reg = <0x12010 0x4>, <0x12210 0x4>;
> > > +      clocks = <&tbg 0>, <&tbg 1>, <&tbg 2>, <&tbg 3>, <&xtalclk>;
> > > +      clock-names = "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal";
> > > +      #clock-cells = <1>;
> > > +    };
> > > -- 
> > > 2.20.1
> > >
Stephen Boyd Oct. 15, 2021, 9:55 p.m. UTC | #5
Quoting Pali Rohár (2021-10-15 02:37:01)
> On Friday 15 October 2021 11:09:37 Pali Rohár wrote:
> > On Thursday 14 October 2021 17:13:03 Stephen Boyd wrote:
> > > Quoting Pali Rohár (2021-09-30 02:58:35)
> > > > diff --git a/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > > > new file mode 100644
> > > > index 000000000000..175f5c8f2bc5
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > > > @@ -0,0 +1,59 @@
> > > [..]
> > > > +  '#clock-cells':
> > > > +    const: 1
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - clocks
> > > > +  - clock-names
> > > > +  - '#clock-cells'
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    uartclk: clock-controller@12010 {
> > > 
> > > The uart device is at 0x12000 and the clock-controller is at 0x12010?
> > > This looks like a node is being put into DT to represent a clk driver.
> > > Why can't we register a clk from the uart device driver itself? I think
> > > we talked about this a month or two ago but it still isn't clear to me.
> > 
> > We have already talked about it and I have already wrote reasons. UART
> > clk is shared for both UART1 and UART2. And UART clk regs are in both
> > address spaces of UART1 and UART2. UART1 or UART2 can be independently
> > disabled on particular board (as pins are MPP which may be configured to
> > different function). So you have a board only with UART2, you have to
> > disable UART1 node, but at the same time you have to access UART clk to
> > drive UART2. And UART clk bits are in UART1 address space.
> 
> It is explained also in commit message of patch 2/6.

Cool, thanks for the pointer.

Why are the two uarts split into different device nodes? It looks like
it's one device that was split into two nodes because they're fairly
similar hardware blocks, and one or the other may not be used on the
board so we want to use status = "disabled" to indicate that. Sadly the
hardware team has delivered them as a single package into the SoC at
address 0x12000 and then stuck a common clk for both uarts into the same
uart wrapper. Here's a clk, job done!

Is it a problem to map UART1 address space when it isn't used on the
board? I'm trying to understand why it can't work to register two uart
ports from one device node and driver. It seems to be the main reason
why we're introducing another node for the clk registers when it feels
like it could all be handled in the existing uart driver.

For example, we could have a static clk pointer in the uart driver
indicating the clk has been registered, and then register the clk if
uart1 or uart2 is the first device to probe and then store that clk in a
global (with clk_hw_get_clk(), I think that's a thing now). If uart2
probes first it can take the reg property and subtract some number to
find the clk, and if uart1 probes first it can take the reg property and
add some number to find the clk. Either way, the binding doesn't change
in this case and we don't have to add another binding for this same uart
hardware.

Then if someone wants to cleanup the binding they can combine both uarts
into one node, make a new compatible string and add some property to
indicate that one or the other uart isn't used. Probably also add some
property to map the uart alias to the uart hardware block inside the
wrapper node.
Mark Kettenis Oct. 15, 2021, 10:08 p.m. UTC | #6
> From: Stephen Boyd <sboyd@kernel.org>
> Date: Fri, 15 Oct 2021 14:55:47 -0700
> 
> Quoting Pali Rohár (2021-10-15 02:37:01)
> > On Friday 15 October 2021 11:09:37 Pali Rohár wrote:
> > > On Thursday 14 October 2021 17:13:03 Stephen Boyd wrote:
> > > > Quoting Pali Rohár (2021-09-30 02:58:35)
> > > > > diff --git a/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..175f5c8f2bc5
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > > > > @@ -0,0 +1,59 @@
> > > > [..]
> > > > > +  '#clock-cells':
> > > > > +    const: 1
> > > > > +
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - reg
> > > > > +  - clocks
> > > > > +  - clock-names
> > > > > +  - '#clock-cells'
> > > > > +
> > > > > +additionalProperties: false
> > > > > +
> > > > > +examples:
> > > > > +  - |
> > > > > +    uartclk: clock-controller@12010 {
> > > > 
> > > > The uart device is at 0x12000 and the clock-controller is at 0x12010?
> > > > This looks like a node is being put into DT to represent a clk driver.
> > > > Why can't we register a clk from the uart device driver itself? I think
> > > > we talked about this a month or two ago but it still isn't clear to me.
> > > 
> > > We have already talked about it and I have already wrote reasons. UART
> > > clk is shared for both UART1 and UART2. And UART clk regs are in both
> > > address spaces of UART1 and UART2. UART1 or UART2 can be independently
> > > disabled on particular board (as pins are MPP which may be configured to
> > > different function). So you have a board only with UART2, you have to
> > > disable UART1 node, but at the same time you have to access UART clk to
> > > drive UART2. And UART clk bits are in UART1 address space.
> > 
> > It is explained also in commit message of patch 2/6.
> 
> Cool, thanks for the pointer.
> 
> Why are the two uarts split into different device nodes? It looks like
> it's one device that was split into two nodes because they're fairly
> similar hardware blocks, and one or the other may not be used on the
> board so we want to use status = "disabled" to indicate that. Sadly the
> hardware team has delivered them as a single package into the SoC at
> address 0x12000 and then stuck a common clk for both uarts into the same
> uart wrapper. Here's a clk, job done!
> 
> Is it a problem to map UART1 address space when it isn't used on the
> board? I'm trying to understand why it can't work to register two uart
> ports from one device node and driver.

Separate nodes are needed because stdin-path and stdout-patch need to
be able to point at a specific device node.
Pali Rohár Oct. 16, 2021, 6:42 a.m. UTC | #7
On Friday 15 October 2021 14:55:47 Stephen Boyd wrote:
> Quoting Pali Rohár (2021-10-15 02:37:01)
> > On Friday 15 October 2021 11:09:37 Pali Rohár wrote:
> > > On Thursday 14 October 2021 17:13:03 Stephen Boyd wrote:
> > > > Quoting Pali Rohár (2021-09-30 02:58:35)
> > > > > diff --git a/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..175f5c8f2bc5
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > > > > @@ -0,0 +1,59 @@
> > > > [..]
> > > > > +  '#clock-cells':
> > > > > +    const: 1
> > > > > +
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - reg
> > > > > +  - clocks
> > > > > +  - clock-names
> > > > > +  - '#clock-cells'
> > > > > +
> > > > > +additionalProperties: false
> > > > > +
> > > > > +examples:
> > > > > +  - |
> > > > > +    uartclk: clock-controller@12010 {
> > > > 
> > > > The uart device is at 0x12000 and the clock-controller is at 0x12010?
> > > > This looks like a node is being put into DT to represent a clk driver.
> > > > Why can't we register a clk from the uart device driver itself? I think
> > > > we talked about this a month or two ago but it still isn't clear to me.
> > > 
> > > We have already talked about it and I have already wrote reasons. UART
> > > clk is shared for both UART1 and UART2. And UART clk regs are in both
> > > address spaces of UART1 and UART2. UART1 or UART2 can be independently
> > > disabled on particular board (as pins are MPP which may be configured to
> > > different function). So you have a board only with UART2, you have to
> > > disable UART1 node, but at the same time you have to access UART clk to
> > > drive UART2. And UART clk bits are in UART1 address space.
> > 
> > It is explained also in commit message of patch 2/6.
> 
> Cool, thanks for the pointer.
> 
> Why are the two uarts split into different device nodes?

I do not know. Looks like decision of people implementing this driver
and providing DT bindings.

I cannot change this existing DT binding due to backward compatibility.

> It looks like
> it's one device that was split into two nodes because they're fairly
> similar hardware blocks, and one or the other may not be used on the
> board so we want to use status = "disabled" to indicate that. Sadly the
> hardware team has delivered them as a single package into the SoC at
> address 0x12000 and then stuck a common clk for both uarts into the same
> uart wrapper. Here's a clk, job done!
> 
> Is it a problem to map UART1 address space when it isn't used on the
> board? I'm trying to understand why it can't work to register two uart
> ports from one device node and driver. It seems to be the main reason
> why we're introducing another node for the clk registers when it feels
> like it could all be handled in the existing uart driver.

Mapping address space should work. Also because in UART1 address space
is configuration of UART clock (which is common for both UART1 and
UART2). Moreover each UART has its own bit for disabling clock and these
two bits are in UART1 address space. Also it is a good idea to disable
unused UART clock (which seems to be magically/automatically done by
kernel when nobody use specified UART clock, as UART clock driver
exports two clocks).

> For example, we could have a static clk pointer in the uart driver
> indicating the clk has been registered, and then register the clk if
> uart1 or uart2 is the first device to probe and then store that clk in a
> global (with clk_hw_get_clk(), I think that's a thing now). If uart2
> probes first it can take the reg property and subtract some number to
> find the clk, and if uart1 probes first it can take the reg property and
> add some number to find the clk. Either way, the binding doesn't change
> in this case and we don't have to add another binding for this same uart
> hardware.
> 
> Then if someone wants to cleanup the binding they can combine both uarts
> into one node, make a new compatible string and add some property to
> indicate that one or the other uart isn't used. Probably also add some
> property to map the uart alias to the uart hardware block inside the
> wrapper node.

If I was designing this driver and DTS bindings I would have choose
something like this:

uart@0x12000 {
    reg = <0x12000 0x18>, <0x12200 0x30>;
    clock-controller {
        ...
    };
    serial1 {
        ...
        status = "disabled";
    };
    serial2 {
        ...
        status = "disabled";
    };
};

Meaning that 0x12000 node would be 3 subnodes and all registers would be
defined in top level nodes and would be handled by one driver.

This is really how hardware block looks like. But it is not backward
compatible...
Stephen Boyd Jan. 15, 2022, 8:02 a.m. UTC | #8
Quoting Pali Rohár (2021-10-15 23:42:10)
> 
> If I was designing this driver and DTS bindings I would have choose
> something like this:
> 
> uart@0x12000 {

Drop the 0x

>     reg = <0x12000 0x18>, <0x12200 0x30>;
>     clock-controller {
>         ...
>     };

Drop this node and put whatever properties are inside into the parent
node.

>     serial1 {
>         ...
>         status = "disabled";
>     };
>     serial2 {
>         ...
>         status = "disabled";
>     };
> };
> 
> Meaning that 0x12000 node would be 3 subnodes and all registers would be
> defined in top level nodes and would be handled by one driver.
> 
> This is really how hardware block looks like. But it is not backward
> compatible...

Sounds good to me. I presume we need the serial child nodes so we can
reference them from the stdout-path?
Pali Rohár Jan. 15, 2022, 11:50 a.m. UTC | #9
On Saturday 15 January 2022 00:02:11 Stephen Boyd wrote:
> Quoting Pali Rohár (2021-10-15 23:42:10)
> > 
> > If I was designing this driver and DTS bindings I would have choose
> > something like this:
> > 
> > uart@0x12000 {
> 
> Drop the 0x
> 
> >     reg = <0x12000 0x18>, <0x12200 0x30>;
> >     clock-controller {
> >         ...
> >     };
> 
> Drop this node and put whatever properties are inside into the parent
> node.
> 
> >     serial1 {
> >         ...
> >         status = "disabled";
> >     };
> >     serial2 {
> >         ...
> >         status = "disabled";
> >     };
> > };
> > 
> > Meaning that 0x12000 node would be 3 subnodes and all registers would be
> > defined in top level nodes and would be handled by one driver.
> > 
> > This is really how hardware block looks like. But it is not backward
> > compatible...
> 
> Sounds good to me. I presume we need the serial child nodes so we can
> reference them from the stdout-path?

Yes, exactly, separate nodes for serial1 and serial2 are still required.

But dropping clock controller is not possible as for higher baudrates we
need to use and configure uart clock controller. Without it we just get
comparable feature support which is already present in driver.

But, I do not fully understand now, why to change this DTS bindings in
this incompatible way? What it brings? Because for me now it looks like
that this change does not bring anything useful, only breaks current DTS
bindings.

Driver changes would still look in the similar / same way like it is in
current patch series because bindings already contains separate nodes,
just they are children of top level node which represents in internal
registers.
Marek Behún Jan. 15, 2022, 12:05 p.m. UTC | #10
On Sat, 15 Jan 2022 12:50:18 +0100
Pali Rohár <pali@kernel.org> wrote:

> On Saturday 15 January 2022 00:02:11 Stephen Boyd wrote:
> > Quoting Pali Rohár (2021-10-15 23:42:10)  
> > > 
> > > If I was designing this driver and DTS bindings I would have choose
> > > something like this:
> > > 
> > > uart@0x12000 {  
> > 
> > Drop the 0x
> >   
> > >     reg = <0x12000 0x18>, <0x12200 0x30>;
> > >     clock-controller {
> > >         ...
> > >     };  
> > 
> > Drop this node and put whatever properties are inside into the parent
> > node.
> >   
> > >     serial1 {
> > >         ...
> > >         status = "disabled";
> > >     };
> > >     serial2 {
> > >         ...
> > >         status = "disabled";
> > >     };
> > > };
> > > 
> > > Meaning that 0x12000 node would be 3 subnodes and all registers would be
> > > defined in top level nodes and would be handled by one driver.
> > > 
> > > This is really how hardware block looks like. But it is not backward
> > > compatible...  
> > 
> > Sounds good to me. I presume we need the serial child nodes so we can
> > reference them from the stdout-path?  
> 
> Yes, exactly, separate nodes for serial1 and serial2 are still required.
> 
> But dropping clock controller is not possible as for higher baudrates we
> need to use and configure uart clock controller. Without it we just get
> comparable feature support which is already present in driver.

What Stephen means is making clock controller out of the uart node
directly. No need to add separate subnode just for clock controller.

Marek
Pali Rohár Jan. 15, 2022, 12:26 p.m. UTC | #11
On Saturday 15 January 2022 13:05:09 Marek Behún wrote:
> On Sat, 15 Jan 2022 12:50:18 +0100
> Pali Rohár <pali@kernel.org> wrote:
> 
> > On Saturday 15 January 2022 00:02:11 Stephen Boyd wrote:
> > > Quoting Pali Rohár (2021-10-15 23:42:10)  
> > > > 
> > > > If I was designing this driver and DTS bindings I would have choose
> > > > something like this:
> > > > 
> > > > uart@0x12000 {  
> > > 
> > > Drop the 0x
> > >   
> > > >     reg = <0x12000 0x18>, <0x12200 0x30>;
> > > >     clock-controller {
> > > >         ...
> > > >     };  
> > > 
> > > Drop this node and put whatever properties are inside into the parent
> > > node.
> > >   
> > > >     serial1 {
> > > >         ...
> > > >         status = "disabled";
> > > >     };
> > > >     serial2 {
> > > >         ...
> > > >         status = "disabled";
> > > >     };
> > > > };
> > > > 
> > > > Meaning that 0x12000 node would be 3 subnodes and all registers would be
> > > > defined in top level nodes and would be handled by one driver.
> > > > 
> > > > This is really how hardware block looks like. But it is not backward
> > > > compatible...  
> > > 
> > > Sounds good to me. I presume we need the serial child nodes so we can
> > > reference them from the stdout-path?  
> > 
> > Yes, exactly, separate nodes for serial1 and serial2 are still required.
> > 
> > But dropping clock controller is not possible as for higher baudrates we
> > need to use and configure uart clock controller. Without it we just get
> > comparable feature support which is already present in driver.
> 
> What Stephen means is making clock controller out of the uart node
> directly. No need to add separate subnode just for clock controller.

This is already implemented in v7 patch series. Clock controller is
already outside of uart nodes.
Stephen Boyd Jan. 19, 2022, 11:16 p.m. UTC | #12
Quoting Pali Rohár (2022-01-15 04:26:18)
> On Saturday 15 January 2022 13:05:09 Marek Behún wrote:
> > On Sat, 15 Jan 2022 12:50:18 +0100
> > Pali Rohár <pali@kernel.org> wrote:
> > 
> > > On Saturday 15 January 2022 00:02:11 Stephen Boyd wrote:
> > > > Quoting Pali Rohár (2021-10-15 23:42:10)  
> > > > > 
> > > > > If I was designing this driver and DTS bindings I would have choose
> > > > > something like this:
> > > > > 
> > > > > uart@0x12000 {  
> > > > 
> > > > Drop the 0x
> > > >   
> > > > >     reg = <0x12000 0x18>, <0x12200 0x30>;
> > > > >     clock-controller {
> > > > >         ...
> > > > >     };  
> > > > 
> > > > Drop this node and put whatever properties are inside into the parent
> > > > node.
> > > >   
> > > > >     serial1 {
> > > > >         ...
> > > > >         status = "disabled";
> > > > >     };
> > > > >     serial2 {
> > > > >         ...
> > > > >         status = "disabled";
> > > > >     };
> > > > > };
> > > > > 
> > > > > Meaning that 0x12000 node would be 3 subnodes and all registers would be
> > > > > defined in top level nodes and would be handled by one driver.
> > > > > 
> > > > > This is really how hardware block looks like. But it is not backward
> > > > > compatible...  
> > > > 
> > > > Sounds good to me. I presume we need the serial child nodes so we can
> > > > reference them from the stdout-path?  
> > > 
> > > Yes, exactly, separate nodes for serial1 and serial2 are still required.
> > > 
> > > But dropping clock controller is not possible as for higher baudrates we
> > > need to use and configure uart clock controller. Without it we just get
> > > comparable feature support which is already present in driver.
> > 
> > What Stephen means is making clock controller out of the uart node
> > directly. No need to add separate subnode just for clock controller.
> 
> This is already implemented in v7 patch series. Clock controller is
> already outside of uart nodes.

I mean to combine the uart node and the clock-controller node together

	uart-wrapper {
		reg = <0x12000 0x18>, <0x12200 0x30>;
		#clock-cells ...

		serial1 {
			...
		};

		serial2 {
			...
		};
	};
Pali Rohár Jan. 20, 2022, 12:06 a.m. UTC | #13
On Wednesday 19 January 2022 15:16:54 Stephen Boyd wrote:
> Quoting Pali Rohár (2022-01-15 04:26:18)
> > On Saturday 15 January 2022 13:05:09 Marek Behún wrote:
> > > On Sat, 15 Jan 2022 12:50:18 +0100
> > > Pali Rohár <pali@kernel.org> wrote:
> > > 
> > > > On Saturday 15 January 2022 00:02:11 Stephen Boyd wrote:
> > > > > Quoting Pali Rohár (2021-10-15 23:42:10)  
> > > > > > 
> > > > > > If I was designing this driver and DTS bindings I would have choose
> > > > > > something like this:
> > > > > > 
> > > > > > uart@0x12000 {  
> > > > > 
> > > > > Drop the 0x
> > > > >   
> > > > > >     reg = <0x12000 0x18>, <0x12200 0x30>;
> > > > > >     clock-controller {
> > > > > >         ...
> > > > > >     };  
> > > > > 
> > > > > Drop this node and put whatever properties are inside into the parent
> > > > > node.
> > > > >   
> > > > > >     serial1 {
> > > > > >         ...
> > > > > >         status = "disabled";
> > > > > >     };
> > > > > >     serial2 {
> > > > > >         ...
> > > > > >         status = "disabled";
> > > > > >     };
> > > > > > };
> > > > > > 
> > > > > > Meaning that 0x12000 node would be 3 subnodes and all registers would be
> > > > > > defined in top level nodes and would be handled by one driver.
> > > > > > 
> > > > > > This is really how hardware block looks like. But it is not backward
> > > > > > compatible...  
> > > > > 
> > > > > Sounds good to me. I presume we need the serial child nodes so we can
> > > > > reference them from the stdout-path?  
> > > > 
> > > > Yes, exactly, separate nodes for serial1 and serial2 are still required.
> > > > 
> > > > But dropping clock controller is not possible as for higher baudrates we
> > > > need to use and configure uart clock controller. Without it we just get
> > > > comparable feature support which is already present in driver.
> > > 
> > > What Stephen means is making clock controller out of the uart node
> > > directly. No need to add separate subnode just for clock controller.
> > 
> > This is already implemented in v7 patch series. Clock controller is
> > already outside of uart nodes.
> 
> I mean to combine the uart node and the clock-controller node together
> 
> 	uart-wrapper {
> 		reg = <0x12000 0x18>, <0x12200 0x30>;
> 		#clock-cells ...
> 
> 		serial1 {
> 			...
> 		};
> 
> 		serial2 {
> 			...
> 		};
> 	};

Ok, now I see what you mean.

But problem is that this is not backward compatible change. And would
not work per existing DT bindings definitions, which defines how
bootloader should set configured clocks.

As I wrote in emails 3 months ago, this new "proposed" DTS definition is
something which I would have chosen if I had designed this driver and
bindings in past. But that did not happen and different approach is
already widely in used.

To support existing DTS definitions and bootloaders, it is really
required to have current structure backward compatible like it is
defined in current DT bindings document. And my changes in this patch
series are backward compatible.

To change DTS structure, it would be needed to provide uart nodes in DTS
files two times: once in old style (the current one) and second time in
this new style.

But such thing would even more complicate updating driver and it needs
to be implemented.

Plus this would open a question how to define default stdout-path if
there would be 4 serial nodes, where one pair would describe old style
and second pair new style; meaning that 2 cross nodes would describe
same define.

For me this looks like a more complications and I do not see any benefit
from it.

It is really important to break backward compatibility, just to try
having new cleaner API at the cost of having more complications and
requirement for more development and also important maintenance?
Stephen Boyd Jan. 20, 2022, 6:01 a.m. UTC | #14
Quoting Pali Rohár (2022-01-19 16:06:51)
> On Wednesday 19 January 2022 15:16:54 Stephen Boyd wrote:
> > Quoting Pali Rohár (2022-01-15 04:26:18)
> > > On Saturday 15 January 2022 13:05:09 Marek Behún wrote:
> > > > On Sat, 15 Jan 2022 12:50:18 +0100
> > > > Pali Rohár <pali@kernel.org> wrote:
> > > > 
> > > > > On Saturday 15 January 2022 00:02:11 Stephen Boyd wrote:
> > > > > > Quoting Pali Rohár (2021-10-15 23:42:10)  
> > > > > > > 
> > > > > > > If I was designing this driver and DTS bindings I would have choose
> > > > > > > something like this:
> > > > > > > 
> > > > > > > uart@0x12000 {  
> > > > > > 
> > > > > > Drop the 0x
> > > > > >   
> > > > > > >     reg = <0x12000 0x18>, <0x12200 0x30>;
> > > > > > >     clock-controller {
> > > > > > >         ...
> > > > > > >     };  
> > > > > > 
> > > > > > Drop this node and put whatever properties are inside into the parent
> > > > > > node.
> > > > > >   
> > > > > > >     serial1 {
> > > > > > >         ...
> > > > > > >         status = "disabled";
> > > > > > >     };
> > > > > > >     serial2 {
> > > > > > >         ...
> > > > > > >         status = "disabled";
> > > > > > >     };
> > > > > > > };
> > > > > > > 
> > > > > > > Meaning that 0x12000 node would be 3 subnodes and all registers would be
> > > > > > > defined in top level nodes and would be handled by one driver.
> > > > > > > 
> > > > > > > This is really how hardware block looks like. But it is not backward
> > > > > > > compatible...  
> > > > > > 
> > > > > > Sounds good to me. I presume we need the serial child nodes so we can
> > > > > > reference them from the stdout-path?  
> > > > > 
> > > > > Yes, exactly, separate nodes for serial1 and serial2 are still required.
> > > > > 
> > > > > But dropping clock controller is not possible as for higher baudrates we
> > > > > need to use and configure uart clock controller. Without it we just get
> > > > > comparable feature support which is already present in driver.
> > > > 
> > > > What Stephen means is making clock controller out of the uart node
> > > > directly. No need to add separate subnode just for clock controller.
> > > 
> > > This is already implemented in v7 patch series. Clock controller is
> > > already outside of uart nodes.
> > 
> > I mean to combine the uart node and the clock-controller node together
> > 
> >       uart-wrapper {
> >               reg = <0x12000 0x18>, <0x12200 0x30>;
> >               #clock-cells ...
> > 
> >               serial1 {
> >                       ...
> >               };
> > 
> >               serial2 {
> >                       ...
> >               };
> >       };
> 
> Ok, now I see what you mean.
> 
> But problem is that this is not backward compatible change. And would
> not work per existing DT bindings definitions, which defines how
> bootloader should set configured clocks.
> 
> As I wrote in emails 3 months ago, this new "proposed" DTS definition is
> something which I would have chosen if I had designed this driver and
> bindings in past. But that did not happen and different approach is
> already widely in used.
> 
> To support existing DTS definitions and bootloaders, it is really
> required to have current structure backward compatible like it is
> defined in current DT bindings document. And my changes in this patch
> series are backward compatible.

I'm lost. Is the bootloader the one that's expecting some particular
serial node format and updating something? What is the bootloader doing?

> 
> To change DTS structure, it would be needed to provide uart nodes in DTS
> files two times: once in old style (the current one) and second time in
> this new style.

That's not a good idea. Why do we need to support both at the same time?

> 
> But such thing would even more complicate updating driver and it needs
> to be implemented.
> 
> Plus this would open a question how to define default stdout-path if
> there would be 4 serial nodes, where one pair would describe old style
> and second pair new style; meaning that 2 cross nodes would describe
> same define.

Huh? We shouldn't have both bindings present in the DTB.

> 
> For me this looks like a more complications and I do not see any benefit
> from it.
> 
> It is really important to break backward compatibility, just to try
> having new cleaner API at the cost of having more complications and
> requirement for more development and also important maintenance?

It's important to not make DT nodes have reg properties that overlap.
Maybe this is a DT purist viewpoint and I'm totally off base! I think
Rob did ack this binding already so I must be coming from the wrong
angle.

Nothing prevents register overlap from happening in practice, but it's
good to avoid such a situation as it clearly divides the I/O space by
assigning an address range to a particular device. In this case, we see
the two uarts are really one device, but we need two nodes in DT for
stdout-path, so we make some child nodes and have the driver figure out
which serial port to use for the console.

We shouldn't be adding more nodes to DT to get drivers to probe for
device I/O spaces that have already been described in DT. When this
happens, we learn that some I/O range is actually a combination of
functions, like uart and clks, and thus we should be able to add any
required properties to the existing DT node to support that new feature
that wasn't described before in the binding.
Pali Rohár Jan. 20, 2022, 9:26 a.m. UTC | #15
On Wednesday 19 January 2022 22:01:47 Stephen Boyd wrote:
> Quoting Pali Rohár (2022-01-19 16:06:51)
> > On Wednesday 19 January 2022 15:16:54 Stephen Boyd wrote:
> > > Quoting Pali Rohár (2022-01-15 04:26:18)
> > > > On Saturday 15 January 2022 13:05:09 Marek Behún wrote:
> > > > > On Sat, 15 Jan 2022 12:50:18 +0100
> > > > > Pali Rohár <pali@kernel.org> wrote:
> > > > > 
> > > > > > On Saturday 15 January 2022 00:02:11 Stephen Boyd wrote:
> > > > > > > Quoting Pali Rohár (2021-10-15 23:42:10)  
> > > > > > > > 
> > > > > > > > If I was designing this driver and DTS bindings I would have choose
> > > > > > > > something like this:
> > > > > > > > 
> > > > > > > > uart@0x12000 {  
> > > > > > > 
> > > > > > > Drop the 0x
> > > > > > >   
> > > > > > > >     reg = <0x12000 0x18>, <0x12200 0x30>;
> > > > > > > >     clock-controller {
> > > > > > > >         ...
> > > > > > > >     };  
> > > > > > > 
> > > > > > > Drop this node and put whatever properties are inside into the parent
> > > > > > > node.
> > > > > > >   
> > > > > > > >     serial1 {
> > > > > > > >         ...
> > > > > > > >         status = "disabled";
> > > > > > > >     };
> > > > > > > >     serial2 {
> > > > > > > >         ...
> > > > > > > >         status = "disabled";
> > > > > > > >     };
> > > > > > > > };
> > > > > > > > 
> > > > > > > > Meaning that 0x12000 node would be 3 subnodes and all registers would be
> > > > > > > > defined in top level nodes and would be handled by one driver.
> > > > > > > > 
> > > > > > > > This is really how hardware block looks like. But it is not backward
> > > > > > > > compatible...  
> > > > > > > 
> > > > > > > Sounds good to me. I presume we need the serial child nodes so we can
> > > > > > > reference them from the stdout-path?  
> > > > > > 
> > > > > > Yes, exactly, separate nodes for serial1 and serial2 are still required.
> > > > > > 
> > > > > > But dropping clock controller is not possible as for higher baudrates we
> > > > > > need to use and configure uart clock controller. Without it we just get
> > > > > > comparable feature support which is already present in driver.
> > > > > 
> > > > > What Stephen means is making clock controller out of the uart node
> > > > > directly. No need to add separate subnode just for clock controller.
> > > > 
> > > > This is already implemented in v7 patch series. Clock controller is
> > > > already outside of uart nodes.
> > > 
> > > I mean to combine the uart node and the clock-controller node together
> > > 
> > >       uart-wrapper {
> > >               reg = <0x12000 0x18>, <0x12200 0x30>;
> > >               #clock-cells ...
> > > 
> > >               serial1 {
> > >                       ...
> > >               };
> > > 
> > >               serial2 {
> > >                       ...
> > >               };
> > >       };
> > 
> > Ok, now I see what you mean.
> > 
> > But problem is that this is not backward compatible change. And would
> > not work per existing DT bindings definitions, which defines how
> > bootloader should set configured clocks.
> > 
> > As I wrote in emails 3 months ago, this new "proposed" DTS definition is
> > something which I would have chosen if I had designed this driver and
> > bindings in past. But that did not happen and different approach is
> > already widely in used.
> > 
> > To support existing DTS definitions and bootloaders, it is really
> > required to have current structure backward compatible like it is
> > defined in current DT bindings document. And my changes in this patch
> > series are backward compatible.
> 
> I'm lost. Is the bootloader the one that's expecting some particular
> serial node format and updating something? What is the bootloader doing?

If bootloader uses or configures UART to different clock it needs to
update "clocks" property in DT. Otherwise UART would be unusable and
there would be no dmesg output.

A3720 heavily depends that bootloader patches at boot time DTB file to
the layout of the current hardware.

> > 
> > To change DTS structure, it would be needed to provide uart nodes in DTS
> > files two times: once in old style (the current one) and second time in
> > this new style.
> 
> That's not a good idea. Why do we need to support both at the same time?

Because old bootloaders do not and will never support this new style. It
is not only linux kernel project who provides DTB files. Also bootloader
itself has own DTB files and use it for booting (e.g kernel). For some
boards is in-kernel-tree DTS file only as a reference. So it is
important that kernel can use and support DTS files from old version and
also from the new patched version. Gregory (A3720 DTS files maintainer)
always ask me what happens if I try to boot new patched kernel drivers
with old unmodified DTS files and wants to know if nothing is broken by
introduced changed.

> > 
> > But such thing would even more complicate updating driver and it needs
> > to be implemented.
> > 
> > Plus this would open a question how to define default stdout-path if
> > there would be 4 serial nodes, where one pair would describe old style
> > and second pair new style; meaning that 2 cross nodes would describe
> > same define.
> 
> Huh? We shouldn't have both bindings present in the DTB.

Ideally yes, I would like to see to prevent it. But for backward
compatibility we really need old bindings still present (as explained
above).

So really I see two options here: Make changes in patches backward
compatible (old nodes stay in DT and also kernel would be able to use
old DT). Or let old bindings untouched in DT and new backward
incompatible definitions would have to be in separate nodes.

> > 
> > For me this looks like a more complications and I do not see any benefit
> > from it.
> > 
> > It is really important to break backward compatibility, just to try
> > having new cleaner API at the cost of having more complications and
> > requirement for more development and also important maintenance?
> 
> It's important to not make DT nodes have reg properties that overlap.
> Maybe this is a DT purist viewpoint and I'm totally off base! I think
> Rob did ack this binding already so I must be coming from the wrong
> angle.

I know this. In case it happens that driver for "one DT node" needs to
access regs of "another DT node" then regmap interface is used and
driver access regs of "another DT node" via regmap. No overlapping is in
DT. But here it is not possible to use regmap as "another DT node" is in
"disabled" state on some boards. And so regmap driver is not bound to
it.

In beginning there was not overlapping in DT because people have not
looked properly that some registers of uart2 are in uart1 space and did
not exported them to driver (bootloader initialized them to some sane
values and nobody noticed that they are required).

This overlapping starting to be required after I properly looked how
driver is working, how it maps to HW and how to implement choosing
clocks and allowing to change baudrate to higher values.

> Nothing prevents register overlap from happening in practice, but it's
> good to avoid such a situation as it clearly divides the I/O space by
> assigning an address range to a particular device. In this case, we see
> the two uarts are really one device, but we need two nodes in DT for
> stdout-path, so we make some child nodes and have the driver figure out
> which serial port to use for the console.
> 
> We shouldn't be adding more nodes to DT to get drivers to probe for
> device I/O spaces that have already been described in DT. When this
> happens, we learn that some I/O range is actually a combination of
> functions, like uart and clks, and thus we should be able to add any
> required properties to the existing DT node to support that new feature
> that wasn't described before in the binding.
Stephen Boyd Jan. 25, 2022, 8:40 p.m. UTC | #16
Quoting Pali Rohár (2022-01-20 01:26:41)
> On Wednesday 19 January 2022 22:01:47 Stephen Boyd wrote:
> > > 
> > > Ok, now I see what you mean.
> > > 
> > > But problem is that this is not backward compatible change. And would
> > > not work per existing DT bindings definitions, which defines how
> > > bootloader should set configured clocks.
> > > 
> > > As I wrote in emails 3 months ago, this new "proposed" DTS definition is
> > > something which I would have chosen if I had designed this driver and
> > > bindings in past. But that did not happen and different approach is
> > > already widely in used.
> > > 
> > > To support existing DTS definitions and bootloaders, it is really
> > > required to have current structure backward compatible like it is
> > > defined in current DT bindings document. And my changes in this patch
> > > series are backward compatible.
> > 
> > I'm lost. Is the bootloader the one that's expecting some particular
> > serial node format and updating something? What is the bootloader doing?
> 
> If bootloader uses or configures UART to different clock it needs to
> update "clocks" property in DT. Otherwise UART would be unusable and
> there would be no dmesg output.

Got it! I didn't see that part mentioned anywhere in the commit text
though. To the uninformed reviewer like me it is hard to know about this
bootloader design unless the commit text explains that there's no other
way to do this.

> 
> A3720 heavily depends that bootloader patches at boot time DTB file to
> the layout of the current hardware.
> 
> > > 
> > > To change DTS structure, it would be needed to provide uart nodes in DTS
> > > files two times: once in old style (the current one) and second time in
> > > this new style.
> > 
> > That's not a good idea. Why do we need to support both at the same time?
> 
> Because old bootloaders do not and will never support this new style. It
> is not only linux kernel project who provides DTB files. Also bootloader
> itself has own DTB files and use it for booting (e.g kernel). For some
> boards is in-kernel-tree DTS file only as a reference. So it is
> important that kernel can use and support DTS files from old version and
> also from the new patched version. Gregory (A3720 DTS files maintainer)
> always ask me what happens if I try to boot new patched kernel drivers
> with old unmodified DTS files and wants to know if nothing is broken by
> introduced changed.
> 
> > > 
> > > But such thing would even more complicate updating driver and it needs
> > > to be implemented.
> > > 
> > > Plus this would open a question how to define default stdout-path if
> > > there would be 4 serial nodes, where one pair would describe old style
> > > and second pair new style; meaning that 2 cross nodes would describe
> > > same define.
> > 
> > Huh? We shouldn't have both bindings present in the DTB.
> 
> Ideally yes, I would like to see to prevent it. But for backward
> compatibility we really need old bindings still present (as explained
> above).
> 
> So really I see two options here: Make changes in patches backward
> compatible (old nodes stay in DT and also kernel would be able to use
> old DT). Or let old bindings untouched in DT and new backward
> incompatible definitions would have to be in separate nodes.

Ok I understand now. We have to keep both the serial nodes because the
bootloader is patching them. To make matters worse, one or the other
node may be disabled so we can't even add the new bits to the uart1
node. Can you update the commit text to record this sad state of affairs
and indicate that the only way to support this is to make a new node in
DT that the bootloader doesn't know about?
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
new file mode 100644
index 000000000000..175f5c8f2bc5
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
@@ -0,0 +1,59 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/marvell,armada-3700-uart-clock.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+title: Marvell Armada 3720 UART clocks
+
+maintainers:
+  - Pali Rohár <pali@kernel.org>
+
+properties:
+  compatible:
+    const: marvell,armada-3700-uart-clock
+
+  reg:
+    items:
+      - description: UART Clock Control Register
+      - description: UART 2 Baud Rate Divisor Register
+
+  clocks:
+    description: |
+      List of parent clocks suitable for UART from following set:
+        "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal"
+      UART clock can use one from this set and when more are provided
+      then kernel would choose and configure the most suitable one.
+      It is suggest to specify at least one TBG clock to achieve
+      baudrates above 230400 and also to specify clock which bootloader
+      used for UART (most probably xtal) for smooth boot log on UART.
+
+  clock-names:
+    items:
+      - const: TBG-A-P
+      - const: TBG-B-P
+      - const: TBG-A-S
+      - const: TBG-B-S
+      - const: xtal
+    minItems: 1
+
+  '#clock-cells':
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    uartclk: clock-controller@12010 {
+      compatible = "marvell,armada-3700-uart-clock";
+      reg = <0x12010 0x4>, <0x12210 0x4>;
+      clocks = <&tbg 0>, <&tbg 1>, <&tbg 2>, <&tbg 3>, <&xtalclk>;
+      clock-names = "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal";
+      #clock-cells = <1>;
+    };