| Message ID | 20181113225734.8026-4-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: designware: Fixing MSI handling flow | expand |
On Tue, 2018-11-13 at 22:57 +0000, Marc Zyngier wrote: > The write to the status register is really an ACK for the HW, > and should be treated as such by the driver. Let's move it to the > irq_ack callback, which will prevent people from moving it around > in order to paper over other bugs. > > > static void dw_pci_bottom_ack(struct irq_data *d) > { > - struct msi_desc *msi = irq_data_get_msi_desc(d); > - struct pcie_port *pp; > + struct pcie_port *pp = irq_data_get_irq_chip_data(d); > + unsigned int res, bit, ctrl; > unsigned long flags; > > - pp = msi_desc_to_pci_sysdata(msi); > + ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL; > + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; > + bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL; > > raw_spin_lock_irqsave(&pp->lock, flags); > > + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit); Does this need to be inside the lock? The single write should allow for an atomic clear without need for a lock. > + > if (pp->ops->msi_irq_ack) > pp->ops->msi_irq_ack(d->hwirq, pp); And couldn't the lock be inside the if here, so that it is never taken if there is no platform specific handler?
On Tue, Nov 13, 2018 at 10:57:34PM +0000, Marc Zyngier wrote: > The write to the status register is really an ACK for the HW, > and should be treated as such by the driver. Let's move it to the > irq_ack callback, which will prevent people from moving it around > in order to paper over other bugs. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > drivers/pci/controller/dwc/pcie-designware-host.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 0a76948ed49e..f06e67c60593 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -99,9 +99,6 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp) > (i * MAX_MSI_IRQS_PER_CTRL) + > pos); > generic_handle_irq(irq); > - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + > - (i * MSI_REG_CTRL_BLOCK_SIZE), > - 4, 1 << pos); > pos++; > } > } > @@ -200,14 +197,18 @@ static void dw_pci_bottom_unmask(struct irq_data *data) > > static void dw_pci_bottom_ack(struct irq_data *d) > { > - struct msi_desc *msi = irq_data_get_msi_desc(d); > - struct pcie_port *pp; > + struct pcie_port *pp = irq_data_get_irq_chip_data(d); > + unsigned int res, bit, ctrl; > unsigned long flags; > > - pp = msi_desc_to_pci_sysdata(msi); > + ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL; > + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; > + bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL; > > raw_spin_lock_irqsave(&pp->lock, flags); > > + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit); > + > if (pp->ops->msi_irq_ack) > pp->ops->msi_irq_ack(d->hwirq, pp); > Tested-by: Niklas Cassel <niklas.cassel@linaro.org>
On 13/11/2018 22:57, Marc Zyngier wrote: > The write to the status register is really an ACK for the HW, > and should be treated as such by the driver. Let's move it to the > irq_ack callback, which will prevent people from moving it around > in order to paper over other bugs. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > drivers/pci/controller/dwc/pcie-designware-host.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 0a76948ed49e..f06e67c60593 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -99,9 +99,6 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp) > (i * MAX_MSI_IRQS_PER_CTRL) + > pos); > generic_handle_irq(irq); > - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + > - (i * MSI_REG_CTRL_BLOCK_SIZE), > - 4, 1 << pos); > pos++; > } > } > @@ -200,14 +197,18 @@ static void dw_pci_bottom_unmask(struct irq_data *data) > > static void dw_pci_bottom_ack(struct irq_data *d) > { > - struct msi_desc *msi = irq_data_get_msi_desc(d); > - struct pcie_port *pp; > + struct pcie_port *pp = irq_data_get_irq_chip_data(d); > + unsigned int res, bit, ctrl; > unsigned long flags; > > - pp = msi_desc_to_pci_sysdata(msi); > + ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL; > + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; > + bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL; > > raw_spin_lock_irqsave(&pp->lock, flags); > > + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit); > + > if (pp->ops->msi_irq_ack) > pp->ops->msi_irq_ack(d->hwirq, pp); > > Tested-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Hi, On 14/11/18 4:27 AM, Marc Zyngier wrote: > The write to the status register is really an ACK for the HW, > and should be treated as such by the driver. Let's move it to the > irq_ack callback, which will prevent people from moving it around > in order to paper over other bugs. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > drivers/pci/controller/dwc/pcie-designware-host.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 0a76948ed49e..f06e67c60593 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -99,9 +99,6 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp) > (i * MAX_MSI_IRQS_PER_CTRL) + > pos); > generic_handle_irq(irq); > - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + > - (i * MSI_REG_CTRL_BLOCK_SIZE), > - 4, 1 << pos); > pos++; > } > } > @@ -200,14 +197,18 @@ static void dw_pci_bottom_unmask(struct irq_data *data) > > static void dw_pci_bottom_ack(struct irq_data *d) > { > - struct msi_desc *msi = irq_data_get_msi_desc(d); > - struct pcie_port *pp; > + struct pcie_port *pp = irq_data_get_irq_chip_data(d); > + unsigned int res, bit, ctrl; > unsigned long flags; > > - pp = msi_desc_to_pci_sysdata(msi); > + ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL; > + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; > + bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL; > > raw_spin_lock_irqsave(&pp->lock, flags); > > + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit); This register should be written only if msi_irq_ack callback is not populated similar to other dw_pci_bottom_*() functions. Thanks Kishon
On Tue, 4 Dec 2018 15:50:32 +0530 Kishon Vijay Abraham I <kishon@ti.com> wrote: > Hi, > > On 14/11/18 4:27 AM, Marc Zyngier wrote: > > The write to the status register is really an ACK for the HW, > > and should be treated as such by the driver. Let's move it to the > > irq_ack callback, which will prevent people from moving it around > > in order to paper over other bugs. > > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > --- > > drivers/pci/controller/dwc/pcie-designware-host.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > > index 0a76948ed49e..f06e67c60593 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > @@ -99,9 +99,6 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp) > > (i * MAX_MSI_IRQS_PER_CTRL) + > > pos); > > generic_handle_irq(irq); > > - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + > > - (i * MSI_REG_CTRL_BLOCK_SIZE), > > - 4, 1 << pos); > > pos++; > > } > > } > > @@ -200,14 +197,18 @@ static void dw_pci_bottom_unmask(struct irq_data *data) > > > > static void dw_pci_bottom_ack(struct irq_data *d) > > { > > - struct msi_desc *msi = irq_data_get_msi_desc(d); > > - struct pcie_port *pp; > > + struct pcie_port *pp = irq_data_get_irq_chip_data(d); > > + unsigned int res, bit, ctrl; > > unsigned long flags; > > > > - pp = msi_desc_to_pci_sysdata(msi); > > + ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL; > > + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; > > + bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL; > > > > raw_spin_lock_irqsave(&pp->lock, flags); > > > > + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit); > > This register should be written only if msi_irq_ack callback is not populated > similar to other dw_pci_bottom_*() functions. Why? This was so far unconditionally written, and my understanding is that without this write, no further MSI can be delivered. What makes you think otherwise? M.
Hi Marc, On 04/12/18 7:15 PM, Marc Zyngier wrote: > On Tue, 4 Dec 2018 15:50:32 +0530 > Kishon Vijay Abraham I <kishon@ti.com> wrote: > >> Hi, >> >> On 14/11/18 4:27 AM, Marc Zyngier wrote: >>> The write to the status register is really an ACK for the HW, >>> and should be treated as such by the driver. Let's move it to the >>> irq_ack callback, which will prevent people from moving it around >>> in order to paper over other bugs. >>> >>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>> --- >>> drivers/pci/controller/dwc/pcie-designware-host.c | 13 +++++++------ >>> 1 file changed, 7 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c >>> index 0a76948ed49e..f06e67c60593 100644 >>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c >>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c >>> @@ -99,9 +99,6 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp) >>> (i * MAX_MSI_IRQS_PER_CTRL) + >>> pos); >>> generic_handle_irq(irq); >>> - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + >>> - (i * MSI_REG_CTRL_BLOCK_SIZE), >>> - 4, 1 << pos); >>> pos++; >>> } >>> } >>> @@ -200,14 +197,18 @@ static void dw_pci_bottom_unmask(struct irq_data *data) >>> >>> static void dw_pci_bottom_ack(struct irq_data *d) >>> { >>> - struct msi_desc *msi = irq_data_get_msi_desc(d); >>> - struct pcie_port *pp; >>> + struct pcie_port *pp = irq_data_get_irq_chip_data(d); >>> + unsigned int res, bit, ctrl; >>> unsigned long flags; >>> >>> - pp = msi_desc_to_pci_sysdata(msi); >>> + ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL; >>> + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; >>> + bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL; >>> >>> raw_spin_lock_irqsave(&pp->lock, flags); >>> >>> + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit); >> >> This register should be written only if msi_irq_ack callback is not populated >> similar to other dw_pci_bottom_*() functions. > > Why? This was so far unconditionally written, and my understanding is > that without this write, no further MSI can be delivered. Not all platforms invoke dw_handle_msi_irq() for handling MSI irq. Platforms that doesn't use the MSI functionality of Designware makes use of the various callbacks like msi_irq_ack, msi_host_init etc., Keystone has MSI controller in the Keystone wrapper, AM654 uses GIC ITS etc., The platforms that doesn't use MSI functionality of Designware doesn't have to write to Designware's MSI configuration registers. Thanks Kishon
On 07/12/2018 08:12, Kishon Vijay Abraham I wrote: > Hi Marc, > > On 04/12/18 7:15 PM, Marc Zyngier wrote: >> On Tue, 4 Dec 2018 15:50:32 +0530 >> Kishon Vijay Abraham I <kishon@ti.com> wrote: >> >>> Hi, >>> >>> On 14/11/18 4:27 AM, Marc Zyngier wrote: >>>> The write to the status register is really an ACK for the HW, >>>> and should be treated as such by the driver. Let's move it to the >>>> irq_ack callback, which will prevent people from moving it around >>>> in order to paper over other bugs. >>>> >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>>> --- >>>> drivers/pci/controller/dwc/pcie-designware-host.c | 13 +++++++------ >>>> 1 file changed, 7 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c >>>> index 0a76948ed49e..f06e67c60593 100644 >>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c >>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c >>>> @@ -99,9 +99,6 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp) >>>> (i * MAX_MSI_IRQS_PER_CTRL) + >>>> pos); >>>> generic_handle_irq(irq); >>>> - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + >>>> - (i * MSI_REG_CTRL_BLOCK_SIZE), >>>> - 4, 1 << pos); >>>> pos++; >>>> } >>>> } >>>> @@ -200,14 +197,18 @@ static void dw_pci_bottom_unmask(struct irq_data *data) >>>> >>>> static void dw_pci_bottom_ack(struct irq_data *d) >>>> { >>>> - struct msi_desc *msi = irq_data_get_msi_desc(d); >>>> - struct pcie_port *pp; >>>> + struct pcie_port *pp = irq_data_get_irq_chip_data(d); >>>> + unsigned int res, bit, ctrl; >>>> unsigned long flags; >>>> >>>> - pp = msi_desc_to_pci_sysdata(msi); >>>> + ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL; >>>> + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; >>>> + bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL; >>>> >>>> raw_spin_lock_irqsave(&pp->lock, flags); >>>> >>>> + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit); >>> >>> This register should be written only if msi_irq_ack callback is not populated >>> similar to other dw_pci_bottom_*() functions. >> >> Why? This was so far unconditionally written, and my understanding is >> that without this write, no further MSI can be delivered. > > Not all platforms invoke dw_handle_msi_irq() for handling MSI irq. > > Platforms that doesn't use the MSI functionality of Designware makes use of the > various callbacks like msi_irq_ack, msi_host_init etc., Keystone has MSI > controller in the Keystone wrapper, AM654 uses GIC ITS etc., > > The platforms that doesn't use MSI functionality of Designware doesn't have to > write to Designware's MSI configuration registers. Let's be clear: a platform that doesn't use the DW MSI functionality should never get anywhere this code. If they do, then that's a terrible bug, and it should be fixed by making the TI stuff standalone instead of calling into the internals. Frankly, this whole thing should be marked as BROKEN until it is sorted out for good. Thanks, M.
Hi, On 07/12/18 3:15 PM, Marc Zyngier wrote: > On 07/12/2018 08:12, Kishon Vijay Abraham I wrote: >> Hi Marc, >> >> On 04/12/18 7:15 PM, Marc Zyngier wrote: >>> On Tue, 4 Dec 2018 15:50:32 +0530 >>> Kishon Vijay Abraham I <kishon@ti.com> wrote: >>> >>>> Hi, >>>> >>>> On 14/11/18 4:27 AM, Marc Zyngier wrote: >>>>> The write to the status register is really an ACK for the HW, >>>>> and should be treated as such by the driver. Let's move it to the >>>>> irq_ack callback, which will prevent people from moving it around >>>>> in order to paper over other bugs. >>>>> >>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>>>> --- >>>>> drivers/pci/controller/dwc/pcie-designware-host.c | 13 +++++++------ >>>>> 1 file changed, 7 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c >>>>> index 0a76948ed49e..f06e67c60593 100644 >>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c >>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c >>>>> @@ -99,9 +99,6 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp) >>>>> (i * MAX_MSI_IRQS_PER_CTRL) + >>>>> pos); >>>>> generic_handle_irq(irq); >>>>> - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + >>>>> - (i * MSI_REG_CTRL_BLOCK_SIZE), >>>>> - 4, 1 << pos); >>>>> pos++; >>>>> } >>>>> } >>>>> @@ -200,14 +197,18 @@ static void dw_pci_bottom_unmask(struct irq_data *data) >>>>> >>>>> static void dw_pci_bottom_ack(struct irq_data *d) >>>>> { >>>>> - struct msi_desc *msi = irq_data_get_msi_desc(d); >>>>> - struct pcie_port *pp; >>>>> + struct pcie_port *pp = irq_data_get_irq_chip_data(d); >>>>> + unsigned int res, bit, ctrl; >>>>> unsigned long flags; >>>>> >>>>> - pp = msi_desc_to_pci_sysdata(msi); >>>>> + ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL; >>>>> + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; >>>>> + bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL; >>>>> >>>>> raw_spin_lock_irqsave(&pp->lock, flags); >>>>> >>>>> + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit); >>>> >>>> This register should be written only if msi_irq_ack callback is not populated >>>> similar to other dw_pci_bottom_*() functions. >>> >>> Why? This was so far unconditionally written, and my understanding is >>> that without this write, no further MSI can be delivered. >> >> Not all platforms invoke dw_handle_msi_irq() for handling MSI irq. >> >> Platforms that doesn't use the MSI functionality of Designware makes use of the >> various callbacks like msi_irq_ack, msi_host_init etc., Keystone has MSI >> controller in the Keystone wrapper, AM654 uses GIC ITS etc., >> >> The platforms that doesn't use MSI functionality of Designware doesn't have to >> write to Designware's MSI configuration registers. > > Let's be clear: a platform that doesn't use the DW MSI functionality > should never get anywhere this code. If they do, then that's a terrible > bug, and it should be fixed by making the TI stuff standalone instead of > calling into the internals. That makes sense to me. We can start by removing msi_set_irq, msi_clear_irq and msi_irq_ack callbacks from dw_pcie_host_ops. This functionality can be added directly in keystone driver. > > Frankly, this whole thing should be marked as BROKEN until it is sorted > out for good. Maybe remove those callbacks and make only Keystone broken? Thanks Kishon
On Fri, Dec 07, 2018 at 03:43:00PM +0530, Kishon Vijay Abraham I wrote: > Hi, > > On 07/12/18 3:15 PM, Marc Zyngier wrote: > > On 07/12/2018 08:12, Kishon Vijay Abraham I wrote: > >> Hi Marc, > >> > >> On 04/12/18 7:15 PM, Marc Zyngier wrote: > >>> On Tue, 4 Dec 2018 15:50:32 +0530 > >>> Kishon Vijay Abraham I <kishon@ti.com> wrote: > >>> > >>>> Hi, > >>>> > >>>> On 14/11/18 4:27 AM, Marc Zyngier wrote: > >>>>> The write to the status register is really an ACK for the HW, > >>>>> and should be treated as such by the driver. Let's move it to the > >>>>> irq_ack callback, which will prevent people from moving it around > >>>>> in order to paper over other bugs. > >>>>> > >>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > >>>>> --- > >>>>> drivers/pci/controller/dwc/pcie-designware-host.c | 13 +++++++------ > >>>>> 1 file changed, 7 insertions(+), 6 deletions(-) > >>>>> > >>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > >>>>> index 0a76948ed49e..f06e67c60593 100644 > >>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c > >>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > >>>>> @@ -99,9 +99,6 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp) > >>>>> (i * MAX_MSI_IRQS_PER_CTRL) + > >>>>> pos); > >>>>> generic_handle_irq(irq); > >>>>> - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + > >>>>> - (i * MSI_REG_CTRL_BLOCK_SIZE), > >>>>> - 4, 1 << pos); > >>>>> pos++; > >>>>> } > >>>>> } > >>>>> @@ -200,14 +197,18 @@ static void dw_pci_bottom_unmask(struct irq_data *data) > >>>>> > >>>>> static void dw_pci_bottom_ack(struct irq_data *d) > >>>>> { > >>>>> - struct msi_desc *msi = irq_data_get_msi_desc(d); > >>>>> - struct pcie_port *pp; > >>>>> + struct pcie_port *pp = irq_data_get_irq_chip_data(d); > >>>>> + unsigned int res, bit, ctrl; > >>>>> unsigned long flags; > >>>>> > >>>>> - pp = msi_desc_to_pci_sysdata(msi); > >>>>> + ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL; > >>>>> + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; > >>>>> + bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL; > >>>>> > >>>>> raw_spin_lock_irqsave(&pp->lock, flags); > >>>>> > >>>>> + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit); > >>>> > >>>> This register should be written only if msi_irq_ack callback is not populated > >>>> similar to other dw_pci_bottom_*() functions. > >>> > >>> Why? This was so far unconditionally written, and my understanding is > >>> that without this write, no further MSI can be delivered. > >> > >> Not all platforms invoke dw_handle_msi_irq() for handling MSI irq. > >> > >> Platforms that doesn't use the MSI functionality of Designware makes use of the > >> various callbacks like msi_irq_ack, msi_host_init etc., Keystone has MSI > >> controller in the Keystone wrapper, AM654 uses GIC ITS etc., > >> > >> The platforms that doesn't use MSI functionality of Designware doesn't have to > >> write to Designware's MSI configuration registers. > > > > Let's be clear: a platform that doesn't use the DW MSI functionality > > should never get anywhere this code. If they do, then that's a terrible > > bug, and it should be fixed by making the TI stuff standalone instead of > > calling into the internals. > > That makes sense to me. We can start by removing msi_set_irq, msi_clear_irq and > msi_irq_ack callbacks from dw_pcie_host_ops. > > This functionality can be added directly in keystone driver. > > > > Frankly, this whole thing should be marked as BROKEN until it is sorted > > out for good. > > Maybe remove those callbacks and make only Keystone broken? Hi Kishon, Murali, Is it possible to rework keystone as discussed on this thread on top of my pci/dwc-msi branch please ? Thanks, Lorenzo
Hi Lorenzo, On 11/12/18 6:05 PM, Lorenzo Pieralisi wrote: > On Fri, Dec 07, 2018 at 03:43:00PM +0530, Kishon Vijay Abraham I wrote: >> Hi, >> >> On 07/12/18 3:15 PM, Marc Zyngier wrote: >>> On 07/12/2018 08:12, Kishon Vijay Abraham I wrote: >>>> Hi Marc, >>>> >>>> On 04/12/18 7:15 PM, Marc Zyngier wrote: >>>>> On Tue, 4 Dec 2018 15:50:32 +0530 >>>>> Kishon Vijay Abraham I <kishon@ti.com> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> On 14/11/18 4:27 AM, Marc Zyngier wrote: >>>>>>> The write to the status register is really an ACK for the HW, >>>>>>> and should be treated as such by the driver. Let's move it to the >>>>>>> irq_ack callback, which will prevent people from moving it around >>>>>>> in order to paper over other bugs. >>>>>>> >>>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>>>>>> --- >>>>>>> drivers/pci/controller/dwc/pcie-designware-host.c | 13 +++++++------ >>>>>>> 1 file changed, 7 insertions(+), 6 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c >>>>>>> index 0a76948ed49e..f06e67c60593 100644 >>>>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c >>>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c >>>>>>> @@ -99,9 +99,6 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp) >>>>>>> (i * MAX_MSI_IRQS_PER_CTRL) + >>>>>>> pos); >>>>>>> generic_handle_irq(irq); >>>>>>> - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + >>>>>>> - (i * MSI_REG_CTRL_BLOCK_SIZE), >>>>>>> - 4, 1 << pos); >>>>>>> pos++; >>>>>>> } >>>>>>> } >>>>>>> @@ -200,14 +197,18 @@ static void dw_pci_bottom_unmask(struct irq_data *data) >>>>>>> >>>>>>> static void dw_pci_bottom_ack(struct irq_data *d) >>>>>>> { >>>>>>> - struct msi_desc *msi = irq_data_get_msi_desc(d); >>>>>>> - struct pcie_port *pp; >>>>>>> + struct pcie_port *pp = irq_data_get_irq_chip_data(d); >>>>>>> + unsigned int res, bit, ctrl; >>>>>>> unsigned long flags; >>>>>>> >>>>>>> - pp = msi_desc_to_pci_sysdata(msi); >>>>>>> + ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL; >>>>>>> + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; >>>>>>> + bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL; >>>>>>> >>>>>>> raw_spin_lock_irqsave(&pp->lock, flags); >>>>>>> >>>>>>> + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit); >>>>>> >>>>>> This register should be written only if msi_irq_ack callback is not populated >>>>>> similar to other dw_pci_bottom_*() functions. >>>>> >>>>> Why? This was so far unconditionally written, and my understanding is >>>>> that without this write, no further MSI can be delivered. >>>> >>>> Not all platforms invoke dw_handle_msi_irq() for handling MSI irq. >>>> >>>> Platforms that doesn't use the MSI functionality of Designware makes use of the >>>> various callbacks like msi_irq_ack, msi_host_init etc., Keystone has MSI >>>> controller in the Keystone wrapper, AM654 uses GIC ITS etc., >>>> >>>> The platforms that doesn't use MSI functionality of Designware doesn't have to >>>> write to Designware's MSI configuration registers. >>> >>> Let's be clear: a platform that doesn't use the DW MSI functionality >>> should never get anywhere this code. If they do, then that's a terrible >>> bug, and it should be fixed by making the TI stuff standalone instead of >>> calling into the internals. >> >> That makes sense to me. We can start by removing msi_set_irq, msi_clear_irq and >> msi_irq_ack callbacks from dw_pcie_host_ops. >> >> This functionality can be added directly in keystone driver. >>> >>> Frankly, this whole thing should be marked as BROKEN until it is sorted >>> out for good. >> >> Maybe remove those callbacks and make only Keystone broken? > > Hi Kishon, Murali, > > Is it possible to rework keystone as discussed on this thread on top of > my pci/dwc-msi branch please ? yeah, I'll work on that. Thanks Kishon
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 0a76948ed49e..f06e67c60593 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -99,9 +99,6 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp) (i * MAX_MSI_IRQS_PER_CTRL) + pos); generic_handle_irq(irq); - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + - (i * MSI_REG_CTRL_BLOCK_SIZE), - 4, 1 << pos); pos++; } } @@ -200,14 +197,18 @@ static void dw_pci_bottom_unmask(struct irq_data *data) static void dw_pci_bottom_ack(struct irq_data *d) { - struct msi_desc *msi = irq_data_get_msi_desc(d); - struct pcie_port *pp; + struct pcie_port *pp = irq_data_get_irq_chip_data(d); + unsigned int res, bit, ctrl; unsigned long flags; - pp = msi_desc_to_pci_sysdata(msi); + ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL; + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; + bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL; raw_spin_lock_irqsave(&pp->lock, flags); + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit); + if (pp->ops->msi_irq_ack) pp->ops->msi_irq_ack(d->hwirq, pp);
The write to the status register is really an ACK for the HW, and should be treated as such by the driver. Let's move it to the irq_ack callback, which will prevent people from moving it around in order to paper over other bugs. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- drivers/pci/controller/dwc/pcie-designware-host.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)