| Message ID | 20220520183114.1356599-4-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: qcom: Fix higher MSI vectors handling | expand |
On Fri, May 20, 2022 at 09:31:10PM +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 | 58 +++++++++++++++++-- > 1 file changed, 54 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index a076abe6611c..381bc24d5715 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -288,6 +288,43 @@ 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_parse_split_msi_irq(struct pcie_port *pp) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + struct device *dev = pci->dev; > + struct platform_device *pdev = to_platform_device(dev); > + int irq; > + u32 ctrl; > + > + irq = platform_get_irq_byname_optional(pdev, split_msi_names[0]); > + if (irq == -ENXIO) > + return -ENXIO; You still need to check for other errors and -EPROBE_DEFER here. > + > + pp->msi_irq[0] = irq; > + > + /* Parse as many IRQs as described in the DTS. */ s/DTS/devicetree/ > + for (ctrl = 1; ctrl < MAX_MSI_CTRLS; ctrl++) { > + irq = platform_get_irq_byname_optional(pdev, split_msi_names[ctrl]); > + if (irq == -ENXIO) > + break; > + 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; > + } > + > + pp->num_vectors = ctrl * MAX_MSI_IRQS_PER_CTRL; > + > + return 0; > +} > + > static int dw_pcie_msi_host_init(struct pcie_port *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > @@ -295,22 +332,34 @@ static int dw_pcie_msi_host_init(struct pcie_port *pp) > struct platform_device *pdev = to_platform_device(dev); > int ret; > u32 ctrl, num_ctrls; > + bool has_split_msi_irq = false; This one should go in the follow-on patch that starts using it. > - num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL; > - for (ctrl = 0; ctrl < num_ctrls; ctrl++) > + for (ctrl = 0; ctrl < MAX_MSI_CTRLS; ctrl++) > pp->irq_mask[ctrl] = ~0; > > + if (!pp->msi_irq[0]) { > + ret = dw_pcie_parse_split_msi_irq(pp); > + if (ret < 0 && ret != -ENXIO) > + return ret; > + } > + > + if (!pp->num_vectors) > + pp->num_vectors = MSI_DEF_NUM_VECTORS; This works, but now you override num_vectors unconditionally when using split msis (and not just when num_vectors is set to zero). Is it work allowing to use num_vectors as a maximum as in previous versions (if only for consistency)? > + num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL; > + > 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; > } > > + dev_dbg(dev, "Using %d MSI vectors\n", pp->num_vectors); > + > pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip; > > ret = dw_pcie_allocate_domains(pp); > @@ -407,7 +456,8 @@ int dw_pcie_host_init(struct pcie_port *pp) > of_property_read_bool(np, "msi-parent") || > of_property_read_bool(np, "msi-map")); > > - if (!pp->num_vectors) { > + /* for the has_msi_ctrl the default assignment is handled inside dw_pcie_msi_host_init() */ > + if (!pp->has_msi_ctrl && !pp->num_vectors) { > pp->num_vectors = MSI_DEF_NUM_VECTORS; > } else if (pp->num_vectors > MAX_MSI_IRQS) { > dev_err(dev, "Invalid number of vectors\n"); Johan
On 23/05/2022 10:53, Johan Hovold wrote: > On Fri, May 20, 2022 at 09:31:10PM +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 | 58 +++++++++++++++++-- >> 1 file changed, 54 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c >> index a076abe6611c..381bc24d5715 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware-host.c >> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c >> @@ -288,6 +288,43 @@ 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_parse_split_msi_irq(struct pcie_port *pp) >> +{ >> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >> + struct device *dev = pci->dev; >> + struct platform_device *pdev = to_platform_device(dev); >> + int irq; >> + u32 ctrl; >> + >> + irq = platform_get_irq_byname_optional(pdev, split_msi_names[0]); >> + if (irq == -ENXIO) >> + return -ENXIO; > > You still need to check for other errors and -EPROBE_DEFER here. I think even the if (irq < 0) return irq; will work here. > >> + >> + pp->msi_irq[0] = irq; >> + >> + /* Parse as many IRQs as described in the DTS. */ > > s/DTS/devicetree/ > >> + for (ctrl = 1; ctrl < MAX_MSI_CTRLS; ctrl++) { >> + irq = platform_get_irq_byname_optional(pdev, split_msi_names[ctrl]); >> + if (irq == -ENXIO) >> + break; >> + 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; >> + } >> + >> + pp->num_vectors = ctrl * MAX_MSI_IRQS_PER_CTRL; >> + >> + return 0; >> +} >> + >> static int dw_pcie_msi_host_init(struct pcie_port *pp) >> { >> struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >> @@ -295,22 +332,34 @@ static int dw_pcie_msi_host_init(struct pcie_port *pp) >> struct platform_device *pdev = to_platform_device(dev); >> int ret; >> u32 ctrl, num_ctrls; >> + bool has_split_msi_irq = false; > > This one should go in the follow-on patch that starts using it. > >> - num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL; >> - for (ctrl = 0; ctrl < num_ctrls; ctrl++) >> + for (ctrl = 0; ctrl < MAX_MSI_CTRLS; ctrl++) >> pp->irq_mask[ctrl] = ~0; >> >> + if (!pp->msi_irq[0]) { >> + ret = dw_pcie_parse_split_msi_irq(pp); >> + if (ret < 0 && ret != -ENXIO) >> + return ret; >> + } >> + >> + if (!pp->num_vectors) >> + pp->num_vectors = MSI_DEF_NUM_VECTORS; > > This works, but now you override num_vectors unconditionally when using > split msis (and not just when num_vectors is set to zero) > > Is it work allowing to use num_vectors as a maximum as in previous > versions (if only for consistency)? Let me take a look. > >> + num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL; >> + >> 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; >> } >> >> + dev_dbg(dev, "Using %d MSI vectors\n", pp->num_vectors); >> + >> pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip; >> >> ret = dw_pcie_allocate_domains(pp); >> @@ -407,7 +456,8 @@ int dw_pcie_host_init(struct pcie_port *pp) >> of_property_read_bool(np, "msi-parent") || >> of_property_read_bool(np, "msi-map")); >> >> - if (!pp->num_vectors) { >> + /* for the has_msi_ctrl the default assignment is handled inside dw_pcie_msi_host_init() */ >> + if (!pp->has_msi_ctrl && !pp->num_vectors) { >> pp->num_vectors = MSI_DEF_NUM_VECTORS; >> } else if (pp->num_vectors > MAX_MSI_IRQS) { >> dev_err(dev, "Invalid number of vectors\n"); > > Johan
On Mon, May 23, 2022 at 04:39:56PM +0300, Dmitry Baryshkov wrote: > On 23/05/2022 10:53, Johan Hovold wrote: > > On Fri, May 20, 2022 at 09:31:10PM +0300, Dmitry Baryshkov wrote: > >> +static int dw_pcie_parse_split_msi_irq(struct pcie_port *pp) > >> +{ > >> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > >> + struct device *dev = pci->dev; > >> + struct platform_device *pdev = to_platform_device(dev); > >> + int irq; > >> + u32 ctrl; > >> + > >> + irq = platform_get_irq_byname_optional(pdev, split_msi_names[0]); > >> + if (irq == -ENXIO) > >> + return -ENXIO; > > > > You still need to check for other errors and -EPROBE_DEFER here. > > I think even the if (irq < 0) return irq; will work here. No need to print errors unless -EPROBEDEFER as you do below? > >> + > >> + pp->msi_irq[0] = irq; > >> + > >> + /* Parse as many IRQs as described in the DTS. */ > > > > s/DTS/devicetree/ > > > >> + for (ctrl = 1; ctrl < MAX_MSI_CTRLS; ctrl++) { > >> + irq = platform_get_irq_byname_optional(pdev, split_msi_names[ctrl]); > >> + if (irq == -ENXIO) > >> + break; > >> + 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; > >> + } > >> + > >> + pp->num_vectors = ctrl * MAX_MSI_IRQS_PER_CTRL; > >> + > >> + return 0; > >> +} Johan
On 23/05/2022 17:02, Johan Hovold wrote: > On Mon, May 23, 2022 at 04:39:56PM +0300, Dmitry Baryshkov wrote: >> On 23/05/2022 10:53, Johan Hovold wrote: >>> On Fri, May 20, 2022 at 09:31:10PM +0300, Dmitry Baryshkov wrote: > >>>> +static int dw_pcie_parse_split_msi_irq(struct pcie_port *pp) >>>> +{ >>>> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >>>> + struct device *dev = pci->dev; >>>> + struct platform_device *pdev = to_platform_device(dev); >>>> + int irq; >>>> + u32 ctrl; >>>> + >>>> + irq = platform_get_irq_byname_optional(pdev, split_msi_names[0]); >>>> + if (irq == -ENXIO) >>>> + return -ENXIO; >>> >>> You still need to check for other errors and -EPROBE_DEFER here. >> >> I think even the if (irq < 0) return irq; will work here. > > No need to print errors unless -EPROBEDEFER as you do below? There is no separate print for the dw_pcie_parse_split_msi_irq() errors. > >>>> + >>>> + pp->msi_irq[0] = irq; >>>> + >>>> + /* Parse as many IRQs as described in the DTS. */ >>> >>> s/DTS/devicetree/ >>> >>>> + for (ctrl = 1; ctrl < MAX_MSI_CTRLS; ctrl++) { >>>> + irq = platform_get_irq_byname_optional(pdev, split_msi_names[ctrl]); >>>> + if (irq == -ENXIO) >>>> + break; >>>> + 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; >>>> + } >>>> + >>>> + pp->num_vectors = ctrl * MAX_MSI_IRQS_PER_CTRL; >>>> + >>>> + return 0; >>>> +} > > Johan
On Mon, May 23, 2022 at 06:17:19PM +0300, Dmitry Baryshkov wrote: > On 23/05/2022 17:02, Johan Hovold wrote: > > On Mon, May 23, 2022 at 04:39:56PM +0300, Dmitry Baryshkov wrote: > >> On 23/05/2022 10:53, Johan Hovold wrote: > >>> On Fri, May 20, 2022 at 09:31:10PM +0300, Dmitry Baryshkov wrote: > > > >>>> +static int dw_pcie_parse_split_msi_irq(struct pcie_port *pp) > >>>> +{ > >>>> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > >>>> + struct device *dev = pci->dev; > >>>> + struct platform_device *pdev = to_platform_device(dev); > >>>> + int irq; > >>>> + u32 ctrl; > >>>> + > >>>> + irq = platform_get_irq_byname_optional(pdev, split_msi_names[0]); > >>>> + if (irq == -ENXIO) > >>>> + return -ENXIO; > >>> > >>> You still need to check for other errors and -EPROBE_DEFER here. > >> > >> I think even the if (irq < 0) return irq; will work here. > > > > No need to print errors unless -EPROBEDEFER as you do below? > > There is no separate print for the dw_pcie_parse_split_msi_irq() errors. I don't understand what you're referring to here. My question is: Why would you not be printing error messages for msi0 as you are for msi1..msi7 in the loop below. > >>>> + > >>>> + pp->msi_irq[0] = irq; > >>>> + > >>>> + /* Parse as many IRQs as described in the DTS. */ > >>> > >>> s/DTS/devicetree/ > >>> > >>>> + for (ctrl = 1; ctrl < MAX_MSI_CTRLS; ctrl++) { > >>>> + irq = platform_get_irq_byname_optional(pdev, split_msi_names[ctrl]); > >>>> + if (irq == -ENXIO) > >>>> + break; > >>>> + 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; > >>>> + } > >>>> + > >>>> + pp->num_vectors = ctrl * MAX_MSI_IRQS_PER_CTRL; > >>>> + > >>>> + return 0; > >>>> +} Johan
On 23/05/2022 18:32, Johan Hovold wrote: > On Mon, May 23, 2022 at 06:17:19PM +0300, Dmitry Baryshkov wrote: >> On 23/05/2022 17:02, Johan Hovold wrote: >>> On Mon, May 23, 2022 at 04:39:56PM +0300, Dmitry Baryshkov wrote: >>>> On 23/05/2022 10:53, Johan Hovold wrote: >>>>> On Fri, May 20, 2022 at 09:31:10PM +0300, Dmitry Baryshkov wrote: >>> >>>>>> +static int dw_pcie_parse_split_msi_irq(struct pcie_port *pp) >>>>>> +{ >>>>>> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >>>>>> + struct device *dev = pci->dev; >>>>>> + struct platform_device *pdev = to_platform_device(dev); >>>>>> + int irq; >>>>>> + u32 ctrl; >>>>>> + >>>>>> + irq = platform_get_irq_byname_optional(pdev, split_msi_names[0]); >>>>>> + if (irq == -ENXIO) >>>>>> + return -ENXIO; >>>>> >>>>> You still need to check for other errors and -EPROBE_DEFER here. >>>> >>>> I think even the if (irq < 0) return irq; will work here. >>> >>> No need to print errors unless -EPROBEDEFER as you do below? >> >> There is no separate print for the dw_pcie_parse_split_msi_irq() errors. > > I don't understand what you're referring to here. > > My question is: Why would you not be printing error messages for msi0 as > you are for msi1..msi7 in the loop below. Yeah, this seems like a correct idea. Thank you! > >>>>>> + >>>>>> + pp->msi_irq[0] = irq; >>>>>> + >>>>>> + /* Parse as many IRQs as described in the DTS. */ >>>>> >>>>> s/DTS/devicetree/ >>>>> >>>>>> + for (ctrl = 1; ctrl < MAX_MSI_CTRLS; ctrl++) { >>>>>> + irq = platform_get_irq_byname_optional(pdev, split_msi_names[ctrl]); >>>>>> + if (irq == -ENXIO) >>>>>> + break; >>>>>> + 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; >>>>>> + } >>>>>> + >>>>>> + pp->num_vectors = ctrl * MAX_MSI_IRQS_PER_CTRL; >>>>>> + >>>>>> + return 0; >>>>>> +} > > Johan
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index a076abe6611c..381bc24d5715 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -288,6 +288,43 @@ 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_parse_split_msi_irq(struct pcie_port *pp) +{ + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); + struct device *dev = pci->dev; + struct platform_device *pdev = to_platform_device(dev); + int irq; + u32 ctrl; + + irq = platform_get_irq_byname_optional(pdev, split_msi_names[0]); + if (irq == -ENXIO) + return -ENXIO; + + pp->msi_irq[0] = irq; + + /* Parse as many IRQs as described in the DTS. */ + for (ctrl = 1; ctrl < MAX_MSI_CTRLS; ctrl++) { + irq = platform_get_irq_byname_optional(pdev, split_msi_names[ctrl]); + if (irq == -ENXIO) + break; + 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; + } + + pp->num_vectors = ctrl * MAX_MSI_IRQS_PER_CTRL; + + return 0; +} + static int dw_pcie_msi_host_init(struct pcie_port *pp) { struct dw_pcie *pci = to_dw_pcie_from_pp(pp); @@ -295,22 +332,34 @@ static int dw_pcie_msi_host_init(struct pcie_port *pp) struct platform_device *pdev = to_platform_device(dev); int ret; u32 ctrl, num_ctrls; + bool has_split_msi_irq = false; - num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL; - for (ctrl = 0; ctrl < num_ctrls; ctrl++) + for (ctrl = 0; ctrl < MAX_MSI_CTRLS; ctrl++) pp->irq_mask[ctrl] = ~0; + if (!pp->msi_irq[0]) { + ret = dw_pcie_parse_split_msi_irq(pp); + if (ret < 0 && ret != -ENXIO) + return ret; + } + + if (!pp->num_vectors) + pp->num_vectors = MSI_DEF_NUM_VECTORS; + num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL; + 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; } + dev_dbg(dev, "Using %d MSI vectors\n", pp->num_vectors); + pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip; ret = dw_pcie_allocate_domains(pp); @@ -407,7 +456,8 @@ int dw_pcie_host_init(struct pcie_port *pp) of_property_read_bool(np, "msi-parent") || of_property_read_bool(np, "msi-map")); - if (!pp->num_vectors) { + /* for the has_msi_ctrl the default assignment is handled inside dw_pcie_msi_host_init() */ + if (!pp->has_msi_ctrl && !pp->num_vectors) { pp->num_vectors = MSI_DEF_NUM_VECTORS; } else if (pp->num_vectors > MAX_MSI_IRQS) { dev_err(dev, "Invalid number of vectors\n");
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 | 58 +++++++++++++++++-- 1 file changed, 54 insertions(+), 4 deletions(-)