On Wed, Jun 15, 2022 at 09:32:01AM -0600, Rob Herring wrote: > On Fri, Jun 10, 2022 at 11:56:55AM +0300, Serge Semin wrote: > > Currently the 'interrupts' and 'interrupt-names' are defined being too > > generic to really describe any actual IRQ interface. Moreover the DW PCIe > > End-point devices are left with no IRQ signals. All of that can be fixed > > by adding the IRQ-related properties to the common DW PCIe DT-schema and > > defining a common and device-specific set of the IRQ names in accordance > > with the hardware reference manual. Seeing there are common and dedicated > > IRQ signals for DW PCIe Root Port and End-point controllers we suggest to > > split the IRQ names up into two sets: common definitions available in the > > snps,dw-pcie-common.yaml schema and Root Port specific names defined in > > the snps,dw-pcie.yaml schema. The former one will be applied to both DW > > PCIe RP and EP controllers, while the later one - for the RP only. > > > > Signed-off-by: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx> > > > > --- > > > > Changelog v3: > > - This is a new patch unpinned from the next one: > > https://lore.kernel.org/linux-pci/20220503214638.1895-2-Sergey.Semin@xxxxxxxxxxxxxxxxxxxx/ > > by the Rob' request. (@Rob) > > --- > > .../bindings/pci/snps,dw-pcie-common.yaml | 51 +++++++++++++++ > > .../bindings/pci/snps,dw-pcie-ep.yaml | 17 +++++ > > .../devicetree/bindings/pci/snps,dw-pcie.yaml | 63 ++++++++++++++++++- > > 3 files changed, 128 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml > > index b2fbe886981b..0a524e916a9f 100644 > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml > > @@ -17,6 +17,25 @@ description: > > select: false > > > > properties: > > + interrupts: > > + description: > > + There are two main sub-blocks which are normally capable of > > + generating interrupts. It's System Information Interface and MSI > > + interface. While the former one has some common for the Host and > > + Endpoint controllers IRQ-signals, the later interface is obviously > > + Root Complex specific since it's responsible for the incoming MSI > > + messages signalling. The System Information IRQ signals are mainly > > + responsible for reporting the generic PCIe hierarchy and Root > > + Complex events like VPD IO request, general AER, PME, Hot-plug, link > > + bandwidth change, link equalization request, INTx asserted/deasserted > > + Message detection, embedded DMA Tx/Rx/Error. > > + minItems: 1 > > + maxItems: 26 > > + > > + interrupt-names: > > + minItems: 1 > > + maxItems: 26 > > + > > phys: > > description: > > There can be up to the number of possible lanes PHYs specified. > > @@ -91,4 +110,36 @@ properties: > > > > additionalProperties: true > > > > +definitions: > > $defs: > > But I suppose this is the applying fixups or not issue. That's certainly > not behavior we should rely on. If we need a way to specify applying > fixups or not, we should do that. But really I'd prefer not to need > that. $defs doesn't work in this case. Please see the patchlog to the v2 of this patch: https://lore.kernel.org/linux-pci/20220503214638.1895-2-Sergey.Semin@xxxxxxxxxxxxxxxxxxxx/ Anyway see my next comment. Let's settle the next issue first, then get back to the implementation details. > > > + interrupt-names: > > + description: > > + IRQ signal names common for the DWC PCIe Root Port and Endpoint > > + controllers. > > + oneOf: > > + - description: > > + Controller request to read or write virtual product data > > + from/to the VPD capability registers. > > + const: vpd > > + - description: > > + Link Equalization Request flag is set in the Link Status 2 > > + register (applicable if the corresponding IRQ is enabled in > > + the Link Control 3 register). > > + const: l_eq > > + - description: > > + Indicates that the eDMA Tx/Rx transfer is complete or that an > > + error has occurred on the corresponding channel. eDMA can have > > + eight Tx (Write) and Rx (Read) eDMA channels thus supporting up > > + to 16 IRQ signals all together. Write eDMA channels shall go > > + first in the ordered row as per default edma_int[*] bus setup. > > + pattern: '^dma([0-9]|1[0-5])?$' > > + - description: > > + PCIe protocol correctable error or a Data Path protection > > + correctable error is detected by the automotive/safety > > + feature. > > + const: sft_ce > > + - description: > > + Indicates that the internal safety mechanism detected and > > + uncorrectable error. > > + const: sft_ue > > I still don't really like this pattern. My first read of it makes me > think only 1 interrupt is supported, and I have to go look that this is > referenced from 'items'. > > Could we do a lot more with json-schema like you have? Yes, but the > schemas are optimized for simplicity and a relatively fixed pattern of > what's allowed as json-schema is new to most folks. It's also easy to > create things that simply don't work (silently). Just reviewing this > series is hard. > > This series is trying to do lots of things. Refactoring, adding > constraints, and adding a new binding. I would split it up if you want > to make progress. This series has been refactored three times already! First I created it as the legacy bindings conversion to the yaml schema. I missed just a few weeks, but someone has already submitted the converted bindings. So I had to rebase my work on top of the already performed conversion. After that you asked me to split it up into the series of patches. Now you want the patchset to be refactored again and to be split up again. Each such action takes a lot of my time which I've already spent too much on this update taking into account the time spent on looking for a way to implement the extendable array property pattern. And there is no guaranty you won't refuse the suggested update should I re-submit the separate patchset. So please don't ask me to split it up again especially seeing there are only eleven DT-related patches here. I just can't afford it, but am still very much eager to get the work merged in in a suitable for you and me form. Let's finally settle the main issue here so I could re-submit the series what you'd be ok with. On each iteration you said you didn't like the pattern I've used here. It looks like this: 1) The most common schema: pci/snps,dw-pcie-common.yaml: > definitions: > interrupt-names: > oneOf: > - const: i1 > - const: i2 2) Generic Dw PCIe Root Port schema: pci/snps,dw-pcie.yaml: > properties: > interrupt-names: > items: > anyOf: > - $ref: /schemas/pci/snps,dw-pcie-common.yaml#/definitions/interrupt-names > - $ref: '#/definitions/interrupt-names' > definitions: > interrupt-names: > oneOf: > - const: i3 > - const: i4 3) Generic Dw PCIe Endpoint schema: pci/snps,dw-pcie-ep.yaml: > properties: > interrupt-names: > items: > anyOf: > - $ref: /schemas/pci/snps,dw-pcie-common.yaml#/definitions/interrupt-names > - $ref: '#/definitions/interrupt-names' > definitions: > interrupt-names: > oneOf: > - const: i5 > - const: i6 I am not that much happy with it either, but first I didn't find any alternative, and second by using it I've solved several complex problems persistent in the currently implemented DW PCIe bindings: 1) Drop the duplicated properties defined in the Root Port and Endpoint schemas and create a common DT bindings for both of these devices seeing in accordance with the ref. manual they are very much alike. 2) Create the generic DW PCIe Root Port and Endpoint DT-schemas with more restrictive constraints so to stop the new drivers from creating their own regs/clocks/resets/interrupts bindings implementation. 3) Fix the already defined DW PCIe vendor-specific DT-bindings to use either 1) or 2) schema depending on what is applicable for them. So to speak I was willing to bring some order to the already implemented DT-schemas and to make sure the new bindings wouldn't define the new names to the already known resources. As a result the next schemas hierarchy has been provided: 1. Common DW PCIe schema snps,dw-pcie-common.yaml | +-----------------------+----------------------+ | | | v v V 2.DW PCIe Root Port 3. DW PCIe Endpoint 4. DW PCIe Vendor-spec snps,dw-pcie.yaml snps,dw-pcie-ep.yaml | | | | v v V baikal,bt1-pcie.yaml hisilicon,kirin-pcie.yaml intel-gw-pcie.yaml sifive,fu740-pcie.yaml toshiba,visconti-pcie.yaml socionext,uniphier-pcie-ep.yaml fsl,imx6q-pcie.yaml As you can see the suggested in this patchset approach is very flexible and permits using the common DW PCIe schema in the particular device bindings while still have the vendor-specific constraints defined in the particular schemas. So the new devices drivers are supposed to use the schemas (2) and (3), while the already added drivers can following the path (4), apply the schema (1), but still use the names "definitions" added to (1), (2) and (3). You keep saying that what I've done here is misleading since what was created under the "definitions" property is perceived as the "only 1 interrupt/clock/reg/reset is supported, and you have to go look that this is referenced from 'items'". If so then what alternative to this solution can you suggest? Do you know a schema pattern which would be more suitable? If there is none, then what? Do you suggest to drop trying to solve the problems I've listed above? Please answer to these questions (or go on on this comment for a possible but IMO less suitable alternative solution). Anyway in my opinion the currently implemented approach of the names array properties: > reg-names: > items: > enum: [dbi, dbi2, config, atu, addr_space, link, atu_dma, appl] isn't much more descriptive, since it doesn't provide much info regarding the resources but just lists all the common and vendor-specific names to the same resources. As IMO a much less suitable, but "definitions"-less alternative to my approach we can use the next pattern: 1) The most common schema: pci/snps,dw-pcie-common.yaml: > properties: > interrupt-names: > anyOf: > - const: i1 > - const: i2 > - true 2) Generic Dw PCIe Root Port schema: pci/snps,dw-pcie.yaml: > allOf: > - $ref: /schemas/pci/snps,dw-pcie-common.yaml# > > properties: > interrupt-names: > items: > anyOf: > - const: i3 > - const: i4 > - true 3) etc It will give us a more generic and less restrictive bindings. Thus due to using the "true" schema in there we won't be able to automatically deny the new resource names adding. But it won't have any "definitions" or "$defs" utilized as you seem do not like. -Sergey > > Rob
