Re: [PATCH v3 07/17] dt-bindings: PCI: dwc: Add interrupts/interrupt-names common properties

[Rob Herring]

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.

> +  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