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

[Stephen Boyd <sboyd@kernel.org>]

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.