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

["Pali Rohár" <pali@kernel.org>]

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?