| Message ID | 1721067215-5832-7-git-send-email-quic_mrana@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Manivannan Sadhasivam |
| Headers | show |
| Series | Add power domain and MSI functionality with PCIe host generic ECAM driver | expand |
On Mon, 15 Jul 2024 11:13:34 -0700, Mayank Rana wrote: > To support MSI functionality using Synopsys DesignWare PCIe controller > based MSI controller with ECAM driver, add "snps,dw-pcie-ecam-msi > compatible binding which uses provided SPIs to support MSI functionality. > > Signed-off-by: Mayank Rana <quic_mrana@quicinc.com> > --- > .../devicetree/bindings/pci/host-generic-pci.yaml | 57 ++++++++++++++++++++++ > 1 file changed, 57 insertions(+) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: ./Documentation/devicetree/bindings/pci/host-generic-pci.yaml:137:9: [warning] wrong indentation: expected 6 but found 8 (indentation) dtschema/dtc warnings/errors: doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1721067215-5832-7-git-send-email-quic_mrana@quicinc.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 15/07/2024 20:13, Mayank Rana wrote: > To support MSI functionality using Synopsys DesignWare PCIe controller > based MSI controller with ECAM driver, add "snps,dw-pcie-ecam-msi > compatible binding which uses provided SPIs to support MSI functionality. To support MSI, you add MSI support... That's a tautology. Describe hardware instead. > > Signed-off-by: Mayank Rana <quic_mrana@quicinc.com> > --- > .../devicetree/bindings/pci/host-generic-pci.yaml | 57 ++++++++++++++++++++++ > 1 file changed, 57 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml > index 9c714fa..9e860d5 100644 > --- a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml > +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml > @@ -81,6 +81,12 @@ properties: > - marvell,armada8k-pcie-ecam > - socionext,synquacer-pcie-ecam > - const: snps,dw-pcie-ecam > + - description: | > + Firmware is configuring Synopsys DesignWare PCIe controller in RC mode with > + ECAM compatible fashion. To use MSI controller of Synopsys DesignWare PCIe > + controller for MSI functionality, this compatible is used. > + items: > + - const: snps,dw-pcie-ecam-msi MSI is already present in the binding, isn't it? Anyway, aren't you forgetting specific compatible? Please open your internal (quite comprehensive) guideline on bindings and DTS. > - description: > CAM or ECAM compliant PCI host controllers without any quirks > enum: > @@ -116,6 +122,20 @@ properties: > A phandle to the node that controls power or/and system resource or interface to firmware > to enable ECAM compliant PCIe root complex. > > + interrupts: > + description: > + DWC PCIe Root Port/Complex specific MSI interrupt/IRQs. > + minItems: 1 > + maxItems: 8 > + > + interrupt-names: > + description: > + MSI interrupt names > + minItems: 1 > + maxItems: 8 > + items: > + pattern: '^msi[0-9]+$' Why the same devices have variable numbers? > + > required: > - compatible > - reg > @@ -146,11 +166,22 @@ allOf: > reg: > maxItems: 1 Best regards, Krzysztof
Hi Krzysztof On 7/16/2024 12:28 AM, Krzysztof Kozlowski wrote: > On 15/07/2024 20:13, Mayank Rana wrote: >> To support MSI functionality using Synopsys DesignWare PCIe controller >> based MSI controller with ECAM driver, add "snps,dw-pcie-ecam-msi >> compatible binding which uses provided SPIs to support MSI functionality. > > To support MSI, you add MSI support... That's a tautology. Describe > hardware instead. Ok. let me repharse it to provide more useful information. >> >> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com> >> --- >> .../devicetree/bindings/pci/host-generic-pci.yaml | 57 ++++++++++++++++++++++ >> 1 file changed, 57 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >> index 9c714fa..9e860d5 100644 >> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >> @@ -81,6 +81,12 @@ properties: >> - marvell,armada8k-pcie-ecam >> - socionext,synquacer-pcie-ecam >> - const: snps,dw-pcie-ecam >> + - description: | >> + Firmware is configuring Synopsys DesignWare PCIe controller in RC mode with >> + ECAM compatible fashion. To use MSI controller of Synopsys DesignWare PCIe >> + controller for MSI functionality, this compatible is used. >> + items: >> + - const: snps,dw-pcie-ecam-msi > > MSI is already present in the binding, isn't it? It is mentioning as msi-parent usage which could be different MSI controller (GIC or vendor specific one) not related to designware PCIe controller based MSI controller. >Anyway, aren't you > forgetting specific compatible? Please open your internal (quite > comprehensive) guideline on bindings and DTS. Here I am trying to define Designware based PCIe ECAM controller supporting MSIcontroller based device. Hence I am not mentioning vendor specific compatible usage and keeping generic compatible binding for such device. > >> - description: >> CAM or ECAM compliant PCI host controllers without any quirks >> enum: >> @@ -116,6 +122,20 @@ properties: >> A phandle to the node that controls power or/and system resource or interface to firmware >> to enable ECAM compliant PCIe root complex. >> >> + interrupts: >> + description: >> + DWC PCIe Root Port/Complex specific MSI interrupt/IRQs. >> + minItems: 1 >> + maxItems: 8 >> + >> + interrupt-names: >> + description: >> + MSI interrupt names >> + minItems: 1 >> + maxItems: 8 >> + items: >> + pattern: '^msi[0-9]+$' > > Why the same devices have variable numbers? Max supported MSI with designware PCIe controller is 8 Only, and it depends if those all are used or some of used on specific vendor based device. Hence I have kept it here variable names. Although here it should be [0 - 7] instead of [0 - 9]. >> + >> required: >> - compatible >> - reg >> @@ -146,11 +166,22 @@ allOf: >> reg: >> maxItems: 1 > > > Best regards, > Krzysztof >
On 17/07/2024 00:09, Mayank Rana wrote: > Hi Krzysztof > > On 7/16/2024 12:28 AM, Krzysztof Kozlowski wrote: >> On 15/07/2024 20:13, Mayank Rana wrote: >>> To support MSI functionality using Synopsys DesignWare PCIe controller >>> based MSI controller with ECAM driver, add "snps,dw-pcie-ecam-msi >>> compatible binding which uses provided SPIs to support MSI functionality. >> >> To support MSI, you add MSI support... That's a tautology. Describe >> hardware instead. > Ok. let me repharse it to provide more useful information. >>> >>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com> >>> --- >>> .../devicetree/bindings/pci/host-generic-pci.yaml | 57 ++++++++++++++++++++++ >>> 1 file changed, 57 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >>> index 9c714fa..9e860d5 100644 >>> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >>> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >>> @@ -81,6 +81,12 @@ properties: >>> - marvell,armada8k-pcie-ecam >>> - socionext,synquacer-pcie-ecam >>> - const: snps,dw-pcie-ecam >>> + - description: | >>> + Firmware is configuring Synopsys DesignWare PCIe controller in RC mode with >>> + ECAM compatible fashion. To use MSI controller of Synopsys DesignWare PCIe >>> + controller for MSI functionality, this compatible is used. >>> + items: >>> + - const: snps,dw-pcie-ecam-msi >> >> MSI is already present in the binding, isn't it? > It is mentioning as msi-parent usage which could be different MSI > controller (GIC or vendor specific one) not related to designware PCIe > controller based MSI controller. > >> Anyway, aren't you >> forgetting specific compatible? Please open your internal (quite >> comprehensive) guideline on bindings and DTS. > Here I am trying to define Designware based PCIe ECAM controller > supporting MSIcontroller based device. Hence I am not mentioning vendor > specific compatible usage > and keeping generic compatible binding for such device. I know what you try, yet it feels simply wrong. Read your guideline. Are you sure you work on Designware core itself, not on one used in Qualcomm? I would expect people from Designware to design Designware cores and people from Qualcomm only to design licensed cores. >> >>> - description: >>> CAM or ECAM compliant PCI host controllers without any quirks >>> enum: >>> @@ -116,6 +122,20 @@ properties: >>> A phandle to the node that controls power or/and system resource or interface to firmware >>> to enable ECAM compliant PCIe root complex. >>> >>> + interrupts: >>> + description: >>> + DWC PCIe Root Port/Complex specific MSI interrupt/IRQs. >>> + minItems: 1 >>> + maxItems: 8 >>> + >>> + interrupt-names: >>> + description: >>> + MSI interrupt names >>> + minItems: 1 >>> + maxItems: 8 >>> + items: >>> + pattern: '^msi[0-9]+$' >> >> Why the same devices have variable numbers? > Max supported MSI with designware PCIe controller is 8 Only, and it > depends if those all are > used or some of used on specific vendor based device. Hence I have kept > it here variable names. Although here it should be [0 - 7] instead of > [0 - 9]. Wait, you just said there is no specific vendor device. Sorry, bring some sanity to this. Best regards, Krzysztof
Hi Krzysztof On 7/16/2024 11:47 PM, Krzysztof Kozlowski wrote: > On 17/07/2024 00:09, Mayank Rana wrote: >> Hi Krzysztof >> >> On 7/16/2024 12:28 AM, Krzysztof Kozlowski wrote: >>> On 15/07/2024 20:13, Mayank Rana wrote: >>>> To support MSI functionality using Synopsys DesignWare PCIe controller >>>> based MSI controller with ECAM driver, add "snps,dw-pcie-ecam-msi >>>> compatible binding which uses provided SPIs to support MSI functionality. >>> >>> To support MSI, you add MSI support... That's a tautology. Describe >>> hardware instead. >> Ok. let me repharse it to provide more useful information. >>>> >>>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com> >>>> --- >>>> .../devicetree/bindings/pci/host-generic-pci.yaml | 57 ++++++++++++++++++++++ >>>> 1 file changed, 57 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >>>> index 9c714fa..9e860d5 100644 >>>> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >>>> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >>>> @@ -81,6 +81,12 @@ properties: >>>> - marvell,armada8k-pcie-ecam >>>> - socionext,synquacer-pcie-ecam >>>> - const: snps,dw-pcie-ecam >>>> + - description: | >>>> + Firmware is configuring Synopsys DesignWare PCIe controller in RC mode with >>>> + ECAM compatible fashion. To use MSI controller of Synopsys DesignWare PCIe >>>> + controller for MSI functionality, this compatible is used. >>>> + items: >>>> + - const: snps,dw-pcie-ecam-msi >>> >>> MSI is already present in the binding, isn't it? >> It is mentioning as msi-parent usage which could be different MSI >> controller (GIC or vendor specific one) not related to designware PCIe >> controller based MSI controller. >> >>> Anyway, aren't you >>> forgetting specific compatible? Please open your internal (quite >>> comprehensive) guideline on bindings and DTS. >> Here I am trying to define Designware based PCIe ECAM controller >> supporting MSIcontroller based device. Hence I am not mentioning vendor >> specific compatible usage >> and keeping generic compatible binding for such device. > > I know what you try, yet it feels simply wrong. Read your guideline. > Are you sure you work on Designware core itself, not on one used in > Qualcomm? I would expect people from Designware to design Designware > cores and people from Qualcomm only to design licensed cores. Ok. let me not make generic comment here. I refereed how it is done with other snps based IP usage for example USB, and would follow same. >>> >>>> - description: >>>> CAM or ECAM compliant PCI host controllers without any quirks >>>> enum: >>>> @@ -116,6 +122,20 @@ properties: >>>> A phandle to the node that controls power or/and system resource or interface to firmware >>>> to enable ECAM compliant PCIe root complex. >>>> >>>> + interrupts: >>>> + description: >>>> + DWC PCIe Root Port/Complex specific MSI interrupt/IRQs. >>>> + minItems: 1 >>>> + maxItems: 8 >>>> + >>>> + interrupt-names: >>>> + description: >>>> + MSI interrupt names >>>> + minItems: 1 >>>> + maxItems: 8 >>>> + items: >>>> + pattern: '^msi[0-9]+$' >>> >>> Why the same devices have variable numbers? >> Max supported MSI with designware PCIe controller is 8 Only, and it >> depends if those all are >> used or some of used on specific vendor based device. Hence I have kept >> it here variable names. Although here it should be [0 - 7] instead of >> [0 - 9]. > > Wait, you just said there is no specific vendor device. > > Sorry, bring some sanity to this I understood that you are having concern about generic vs platform specific usage here in binding document. I would try avoid mixing those, and will try to do just platform specific usage. Thanks for having patience with me, and helping me to here. Regards, Mayank
On 17/07/2024 19:20, Mayank Rana wrote: > Hi Krzysztof > > On 7/16/2024 11:47 PM, Krzysztof Kozlowski wrote: >> On 17/07/2024 00:09, Mayank Rana wrote: >>> Hi Krzysztof >>> >>> On 7/16/2024 12:28 AM, Krzysztof Kozlowski wrote: >>>> On 15/07/2024 20:13, Mayank Rana wrote: >>>>> To support MSI functionality using Synopsys DesignWare PCIe controller >>>>> based MSI controller with ECAM driver, add "snps,dw-pcie-ecam-msi >>>>> compatible binding which uses provided SPIs to support MSI functionality. >>>> >>>> To support MSI, you add MSI support... That's a tautology. Describe >>>> hardware instead. >>> Ok. let me repharse it to provide more useful information. >>>>> >>>>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com> >>>>> --- >>>>> .../devicetree/bindings/pci/host-generic-pci.yaml | 57 ++++++++++++++++++++++ >>>>> 1 file changed, 57 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >>>>> index 9c714fa..9e860d5 100644 >>>>> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >>>>> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >>>>> @@ -81,6 +81,12 @@ properties: >>>>> - marvell,armada8k-pcie-ecam >>>>> - socionext,synquacer-pcie-ecam >>>>> - const: snps,dw-pcie-ecam >>>>> + - description: | >>>>> + Firmware is configuring Synopsys DesignWare PCIe controller in RC mode with >>>>> + ECAM compatible fashion. To use MSI controller of Synopsys DesignWare PCIe >>>>> + controller for MSI functionality, this compatible is used. >>>>> + items: >>>>> + - const: snps,dw-pcie-ecam-msi >>>> >>>> MSI is already present in the binding, isn't it? >>> It is mentioning as msi-parent usage which could be different MSI >>> controller (GIC or vendor specific one) not related to designware PCIe >>> controller based MSI controller. >>> >>>> Anyway, aren't you >>>> forgetting specific compatible? Please open your internal (quite >>>> comprehensive) guideline on bindings and DTS. >>> Here I am trying to define Designware based PCIe ECAM controller >>> supporting MSIcontroller based device. Hence I am not mentioning vendor >>> specific compatible usage >>> and keeping generic compatible binding for such device. >> >> I know what you try, yet it feels simply wrong. Read your guideline. >> Are you sure you work on Designware core itself, not on one used in >> Qualcomm? I would expect people from Designware to design Designware >> cores and people from Qualcomm only to design licensed cores. > Ok. let me not make generic comment here. I refereed how it is done with > other > snps based IP usage for example USB, and would follow same. Well, it is not. If you read their bindings or any reviews related to such cores, you would see that single Designware compatible is always never appropriate. Such cores always have customization per user. You can also look at the binding you are changing. Do you see Designware alone? No. Best regards, Krzysztof
Hi Krzysztof On 7/17/2024 11:05 PM, Krzysztof Kozlowski wrote: > On 17/07/2024 19:20, Mayank Rana wrote: >> Hi Krzysztof >> >> On 7/16/2024 11:47 PM, Krzysztof Kozlowski wrote: >>> On 17/07/2024 00:09, Mayank Rana wrote: >>>> Hi Krzysztof >>>> >>>> On 7/16/2024 12:28 AM, Krzysztof Kozlowski wrote: >>>>> On 15/07/2024 20:13, Mayank Rana wrote: >>>>>> To support MSI functionality using Synopsys DesignWare PCIe controller >>>>>> based MSI controller with ECAM driver, add "snps,dw-pcie-ecam-msi >>>>>> compatible binding which uses provided SPIs to support MSI functionality. >>>>> >>>>> To support MSI, you add MSI support... That's a tautology. Describe >>>>> hardware instead. >>>> Ok. let me repharse it to provide more useful information. >>>>>> >>>>>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com> >>>>>> --- >>>>>> .../devicetree/bindings/pci/host-generic-pci.yaml | 57 ++++++++++++++++++++++ >>>>>> 1 file changed, 57 insertions(+) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >>>>>> index 9c714fa..9e860d5 100644 >>>>>> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >>>>>> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >>>>>> @@ -81,6 +81,12 @@ properties: >>>>>> - marvell,armada8k-pcie-ecam >>>>>> - socionext,synquacer-pcie-ecam >>>>>> - const: snps,dw-pcie-ecam >>>>>> + - description: | >>>>>> + Firmware is configuring Synopsys DesignWare PCIe controller in RC mode with >>>>>> + ECAM compatible fashion. To use MSI controller of Synopsys DesignWare PCIe >>>>>> + controller for MSI functionality, this compatible is used. >>>>>> + items: >>>>>> + - const: snps,dw-pcie-ecam-msi >>>>> >>>>> MSI is already present in the binding, isn't it? >>>> It is mentioning as msi-parent usage which could be different MSI >>>> controller (GIC or vendor specific one) not related to designware PCIe >>>> controller based MSI controller. >>>> >>>>> Anyway, aren't you >>>>> forgetting specific compatible? Please open your internal (quite >>>>> comprehensive) guideline on bindings and DTS. >>>> Here I am trying to define Designware based PCIe ECAM controller >>>> supporting MSIcontroller based device. Hence I am not mentioning vendor >>>> specific compatible usage >>>> and keeping generic compatible binding for such device. >>> >>> I know what you try, yet it feels simply wrong. Read your guideline. >>> Are you sure you work on Designware core itself, not on one used in >>> Qualcomm? I would expect people from Designware to design Designware >>> cores and people from Qualcomm only to design licensed cores. >> Ok. let me not make generic comment here. I refereed how it is done with >> other >> snps based IP usage for example USB, and would follow same. > > Well, it is not. If you read their bindings or any reviews related to > such cores, you would see that single Designware compatible is always > never appropriate. Such cores always have customization per user. > > You can also look at the binding you are changing. Do you see Designware > alone? No. I found reference in this binding as below: 79 items: 80 - enum: 81 - marvell,armada8k-pcie-ecam 82 - socionext,synquacer-pcie-ecam 83 - const: snps,dw-pcie-ecam And as you mentioned in previous emails about how to add such usage, I ACKed it but let me put reason why I tried to add differently to start with it. This specific driver under-discussion is really not vendor specific driver. It can work with any PCIe controller which is already configured in ECAM mode by firmware (i.e. PCIe controller from SNPS or any other vendor). There are few quirks only added to get specific vendor based SOC configuration for SNPS PCIe controller by different SOC vendors. 90 - description: 91 CAM or ECAM compliant PCI host controllers without any quirks 92 enum: 93 - pci-host-cam-generic 94 - pci-host-ecam-generic Above enum based usage works for SA8775P platform which is having SNPS PCIe controller which doesn't need any quirks, and firmware is able to configure PCIe controller into ECAM mode. Here I tried adding PCIe SNPS controller based MSI support as SA8775P doesn't support LPI/ITS for MSI. I need to differentiate it hence added generic enum as MSI controller is part of SNPS PCIe controller, and not separate MSI IP here. Although how many MSI can be supported it depends on how many SPIs are available/used with MSI controller depend on particular SOC. Hence put as snps,dw-pcie-ecam-msi as usage and variable number of MSI. hopefully I am able to convey why this driver binding modified differently. Regards, Mayank
On 19/07/2024 01:19, Mayank Rana wrote: > Hi Krzysztof > > On 7/17/2024 11:05 PM, Krzysztof Kozlowski wrote: >> On 17/07/2024 19:20, Mayank Rana wrote: >>> Hi Krzysztof >>> >>> On 7/16/2024 11:47 PM, Krzysztof Kozlowski wrote: >>>> On 17/07/2024 00:09, Mayank Rana wrote: >>>>> Hi Krzysztof >>>>> >>>>> On 7/16/2024 12:28 AM, Krzysztof Kozlowski wrote: >>>>>> On 15/07/2024 20:13, Mayank Rana wrote: >>>>>>> To support MSI functionality using Synopsys DesignWare PCIe controller >>>>>>> based MSI controller with ECAM driver, add "snps,dw-pcie-ecam-msi >>>>>>> compatible binding which uses provided SPIs to support MSI functionality. >>>>>> >>>>>> To support MSI, you add MSI support... That's a tautology. Describe >>>>>> hardware instead. >>>>> Ok. let me repharse it to provide more useful information. >>>>>>> >>>>>>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com> >>>>>>> --- >>>>>>> .../devicetree/bindings/pci/host-generic-pci.yaml | 57 ++++++++++++++++++++++ >>>>>>> 1 file changed, 57 insertions(+) >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >>>>>>> index 9c714fa..9e860d5 100644 >>>>>>> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >>>>>>> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml >>>>>>> @@ -81,6 +81,12 @@ properties: >>>>>>> - marvell,armada8k-pcie-ecam >>>>>>> - socionext,synquacer-pcie-ecam >>>>>>> - const: snps,dw-pcie-ecam >>>>>>> + - description: | >>>>>>> + Firmware is configuring Synopsys DesignWare PCIe controller in RC mode with >>>>>>> + ECAM compatible fashion. To use MSI controller of Synopsys DesignWare PCIe >>>>>>> + controller for MSI functionality, this compatible is used. >>>>>>> + items: >>>>>>> + - const: snps,dw-pcie-ecam-msi >>>>>> >>>>>> MSI is already present in the binding, isn't it? >>>>> It is mentioning as msi-parent usage which could be different MSI >>>>> controller (GIC or vendor specific one) not related to designware PCIe >>>>> controller based MSI controller. >>>>> >>>>>> Anyway, aren't you >>>>>> forgetting specific compatible? Please open your internal (quite >>>>>> comprehensive) guideline on bindings and DTS. >>>>> Here I am trying to define Designware based PCIe ECAM controller >>>>> supporting MSIcontroller based device. Hence I am not mentioning vendor >>>>> specific compatible usage >>>>> and keeping generic compatible binding for such device. >>>> >>>> I know what you try, yet it feels simply wrong. Read your guideline. >>>> Are you sure you work on Designware core itself, not on one used in >>>> Qualcomm? I would expect people from Designware to design Designware >>>> cores and people from Qualcomm only to design licensed cores. >>> Ok. let me not make generic comment here. I refereed how it is done with >>> other >>> snps based IP usage for example USB, and would follow same. >> >> Well, it is not. If you read their bindings or any reviews related to >> such cores, you would see that single Designware compatible is always >> never appropriate. Such cores always have customization per user. >> >> You can also look at the binding you are changing. Do you see Designware >> alone? No. > I found reference in this binding as below: > 79 items: > 80 - enum: > 81 - marvell,armada8k-pcie-ecam > 82 - socionext,synquacer-pcie-ecam > 83 - const: snps,dw-pcie-ecam > > And as you mentioned in previous emails about how to add such usage, I > ACKed it but let me put reason why I tried to add differently to start > with it. > > This specific driver under-discussion is really not vendor specific > driver. It can work with any PCIe controller which is already configured > in ECAM mode by firmware (i.e. PCIe controller from SNPS or any other > vendor). There are few quirks only added to get specific vendor based > SOC configuration for SNPS PCIe controller by different SOC vendors. > > 90 - description: > 91 CAM or ECAM compliant PCI host controllers without any quirks > 92 enum: > 93 - pci-host-cam-generic > 94 - pci-host-ecam-generic > > Above enum based usage works for SA8775P platform which is having SNPS > PCIe controller which doesn't need any quirks, and firmware is able to > configure PCIe controller into ECAM mode. Here I tried adding PCIe SNPS > controller based MSI support as SA8775P doesn't support LPI/ITS for MSI. > I need to differentiate it hence added generic enum as MSI controller is > part of SNPS PCIe controller, and not separate MSI IP here. Although how > many MSI can be supported it depends on how many SPIs are available/used > with MSI controller depend on particular SOC. Hence put as > snps,dw-pcie-ecam-msi as usage and variable number of MSI. hopefully I > am able to convey why this driver binding modified differently. Your binding already defines some specifics for specific device, but you still claim all of them are identical. Sorry, I am confused. Read carefully writing bindings, consult internal guideline (go/upstream - it is really detailed!) and then come with a solution. Best regards, Krzysztof
On Mon, Jul 15, 2024 at 11:13:34AM -0700, Mayank Rana wrote: > To support MSI functionality using Synopsys DesignWare PCIe controller > based MSI controller with ECAM driver, add "snps,dw-pcie-ecam-msi > compatible binding which uses provided SPIs to support MSI functionality. > > Signed-off-by: Mayank Rana <quic_mrana@quicinc.com> > --- > .../devicetree/bindings/pci/host-generic-pci.yaml | 57 ++++++++++++++++++++++ > 1 file changed, 57 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml > index 9c714fa..9e860d5 100644 > --- a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml > +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml > @@ -81,6 +81,12 @@ properties: > - marvell,armada8k-pcie-ecam > - socionext,synquacer-pcie-ecam > - const: snps,dw-pcie-ecam > + - description: | > + Firmware is configuring Synopsys DesignWare PCIe controller in RC mode with > + ECAM compatible fashion. To use MSI controller of Synopsys DesignWare PCIe > + controller for MSI functionality, this compatible is used. > + items: > + - const: snps,dw-pcie-ecam-msi There is no MSI ECAM. You can have Qcom specific ECAM implementation. Even generalising this as DWC ECAM is wrong, since it won't work on DWC based systems (especially with SCMI power domain). - Mani > - description: > CAM or ECAM compliant PCI host controllers without any quirks > enum: > @@ -116,6 +122,20 @@ properties: > A phandle to the node that controls power or/and system resource or interface to firmware > to enable ECAM compliant PCIe root complex. > > + interrupts: > + description: > + DWC PCIe Root Port/Complex specific MSI interrupt/IRQs. > + minItems: 1 > + maxItems: 8 > + > + interrupt-names: > + description: > + MSI interrupt names > + minItems: 1 > + maxItems: 8 > + items: > + pattern: '^msi[0-9]+$' > + > required: > - compatible > - reg > @@ -146,11 +166,22 @@ allOf: > reg: > maxItems: 1 > > + - if: > + properties: > + compatible: > + contains: > + const: snps,dw-pcie-ecam-msi > + then: > + required: > + - interrupts > + - interrupt-names > + > unevaluatedProperties: false > > examples: > - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > bus { > #address-cells = <2>; > #size-cells = <2>; > @@ -180,5 +211,31 @@ examples: > interrupt-map-mask = <0xf800 0x0 0x0 0x7>; > power-domains = <&scmi5_pd 0>; > }; > + > + pcie0: pci@1c00000 { > + compatible = "snps,dw-pcie-ecam-msi"; > + reg = <0x4 0x00000000 0 0x10000000>; > + device_type = "pci"; > + #address-cells = <3>; > + #size-cells = <2>; > + ranges = <0x02000000 0x0 0x40100000 0x0 0x40100000 0x0 0x1ff00000>, > + <0x43000000 0x4 0x10100000 0x4 0x10100000 0x0 0x40000000>; > + bus-range = <0x00 0xff>; > + dma-coherent; > + linux,pci-domain = <0>; > + power-domains = <&scmi5_pd 0>; > + iommu-map = <0x0 &pcie_smmu 0x0000 0x1>, > + <0x100 &pcie_smmu 0x0001 0x1>; > + > + interrupts = <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 308 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 313 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 314 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 374 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 375 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "msi0", "msi1", "msi2", "msi3", "msi4", "msi5", "msi6", "msi7"; > + }; > }; > ... > -- > 2.7.4 >
diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml index 9c714fa..9e860d5 100644 --- a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml @@ -81,6 +81,12 @@ properties: - marvell,armada8k-pcie-ecam - socionext,synquacer-pcie-ecam - const: snps,dw-pcie-ecam + - description: | + Firmware is configuring Synopsys DesignWare PCIe controller in RC mode with + ECAM compatible fashion. To use MSI controller of Synopsys DesignWare PCIe + controller for MSI functionality, this compatible is used. + items: + - const: snps,dw-pcie-ecam-msi - description: CAM or ECAM compliant PCI host controllers without any quirks enum: @@ -116,6 +122,20 @@ properties: A phandle to the node that controls power or/and system resource or interface to firmware to enable ECAM compliant PCIe root complex. + interrupts: + description: + DWC PCIe Root Port/Complex specific MSI interrupt/IRQs. + minItems: 1 + maxItems: 8 + + interrupt-names: + description: + MSI interrupt names + minItems: 1 + maxItems: 8 + items: + pattern: '^msi[0-9]+$' + required: - compatible - reg @@ -146,11 +166,22 @@ allOf: reg: maxItems: 1 + - if: + properties: + compatible: + contains: + const: snps,dw-pcie-ecam-msi + then: + required: + - interrupts + - interrupt-names + unevaluatedProperties: false examples: - | + #include <dt-bindings/interrupt-controller/arm-gic.h> bus { #address-cells = <2>; #size-cells = <2>; @@ -180,5 +211,31 @@ examples: interrupt-map-mask = <0xf800 0x0 0x0 0x7>; power-domains = <&scmi5_pd 0>; }; + + pcie0: pci@1c00000 { + compatible = "snps,dw-pcie-ecam-msi"; + reg = <0x4 0x00000000 0 0x10000000>; + device_type = "pci"; + #address-cells = <3>; + #size-cells = <2>; + ranges = <0x02000000 0x0 0x40100000 0x0 0x40100000 0x0 0x1ff00000>, + <0x43000000 0x4 0x10100000 0x4 0x10100000 0x0 0x40000000>; + bus-range = <0x00 0xff>; + dma-coherent; + linux,pci-domain = <0>; + power-domains = <&scmi5_pd 0>; + iommu-map = <0x0 &pcie_smmu 0x0000 0x1>, + <0x100 &pcie_smmu 0x0001 0x1>; + + interrupts = <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 308 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 313 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 314 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 374 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 375 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "msi0", "msi1", "msi2", "msi3", "msi4", "msi5", "msi6", "msi7"; + }; }; ...
To support MSI functionality using Synopsys DesignWare PCIe controller based MSI controller with ECAM driver, add "snps,dw-pcie-ecam-msi compatible binding which uses provided SPIs to support MSI functionality. Signed-off-by: Mayank Rana <quic_mrana@quicinc.com> --- .../devicetree/bindings/pci/host-generic-pci.yaml | 57 ++++++++++++++++++++++ 1 file changed, 57 insertions(+)