Message-Id: <20220125204006.A6D09C340E0@smtp.kernel.org>
Date:   Tue, 25 Jan 2022 12:40:04 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     Pali Rohár <pali@...nel.org>
Cc:     Marek Behún <kabel@...nel.org>,
        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>,
        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

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?

Powered by blists - more mailing lists