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(-) -- 2.35.1
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 high MSI vectors are not delivered on tested platforms. Additional research pointed to a patch in msm-4.14 ([1]), which describes that each group of MSI vectors is mapped to the separate interrupt. Without these changes specifying num_vectors can lead to missing MSI interrupts and thus to devices malfunction. Fixes: 20f1bfb8dd62 ("PCI: qcom: Add support for handling MSIs from 8 endpoints") Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Reviewed-by: Rob Herring <robh@kernel.org> --- drivers/pci/controller/dwc/pcie-qcom.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index f9a61ad6d1f0..2e5464edc36e 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -1587,7 +1587,6 @@ static int qcom_pcie_probe(struct platform_device *pdev) pci->dev = dev; pci->ops = &dw_pcie_ops; pp = &pci->pp; - pp->num_vectors = MAX_MSI_IRQS; pcie->pci = pci; -- 2.35.1
The subdrivers pass -ESOMETHING if they do not want the core to touch MSI IRQ. dw_pcie_host_init() also checks if (msi_irq > 0) rather than just if (msi_irq). So let's make dw_pcie_free_msi() also check that msi_irq is greater than zero. Reviewed-by: Rob Herring <robh@kernel.org> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/pci/controller/dwc/pcie-designware-host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 2fa86f32d964..43d1d6116007 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -257,7 +257,7 @@ int dw_pcie_allocate_domains(struct pcie_port *pp) static void dw_pcie_free_msi(struct pcie_port *pp) { - if (pp->msi_irq) + if (pp->msi_irq > 0) irq_set_chained_handler_and_data(pp->msi_irq, NULL, NULL); irq_domain_remove(pp->msi_domain); -- 2.35.1
Qualcomm version of DWC PCIe controller supports more than 32 MSI interrupts, but they are routed to separate interrupts in groups of 32 vectors. To support such configuration, change the msi_irq field into an array. Let the DWC core handle all interrupts that were set in this array. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/pci/controller/dwc/pci-dra7xx.c | 2 +- drivers/pci/controller/dwc/pci-exynos.c | 2 +- .../pci/controller/dwc/pcie-designware-host.c | 30 +++++++++++-------- drivers/pci/controller/dwc/pcie-designware.h | 2 +- drivers/pci/controller/dwc/pcie-keembay.c | 2 +- drivers/pci/controller/dwc/pcie-spear13xx.c | 2 +- drivers/pci/controller/dwc/pcie-tegra194.c | 2 +- 7 files changed, 24 insertions(+), 18 deletions(-) diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c index dfcdeb432dc8..0919c96dcdbd 100644 --- a/drivers/pci/controller/dwc/pci-dra7xx.c +++ b/drivers/pci/controller/dwc/pci-dra7xx.c @@ -483,7 +483,7 @@ static int dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, return pp->irq; /* MSI IRQ is muxed */ - pp->msi_irq = -ENODEV; + pp->msi_irq[0] = -ENODEV; ret = dra7xx_pcie_init_irq_domain(pp); if (ret < 0) diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c index 467c8d1cd7e4..4f2010bd9cd7 100644 --- a/drivers/pci/controller/dwc/pci-exynos.c +++ b/drivers/pci/controller/dwc/pci-exynos.c @@ -292,7 +292,7 @@ static int exynos_add_pcie_port(struct exynos_pcie *ep, } pp->ops = &exynos_pcie_host_ops; - pp->msi_irq = -ENODEV; + pp->msi_irq[0] = -ENODEV; ret = dw_pcie_host_init(pp); if (ret) { diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 43d1d6116007..0d4f35c7560e 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -257,8 +257,11 @@ int dw_pcie_allocate_domains(struct pcie_port *pp) static void dw_pcie_free_msi(struct pcie_port *pp) { - if (pp->msi_irq > 0) - irq_set_chained_handler_and_data(pp->msi_irq, NULL, NULL); + u32 ctrl; + + for (ctrl = 0; ctrl < MAX_MSI_CTRLS; ctrl++) + if (pp->msi_irq[ctrl] > 0) + irq_set_chained_handler_and_data(pp->msi_irq[ctrl], NULL, NULL); irq_domain_remove(pp->msi_domain); irq_domain_remove(pp->irq_domain); @@ -368,13 +371,15 @@ int dw_pcie_host_init(struct pcie_port *pp) for (ctrl = 0; ctrl < num_ctrls; ctrl++) pp->irq_mask[ctrl] = ~0; - if (!pp->msi_irq) { - pp->msi_irq = platform_get_irq_byname_optional(pdev, "msi"); - if (pp->msi_irq < 0) { - pp->msi_irq = platform_get_irq(pdev, 0); - if (pp->msi_irq < 0) - return pp->msi_irq; + 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; } + pp->msi_irq[0] = irq; } pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip; @@ -383,10 +388,11 @@ int dw_pcie_host_init(struct pcie_port *pp) if (ret) return ret; - if (pp->msi_irq > 0) - irq_set_chained_handler_and_data(pp->msi_irq, - dw_chained_msi_isr, - pp); + for (ctrl = 0; ctrl < num_ctrls; ctrl++) + if (pp->msi_irq[ctrl] > 0) + irq_set_chained_handler_and_data(pp->msi_irq[ctrl], + dw_chained_msi_isr, + pp); ret = dma_set_mask(pci->dev, DMA_BIT_MASK(32)); if (ret) diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 7d6e9b7576be..9c1a38b0a6b3 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -187,7 +187,7 @@ struct pcie_port { u32 io_size; int irq; const struct dw_pcie_host_ops *ops; - int msi_irq; + int msi_irq[MAX_MSI_CTRLS]; struct irq_domain *irq_domain; struct irq_domain *msi_domain; u16 msi_msg; diff --git a/drivers/pci/controller/dwc/pcie-keembay.c b/drivers/pci/controller/dwc/pcie-keembay.c index 1ac29a6eef22..297e6e926c00 100644 --- a/drivers/pci/controller/dwc/pcie-keembay.c +++ b/drivers/pci/controller/dwc/pcie-keembay.c @@ -338,7 +338,7 @@ static int keembay_pcie_add_pcie_port(struct keembay_pcie *pcie, int ret; pp->ops = &keembay_pcie_host_ops; - pp->msi_irq = -ENODEV; + pp->msi_irq[0] = -ENODEV; ret = keembay_pcie_setup_msi_irq(pcie); if (ret) diff --git a/drivers/pci/controller/dwc/pcie-spear13xx.c b/drivers/pci/controller/dwc/pcie-spear13xx.c index 1569e82b5568..cc7776833810 100644 --- a/drivers/pci/controller/dwc/pcie-spear13xx.c +++ b/drivers/pci/controller/dwc/pcie-spear13xx.c @@ -172,7 +172,7 @@ static int spear13xx_add_pcie_port(struct spear13xx_pcie *spear13xx_pcie, } pp->ops = &spear13xx_pcie_host_ops; - pp->msi_irq = -ENODEV; + pp->msi_irq[0] = -ENODEV; ret = dw_pcie_host_init(pp); if (ret) { diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c index b1b5f836a806..e75712db85b0 100644 --- a/drivers/pci/controller/dwc/pcie-tegra194.c +++ b/drivers/pci/controller/dwc/pcie-tegra194.c @@ -2271,7 +2271,7 @@ static void tegra194_pcie_shutdown(struct platform_device *pdev) disable_irq(pcie->pci.pp.irq); if (IS_ENABLED(CONFIG_PCI_MSI)) - disable_irq(pcie->pci.pp.msi_irq); + disable_irq(pcie->pci.pp.msi_irq[0]); tegra194_pcie_pme_turnoff(pcie); tegra_pcie_unconfig_controller(pcie); -- 2.35.1
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 0d4f35c7560e..5f6590929319 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -402,8 +402,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
Split handling of MSI host IRQs to a separate dw_pcie_msi_host_init() function. The code is complex enough to warrant a separte function. Adding another bit to support multiple host MSI IRQs would make it overcomplicated. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- .../pci/controller/dwc/pcie-designware-host.c | 97 +++++++++++-------- 1 file changed, 55 insertions(+), 42 deletions(-) diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 5f6590929319..6b0c7b75391f 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -288,6 +288,59 @@ 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 int dw_pcie_msi_host_init(struct pcie_port *pp) +{ + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); + struct platform_device *pdev = to_platform_device(pci->dev); + int ret; + u32 ctrl, num_ctrls; + + num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL; + for (ctrl = 0; ctrl < num_ctrls; ctrl++) + pp->irq_mask[ctrl] = ~0; + + 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; + } + pp->msi_irq[0] = irq; + } + + pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip; + + ret = dw_pcie_allocate_domains(pp); + if (ret) + return ret; + + for (ctrl = 0; ctrl < num_ctrls; ctrl++) + if (pp->msi_irq[ctrl] > 0) + irq_set_chained_handler_and_data(pp->msi_irq[ctrl], + dw_chained_msi_isr, + pp); + + ret = dma_set_mask(pci->dev, DMA_BIT_MASK(32)); + if (ret) + dev_warn(pci->dev, "Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n"); + + pp->msi_data = dma_map_single_attrs(pci->dev, &pp->msi_msg, + sizeof(pp->msi_msg), + DMA_FROM_DEVICE, + DMA_ATTR_SKIP_CPU_SYNC); + 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; + dw_pcie_free_msi(pp); + return ret; + } + + return 0; +} + int dw_pcie_host_init(struct pcie_port *pp) { struct dw_pcie *pci = to_dw_pcie_from_pp(pp); @@ -365,49 +418,9 @@ int dw_pcie_host_init(struct pcie_port *pp) if (ret < 0) return ret; } else if (pp->has_msi_ctrl) { - u32 ctrl, num_ctrls; - - num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL; - for (ctrl = 0; ctrl < num_ctrls; ctrl++) - pp->irq_mask[ctrl] = ~0; - - 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; - } - pp->msi_irq[0] = irq; - } - - pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip; - - ret = dw_pcie_allocate_domains(pp); - if (ret) + ret = dw_pcie_msi_host_init(pp); + if (ret < 0) return ret; - - for (ctrl = 0; ctrl < num_ctrls; ctrl++) - if (pp->msi_irq[ctrl] > 0) - irq_set_chained_handler_and_data(pp->msi_irq[ctrl], - dw_chained_msi_isr, - pp); - - ret = dma_set_mask(pci->dev, DMA_BIT_MASK(32)); - if (ret) - dev_warn(pci->dev, "Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n"); - - pp->msi_data = dma_map_single_attrs(pci->dev, &pp->msi_msg, - sizeof(pp->msi_msg), - DMA_FROM_DEVICE, - DMA_ATTR_SKIP_CPU_SYNC); - 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
On some of Qualcomm platforms each group of 32 MSI vectors is routed to the separate GIC interrupt. Implement support for such configuraions by parsing "msi0" ... "msi7" 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 | 33 ++++++++++++++++++- drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 6b0c7b75391f..258bafa306dc 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -291,7 +291,8 @@ static void dw_pcie_msi_init(struct pcie_port *pp) static int dw_pcie_msi_host_init(struct pcie_port *pp) { struct dw_pcie *pci = to_dw_pcie_from_pp(pp); - struct platform_device *pdev = to_platform_device(pci->dev); + struct device *dev = pci->dev; + struct platform_device *pdev = to_platform_device(dev); int ret; u32 ctrl, num_ctrls; @@ -299,6 +300,36 @@ 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) { + char irq_name[] = "msiXX"; + int irq; + + if (!pp->msi_irq[0]) { + irq = platform_get_irq_byname_optional(pdev, irq_name); + if (irq == -ENXIO) { + num_ctrls = 1; + pp->num_vectors = min((u32)MAX_MSI_IRQS_PER_CTRL, pp->num_vectors); + dev_warn(dev, "No additional MSI IRQs, limiting amount of MSI vectors to %d\n", + pp->num_vectors); + } else { + pp->msi_irq[0] = irq; + } + } + + /* If we fallback to the single MSI ctrl IRQ, this loop will be skipped as num_ctrls is 1 */ + for (ctrl = 1; ctrl < num_ctrls; ctrl++) { + if (pp->msi_irq[ctrl]) + continue; + + snprintf(irq_name, sizeof(irq_name), "msi%d", ctrl); + irq = platform_get_irq_byname(pdev, irq_name); + if (irq < 0) + return irq; + + pp->msi_irq[ctrl] = irq; + } + } + if (!pp->msi_irq[0]) { int irq = platform_get_irq_byname_optional(pdev, "msi"); 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; -- 2.35.1
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, }; static const struct qcom_pcie_cfg ipq8074_cfg = { @@ -1514,6 +1516,7 @@ static const struct qcom_pcie_cfg ipq4019_cfg = { static const struct qcom_pcie_cfg sdm845_cfg = { .ops = &ops_2_7_0, + .has_split_msi_irq = true, .has_tbu_clk = true, }; @@ -1526,12 +1529,14 @@ static const struct qcom_pcie_cfg sm8150_cfg = { static const struct qcom_pcie_cfg sm8250_cfg = { .ops = &ops_1_9_0, + .has_split_msi_irq = true, .has_tbu_clk = true, .has_ddrss_sf_tbu_clk = true, }; static const struct qcom_pcie_cfg sm8450_pcie0_cfg = { .ops = &ops_1_9_0, + .has_split_msi_irq = true, .has_ddrss_sf_tbu_clk = true, .pipe_clk_need_muxing = true, .has_aggre0_clk = true, @@ -1540,6 +1545,7 @@ static const struct qcom_pcie_cfg sm8450_pcie0_cfg = { static const struct qcom_pcie_cfg sm8450_pcie1_cfg = { .ops = &ops_1_9_0, + .has_split_msi_irq = true, .has_ddrss_sf_tbu_clk = true, .pipe_clk_need_muxing = true, .has_aggre1_clk = true, @@ -1547,6 +1553,7 @@ static const struct qcom_pcie_cfg sm8450_pcie1_cfg = { static const struct qcom_pcie_cfg sc7280_cfg = { .ops = &ops_1_9_0, + .has_split_msi_irq = true, .has_tbu_clk = true, .pipe_clk_need_muxing = 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); -- 2.35.1
If the PCIe DWC controller uses split MSI IRQs for reporting MSI vectors, it is possible to detect, which group triggered the interrupt. Provide an optimized version of MSI ISR handler that will handle just a single MSI group instead of handling all of them. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- .../pci/controller/dwc/pcie-designware-host.c | 83 ++++++++++++++----- 1 file changed, 62 insertions(+), 21 deletions(-) diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 258bafa306dc..b1b8d924ea8c 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -52,34 +52,42 @@ static struct msi_domain_info dw_pcie_msi_domain_info = { .chip = &dw_pcie_msi_irq_chip, }; +static inline irqreturn_t dw_handle_single_msi_group(struct pcie_port *pp, int i) +{ + int pos; + unsigned long val; + u32 status; + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); + + status = dw_pcie_readl_dbi(pci, PCIE_MSI_INTR0_STATUS + + (i * MSI_REG_CTRL_BLOCK_SIZE)); + if (!status) + return IRQ_NONE; + + val = status; + pos = 0; + while ((pos = find_next_bit(&val, MAX_MSI_IRQS_PER_CTRL, + pos)) != MAX_MSI_IRQS_PER_CTRL) { + generic_handle_domain_irq(pp->irq_domain, + (i * MAX_MSI_IRQS_PER_CTRL) + + pos); + pos++; + } + + return IRQ_HANDLED; +} + /* MSI int handler */ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp) { - int i, pos; - unsigned long val; - u32 status, num_ctrls; + int i; + u32 num_ctrls; irqreturn_t ret = IRQ_NONE; - struct dw_pcie *pci = to_dw_pcie_from_pp(pp); num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL; - for (i = 0; i < num_ctrls; i++) { - status = dw_pcie_readl_dbi(pci, PCIE_MSI_INTR0_STATUS + - (i * MSI_REG_CTRL_BLOCK_SIZE)); - if (!status) - continue; - - ret = IRQ_HANDLED; - val = status; - pos = 0; - while ((pos = find_next_bit(&val, MAX_MSI_IRQS_PER_CTRL, - pos)) != MAX_MSI_IRQS_PER_CTRL) { - generic_handle_domain_irq(pp->irq_domain, - (i * MAX_MSI_IRQS_PER_CTRL) + - pos); - pos++; - } - } + for (i = 0; i < num_ctrls; i++) + ret |= dw_handle_single_msi_group(pp, i); return ret; } @@ -98,6 +106,38 @@ static void dw_chained_msi_isr(struct irq_desc *desc) chained_irq_exit(chip, desc); } +static void dw_split_msi_isr(struct irq_desc *desc) +{ + struct irq_chip *chip = irq_desc_get_chip(desc); + int irq = irq_desc_get_irq(desc); + struct pcie_port *pp; + int i; + u32 num_ctrls; + struct dw_pcie *pci; + + chained_irq_enter(chip, desc); + + pp = irq_desc_get_handler_data(desc); + pci = to_dw_pcie_from_pp(pp); + + /* + * Unlike generic dw_handle_msi_irq(), we can determine which group of + * MSIs triggered the IRQ, so process just that group. + */ + num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL; + + for (i = 0; i < num_ctrls; i++) { + if (pp->msi_irq[i] == irq) { + dw_handle_single_msi_group(pp, i); + break; + } + } + + WARN_ON_ONCE(i == num_ctrls); + + chained_irq_exit(chip, desc); +} + static void dw_pci_setup_msi_msg(struct irq_data *d, struct msi_msg *msg) { struct pcie_port *pp = irq_data_get_irq_chip_data(d); @@ -350,6 +390,7 @@ static int dw_pcie_msi_host_init(struct pcie_port *pp) for (ctrl = 0; ctrl < num_ctrls; ctrl++) if (pp->msi_irq[ctrl] > 0) irq_set_chained_handler_and_data(pp->msi_irq[ctrl], + pp->has_split_msi_irq ? dw_split_msi_isr : dw_chained_msi_isr, pp); -- 2.35.1
On Qualcomm platforms each group of 32 MSI vectors is routed to the separate GIC interrupt. Document mapping of additional interrupts. Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- .../devicetree/bindings/pci/qcom,pcie.yaml | 53 +++++++++++++++++-- 1 file changed, 50 insertions(+), 3 deletions(-) diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml index 0b69b12b849e..fe8f9a62a665 100644 --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml @@ -43,11 +43,12 @@ properties: maxItems: 5 interrupts: - maxItems: 1 + minItems: 1 + maxItems: 8 interrupt-names: - items: - - const: msi + minItems: 1 + maxItems: 8 # Common definitions for clocks, clock-names and reset. # Platform constraints are described later. @@ -623,6 +624,52 @@ allOf: - resets - reset-names + # On newer chipsets support either 1 or 8 msi interrupts + # On older chipsets it's always 1 msi interrupt + - if: + properties: + compatibles: + contains: + enum: + - qcom,pcie-msm8996 + - qcom,pcie-sc7280 + - qcom,pcie-sc8180x + - qcom,pcie-sdm845 + - qcom,pcie-sm8150 + - qcom,pcie-sm8250 + - qcom,pcie-sm8450-pcie0 + - qcom,pcie-sm8450-pcie1 + then: + oneOf: + - properties: + interrupts: + maxItems: 1 + interrupt-names: + maxItems: 1 + items: + - const: msi + - 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 + else: + properties: + interrupts: + maxItems: 1 + interrupt-names: + items: + - const: msi + unevaluatedProperties: false examples: -- 2.35.1
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 | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi index 410272a1e19b..ef683a2f7412 100644 --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi @@ -1807,8 +1807,15 @@ 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 = "msi", "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 */ -- 2.35.1
On Thu, May 12, 2022 at 01:45:36PM +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 high MSI vectors are not > delivered on tested platforms. Additional research pointed to > a patch in msm-4.14 ([1]), which describes that each group of MSI > vectors is mapped to the separate interrupt. > > Without these changes specifying num_vectors can lead to missing MSI > interrupts and thus to devices malfunction. > > Fixes: 20f1bfb8dd62 ("PCI: qcom: Add support for handling MSIs from 8 endpoints") Hopefully Lorenzo will drop both 20f1bfb8dd62 and this patch if/when he applies this series. This commit log would need some rework because [1] and [2] are mentioned in the cover letter, but that doesn't get included when the series is applied. Best if we can just avoid all this confusion by dropping 20f1bfb8dd62 and this patch. > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Reviewed-by: Rob Herring <robh@kernel.org> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index f9a61ad6d1f0..2e5464edc36e 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -1587,7 +1587,6 @@ static int qcom_pcie_probe(struct platform_device *pdev) > pci->dev = dev; > pci->ops = &dw_pcie_ops; > pp = &pci->pp; > - pp->num_vectors = MAX_MSI_IRQS; > > pcie->pci = pci; > > -- > 2.35.1 >
Only if you have other occasion to repost, in subject: PCI: dwc: Split On Thu, May 12, 2022 at 01:45:40PM +0300, Dmitry Baryshkov wrote: > Split handling of MSI host IRQs to a separate dw_pcie_msi_host_init() > function. The code is complex enough to warrant a separte function. s/separte/separate/ It looks like this is simply a split, with no expected functional impact. The above is sufficient; the sentence below is not really necessary. > Adding another bit to support multiple host MSI IRQs would make it > overcomplicated. > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > .../pci/controller/dwc/pcie-designware-host.c | 97 +++++++++++-------- > 1 file changed, 55 insertions(+), 42 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 5f6590929319..6b0c7b75391f 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -288,6 +288,59 @@ 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 int dw_pcie_msi_host_init(struct pcie_port *pp) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + struct platform_device *pdev = to_platform_device(pci->dev); > + int ret; > + u32 ctrl, num_ctrls; > + > + num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL; > + for (ctrl = 0; ctrl < num_ctrls; ctrl++) > + pp->irq_mask[ctrl] = ~0; > + > + 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; > + } > + pp->msi_irq[0] = irq; > + } > + > + pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip; > + > + ret = dw_pcie_allocate_domains(pp); > + if (ret) > + return ret; > + > + for (ctrl = 0; ctrl < num_ctrls; ctrl++) > + if (pp->msi_irq[ctrl] > 0) > + irq_set_chained_handler_and_data(pp->msi_irq[ctrl], > + dw_chained_msi_isr, > + pp); > + > + ret = dma_set_mask(pci->dev, DMA_BIT_MASK(32)); > + if (ret) > + dev_warn(pci->dev, "Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n"); > + > + pp->msi_data = dma_map_single_attrs(pci->dev, &pp->msi_msg, > + sizeof(pp->msi_msg), > + DMA_FROM_DEVICE, > + DMA_ATTR_SKIP_CPU_SYNC); > + 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; > + dw_pcie_free_msi(pp); > + return ret; > + } > + > + return 0; > +} > + > int dw_pcie_host_init(struct pcie_port *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > @@ -365,49 +418,9 @@ int dw_pcie_host_init(struct pcie_port *pp) > if (ret < 0) > return ret; > } else if (pp->has_msi_ctrl) { > - u32 ctrl, num_ctrls; > - > - num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL; > - for (ctrl = 0; ctrl < num_ctrls; ctrl++) > - pp->irq_mask[ctrl] = ~0; > - > - 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; > - } > - pp->msi_irq[0] = irq; > - } > - > - pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip; > - > - ret = dw_pcie_allocate_domains(pp); > - if (ret) > + ret = dw_pcie_msi_host_init(pp); > + if (ret < 0) > return ret; > - > - for (ctrl = 0; ctrl < num_ctrls; ctrl++) > - if (pp->msi_irq[ctrl] > 0) > - irq_set_chained_handler_and_data(pp->msi_irq[ctrl], > - dw_chained_msi_isr, > - pp); > - > - ret = dma_set_mask(pci->dev, DMA_BIT_MASK(32)); > - if (ret) > - dev_warn(pci->dev, "Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n"); > - > - pp->msi_data = dma_map_single_attrs(pci->dev, &pp->msi_msg, > - sizeof(pp->msi_msg), > - DMA_FROM_DEVICE, > - DMA_ATTR_SKIP_CPU_SYNC); > - 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 >
On Thu, May 12, 2022 at 01:45:41PM +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 configuraions by > parsing "msi0" ... "msi7" interrupts and attaching them to the chained > handler. Again, only if you have some other reason to repost: s/configuraions/configurations/ > 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 | 33 ++++++++++++++++++- > drivers/pci/controller/dwc/pcie-designware.h | 1 + > 2 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 6b0c7b75391f..258bafa306dc 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -291,7 +291,8 @@ static void dw_pcie_msi_init(struct pcie_port *pp) > static int dw_pcie_msi_host_init(struct pcie_port *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > - struct platform_device *pdev = to_platform_device(pci->dev); > + struct device *dev = pci->dev; > + struct platform_device *pdev = to_platform_device(dev); > int ret; > u32 ctrl, num_ctrls; > > @@ -299,6 +300,36 @@ 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) { > + char irq_name[] = "msiXX"; > + int irq; > + > + if (!pp->msi_irq[0]) { > + irq = platform_get_irq_byname_optional(pdev, irq_name); > + if (irq == -ENXIO) { > + num_ctrls = 1; > + pp->num_vectors = min((u32)MAX_MSI_IRQS_PER_CTRL, pp->num_vectors); > + dev_warn(dev, "No additional MSI IRQs, limiting amount of MSI vectors to %d\n", > + pp->num_vectors); > + } else { > + pp->msi_irq[0] = irq; > + } > + } > + > + /* If we fallback to the single MSI ctrl IRQ, this loop will be skipped as num_ctrls is 1 */ > + for (ctrl = 1; ctrl < num_ctrls; ctrl++) { > + if (pp->msi_irq[ctrl]) > + continue; > + > + snprintf(irq_name, sizeof(irq_name), "msi%d", ctrl); > + irq = platform_get_irq_byname(pdev, irq_name); > + if (irq < 0) > + return irq; > + > + pp->msi_irq[ctrl] = irq; > + } > + } > + > if (!pp->msi_irq[0]) { > int irq = platform_get_irq_byname_optional(pdev, "msi"); > > 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; > -- > 2.35.1 >
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.
--
With best wishes
Dmitry
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
--
With best wishes
Dmitry
On Thu, May 12, 2022 at 01:45:41PM +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 configuraions by > parsing "msi0" ... "msi7" 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 | 33 ++++++++++++++++++- > drivers/pci/controller/dwc/pcie-designware.h | 1 + > 2 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 6b0c7b75391f..258bafa306dc 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -291,7 +291,8 @@ static void dw_pcie_msi_init(struct pcie_port *pp) > static int dw_pcie_msi_host_init(struct pcie_port *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > - struct platform_device *pdev = to_platform_device(pci->dev); > + struct device *dev = pci->dev; > + struct platform_device *pdev = to_platform_device(dev); > int ret; > u32 ctrl, num_ctrls; > > @@ -299,6 +300,36 @@ 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) { > + char irq_name[] = "msiXX"; > + int irq; > + > + if (!pp->msi_irq[0]) { > + irq = platform_get_irq_byname_optional(pdev, irq_name); This looks broken; you're requesting "msiXX", not "msi0". > + if (irq == -ENXIO) { > + num_ctrls = 1; > + pp->num_vectors = min((u32)MAX_MSI_IRQS_PER_CTRL, pp->num_vectors); > + dev_warn(dev, "No additional MSI IRQs, limiting amount of MSI vectors to %d\n", > + pp->num_vectors); > + } else { > + pp->msi_irq[0] = irq; > + } > + } > + > + /* If we fallback to the single MSI ctrl IRQ, this loop will be skipped as num_ctrls is 1 */ > + for (ctrl = 1; ctrl < num_ctrls; ctrl++) { > + if (pp->msi_irq[ctrl]) > + continue; > + > + snprintf(irq_name, sizeof(irq_name), "msi%d", ctrl); > + irq = platform_get_irq_byname(pdev, irq_name); > + if (irq < 0) > + return irq; > + > + pp->msi_irq[ctrl] = irq; > + } > + } > + > if (!pp->msi_irq[0]) { > int irq = platform_get_irq_byname_optional(pdev, "msi"); Johan
On Thu, May 12, 2022 at 01:45:45PM +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 | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > index 410272a1e19b..ef683a2f7412 100644 > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > @@ -1807,8 +1807,15 @@ 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 = "msi", "msi1", "msi2", "msi3", "msi4", "msi5", "msi6", "msi7"; You must use "msi0" instead of "msi" or you only get 32 MSI regardless of what follows currently (and this wouldn't pass DT validation either). > #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 */ Johan
On Fri, 13 May 2022 at 14:52, Johan Hovold <johan@kernel.org> wrote: > > On Thu, May 12, 2022 at 01:45:41PM +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 configuraions by > > parsing "msi0" ... "msi7" 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 | 33 ++++++++++++++++++- > > drivers/pci/controller/dwc/pcie-designware.h | 1 + > > 2 files changed, 33 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > > index 6b0c7b75391f..258bafa306dc 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > @@ -291,7 +291,8 @@ static void dw_pcie_msi_init(struct pcie_port *pp) > > static int dw_pcie_msi_host_init(struct pcie_port *pp) > > { > > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > > - struct platform_device *pdev = to_platform_device(pci->dev); > > + struct device *dev = pci->dev; > > + struct platform_device *pdev = to_platform_device(dev); > > int ret; > > u32 ctrl, num_ctrls; > > > > @@ -299,6 +300,36 @@ 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) { > > + char irq_name[] = "msiXX"; > > + int irq; > > + > > + if (!pp->msi_irq[0]) { > > + irq = platform_get_irq_byname_optional(pdev, irq_name); > > This looks broken; you're requesting "msiXX", not "msi0". Yes, I'm preparing the updated patch. > > > + if (irq == -ENXIO) { > > + num_ctrls = 1; > > + pp->num_vectors = min((u32)MAX_MSI_IRQS_PER_CTRL, pp->num_vectors); > > + dev_warn(dev, "No additional MSI IRQs, limiting amount of MSI vectors to %d\n", > > + pp->num_vectors); > > + } else { > > + pp->msi_irq[0] = irq; > > + } > > + } > > + > > + /* If we fallback to the single MSI ctrl IRQ, this loop will be skipped as num_ctrls is 1 */ > > + for (ctrl = 1; ctrl < num_ctrls; ctrl++) { > > + if (pp->msi_irq[ctrl]) > > + continue; > > + > > + snprintf(irq_name, sizeof(irq_name), "msi%d", ctrl); > > + irq = platform_get_irq_byname(pdev, irq_name); > > + if (irq < 0) > > + return irq; > > + > > + pp->msi_irq[ctrl] = irq; > > + } > > + } > > + > > if (!pp->msi_irq[0]) { > > int irq = platform_get_irq_byname_optional(pdev, "msi"); > > Johan -- With best wishes Dmitry
On Fri, 13 May 2022 at 14:55, Johan Hovold <johan@kernel.org> wrote: > > On Thu, May 12, 2022 at 01:45:45PM +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 | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > > index 410272a1e19b..ef683a2f7412 100644 > > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > > @@ -1807,8 +1807,15 @@ 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 = "msi", "msi1", "msi2", "msi3", "msi4", "msi5", "msi6", "msi7"; > > You must use "msi0" instead of "msi" or you only get 32 MSI regardless > of what follows currently (and this wouldn't pass DT validation either). Yes. And that's why I didn't notice that I broke msi0 parsing. > > > #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 */ > > Johan -- With best wishes Dmitry
On Thu, May 12, 2022 at 01:45:41PM +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 configuraions by > parsing "msi0" ... "msi7" 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 | 33 ++++++++++++++++++- > drivers/pci/controller/dwc/pcie-designware.h | 1 + > 2 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 6b0c7b75391f..258bafa306dc 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -291,7 +291,8 @@ static void dw_pcie_msi_init(struct pcie_port *pp) > static int dw_pcie_msi_host_init(struct pcie_port *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > - struct platform_device *pdev = to_platform_device(pci->dev); > + struct device *dev = pci->dev; > + struct platform_device *pdev = to_platform_device(dev); > int ret; > u32 ctrl, num_ctrls; > > @@ -299,6 +300,36 @@ 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) { > + char irq_name[] = "msiXX"; > + int irq; > + > + if (!pp->msi_irq[0]) { > + irq = platform_get_irq_byname_optional(pdev, irq_name); > + if (irq == -ENXIO) { > + num_ctrls = 1; > + pp->num_vectors = min((u32)MAX_MSI_IRQS_PER_CTRL, pp->num_vectors); min_t()? > + dev_warn(dev, "No additional MSI IRQs, limiting amount of MSI vectors to %d\n", > + pp->num_vectors); We already print the number of vectors used a bit further down in this function, no need to repeat it here. This will also be printed when booting with devicetrees which only have a single "msi" interrupt (which isn't deprecated). Perhaps a debug printk is sufficient, or at least something less verbose. > + } else { > + pp->msi_irq[0] = irq; > + } > + } > + > + /* If we fallback to the single MSI ctrl IRQ, this loop will be skipped as num_ctrls is 1 */ Please break lines at 80 columns unless not doing so improves readability. > + for (ctrl = 1; ctrl < num_ctrls; ctrl++) { > + if (pp->msi_irq[ctrl]) > + continue; > + > + snprintf(irq_name, sizeof(irq_name), "msi%d", ctrl); > + irq = platform_get_irq_byname(pdev, irq_name); > + if (irq < 0) > + return irq; > + > + pp->msi_irq[ctrl] = irq; > + } > + } > + > if (!pp->msi_irq[0]) { > int irq = platform_get_irq_byname_optional(pdev, "msi"); Johan
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 -- With best wishes Dmitry
On Thu, May 12, 2022 at 01:45:42PM +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; > @@ -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; > + } If all qcom platform that can support more than 32 MSI require multiple IRQs, how about adding num_vectors to the config instead and set pp->has_split_msi_irq when cfg->num_vectors is set (or unconditionally if you remove the corresponding warning you just added to the dwc host code)? At least some sc8280xp seem to only support 128 MSI (using 4 IRQs). > + > pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH); > if (IS_ERR(pcie->reset)) { > ret = PTR_ERR(pcie->reset); Johan
On 13/05/2022 15:42, Johan Hovold wrote: > On Thu, May 12, 2022 at 01:45:42PM +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; > >> @@ -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; >> + } > > If all qcom platform that can support more than 32 MSI require multiple > IRQs, how about adding num_vectors to the config instead and set > pp->has_split_msi_irq when cfg->num_vectors is set (or unconditionally > if you remove the corresponding warning you just added to the dwc host > code)? > > At least some sc8280xp seem to only support 128 MSI (using 4 IRQs). Nice idea, let's do this. > >> + >> pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH); >> if (IS_ERR(pcie->reset)) { >> ret = PTR_ERR(pcie->reset); > > Johan -- With best wishes Dmitry
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 Fri, May 13, 2022 at 03:48:09PM +0300, Dmitry Baryshkov wrote: > On 13/05/2022 15:42, Johan Hovold wrote: > > If all qcom platform that can support more than 32 MSI require multiple > > IRQs, how about adding num_vectors to the config instead and set > > pp->has_split_msi_irq when cfg->num_vectors is set (or unconditionally > > if you remove the corresponding warning you just added to the dwc host > > code)? > > > > At least some sc8280xp seem to only support 128 MSI (using 4 IRQs). > > Nice idea, let's do this. If at all possible it would be even better to just infer it from the devicetree to avoid having to describe things in two places, though. 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.
--
With best wishes
Dmitry
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. -- With best wishes Dmitry
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