Re: [PATCH v2 01/17] dt-bindings: PCI: dwc: Define common and native DT bindings

[Serge Semin <fancer.lancer@gmail.com>]

On Mon, May 16, 2022 at 03:11:53PM -0500, Rob Herring wrote:
> On Wed, May 04, 2022 at 12:46:22AM +0300, Serge Semin wrote:
> > Currently both DW PCIe Root Port and End-point DT bindings are too generic
> > to be used as a descriptive set the device properties. Yes, it's very handy
> > to have them that way so the DT-schemas could be used to evaluate as many
> > DW PCIe-related DT-nodes as possible. But at the same time they don't
> > provide well defined DW PCIe RP/EP DT interface description thus leaving
> > too much flexibility for the new platforms, no encouraging the developers
> 

> s/no/not/

Thanks. will be fixed in v3.

> 
> > to preserve a compatible interface. It causes having many
> > platform-specific DT bindings of the same generic properties.
> > 
> > Instead of currently implemented approach we suggest to be more
> > restrictive and yet preserve some level of flexibility in the DW PCIe
> > DT-bindings description. The device tree DT-schema is split up into
> > three parts: a common YAML-schema applicable for both DWC Root Port and
> > End-point controller configs, DWC PCIe Root Port-specific YAML-schema
> > and DWC PCIe End-point-specific YAML-schema, where
> > 1) pci/snps,dw-pcie-common.yaml - the common DT-schema describes the most
> > generic constraints of the "reg", "interrupts", "clocks", "resets" and
> > "phys" properties together with a set of common for both device types
> > PCIe/AXI bus properties like a maximum number of lanes or a maximum link
> > speed, number of inbound and outbound iATU windows. In addition to that a
> > set of schema definitions declared under the "definitions" property with
> > "reg", "interrupt", "clock" and "reset" names common for DWC PCIe Root
> > Port and End-point devices. They can be used by the successive DT-schemas
> > in case they are supposed to be compatible with the generic DWC PCIe
> > controller DT-bindings.
> > 2) pci/snps,dw-pcie.yaml, pci/snps,dw-pcie-ep.yaml - generic DW PCIe Root
> > Port and End-point DT-bindings which aside with the device-specific
> > properties set also contain more restrictive constraints. All new DW PCIe
> > platforms are supposed to be compatible with one of these bindings by
> > using "allOf: " property and additionally defining their own constraints
> > to close up the DT-bindings set.
> > 
> > So to speak in case if a DW PCIe-based device for some reason has too many
> > specific properties or it's bindings have already been defined in a
> > non-generic way, it's DT-schema is supposed to include 1) YAML-file and
> > provide its own constraints. Otherwise the ready-to-use bindings from 2)
> > should be utilized. There are only two DT-schemas compatible with 2) at the
> > moment are samsung,axynos-pcie.yaml and intel-gw-pcie.yaml. The
> > rest of the DW PCIe-related DT-schemas are supposed to use more generic
> > DW PCIe DT-bindings - pci/snps,dw-pcie-common.yaml.
> > 
> > Note the provided here generic properties and their possible values are
> > derived from the DW PCIe RC/EP hardware manuals and from the interface
> > implemented in the driver. The DT-bindings schemas are created to be as
> > full as possible with detailed properties/names description for the
> > easier interface comprehension and new platforms bindings development.
> > 
> > Also note since there are no generic DT-nodes can be found in the kernel
> > dts-es which would have a pure "snps,dw-pcie" compatible string, these
> > DT-bindings have been created to not be selected by default for
> > evaluation. They are supposed to be used by the new vendor-specific
> > DT-schemas to at least stop adding new bindings for the same set of DWC
> > PCIe signals or properties.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > ---
> > 
> > If the '$defs' property is used instead of the 'definitions' one, the next
> > error will be spotted:
> > 
> > >  DTC     Documentation/devicetree/bindings/pci/snps,dw-pcie.example.dt.yaml
> > >  CHECK   Documentation/devicetree/bindings/pci/snps,dw-pcie.example.dt.yaml
> > > /.../snps,dw-pcie.example.dt.yaml: pcie@1f052000: reg-names:0: 'oneOf' conditional failed, one must be fixed:
> > >        'dbi' is not of type 'array'
> > >        From schema: /.../snps,dw-pcie.yaml
> > > /.../snps,dw-pcie.example.dt.yaml: pcie@1f052000: reg-names:1: 'oneOf' conditional failed, one must be fixed:
> > >         'config' is not of type 'array'
> > >         From schema: /.../snps,dw-pcie.yaml
> > > /.../snps,dw-pcie.example.dt.yaml: pcie@1f052000: interrupt-names:0: 'anyOf' conditional failed, one must be fixed:
> > >         /.../snps,dw-pcie.example.dt.yaml: pcie@1f052000: interrupt-names:0: 'oneOf' conditional failed, one must be fixed:
> > >                 'msi' is not of type 'array'
> > >         /../snps,dw-pcie.example.dt.yaml: pcie@1f052000: interrupt-names:0: 'oneOf' conditional failed, one must be fixed:
> > >                 'msi' is not of type 'array'
> > >         From schema: /.../snps,dw-pcie.yaml
> > 
> > The problem is caused by the 'def fixup_sub_schema()' method defined in
> > the dtschema parser. AFAIU It skips the sub-'$defs'-schemas evaluation thus
> > not having them fixed up properly. The next patch solves the denoted
> 

