lkml.org 
[lkml]   [2022]   [Jun]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 07/17] dt-bindings: PCI: dwc: Add interrupts/interrupt-names common properties
    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

    \
     
     \ /
      Last update: 2022-06-28 14:19    [W:3.336 / U:0.316 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site