On Wed, 14 Nov 2018 22:25:37 +0000,
Trent Piepho <tpiepho@xxxxxxxxxx> wrote:
>
> On Wed, 2018-11-14 at 22:01 +0000, Marc Zyngier wrote:
> > On Wed, 14 Nov 2018 19:19:27 +0000,
> > Trent Piepho <tpiepho@xxxxxxxxxx> wrote:
> > >
> > > On Wed, 2018-11-14 at 09:54 +0000, Marc Zyngier wrote:
> > > > /* Initialize IRQ Status array */
> > > > - for (ctrl = 0; ctrl < num_ctrls; ctrl++)
> > > > - dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE +
> > > > + for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
> > > > + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK +
> > > > (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
> > > > - 4, &pp->irq_status[ctrl]);
> > > > + 4, ~0);
> > > > + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE +
> > > > + (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
> > > > + 4, ~0);
> > > > + pp->irq_status[ctrl] = 0;
> > > > + }
> > > >
> > >
> > > I tested yesterday before this patch was sent and fixed this issue
> > > another way. I pretty sure this would work as well, though it's not
> > > clear to me it's more correct.
> >
> > Given that we don't have a spec or any form of useful documentation
> > (except for the information that Gustavo gave us), I don't think
> > *anything* we'll write here has a remote chance of being
> > correct. We're simply poking in the dark.
>
> Should all MSIs start enabled, or should they start disabled and be
> enabled via the irq_enable method of the irq_chip, seems like a Linux
> design decision to me. Decide that, then try to figure out how to make
> the hardware do what Linux expects it to do.
>
> Starting disabled seems like the right design to me. So here's my
> attempt to make the driver do this. Works in my tests. I've not
> tracked down all uses of irq_status outside the driver to determine how
> it's supposed to work.
>
> From dfc015f9821f5105cbcf9686d360105ffbac4ffb Mon Sep 17 00:00:00 2001
> From: Trent Piepho <tpiepho@xxxxxxxxxx>
> Date: Wed, 14 Nov 2018 14:12:47 -0800
> Subject: [PATCH] PCI: dwc: Allow enabling MSIs and start with disabled
>
> Add irq_enable callbacks to let MSIs be enabled.
>
> Previously the driver would leave any MSIs enabled when it initialized
> that way. Rather than that, disable them all.
>
> Signed-off-by: Trent Piepho <tpiepho@xxxxxxxxxx>
> ---
> drivers/pci/controller/dwc/pcie-designware-host.c | 37
> ++++++++++++++++++++---
> 1 file changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> b/drivers/pci/controller/dwc/pcie-designware-host.c
> index f06e67c60593..e7770fb1ced8 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -61,11 +61,17 @@ static void dw_msi_unmask_irq(struct irq_data *d)
> irq_chip_unmask_parent(d);
> }
>
> +static void dw_msi_enable_irq(struct irq_data *d)
> +{
> + irq_chip_enable_parent(d);
> +}
> +
> static struct irq_chip dw_pcie_msi_irq_chip = {
> .name = "PCI-MSI",
> .irq_ack = dw_msi_ack_irq,
> .irq_mask = dw_msi_mask_irq,
> .irq_unmask = dw_msi_unmask_irq,
> + .irq_enable = dw_msi_enable_irq,
If you're doing that, please implement both enable *and* disable.
> };
>
> static struct msi_domain_info dw_pcie_msi_domain_info = {
> @@ -215,6 +221,26 @@ static void dw_pci_bottom_ack(struct irq_data *d)
> raw_spin_unlock_irqrestore(&pp->lock, flags);
> }
>
> +static void dw_pci_bottom_enable(struct irq_data *data)
> +{
> + struct pcie_port *pp = irq_data_get_irq_chip_data(data);
> + unsigned int res, bit, ctrl;
> + unsigned long flags;
> + u32 enable;
> +
> + ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
> + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
> + bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
> +
> + raw_spin_lock_irqsave(&pp->lock, flags);
> +
> + dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4,
> &enable);
> + enable |= BIT(bit);
> + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4,
> enable);
> +
> + raw_spin_unlock_irqrestore(&pp->lock, flags);
> +}
How does it work for drivers that use the callbacks stuff?
M.
--
Jazz is not dead, it just smell funny.
