[PATCH] arm64: dts: Reformat PCI ranges/dma-ranges entries

[Rob Herring]

On Fri, Aug 21, 2020 at 11:14 AM Olof Johansson <olof at lixom.net> wrote:
>
> On Fri, Aug 21, 2020 at 9:14 AM Rob Herring <robh at kernel.org> wrote:
> >
> > On Thu, Aug 20, 2020 at 8:53 PM Olof Johansson <olof at lixom.net> wrote:
> > >
> > > On Wed, Aug 19, 2020 at 04:17:50PM -0600, Rob Herring wrote:
> > > > While bracketing doesn't matter for a DTB, the DT schema checks rely on
> > > > bracketing around each distinct entry. Reformat ranges and dma-ranges
> > > > entries to fix warnings such as:
> > > >
> > > > arch/arm64/boot/dts/rockchip/rk3399-sapphire-excavator.dt.yaml: pcie at f8000000: ranges: [[2197815296, 0, 4194304000, 0, 4194304000, 0, 31457280, 2164260864, 0, 4225761280, 0, 4225761280, 0, 1048576]] is not valid under any of the given schemas (Possible causes of the failure):
> > > >         arch/arm64/boot/dts/rockchip/rk3399-sapphire-excavator.dt.yaml: pcie at f8000000: ranges: True was expected
> > > >         arch/arm64/boot/dts/rockchip/rk3399-sapphire-excavator.dt.yaml: pcie at f8000000: ranges:0: [2197815296, 0, 4194304000, 0, 4194304000, 0, 31457280, 2164260864, 0, 4225761280, 0, 4225761280, 0, 1048576] is too long
> > > >         arch/arm64/boot/dts/rockchip/rk3399-sapphire-excavator.dt.yaml: pcie at f8000000: ranges:0:0: 2197815296 is not one of [16777216, 33554432, 50331648, 1107296256, 1124073472]
> > >
> > > Seems like a bug in your tool? Why would we bother with this churn?
> >
> > It's a feature. :)
>
> The feature is better linting of ranges, the new lack of being able to
> do that for the way we've always been allowed to write ranges is a
> bug.

You are free to write your dts files however you like. You can still
include all the errors, too. :)

> > In this case, with the entries bracketed, we can check the PCI top
> > address cell contents in the common PCI schema and check how many
> > ranges entries in the specific bridge schemas if they have some
> > constraints. The use of bracketing is useful to defining the number of
> > entries not just for ranges, but everywhere. A device binding defines
> > how many register regions or interrupts for example. It makes sense to
> > delineate entries in some way. While yes, we have #.*-cells, there's
> > not really any way to handle that in json-schema. And let's face it,
> > #.*-cells is an odd construct.
> >
> > Currently, the dtc dts->yaml output maintains all this formatting. I
> > suppose we could change that such that we parse #.*-cells and define
> > the boundaries based on them. But then dtc has to know about every
> > case of #.*-cells. That's not impossible to do, but doesn't keep the
> > tool and binding data separate.
>
> That's already the case, isn't it? Run that part of the check if you
> have a new enough dtc, otherwise warn that it's too old but do all the
> other checks.

Not really, I think. Not sure I follow though.

> It's also just a change in one place: the tool, instead of evolving
> the language by adding ad-hoc restrictions that the compiler doesn't
> even know about, and requiring old code to change.

Uh, it's 2 tools. There's no way to know if the dtc has the right
support in order to enable a schema or not. We'd need that on a per
property basis. For example, where we have:

properties:
  ranges:
    minItems: 2
    maxItems: 3

We'd have to process the schema to remove ranges if dtc version didn't
support formatting ranges.

I would like to have more granular warnings with either levels or
named checks, but there's not really any defined way to do that in
json-schema. The easiest would be something at the file level. We have
that somewhat in that we can run checks using a single schema file.

> This way DTC could even warn/recommend bracketing per cell, when you
> give it the ability to parse/handle it.
>
> > Looking at it another way, do we really want to just allow anything? A
> > device with 3 interrupts could be written as:
> >
> > interrupts = <1>, <2>, <3>;
> > interrupts = <1 2 3>;
> > interrupts = <1>, <2 3>;
> > interrupts = [ 00 00 00 01 00 00 00 02 00 00 00 03 ];
> >
> > Or can we have some coding standards that are no more onerous or
> > pedantic than what we have everywhere else?
>
> We're generally quite careful about applying new restrictions and
> expectations on coding standards all across the tree, and when we add
> new ones we usually don't require legacy code to change overnight,
> only when there are other relevant changes to those files.

But eventually we tend to make the changes. Can we at least agree the
first form of bracketing is preferred over the 2nd and is what new dts
files should use? If so, then most of the above is mute and it's a
question of how to enable checks per dts file with new ones enabled.
That was kind of what prompted this patch in the first place. I'm not
really trying to be in the business of fixing dts files. I was looking
at how to enable schema checks by default per platform (for 'make
dtbs') with Marc's complaint about not having checks enabled by
default[1]. There's a few platforms with a manageable number of
warnings to fix, and I started looking at some of the PCI related
warnings, fixed some false-positives in the schemas and did this patch
along the way.

In any case, we can enable schema checks with something like this:

extra-y += $(patsubst %.dtb,%.dt.yaml,$(dtb-y))

Or maybe "extra-$(CONFIG_HAS_DT_SCHEMA)" based on some check for the
tools, so I don't get yelled at for breaking everyone's make
allmodconfig. On the flip side, if I don't break allmodconfig for
missing schema tools, the warnings will just continue to be ignored.
And who will check and enforce all new platforms get enabled for
checks?

Rob

[1] https://lore.kernel.org/lkml/34ac2d7c0770f9aa450aaa3905c82f52@kernel.org/