Re: [PATCH v3 6/8] dm: treewide: Complete migration to new driver model schema

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 7308 bytes --]

On Tue, Feb 07, 2023 at 06:32:58PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 7 Feb 2023 at 17:16, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Feb 07, 2023 at 03:25:18PM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Tue, 7 Feb 2023 at 14:46, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Tue, Feb 07, 2023 at 02:43:49PM -0700, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Tue, 7 Feb 2023 at 14:06, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Mon, Feb 06, 2023 at 10:12:27AM -0700, Simon Glass wrote:
> > > > > > [snip]
> > > > > > > On Mon, 6 Feb 2023 at 07:56, Michal Simek <michal.simek@amd.com> wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > On 2/6/23 15:44, Tom Rini wrote:
> > > > > > > > > On Mon, Feb 06, 2023 at 01:22:48PM +0100, Michal Simek wrote:
> > > > > > > > >> Hi Simon,
> > > > > > > > >>
> > > > > > > > >> On 2/1/23 23:54, Simon Glass wrote:
> > > > > > > > >>> Update various build and test components to use the new schema.
> > > > > > > > >>>
> > > > > > > > >>> Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > > >>> ---
> > > > > > > > >>>
> > > > > > > > >>> (no changes since v1)
> > > > > > > > >>>
> > > > > > > > >>>    drivers/core/ofnode.c            | 10 +++++-----
> > > > > > > > >>>    drivers/video/video-uclass.c     |  4 ++--
> > > > > > > > >>>    dts/Kconfig                      |  2 +-
> > > > > > > > >>>    include/dm/device.h              |  2 +-
> > > > > > > > >>>    include/dm/ofnode.h              | 10 +++++-----
> > > > > > > > >>>    scripts/Makefile.lib             | 12 ++++++------
> > > > > > > > >>>    test/dm/test-fdt.c               |  2 +-
> > > > > > > > >>>    test/py/tests/test_ofplatdata.py |  8 ++++----
> > > > > > > > >>>    tools/binman/binman.rst          |  3 +--
> > > > > > > > >>>    tools/dtoc/test_fdt.py           |  8 ++++----
> > > > > > > > >>>    10 files changed, 30 insertions(+), 31 deletions(-)
> > > > > > > > >>>
> > > > > > > > >>> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
> > > > > > > > >>> index 4d56b1a7675..5249a60639b 100644
> > > > > > > > >>> --- a/drivers/core/ofnode.c
> > > > > > > > >>> +++ b/drivers/core/ofnode.c
> > > > > > > > >>> @@ -1265,22 +1265,22 @@ bool ofnode_pre_reloc(ofnode node)
> > > > > > > > >>>    {
> > > > > > > > >>>    #if defined(CONFIG_SPL_BUILD) || defined(CONFIG_TPL_BUILD)
> > > > > > > > >>>     /* for SPL and TPL the remaining nodes after the fdtgrep 1st pass
> > > > > > > > >>> -    * had property dm-pre-reloc or u-boot,dm-spl/tpl.
> > > > > > > > >>> +    * had property bootph-all or bootph-pre-sram/bootph-pre-ram.
> > > > > > > > >>>      * They are removed in final dtb (fdtgrep 2nd pass)
> > > > > > > > >>>      */
> > > > > > > > >>>     return true;
> > > > > > > > >>>    #else
> > > > > > > > >>> -   if (ofnode_read_bool(node, "u-boot,dm-pre-reloc"))
> > > > > > > > >>> +   if (ofnode_read_bool(node, "bootph-all"))
> > > > > > > > >>>             return true;
> > > > > > > > >>> -   if (ofnode_read_bool(node, "u-boot,dm-pre-proper"))
> > > > > > > > >>> +   if (ofnode_read_bool(node, "bootph-some-ram"))
> > > > > > > > >>>             return true;
> > > > > > > > >>>     /*
> > > > > > > > >>>      * In regular builds individual spl and tpl handling both
> > > > > > > > >>>      * count as handled pre-relocation for later second init.
> > > > > > > > >>>      */
> > > > > > > > >>> -   if (ofnode_read_bool(node, "u-boot,dm-spl") ||
> > > > > > > > >>> -       ofnode_read_bool(node, "u-boot,dm-tpl"))
> > > > > > > > >>> +   if (ofnode_read_bool(node, "bootph-pre-ram") ||
> > > > > > > > >>> +       ofnode_read_bool(node, "bootph-pre-sram"))
> > > > > > > > >>>             return true;
> > > > > > > > >>
> > > > > > > > >> Please correct me if I am wrong but this change will likely break all boards
> > > > > > > > >> which didn't migrate to this at this stage. And because targeting early
> > > > > > > > >> stages people will be without console.
> > > > > > > > >> I think we should have transition period for 1-2 releases to give people
> > > > > > > > >> enough time to migrate. It means print big warning that they have to migrate
> > > > > > > > >> their DTS.
> > > > > > > > >
> > > > > > > > > What's the migration case here we're missing? Is it platforms that
> > > > > > > > > maintain a dts externally, via tooling / etc, that populate those nodes?
> > > > > > > >
> > > > > > > > Yes and I expect there will be a lot of DTs around with some changes for
> > > > > > > > specific products.
> > > > > > > >
> > > > > > > > Also for example QEMU is also generating DT based on it's configuration and
> > > > > > > > provide it to U-Boot.
> > > > > > > > https://gitlab.com/qemu-project/qemu/-/blob/master/hw/arm/xlnx-versal-virt.c#L91
> > > > > > > > When this patch is applied CI loop should fail for Versal.
> > > > > > >
> > > > > > > I am not sure how it helps us to drag this out. It is a breaking
> > > > > > > change, but a drawn-out process is just going to create a lot of
> > > > > > > confusion. People should be free to use the schema in Linux .dts files
> > > > > > > from now on, but if it is not immediately supported in U-Boot then
> > > > > > > they cannot. This is the most important point, after all.
> > > > > >
> > > > > > Now that we've had some of the external migration issues laid out, what
> > > > > > would it look like to have some sort of backwards compatible hook here
> > > > > > to fixup an older tree we've been passed?
> > > > >
> > > > > We can't do it for SPL, since the processing happens at built time,
> > > > > but for U-Boot proper we can do something like what Michal suggests,
> > > > > although perhaps with a warning rather than an error. Likely the
> > > > > warning would have to be displayed later (if/when U-Boot starts up)
> > > > > since the console may be one of the problem nodes.
> > > >
> > > > Right, if it's a build time thing, we should be able to ... something.
> > > > Maybe a detect and rename for now, detect and fail in a bit. But
> > >
> > > But we only have this problem with out-of-tree .dts files, so I'm not
> > > sure what you are suggesting here.
> >
> > I'm suggesting that you add the logic to detect these cases and deal
> > with it. We aren't talking about stuff that should have been upstreamed
> > but wasn't, we're talking about tooling that generates valid dts files
> > and needs time to update.
> 
> For runtime I think we can do something like what Michal suggested,
> but more permissive. We can warn about it when U-Boot has started up.
> There is no point in doing anything like that in the SPL phase as it
> is too late.
> 
> But we can also add a build-time check, as you say. I think you are
> saying that it should work correctly in that case, rather than just
> fail? That is easy enough for U-Boot proper I think. For SPL I am not
> sure.
> 
> I will take a look.

For build time, if we're passed the old form, we should be able to
detect it, and regex the input to be correct. And not for forever, but
to give external (and yes, valid use case here!) tools time to catch up.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]