| Message ID | 20220610085706.15741-8-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: dwc: Add generic resources and Baikal-T1 support | expand |
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@baikalelectronics.ru> > > --- > > Changelog v3: > - This is a new patch unpinned from the next one: > https://lore.kernel.org/linux-pci/20220503214638.1895-2-Sergey.Semin@baikalelectronics.ru/ > 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. > + 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. Rob
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@baikalelectronics.ru> > > > > --- > > > > Changelog v3: > > - This is a new patch unpinned from the next one: > > https://lore.kernel.org/linux-pci/20220503214638.1895-2-Sergey.Semin@baikalelectronics.ru/ > > 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@baikalelectronics.ru/ 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
Rob, Could you please get your attention back to this this thread? A possible solution have been suggested below. Let's finally settle things down, so we could move on with the series further. -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@baikalelectronics.ru> > > > > > > --- > > > > > > Changelog v3: > > > - This is a new patch unpinned from the next one: > > > https://lore.kernel.org/linux-pci/20220503214638.1895-2-Sergey.Semin@baikalelectronics.ru/ > > > 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@baikalelectronics.ru/ > > 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
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@baikalelectronics.ru> > > > > > > --- > > > > > > Changelog v3: > > > - This is a new patch unpinned from the next one: > > > https://lore.kernel.org/linux-pci/20220503214638.1895-2-Sergey.Semin@baikalelectronics.ru/ > > > 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@baikalelectronics.ru/ > > 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
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: + 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 + ... diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml index 9411366d6ca7..5f12a6ac08d8 100644 --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml @@ -53,6 +53,20 @@ properties: items: enum: [dbi, dbi2, config, atu, addr_space, link, atu_dma, appl] + interrupts: + description: + There is no mandatory IRQ signals for the normal controller functioning, + but in addition to the native set the platforms may have a link- or + PM-related IRQs specified. + minItems: 1 + maxItems: 20 + + interrupt-names: + minItems: 1 + maxItems: 20 + items: + $ref: /schemas/pci/snps,dw-pcie-common.yaml#/definitions/interrupt-names + max-functions: maximum: 32 @@ -72,6 +86,9 @@ examples: <0xd0000000 0x2000000>; /* Configuration space */ reg-names = "dbi", "dbi2", "addr_space"; + interrupts = <23>, <24>; + interrupt-names = "dma0", "dma1"; + phys = <&pcie_phy0>, <&pcie_phy1>, <&pcie_phy2>, <&pcie_phy3>; phy-names = "pcie0", "pcie1", "pcie2", "pcie3"; diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml index 8b2e3210e3e2..e0020b72288a 100644 --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml @@ -54,9 +54,23 @@ properties: enum: [ dbi, dbi2, config, atu, app, elbi, mgmt, ctrl, parf, cfg, link, ulreg, smu, mpu, apb, phy ] - interrupts: true - - interrupt-names: true + interrupts: + description: + At least MSI interrupt signal is supposed to be specified for + the DWC PCIe host controller. + minItems: 1 + maxItems: 26 + + interrupt-names: + minItems: 1 + maxItems: 26 + items: + anyOf: + - $ref: /schemas/pci/snps,dw-pcie-common.yaml#/definitions/interrupt-names + - $ref: '#/definitions/interrupt-names' + allOf: + - contains: + const: msi clocks: true @@ -67,6 +81,48 @@ required: - reg - reg-names +definitions: + interrupt-names: + description: + DWC PCIe Root Port/Complex specific IRQ signal names. + oneOf: + - description: + DSP AXI MSI Interrupt detected. It gets de-asserted when there is + no more MSI interrupt pending. The interrupt is relevant to the + iMSI-RX - Integrated MSI Receiver (AXI bridge). + const: msi + - description: + Legacy A/B/C/D interrupt signal. Basically it's triggered by + receiving a Assert_INT{A,B,C,D}/Desassert_INT{A,B,C,D} message + from the downstream device. + pattern: "^int(a|b|c|d)$" + - description: + Error condition detected and a bit is set in the Root Error Status + register of the AER capability. It's asserted when the RC + internally generated an error or an error message is received by + the RC. + const: aer + - description: + PME message is received by the port. That means having the PME + status bit set in the Root Status register (the event is + supposed to be unmasked in the Root Control register). + const: pme + - description: + Hot-plug event is detected. That is a bit has been set in the + Slot Status register and the corresponding event is enabled in + the Slot Control register. + const: hp + - description: + Link Autonomous Bandwidth Status flag has been set in the Link + Status register (the event is supposed to be unmasked in the + Link Control register). + const: bw_au + - description: + Bandwidth Management Status flag has been set in the Link + Status register (the event is supposed to be unmasked in the + Link Control register). + const: bw_mg + examples: - | pcie@dfc00000 { @@ -82,6 +138,7 @@ examples: bus-range = <0x0 0xff>; interrupts = <25>, <24>; + interrupt-names = "msi", "hp"; #interrupt-cells = <1>; reset-gpios = <&port0 0 1>;
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@baikalelectronics.ru> --- Changelog v3: - This is a new patch unpinned from the next one: https://lore.kernel.org/linux-pci/20220503214638.1895-2-Sergey.Semin@baikalelectronics.ru/ 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(-)