> It's actually the opposite. The schemas in $defs are fixed-up, but you 
> don't want them to be because your definition entries apply to a single 
> entry where-as what the fix-ups do is make everything an array. It's 
> debatable whether $defs should be fixed up or not, but I'm pretty sure 
> we already have cases depending on the fixups.
> 
> > problem:
> > --- a/lib.py     2022-04-23 19:51:38.829759258 +0300
> > +++ b/lib.py     2022-04-23 20:17:16.218137170 +0300
> > @@ -470,7 +470,7 @@
> >              for subschema in v:
> >                  fixup_sub_schema(subschema, True)
> > 
> > -        if k not in ['dependentRequired', 'dependentSchemas', 'dependencies', 'properties', 'patternProperties', '$defs']:
> > +        if k not in ['dependentRequired', 'dependentSchemas', 'dependencies', 'properties', 'patternProperties']:
> 
> This list is all the json-schema keywords which have a dictionary of DT 
> properties under them. So $def belongs in the list. 'definitions' is not 
> there as it is used in limited places and those that exist don't need 
> fixups. Within the kernel tree, only $defs should be used and only if 
> really necessary. Primarily, I think it is cases where it saves writing 
> the same property schema more than once.

Got it. Thanks for clarification.

> 
> 

> This patch is doing multiple things and should be split up some if 
> possible. 

Ok, but we need to settle the suggested modification design before
the next patchset re-submission. The splitting up is a lot of additional work,
while I have already spent so much time on this and on the rest of the
DW PCIe-related patches.

> No doubt splitting the common schema from the 'generic' device 
> schema is needed. We've done that in other cases. 

> 
> The main part I have issue with is how reg-names and interrupt-names are 
> handled. For reg-names currently, if a platform wants to add another 
> name, they have to modify the commmon binding. That's not great, but we 
> don't want that to happen and don't want to make that easier to do.

When it comes to the generic and vendor-specific IP-core device
implementations the approach suggested by me is more flexible, but yet
restrictive enough to cover your case. It is suitable for all already
defined vendor-specific DT bindings, the generic DW PCIe-compatible
bindings, new DT bindings yet to come. Here are the cases it can be
used for:

1) DW PCIe platforms fully compatible with the generic IP-core
schema, but with already defined vendor-specific resource
names. Solution:
$ref: /schemas/pci/snps,dw-pcie-common.yaml#
and define all the vendor-specific resource names. If the
reg/interrupt/clock/reset-names are partly compatible with the
generic schema, then they can be directly referenced from the
"definitions" property in the "oneOf" composition.

2) DW PCIe platforms partly compatible with the generic IP-core
DT schema. Solution:
$ref: /schemas/pci/snps,dw-pcie-common.yaml#
and define all the vendor-specific resource names. Refer to the
generic "reg/interrupt/clock/reset"-names in the particular names
arrays aside with the platform-specific resource names (this especially
concerns the reg-names property, which aside with the standard CSRs
may have additional non-standard CSRs defined).

3) DW PCIe platforms fully compatible with the generic IP-core
DT schema. Solution:
$ref: /schemas/pci/snps,dw-pcie.yaml#
or
$ref: /schemas/pci/snps,dw-pcie-ep.yaml#
If there are resource names which are defined in the generic schema
and are applicable to the platform-specific device (like additional
required names) they can be added to the vendor DT-schema otherwise
the generic schema reference is enough (see the baikal,bt1-pcie.yaml
DT-schema being added in the framework of this patchset).

New DW PCIe platforms are encouraged to use schema 3). In case if
there are resources not specific to the generic DW PCIe IP-core,
they need to have the schema 2) utilized. Since we can't change the
already defined bindings, we have no choice but to use schema 1) for
them. Thus the suggested here schemas can be used to create more
descriptive and restrictive enough DT-schemas hierarchy covering all
the currently defined bindings and new ones.

In anyway regarding what you said in the message above adding new
generic names is possible only in case if they are applicable to the
generic DW PCIe IP-core (almost all of them I've already listed in the
snps,dw-pcie-common.yaml#definitions property) otherwise such names
need to be defined in the platform-specific schema.

Traditional generic schema design when all the generic properties are
directly listed in the generic DT-schema in the corresponding property
is too restrictive to implement the cases 1) and 2). Thus such generic
schema needs to be extended with the non-generic names (or not to have
the names listed in the generic schema at all). It implicitly
makes all the platforms permitting such resources, which isn't true.

> Part 
> of the reason we have so many already is because no one stopped all the 
> random variations of the same names.

Sadly, but you're right. In the framework of this patchset I suggest a
method to organize all old and new DT bindings (for DW PCIe devices)
in a way so them all be using the generic DW PCIe DT-schema at the
most applicable part. 

> The biggest validation gap I see is 
> we don't enforce the order and this patch does nothing to address that.

I happen to have the opposite opinion regarding the order of the
names listed in the *-names properties. Why do you need them to be
defined in a strict order? reg and reg-names are tightly coupled.
The DW PCIe driver refer to the CSRs and IRQs by their names. So the
order is irrelevant in this case. If the order is required, then it
can be constrained in the platform-specific schema.

-Sergey

> 
> Rob