Date:   Thu, 7 Jul 2022 22:25:44 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Rob Herring <robh@...nel.org>
Cc:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Jingoo Han <jingoohan1@...il.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Frank Li <Frank.Li@....com>,
        Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 07/17] dt-bindings: PCI: dwc: Add
 interrupts/interrupt-names common properties

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@...kalelectronics.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

Powered by blists - more mailing lists