| Message ID | 20220512104545.2204523-1-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | PCI: qcom: Fix higher MSI vectors handling | expand |
On Thu, May 12, 2022 at 01:45:35PM +0300, Dmitry Baryshkov wrote: > I have replied with my Tested-by to the patch at [2], which has landed > in the linux-next as the commit 20f1bfb8dd62 ("PCI: qcom: > Add support for handling MSIs from 8 endpoints"). However lately I > noticed that during the tests I still had 'pcie_pme=nomsi', so the > device was not forced to use higher MSI vectors. > > After removing this option I noticed that hight MSI vectors are not > delivered on tested platforms. After additional research I stumbled upon > a patch in msm-4.14 ([1]), which describes that each group of MSI > vectors is mapped to the separate interrupt. Implement corresponding > mapping. > > The first patch in the series is a revert of [2] (landed in pci-next). > Either both patches should be applied or both should be dropped. > > Patchseries dependecies: [3] (for the schema change). > > Changes since v7: > - Move code back to the dwc core driver (as required by Rob), > - Change dt schema to require either a single "msi" interrupt or an > array of "msi0", "msi1", ... "msi7" IRQs. Disallow specifying a > part of the array (the DT should specify the exact amount of MSI IRQs > allowing fallback to a single "msi" IRQ), Why this new constraint? I've been using your v7 with an sc8280xp which only has four IRQs (and hence 128 MSIs). Looks like this version of the series would not allow that anymore. Johan
On Fri, 13 May 2022 at 11:58, Johan Hovold <johan@kernel.org> wrote: > > On Thu, May 12, 2022 at 01:45:35PM +0300, Dmitry Baryshkov wrote: > > I have replied with my Tested-by to the patch at [2], which has landed > > in the linux-next as the commit 20f1bfb8dd62 ("PCI: qcom: > > Add support for handling MSIs from 8 endpoints"). However lately I > > noticed that during the tests I still had 'pcie_pme=nomsi', so the > > device was not forced to use higher MSI vectors. > > > > After removing this option I noticed that hight MSI vectors are not > > delivered on tested platforms. After additional research I stumbled upon > > a patch in msm-4.14 ([1]), which describes that each group of MSI > > vectors is mapped to the separate interrupt. Implement corresponding > > mapping. > > > > The first patch in the series is a revert of [2] (landed in pci-next). > > Either both patches should be applied or both should be dropped. > > > > Patchseries dependecies: [3] (for the schema change). > > > > Changes since v7: > > - Move code back to the dwc core driver (as required by Rob), > > - Change dt schema to require either a single "msi" interrupt or an > > array of "msi0", "msi1", ... "msi7" IRQs. Disallow specifying a > > part of the array (the DT should specify the exact amount of MSI IRQs > > allowing fallback to a single "msi" IRQ), > > Why this new constraint? > > I've been using your v7 with an sc8280xp which only has four IRQs (and > hence 128 MSIs). > > Looks like this version of the series would not allow that anymore. It allows it, provided that you set pp->num_vectors correctly (to 128 in your case). The main idea was to disallow mistakes in the platform configuration. If the platform says that it supports 256 vectors (and 8 groups), there must be 8 groups. Or a single backwards-compatible group.
On Fri, May 13, 2022 at 12:28:40PM +0300, Dmitry Baryshkov wrote: > On Fri, 13 May 2022 at 11:58, Johan Hovold <johan@kernel.org> wrote: > > > > On Thu, May 12, 2022 at 01:45:35PM +0300, Dmitry Baryshkov wrote: > > > I have replied with my Tested-by to the patch at [2], which has landed > > > in the linux-next as the commit 20f1bfb8dd62 ("PCI: qcom: > > > Add support for handling MSIs from 8 endpoints"). However lately I > > > noticed that during the tests I still had 'pcie_pme=nomsi', so the > > > device was not forced to use higher MSI vectors. > > > > > > After removing this option I noticed that hight MSI vectors are not > > > delivered on tested platforms. After additional research I stumbled upon > > > a patch in msm-4.14 ([1]), which describes that each group of MSI > > > vectors is mapped to the separate interrupt. Implement corresponding > > > mapping. > > > > > > The first patch in the series is a revert of [2] (landed in pci-next). > > > Either both patches should be applied or both should be dropped. > > > > > > Patchseries dependecies: [3] (for the schema change). > > > > > > Changes since v7: > > > - Move code back to the dwc core driver (as required by Rob), > > > - Change dt schema to require either a single "msi" interrupt or an > > > array of "msi0", "msi1", ... "msi7" IRQs. Disallow specifying a > > > part of the array (the DT should specify the exact amount of MSI IRQs > > > allowing fallback to a single "msi" IRQ), > > > > Why this new constraint? > > > > I've been using your v7 with an sc8280xp which only has four IRQs (and > > hence 128 MSIs). > > > > Looks like this version of the series would not allow that anymore. > > It allows it, provided that you set pp->num_vectors correctly (to 128 > in your case). > The main idea was to disallow mistakes in the platform configuration. > If the platform says that it supports 256 vectors (and 8 groups), > there must be 8 groups. Or a single backwards-compatible group. But you also added + - properties: + interrupts: + minItems: 8 + interrupt-names: + minItems: 8 + items: + - const: msi0 + - const: msi1 + - const: msi2 + - const: msi3 + - const: msi4 + - const: msi5 + - const: msi6 + - const: msi7 which means that I can no longer describe the four interrupts in DT. I didn't check the implementation, but it seems you should derive the number of MSIs based on the devicetree as I guess you did in v7. Johan
On Fri, 13 May 2022 at 12:36, Johan Hovold <johan@kernel.org> wrote: > > On Fri, May 13, 2022 at 12:28:40PM +0300, Dmitry Baryshkov wrote: > > On Fri, 13 May 2022 at 11:58, Johan Hovold <johan@kernel.org> wrote: > > > > > > On Thu, May 12, 2022 at 01:45:35PM +0300, Dmitry Baryshkov wrote: > > > > I have replied with my Tested-by to the patch at [2], which has landed > > > > in the linux-next as the commit 20f1bfb8dd62 ("PCI: qcom: > > > > Add support for handling MSIs from 8 endpoints"). However lately I > > > > noticed that during the tests I still had 'pcie_pme=nomsi', so the > > > > device was not forced to use higher MSI vectors. > > > > > > > > After removing this option I noticed that hight MSI vectors are not > > > > delivered on tested platforms. After additional research I stumbled upon > > > > a patch in msm-4.14 ([1]), which describes that each group of MSI > > > > vectors is mapped to the separate interrupt. Implement corresponding > > > > mapping. > > > > > > > > The first patch in the series is a revert of [2] (landed in pci-next). > > > > Either both patches should be applied or both should be dropped. > > > > > > > > Patchseries dependecies: [3] (for the schema change). > > > > > > > > Changes since v7: > > > > - Move code back to the dwc core driver (as required by Rob), > > > > - Change dt schema to require either a single "msi" interrupt or an > > > > array of "msi0", "msi1", ... "msi7" IRQs. Disallow specifying a > > > > part of the array (the DT should specify the exact amount of MSI IRQs > > > > allowing fallback to a single "msi" IRQ), > > > > > > Why this new constraint? > > > > > > I've been using your v7 with an sc8280xp which only has four IRQs (and > > > hence 128 MSIs). > > > > > > Looks like this version of the series would not allow that anymore. > > > > It allows it, provided that you set pp->num_vectors correctly (to 128 > > in your case). > > The main idea was to disallow mistakes in the platform configuration. > > If the platform says that it supports 256 vectors (and 8 groups), > > there must be 8 groups. Or a single backwards-compatible group. > > But you also added > > + - properties: > + interrupts: > + minItems: 8 > + interrupt-names: > + minItems: 8 > + items: > + - const: msi0 > + - const: msi1 > + - const: msi2 > + - const: msi3 > + - const: msi4 > + - const: msi5 > + - const: msi6 > + - const: msi7 > > which means that I can no longer describe the four interrupts in DT. > > I didn't check the implementation, but it seems you should derive the > number of MSIs based on the devicetree as I guess you did in v7. It is a conditional, so you can add another conditional for the sc8280xp platform describing just 4 interrupts. And as you don't have legacy DTS, you can completely omit the backwards compatible clause in your case. So, something like: - if: properties: contains: enum: - qcom,pcie-sc8280xp then: properties: interrupts: minItems: 4 maxItems: 4 interrupt-names: items: - const: msi0 - const: msi1 - const: msi2 - const: msi3
On 13/05/2022 11:58, Johan Hovold wrote: > On Thu, May 12, 2022 at 01:45:35PM +0300, Dmitry Baryshkov wrote: >> I have replied with my Tested-by to the patch at [2], which has landed >> in the linux-next as the commit 20f1bfb8dd62 ("PCI: qcom: >> Add support for handling MSIs from 8 endpoints"). However lately I >> noticed that during the tests I still had 'pcie_pme=nomsi', so the >> device was not forced to use higher MSI vectors. >> >> After removing this option I noticed that hight MSI vectors are not >> delivered on tested platforms. After additional research I stumbled upon >> a patch in msm-4.14 ([1]), which describes that each group of MSI >> vectors is mapped to the separate interrupt. Implement corresponding >> mapping. >> >> The first patch in the series is a revert of [2] (landed in pci-next). >> Either both patches should be applied or both should be dropped. >> >> Patchseries dependecies: [3] (for the schema change). >> >> Changes since v7: >> - Move code back to the dwc core driver (as required by Rob), >> - Change dt schema to require either a single "msi" interrupt or an >> array of "msi0", "msi1", ... "msi7" IRQs. Disallow specifying a >> part of the array (the DT should specify the exact amount of MSI IRQs >> allowing fallback to a single "msi" IRQ), > > Why this new constraint? > > I've been using your v7 with an sc8280xp which only has four IRQs (and > hence 128 MSIs). > > Looks like this version of the series would not allow that anymore. As a second thought, let's relax parsing needs. > > Johan
On Fri, May 13, 2022 at 01:10:44PM +0300, Dmitry Baryshkov wrote: > On Fri, 13 May 2022 at 12:36, Johan Hovold <johan@kernel.org> wrote: > > > > On Fri, May 13, 2022 at 12:28:40PM +0300, Dmitry Baryshkov wrote: > > > On Fri, 13 May 2022 at 11:58, Johan Hovold <johan@kernel.org> wrote: > > But you also added > > > > + - properties: > > + interrupts: > > + minItems: 8 > > + interrupt-names: > > + minItems: 8 > > + items: > > + - const: msi0 > > + - const: msi1 > > + - const: msi2 > > + - const: msi3 > > + - const: msi4 > > + - const: msi5 > > + - const: msi6 > > + - const: msi7 > > > > which means that I can no longer describe the four interrupts in DT. > > > > I didn't check the implementation, but it seems you should derive the > > number of MSIs based on the devicetree as I guess you did in v7. > > It is a conditional, so you can add another conditional for the > sc8280xp platform describing just 4 interrupts. And as you don't have > legacy DTS, you can completely omit the backwards compatible clause in > your case. > So, something like: > - if: > properties: > contains: > enum: > - qcom,pcie-sc8280xp > then: > properties: > interrupts: > minItems: 4 > maxItems: 4 > interrupt-names: > items: > - const: msi0 > - const: msi1 > - const: msi2 > - const: msi3 Ok, so the driver code still handles it, thanks. Are you able to confirm that all sc8280xp systems have only four msi IRQs? This seems like another case of using the kernel as a DT validator by describing things in two places and making sure that they match. I assume the number of vectors will always be a multiple of the numbers of msi IRQs. Right? Then we don't need to encode this number for every supported platform in the corresponding PCIe driver even if we end up describing it in the binding. Johan
On 13/05/2022 15:39, Dmitry Baryshkov wrote: > On 13/05/2022 11:58, Johan Hovold wrote: >> On Thu, May 12, 2022 at 01:45:35PM +0300, Dmitry Baryshkov wrote: >>> I have replied with my Tested-by to the patch at [2], which has landed >>> in the linux-next as the commit 20f1bfb8dd62 ("PCI: qcom: >>> Add support for handling MSIs from 8 endpoints"). However lately I >>> noticed that during the tests I still had 'pcie_pme=nomsi', so the >>> device was not forced to use higher MSI vectors. >>> >>> After removing this option I noticed that hight MSI vectors are not >>> delivered on tested platforms. After additional research I stumbled upon >>> a patch in msm-4.14 ([1]), which describes that each group of MSI >>> vectors is mapped to the separate interrupt. Implement corresponding >>> mapping. >>> >>> The first patch in the series is a revert of [2] (landed in pci-next). >>> Either both patches should be applied or both should be dropped. >>> >>> Patchseries dependecies: [3] (for the schema change). >>> >>> Changes since v7: >>> - Move code back to the dwc core driver (as required by Rob), >>> - Change dt schema to require either a single "msi" interrupt or an >>> array of "msi0", "msi1", ... "msi7" IRQs. Disallow specifying a >>> part of the array (the DT should specify the exact amount of MSI >>> IRQs >>> allowing fallback to a single "msi" IRQ), >> >> Why this new constraint? >> >> I've been using your v7 with an sc8280xp which only has four IRQs (and >> hence 128 MSIs). >> >> Looks like this version of the series would not allow that anymore. > > As a second thought, let's relax parsing needs. Hmm, with num_vectors being specified in the qcom cfg data, this is not required anymore.
On Fri, May 13, 2022 at 04:08:30PM +0300, Dmitry Baryshkov wrote: > On 13/05/2022 15:39, Dmitry Baryshkov wrote: > > On 13/05/2022 11:58, Johan Hovold wrote: > >> I've been using your v7 with an sc8280xp which only has four IRQs (and > >> hence 128 MSIs). > >> > >> Looks like this version of the series would not allow that anymore. > > > > As a second thought, let's relax parsing needs. > > Hmm, with num_vectors being specified in the qcom cfg data, this is not > required anymore. Right, but I'd prefer it the other way round; to use devicetree to describe the system and not duplicate that information in the driver if possible. Johan
On 13/05/2022 15:52, Johan Hovold wrote: > On Fri, May 13, 2022 at 01:10:44PM +0300, Dmitry Baryshkov wrote: >> On Fri, 13 May 2022 at 12:36, Johan Hovold <johan@kernel.org> wrote: >>> >>> On Fri, May 13, 2022 at 12:28:40PM +0300, Dmitry Baryshkov wrote: >>>> On Fri, 13 May 2022 at 11:58, Johan Hovold <johan@kernel.org> wrote: > >>> But you also added >>> >>> + - properties: >>> + interrupts: >>> + minItems: 8 >>> + interrupt-names: >>> + minItems: 8 >>> + items: >>> + - const: msi0 >>> + - const: msi1 >>> + - const: msi2 >>> + - const: msi3 >>> + - const: msi4 >>> + - const: msi5 >>> + - const: msi6 >>> + - const: msi7 >>> >>> which means that I can no longer describe the four interrupts in DT. >>> >>> I didn't check the implementation, but it seems you should derive the >>> number of MSIs based on the devicetree as I guess you did in v7. >> >> It is a conditional, so you can add another conditional for the >> sc8280xp platform describing just 4 interrupts. And as you don't have >> legacy DTS, you can completely omit the backwards compatible clause in >> your case. >> So, something like: >> - if: >> properties: >> contains: >> enum: >> - qcom,pcie-sc8280xp >> then: >> properties: >> interrupts: >> minItems: 4 >> maxItems: 4 >> interrupt-names: >> items: >> - const: msi0 >> - const: msi1 >> - const: msi2 >> - const: msi3 > > Ok, so the driver code still handles it, thanks. > > Are you able to confirm that all sc8280xp systems have only four msi > IRQs? Unfortunately no. I don't have access to the sc8280xp docs. Let's see if BjornA can confirm this. > This seems like another case of using the kernel as a DT validator by > describing things in two places and making sure that they match. Yep, this seems like a bad habit of mine: to distrust the DT. > > I assume the number of vectors will always be a multiple of the numbers > of msi IRQs. Right? Then we don't need to encode this number for every > supported platform in the corresponding PCIe driver even if we end up > describing it in the binding. But it was your suggestion! Let's drop the warning then, parse what was passed by the DT and just print the total amount of MSI IRQs.
On Fri, May 13, 2022 at 04:50:11PM +0300, Dmitry Baryshkov wrote: > On 13/05/2022 15:52, Johan Hovold wrote: > > On Fri, May 13, 2022 at 01:10:44PM +0300, Dmitry Baryshkov wrote: > >> On Fri, 13 May 2022 at 12:36, Johan Hovold <johan@kernel.org> wrote: > > Are you able to confirm that all sc8280xp systems have only four msi > > IRQs? > > Unfortunately no. I don't have access to the sc8280xp docs. Let's see if > BjornA can confirm this. I just talked to Bjorn and he said they should be the same for all sc8280xp, but of course there are complications like some of the controllers actually having more than 4 interrupts (even if they may not be usable). Probably best to describe this in DT. > > This seems like another case of using the kernel as a DT validator by > > describing things in two places and making sure that they match. > > Yep, this seems like a bad habit of mine: to distrust the DT. > > > > > I assume the number of vectors will always be a multiple of the numbers > > of msi IRQs. Right? Then we don't need to encode this number for every > > supported platform in the corresponding PCIe driver even if we end up > > describing it in the binding. > > But it was your suggestion! Heh. But with the caveat that I'd still prefer this to come from DT if possible. :) > Let's drop the warning then, parse what was passed by the DT and just > print the total amount of MSI IRQs. Perfect, thanks! Johan
I have replied with my Tested-by to the patch at [2], which has landed in the linux-next as the commit 20f1bfb8dd62 ("PCI: qcom: Add support for handling MSIs from 8 endpoints"). However lately I noticed that during the tests I still had 'pcie_pme=nomsi', so the device was not forced to use higher MSI vectors. After removing this option I noticed that hight MSI vectors are not delivered on tested platforms. After additional research I stumbled upon a patch in msm-4.14 ([1]), which describes that each group of MSI vectors is mapped to the separate interrupt. Implement corresponding mapping. The first patch in the series is a revert of [2] (landed in pci-next). Either both patches should be applied or both should be dropped. Patchseries dependecies: [3] (for the schema change). Changes since v7: - Move code back to the dwc core driver (as required by Rob), - Change dt schema to require either a single "msi" interrupt or an array of "msi0", "msi1", ... "msi7" IRQs. Disallow specifying a part of the array (the DT should specify the exact amount of MSI IRQs allowing fallback to a single "msi" IRQ), - Fix in the DWC init code for the dma_mapping_error() return value. Changes since v6: - Fix indentation of the arguments as requested by Stanimir Changes since v5: - Fixed commit subject and in-comment code according to Bjorn's suggestion, - Changed variable idx to i to follow dw_handle_msi_irq() style. Changes since v4: - Fix the minItems/maxItems properties in the YAML schema. Changes since v3: - Reimplement MSI handling scheme in the Qualcomm host controller driver. Changes since v2: - Fix and rephrase commit message for patch 2. Changes since v1: - Split a huge patch into three patches as suggested by Bjorn Helgaas - snps,dw-pcie removal is now part of [3] [1] https://git.codelinaro.org/clo/la/kernel/msm-4.14/-/commit/671a3d5f129f4bfe477152292ada2194c8440d22 [2] https://lore.kernel.org/linux-arm-msm/20211214101319.25258-1-manivannan.sadhasivam@linaro.org/ [3] https://lore.kernel.org/linux-arm-msm/20220422211002.2012070-1-dmitry.baryshkov@linaro.org/ Dmitry Baryshkov (10): PCI: qcom: Revert "PCI: qcom: Add support for handling MSIs from 8 endpoints" PCI: dwc: Correct msi_irq condition in dw_pcie_free_msi() PCI: dwc: Convert msi_irq to the array PCI: dwc: Propagate error from dma_mapping_error() PCI: dwc: split MSI IRQ parsing/allocation to a separate function PCI: dwc: Handle MSIs routed to multiple GIC interrupts PCI: qcom: Handle MSIs routed to multiple GIC interrupts PCI: dwc: Implement special ISR handler for split MSI IRQ setup dt-bindings: PCI: qcom: Support additional MSI interrupts arm64: dts: qcom: sm8250: provide additional MSI interrupts .../devicetree/bindings/pci/qcom,pcie.yaml | 53 ++++- arch/arm64/boot/dts/qcom/sm8250.dtsi | 11 +- drivers/pci/controller/dwc/pci-dra7xx.c | 2 +- drivers/pci/controller/dwc/pci-exynos.c | 2 +- .../pci/controller/dwc/pcie-designware-host.c | 214 +++++++++++++----- drivers/pci/controller/dwc/pcie-designware.h | 3 +- drivers/pci/controller/dwc/pcie-keembay.c | 2 +- drivers/pci/controller/dwc/pcie-qcom.c | 13 +- drivers/pci/controller/dwc/pcie-spear13xx.c | 2 +- drivers/pci/controller/dwc/pcie-tegra194.c | 2 +- 10 files changed, 231 insertions(+), 73 deletions(-)