| Message ID | 20250225-qps615_v4_1-v4-9-e08633a7bdf8@oss.qualcomm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | PCI: Enable Power and configure the TC956x PCIe switch | expand |
On Tue, Feb 25, 2025 at 03:04:06PM +0530, Krishna Chaitanya Chundru wrote: > Qcom PCIe RC controllers are capable of generating 'global' SPI interrupt > to the host CPU. This interrupt can be used by the device driver to handle > PCIe link specific events such as Link up and Link down, which give the > driver a chance to start bus enumeration on its own when link is up and > initiate link training if link goes to a bad state. The PCIe driver can > still work without this interrupt but it will provide a nice user > experience when device gets plugged and removed. > > Hence, document it in the binding along with the existing MSI interrupts. > Global interrupt is parsed as optional in driver, so adding it in bindings > will not break the ABI. > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com> > --- > Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml > index 76cb9fbfd476..7ae09ba8da60 100644 > --- a/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml > @@ -54,7 +54,7 @@ properties: > > interrupts: > minItems: 8 > - maxItems: 8 > + maxItems: 9 > > interrupt-names: > items: > @@ -66,6 +66,7 @@ properties: > - const: msi5 > - const: msi6 > - const: msi7 > + - const: global Either context is missing or these are not synced with interrupts. Best regards, Krzysztof
On Wed, Feb 26, 2025 at 08:32:42AM +0100, Krzysztof Kozlowski wrote: > On Tue, Feb 25, 2025 at 03:04:06PM +0530, Krishna Chaitanya Chundru wrote: > > Qcom PCIe RC controllers are capable of generating 'global' SPI interrupt > > to the host CPU. This interrupt can be used by the device driver to handle > > PCIe link specific events such as Link up and Link down, which give the > > driver a chance to start bus enumeration on its own when link is up and > > initiate link training if link goes to a bad state. The PCIe driver can > > still work without this interrupt but it will provide a nice user > > experience when device gets plugged and removed. > > > > Hence, document it in the binding along with the existing MSI interrupts. > > Global interrupt is parsed as optional in driver, so adding it in bindings > > will not break the ABI. > > > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com> > > --- > > Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml > > index 76cb9fbfd476..7ae09ba8da60 100644 > > --- a/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml > > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml > > @@ -54,7 +54,7 @@ properties: > > > > interrupts: > > minItems: 8 > > - maxItems: 8 > > + maxItems: 9 > > > > interrupt-names: > > items: > > @@ -66,6 +66,7 @@ properties: > > - const: msi5 > > - const: msi6 > > - const: msi7 > > + - const: global > > Either context is missing or these are not synced with interrupts. > I think the patch context ("properties") is confusing here, but it looks to me that these are in sync: interrupts is defined to have 8 items, and interrupt-names is a list of msi0 through msi7. @Krishna, these two last patches (adding the global interrupt) doesn't seem strongly connected to the switch patches. So, if Krzysztof agrees with above assessment, please submit them separately (i.e. a new series, 2 patches, v5). Regards, Bjorn > Best regards, > Krzysztof >
On 26/02/2025 17:29, Bjorn Andersson wrote: >>> @@ -54,7 +54,7 @@ properties: >>> >>> interrupts: >>> minItems: 8 >>> - maxItems: 8 >>> + maxItems: 9 >>> >>> interrupt-names: >>> items: >>> @@ -66,6 +66,7 @@ properties: >>> - const: msi5 >>> - const: msi6 >>> - const: msi7 >>> + - const: global >> >> Either context is missing or these are not synced with interrupts. >> > > I think the patch context ("properties") is confusing here, but it looks > to me that these are in sync: interrupts is defined to have 8 items, and > interrupt-names is a list of msi0 through msi7. interrupt-names has minItems 9 in this case, so they are not synced. That's my concern Best regards, Krzysztof
On 2/27/2025 3:03 AM, Krzysztof Kozlowski wrote: > On 26/02/2025 17:29, Bjorn Andersson wrote: >>>> @@ -54,7 +54,7 @@ properties: >>>> >>>> interrupts: >>>> minItems: 8 >>>> - maxItems: 8 >>>> + maxItems: 9 >>>> >>>> interrupt-names: >>>> items: >>>> @@ -66,6 +66,7 @@ properties: >>>> - const: msi5 >>>> - const: msi6 >>>> - const: msi7 >>>> + - const: global >>> >>> Either context is missing or these are not synced with interrupts. >>> >> >> I think the patch context ("properties") is confusing here, but it looks >> to me that these are in sync: interrupts is defined to have 8 items, and >> interrupt-names is a list of msi0 through msi7. > > interrupt-names has minItems 9 in this case, so they are not synced. > That's my concern > Ok I will update the minItems to 9 as suggested. - Krishna Chaitanya. > > Best regards, > Krzysztof
On Thu, Feb 27, 2025 at 09:09:38AM +0530, Krishna Chaitanya Chundru wrote: > > > On 2/27/2025 3:03 AM, Krzysztof Kozlowski wrote: > > On 26/02/2025 17:29, Bjorn Andersson wrote: > > > > > @@ -54,7 +54,7 @@ properties: > > > > > interrupts: > > > > > minItems: 8 > > > > > - maxItems: 8 > > > > > + maxItems: 9 > > > > > interrupt-names: > > > > > items: > > > > > @@ -66,6 +66,7 @@ properties: > > > > > - const: msi5 > > > > > - const: msi6 > > > > > - const: msi7 > > > > > + - const: global > > > > > > > > Either context is missing or these are not synced with interrupts. > > > > > > > > > > I think the patch context ("properties") is confusing here, but it looks > > > to me that these are in sync: interrupts is defined to have 8 items, and > > > interrupt-names is a list of msi0 through msi7. > > > > interrupt-names has minItems 9 in this case, so they are not synced. > > That's my concern > > > Ok I will update the minItems to 9 as suggested. > You got it backwards. interrupt-names should have minItems as 8. Otherwise, old DTS will break. - Mani
On 26/02/25 10:29:43, Bjorn Andersson wrote: > On Wed, Feb 26, 2025 at 08:32:42AM +0100, Krzysztof Kozlowski wrote: > > On Tue, Feb 25, 2025 at 03:04:06PM +0530, Krishna Chaitanya Chundru wrote: > > > Qcom PCIe RC controllers are capable of generating 'global' SPI interrupt > > > to the host CPU. This interrupt can be used by the device driver to handle > > > PCIe link specific events such as Link up and Link down, which give the > > > driver a chance to start bus enumeration on its own when link is up and > > > initiate link training if link goes to a bad state. The PCIe driver can > > > still work without this interrupt but it will provide a nice user > > > experience when device gets plugged and removed. > > > > > > Hence, document it in the binding along with the existing MSI interrupts. > > > Global interrupt is parsed as optional in driver, so adding it in bindings > > > will not break the ABI. > > > > > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com> > > > --- > > > Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml | 8 +++++--- > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml > > > index 76cb9fbfd476..7ae09ba8da60 100644 > > > --- a/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml > > > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml > > > @@ -54,7 +54,7 @@ properties: > > > > > > interrupts: > > > minItems: 8 > > > - maxItems: 8 > > > + maxItems: 9 > > > > > > interrupt-names: > > > items: > > > @@ -66,6 +66,7 @@ properties: > > > - const: msi5 > > > - const: msi6 > > > - const: msi7 > > > + - const: global > > > > Either context is missing or these are not synced with interrupts. > > > > I think the patch context ("properties") is confusing here, but it looks > to me that these are in sync: interrupts is defined to have 8 items, and > interrupt-names is a list of msi0 through msi7. > > @Krishna, these two last patches (adding the global interrupt) doesn't > seem strongly connected to the switch patches. So, if Krzysztof agrees > with above assessment, please submit them separately (i.e. a new series, > 2 patches, v5). um, but without these two patches, the functionality is broken requiring users to manually rescan the pci bus (ie, via sysfs) to see what is behind the bridge. shouldnt the set include all the necessary patches? > > Regards, > Bjorn > > > Best regards, > > Krzysztof > >
On Wed, Mar 05, 2025 at 08:36:26AM +0100, Jorge Ramirez wrote: > On 26/02/25 10:29:43, Bjorn Andersson wrote: > > On Wed, Feb 26, 2025 at 08:32:42AM +0100, Krzysztof Kozlowski wrote: > > > On Tue, Feb 25, 2025 at 03:04:06PM +0530, Krishna Chaitanya Chundru wrote: > > > > Qcom PCIe RC controllers are capable of generating 'global' SPI interrupt > > > > to the host CPU. This interrupt can be used by the device driver to handle > > > > PCIe link specific events such as Link up and Link down, which give the > > > > driver a chance to start bus enumeration on its own when link is up and > > > > initiate link training if link goes to a bad state. The PCIe driver can > > > > still work without this interrupt but it will provide a nice user > > > > experience when device gets plugged and removed. > > > > > > > > Hence, document it in the binding along with the existing MSI interrupts. > > > > Global interrupt is parsed as optional in driver, so adding it in bindings > > > > will not break the ABI. > > > > > > > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com> > > > > --- > > > > Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml | 8 +++++--- > > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml > > > > index 76cb9fbfd476..7ae09ba8da60 100644 > > > > --- a/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml > > > > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml > > > > @@ -54,7 +54,7 @@ properties: > > > > > > > > interrupts: > > > > minItems: 8 > > > > - maxItems: 8 > > > > + maxItems: 9 > > > > > > > > interrupt-names: > > > > items: > > > > @@ -66,6 +66,7 @@ properties: > > > > - const: msi5 > > > > - const: msi6 > > > > - const: msi7 > > > > + - const: global > > > > > > Either context is missing or these are not synced with interrupts. > > > > > > > I think the patch context ("properties") is confusing here, but it looks > > to me that these are in sync: interrupts is defined to have 8 items, and > > interrupt-names is a list of msi0 through msi7. > > > > @Krishna, these two last patches (adding the global interrupt) doesn't > > seem strongly connected to the switch patches. So, if Krzysztof agrees > > with above assessment, please submit them separately (i.e. a new series, > > 2 patches, v5). > > um, but without these two patches, the functionality is broken requiring > users to manually rescan the pci bus (ie, via sysfs) to see what is > behind the bridge. > It is not *broken* actually. The series is for enabling the PCIe switch and the 'global' IRQ is a host behavior. So technically both are not dependent on each other. > shouldnt the set include all the necessary patches? > FWIW, I have submitted a series that adds the IRQ for most of the arm64 platforms: https://lore.kernel.org/linux-arm-msm/20250227-pcie-global-irq-v1-0-2b70a7819d1e@linaro.org/ There is a possibility that the above series could get merged before this one. - Mani
On 05/03/2025 08:36, Jorge Ramirez wrote: >> I think the patch context ("properties") is confusing here, but it looks >> to me that these are in sync: interrupts is defined to have 8 items, and >> interrupt-names is a list of msi0 through msi7. >> >> @Krishna, these two last patches (adding the global interrupt) doesn't >> seem strongly connected to the switch patches. So, if Krzysztof agrees >> with above assessment, please submit them separately (i.e. a new series, >> 2 patches, v5). > > um, but without these two patches, the functionality is broken requiring > users to manually rescan the pci bus (ie, via sysfs) to see what is > behind the bridge. > > shouldnt the set include all the necessary patches? Broken? Then the patchset should be NAKed because nothing can depend on the DTS and the DTS should not depend on the drivers without some sort of explanation, either. Also nothing in the commit msg explains this dependency, so I don't get the problem here. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml index 76cb9fbfd476..7ae09ba8da60 100644 --- a/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml @@ -54,7 +54,7 @@ properties: interrupts: minItems: 8 - maxItems: 8 + maxItems: 9 interrupt-names: items: @@ -66,6 +66,7 @@ properties: - const: msi5 - const: msi6 - const: msi7 + - const: global resets: maxItems: 1 @@ -149,9 +150,10 @@ examples: <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>; + <GIC_SPI 375 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "msi0", "msi1", "msi2", "msi3", - "msi4", "msi5", "msi6", "msi7"; + "msi4", "msi5", "msi6", "msi7", "global"; #interrupt-cells = <1>; interrupt-map-mask = <0 0 0 0x7>; interrupt-map = <0 0 0 1 &intc 0 0 0 434 IRQ_TYPE_LEVEL_HIGH>,
Qcom PCIe RC controllers are capable of generating 'global' SPI interrupt to the host CPU. This interrupt can be used by the device driver to handle PCIe link specific events such as Link up and Link down, which give the driver a chance to start bus enumeration on its own when link is up and initiate link training if link goes to a bad state. The PCIe driver can still work without this interrupt but it will provide a nice user experience when device gets plugged and removed. Hence, document it in the binding along with the existing MSI interrupts. Global interrupt is parsed as optional in driver, so adding it in bindings will not break the ABI. Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com> --- Documentation/devicetree/bindings/pci/qcom,pcie-sc7280.yaml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)