On Thu, Nov 22, 2018 at 12:03:25PM +0000, Gustavo Pimentel wrote:
>
> Hi all,
>
>
> >
> > I just realised (at 1am!) that the first patch is awfully buggy:
> > - ENABLE and MASK have opposite polarities
> > - There is nothing initialising the ENABLE and MASK registers
> >
> > I've stashed the following fix-up on top of the existing series:
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index f06e67c60593..0fa9e8fdce66 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -166,7 +166,7 @@ static void dw_pci_bottom_mask(struct irq_data *data)
>
> (snip)
>
> I used your patch and made it more perceptible in my opinion, (basically I
> transformed the variable irq_mask (previous irq_status) into a mirror of MASK
> registers, which removed the need for the *NOT* operation and forced the mask
> operation swap in the callbacks)
>
> Lorenzo can you apply this fix-up in your branch test/pci-dwc-msi? Maybe merging
> with eca44651920c ("PCI: designware: Move interrupt acking into the proper
> callback"), perhaps.
>
> I've tested Marc's patch series (with and without my fix-up and both) with a
> NVme EP that generates a burst of 19 MSI-X interrupts. Both approaches are
> working fine.
I want to see more testing before getting this series merged upstream,
especially for the platform configurations on which this bug was
reported.
I am a bit annoyed with DWC platform maintainers in this regard.
> Just a couple of suggestions Lorenzo, maybe you could exchange the *designware*
> by *dwc* on all patch series titles and on eca44651920c("PCI: designware: Move
> interrupt acking into the proper callback") replace *acking* by *ACKing* like
> previous patch has.
>
> Marc thanks for this patch fix! :)
>
> Tested-by: Gustavo Pimentel <gustavo.pimentel@xxxxxxxxxxxx>
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 0fa9e8f..a5132b3 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -164,9 +164,9 @@ static void dw_pci_bottom_mask(struct irq_data *data)
> res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
> bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
>
> - pp->irq_status[ctrl] &= ~(1 << bit);
> + pp->irq_mask[ctrl] |= BIT(bit);
> dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4,
> - ~pp->irq_status[ctrl]);
> + pp->irq_mask[ctrl]);
> }
>
> raw_spin_unlock_irqrestore(&pp->lock, flags);
> @@ -187,30 +187,30 @@ static void dw_pci_bottom_unmask(struct irq_data *data)
> res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
> bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
>
> - pp->irq_status[ctrl] |= 1 << bit;
> + pp->irq_mask[ctrl] &= ~BIT(bit);
> dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4,
> - ~pp->irq_status[ctrl]);
> + pp->irq_mask[ctrl]);
> }
>
> raw_spin_unlock_irqrestore(&pp->lock, flags);
> }
>
> -static void dw_pci_bottom_ack(struct irq_data *d)
> +static void dw_pci_bottom_ack(struct irq_data *data)
> {
> - struct pcie_port *pp = irq_data_get_irq_chip_data(d);
> + struct pcie_port *pp = irq_data_get_irq_chip_data(data);
> unsigned int res, bit, ctrl;
> unsigned long flags;
>
> - ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL;
> + ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
> res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
> - bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL;
> + bit = data->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);
> + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, BIT(bit));
>
> if (pp->ops->msi_irq_ack)
> - pp->ops->msi_irq_ack(d->hwirq, pp);
> + pp->ops->msi_irq_ack(data->hwirq, pp);
Changes in this hunk are unrelated, I won't squash them in.
Lorenzo
>
> raw_spin_unlock_irqrestore(&pp->lock, flags);
> }
> @@ -665,13 +665,13 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>
> /* Initialize IRQ Status array */
> for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
> + pp->irq_mask[ctrl] = ~0;
> dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK +
> (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
> - 4, ~0);
> + 4, pp->irq_mask[ctrl]);
> dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE +
> (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
> 4, ~0);
> - pp->irq_status[ctrl] = 0;
> }
>
> /* Setup RC BARs */
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> b/drivers/pci/controller/dwc/pcie-designware.h
> index 0989d88..9d1614a 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -169,7 +169,7 @@ struct pcie_port {
> struct irq_domain *msi_domain;
> dma_addr_t msi_data;
> u32 num_vectors;
> - u32 irq_status[MAX_MSI_CTRLS];
> + u32 irq_mask[MAX_MSI_CTRLS];
> raw_spinlock_t lock;
> DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
> };
>
