Rob, You must have missed this email. I desperately need to get your opinion on the possible solution suggested below. It's the main issue regarding this series, which blocks me from going further with the patchset re-spin. Let's finally settle things down. -Sergey On Sun, Jun 19, 2022 at 07:37:27PM +0300, Serge Semin wrote: > 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
