Message-ID: <20211016064210.7ahqfqcvf66wtt66@pali>
Date:   Sat, 16 Oct 2021 08:42:10 +0200
From:   Pali Rohár <pali@...nel.org>
To:     Stephen Boyd <sboyd@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Michael Turquette <mturquette@...libre.com>,
        Rob Herring <robh+dt@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Gregory Clement <gregory.clement@...tlin.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Vladimir Vid <vladimir.vid@...tura.hr>,
        Marek Behún <kabel@...nel.org>,
        linux-clk@...r.kernel.org, linux-serial@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for
 marvell,armada-3700-uart-clock

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@...10 {
> > > > 
> > > > 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@...2000 {
    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...

Powered by blists - more mailing lists