| Message ID | 20220513172622.2968887-1-dmitry.baryshkov@linaro.org |
|---|---|
| Headers | show |
| Series | PCI: qcom: Fix higher MSI vectors handling | expand |
On Fri, May 13, 2022 at 08:26:14PM +0300, Dmitry Baryshkov wrote: > If dma mapping fails, dma_mapping_error() will return an error. > Propagate it to the dw_pcie_host_init() return value rather than > incorrectly returning 0 in this case. > > Fixes: 07940c369a6b ("PCI: dwc: Fix MSI page leakage in suspend/resume") > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/pci/controller/dwc/pcie-designware-host.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 2fa86f32d964..a9a31e9e7b6e 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -396,8 +396,9 @@ int dw_pcie_host_init(struct pcie_port *pp) > sizeof(pp->msi_msg), > DMA_FROM_DEVICE, > DMA_ATTR_SKIP_CPU_SYNC); > - if (dma_mapping_error(pci->dev, pp->msi_data)) { > - dev_err(pci->dev, "Failed to map MSI data\n"); > + ret = dma_mapping_error(pci->dev, pp->msi_data); > + if (ret) { > + dev_err(pci->dev, "Failed to map MSI data: %d\n", ret); > pp->msi_data = 0; > goto err_free_msi; > } This has already been fixed by commit 88557685cd72 ("PCI: dwc: Fix setting error return on MSI DMA mapping failure"), which prevents the series from applying cleanly. Johan
On Fri, May 13, 2022 at 08:26:18PM +0300, Dmitry Baryshkov wrote: > On some of Qualcomm platforms each group of 32 MSI vectors is routed to the > separate GIC interrupt. Implement support for such configurations by > parsing "msi0" ... "msiN" interrupts and attaching them to the chained > handler. > > Note, that if DT doesn't list an array of MSI interrupts and uses single > "msi" IRQ, the driver will limit the amount of supported MSI vectors > accordingly (to 32). > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > .../pci/controller/dwc/pcie-designware-host.c | 38 ++++++++++++++++++- > drivers/pci/controller/dwc/pcie-designware.h | 1 + > 2 files changed, 38 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 70f0435907c1..320a968dd366 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -288,6 +288,11 @@ static void dw_pcie_msi_init(struct pcie_port *pp) > dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target)); > } > > +static const char * const split_msi_names[] = { > + "msi0", "msi1", "msi2", "msi3", > + "msi4", "msi5", "msi6", "msi7", > +}; > + > static int dw_pcie_msi_host_init(struct pcie_port *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > @@ -300,17 +305,48 @@ static int dw_pcie_msi_host_init(struct pcie_port *pp) > for (ctrl = 0; ctrl < num_ctrls; ctrl++) > pp->irq_mask[ctrl] = ~0; > > + if (pp->has_split_msi_irq) { You don't need to add this configuration parameter as it can be inferred from the devicetree (e.g. if "msi0" is specified). > + /* > + * Parse as many IRQs as described in the DTS. If there are > + * none, fallback to the single "msi" IRQ. > + */ > + for (ctrl = 0; ctrl < num_ctrls; ctrl++) { > + int irq; > + > + if (pp->msi_irq[ctrl]) > + continue; > + > + irq = platform_get_irq_byname(pdev, split_msi_names[ctrl]); You need to use platform_get_irq_byname_optional() here or an error will still printed if the number of "msi" interrupts is less than 8. > + if (irq == -ENXIO) { > + num_ctrls = ctrl; > + break; > + } else if (irq < 0) { > + return dev_err_probe(dev, irq, > + "Failed to parse MSI IRQ '%s'\n", > + split_msi_names[ctrl]); > + } > + > + pp->msi_irq[ctrl] = irq; > + } > + > + if (num_ctrls == 0) > + num_ctrls = 1; > + } > + > if (!pp->msi_irq[0]) { > int irq = platform_get_irq_byname_optional(pdev, "msi"); > > if (irq < 0) { > irq = platform_get_irq(pdev, 0); > if (irq < 0) > - return irq; > + return dev_err_probe(dev, irq, "Failed to parse MSI irq\n"); > } > pp->msi_irq[0] = irq; > } > > + pp->num_vectors = min_t(u32, pp->num_vectors, num_ctrls * MAX_MSI_IRQS_PER_CTRL); > + dev_dbg(dev, "Using %d MSI vectors\n", pp->num_vectors); Can you rework the handling of num_vectors == 0 (in dw_pcie_host_init()) so that the number is always inferred from the number of "msi" interrupts without having to pass in num_vectors == MAX_MSI_IRQS? That is num_vectors == 0 && "msi" => num_vectors = MSI_DEF_NUM_VECTORS (32) num_vectors == 0 && "msi0".."msin" => num_vectors = n * MAX_MSI_IRQS_PER_CTRL (n * 32) > + > pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip; > > ret = dw_pcie_allocate_domains(pp); > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 9c1a38b0a6b3..3aa840a5b19c 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -179,6 +179,7 @@ struct dw_pcie_host_ops { > > struct pcie_port { > bool has_msi_ctrl:1; > + bool has_split_msi_irq:1; > u64 cfg0_base; > void __iomem *va_cfg0_base; > u32 cfg0_size; Johan
On Fri, May 13, 2022 at 08:26:20PM +0300, Dmitry Baryshkov wrote: > On some of Qualcomm platforms each group of 32 MSI vectors is routed to the > separate GIC interrupt. Thus, to receive higher MSI vectors properly, > declare that the host should use split MSI IRQ handling on these > platforms. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 2e5464edc36e..f79752d1d680 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -194,6 +194,7 @@ struct qcom_pcie_ops { > > struct qcom_pcie_cfg { > const struct qcom_pcie_ops *ops; > + unsigned int has_split_msi_irq:1; > unsigned int pipe_clk_need_muxing:1; > unsigned int has_tbu_clk:1; > unsigned int has_ddrss_sf_tbu_clk:1; > @@ -1502,6 +1503,7 @@ static const struct qcom_pcie_cfg ipq8064_cfg = { > > static const struct qcom_pcie_cfg msm8996_cfg = { > .ops = &ops_2_3_2, > + .has_split_msi_irq = true, > }; > @@ -1592,6 +1599,11 @@ static int qcom_pcie_probe(struct platform_device *pdev) > > pcie->cfg = pcie_cfg; > > + if (pcie->cfg->has_split_msi_irq) { > + pp->num_vectors = MAX_MSI_IRQS; > + pp->has_split_msi_irq = true; > + } > + > pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH); > if (IS_ERR(pcie->reset)) { > ret = PTR_ERR(pcie->reset); So if you fix dwc core to always infer num_vectors when passed in as 0 as I just suggested, then this patch isn't needed. It also avoids having to pass in MAX_MSI_IRQS (or complicate the qcom driver) for sc8280xp which only uses four "msi" GIC interrupts and hence only MAX_MSI_IRQS/2 vectors. Johan
On Fri, May 13, 2022 at 08:26:22PM +0300, Dmitry Baryshkov wrote: > On SM8250 each group of MSI interrupts is mapped to the separate host > interrupt. Describe each of interrupts in the device tree for PCIe0 > host. > > Tested on Qualcomm RB5 platform with first group of MSI interrupts being > used by the PME and attached ath11k WiFi chip using second group of MSI > interrupts. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > arch/arm64/boot/dts/qcom/sm8250.dtsi | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > index 410272a1e19b..523a035ffc5f 100644 > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > @@ -1807,8 +1807,16 @@ pcie0: pci@1c00000 { > ranges = <0x01000000 0x0 0x60200000 0 0x60200000 0x0 0x100000>, > <0x02000000 0x0 0x60300000 0 0x60300000 0x0 0x3d00000>; > > - interrupts = <GIC_SPI 141 IRQ_TYPE_LEVEL_HIGH>; > - interrupt-names = "msi"; > + interrupts = <GIC_SPI 141 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 142 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "msi0", "msi1", "msi2", "msi3", > + "msi4", "msi5", "msi6", "msi7"; > #interrupt-cells = <1>; > interrupt-map-mask = <0 0 0 0x7>; > interrupt-map = <0 0 0 1 &intc 0 149 IRQ_TYPE_LEVEL_HIGH>, /* int_a */ Looks correct now: Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
On 18/05/2022 12:30, Johan Hovold wrote: > On Fri, May 13, 2022 at 08:26:14PM +0300, Dmitry Baryshkov wrote: >> If dma mapping fails, dma_mapping_error() will return an error. >> Propagate it to the dw_pcie_host_init() return value rather than >> incorrectly returning 0 in this case. >> >> Fixes: 07940c369a6b ("PCI: dwc: Fix MSI page leakage in suspend/resume") >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/pci/controller/dwc/pcie-designware-host.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c >> index 2fa86f32d964..a9a31e9e7b6e 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware-host.c >> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c >> @@ -396,8 +396,9 @@ int dw_pcie_host_init(struct pcie_port *pp) >> sizeof(pp->msi_msg), >> DMA_FROM_DEVICE, >> DMA_ATTR_SKIP_CPU_SYNC); >> - if (dma_mapping_error(pci->dev, pp->msi_data)) { >> - dev_err(pci->dev, "Failed to map MSI data\n"); >> + ret = dma_mapping_error(pci->dev, pp->msi_data); >> + if (ret) { >> + dev_err(pci->dev, "Failed to map MSI data: %d\n", ret); >> pp->msi_data = 0; >> goto err_free_msi; >> } > > This has already been fixed by commit 88557685cd72 ("PCI: dwc: Fix > setting error return on MSI DMA mapping failure"), which prevents the > series from applying cleanly. Ack, will rebase and send. > > Johan
On 18/05/2022 12:43, Johan Hovold wrote: > On Fri, May 13, 2022 at 08:26:18PM +0300, Dmitry Baryshkov wrote: >> On some of Qualcomm platforms each group of 32 MSI vectors is routed to the >> separate GIC interrupt. Implement support for such configurations by >> parsing "msi0" ... "msiN" interrupts and attaching them to the chained >> handler. >> >> Note, that if DT doesn't list an array of MSI interrupts and uses single >> "msi" IRQ, the driver will limit the amount of supported MSI vectors >> accordingly (to 32). >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> .../pci/controller/dwc/pcie-designware-host.c | 38 ++++++++++++++++++- >> drivers/pci/controller/dwc/pcie-designware.h | 1 + >> 2 files changed, 38 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c >> index 70f0435907c1..320a968dd366 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware-host.c >> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c >> @@ -288,6 +288,11 @@ static void dw_pcie_msi_init(struct pcie_port *pp) >> dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target)); >> } >> >> +static const char * const split_msi_names[] = { >> + "msi0", "msi1", "msi2", "msi3", >> + "msi4", "msi5", "msi6", "msi7", >> +}; >> + >> static int dw_pcie_msi_host_init(struct pcie_port *pp) >> { >> struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >> @@ -300,17 +305,48 @@ static int dw_pcie_msi_host_init(struct pcie_port *pp) >> for (ctrl = 0; ctrl < num_ctrls; ctrl++) >> pp->irq_mask[ctrl] = ~0; >> >> + if (pp->has_split_msi_irq) { > > You don't need to add this configuration parameter as it can be inferred > from the devicetree (e.g. if "msi0" is specified). > >> + /* >> + * Parse as many IRQs as described in the DTS. If there are >> + * none, fallback to the single "msi" IRQ. >> + */ >> + for (ctrl = 0; ctrl < num_ctrls; ctrl++) { >> + int irq; >> + >> + if (pp->msi_irq[ctrl]) >> + continue; >> + >> + irq = platform_get_irq_byname(pdev, split_msi_names[ctrl]); > > You need to use platform_get_irq_byname_optional() here or an error will > still printed if the number of "msi" interrupts is less than 8. > >> + if (irq == -ENXIO) { >> + num_ctrls = ctrl; >> + break; >> + } else if (irq < 0) { >> + return dev_err_probe(dev, irq, >> + "Failed to parse MSI IRQ '%s'\n", >> + split_msi_names[ctrl]); >> + } >> + >> + pp->msi_irq[ctrl] = irq; >> + } >> + >> + if (num_ctrls == 0) >> + num_ctrls = 1; >> + } >> + >> if (!pp->msi_irq[0]) { >> int irq = platform_get_irq_byname_optional(pdev, "msi"); >> >> if (irq < 0) { >> irq = platform_get_irq(pdev, 0); >> if (irq < 0) >> - return irq; >> + return dev_err_probe(dev, irq, "Failed to parse MSI irq\n"); >> } >> pp->msi_irq[0] = irq; >> } >> >> + pp->num_vectors = min_t(u32, pp->num_vectors, num_ctrls * MAX_MSI_IRQS_PER_CTRL); >> + dev_dbg(dev, "Using %d MSI vectors\n", pp->num_vectors); > > Can you rework the handling of num_vectors == 0 (in dw_pcie_host_init()) > so that the number is always inferred from the number of "msi" > interrupts without having to pass in num_vectors == MAX_MSI_IRQS? It wasn't that easy, but I think I ended up doing it properly. > > That is > > num_vectors == 0 && "msi" => num_vectors = MSI_DEF_NUM_VECTORS (32) > num_vectors == 0 && "msi0".."msin" => num_vectors = n * MAX_MSI_IRQS_PER_CTRL (n * 32) > >> + >> pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip; >> >> ret = dw_pcie_allocate_domains(pp); >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h >> index 9c1a38b0a6b3..3aa840a5b19c 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.h >> +++ b/drivers/pci/controller/dwc/pcie-designware.h >> @@ -179,6 +179,7 @@ struct dw_pcie_host_ops { >> >> struct pcie_port { >> bool has_msi_ctrl:1; >> + bool has_split_msi_irq:1; >> u64 cfg0_base; >> void __iomem *va_cfg0_base; >> u32 cfg0_size; > > Johan
On Fri, May 13, 2022 at 08:26:14PM +0300, Dmitry Baryshkov wrote: > If dma mapping fails, dma_mapping_error() will return an error. > Propagate it to the dw_pcie_host_init() return value rather than > incorrectly returning 0 in this case. > > Fixes: 07940c369a6b ("PCI: dwc: Fix MSI page leakage in suspend/resume") > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/pci/controller/dwc/pcie-designware-host.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) There's already a similar fix applied. > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 2fa86f32d964..a9a31e9e7b6e 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -396,8 +396,9 @@ int dw_pcie_host_init(struct pcie_port *pp) > sizeof(pp->msi_msg), > DMA_FROM_DEVICE, > DMA_ATTR_SKIP_CPU_SYNC); > - if (dma_mapping_error(pci->dev, pp->msi_data)) { > - dev_err(pci->dev, "Failed to map MSI data\n"); > + ret = dma_mapping_error(pci->dev, pp->msi_data); > + if (ret) { > + dev_err(pci->dev, "Failed to map MSI data: %d\n", ret); > pp->msi_data = 0; > goto err_free_msi; > } > -- > 2.35.1 >
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 v9: - Relax requirements and stop validating the DT. If the has_split_msi was specified, parse as many msiN irqs as specified in DT. If there are none, fallback to the single "msi" IRQ. Changes since v8: - Fix typos noted by Bjorn Helgaas - Add missing links to the patch 1 (revert) - Fix sm8250 interrupt-names (Johan) - Specify num_vectors in qcom configuration data (Johan) - Rework parsing of MSI IRQs (Johan) 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: Propagate error from dma_mapping_error() PCI: dwc: Correct msi_irq condition in dw_pcie_free_msi() PCI: dwc: Convert msi_irq to the array PCI: dwc: split MSI IRQ parsing/allocation to a separate function PCI: dwc: Handle MSIs routed to multiple GIC interrupts PCI: dwc: Implement special ISR handler for split MSI IRQ setup PCI: qcom: Handle MSIs routed to multiple GIC interrupts 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 | 12 +- drivers/pci/controller/dwc/pci-dra7xx.c | 2 +- drivers/pci/controller/dwc/pci-exynos.c | 2 +- .../pci/controller/dwc/pcie-designware-host.c | 220 +++++++++++++----- 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, 238 insertions(+), 73 deletions(-)