diff mbox series

[3/3] PCI: designware: Move interrupt acking into the proper callback

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

Commit Message

Marc Zyngier Nov. 13, 2018, 10:57 p.m. UTC
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(-)

Comments

Trent Piepho Nov. 14, 2018, 7:01 p.m. UTC | #1
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?
Niklas Cassel Dec. 3, 2018, 6:02 p.m. UTC | #2
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>
Gustavo Pimentel Dec. 4, 2018, 9:41 a.m. UTC | #3
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>
Kishon Vijay Abraham I Dec. 4, 2018, 10:20 a.m. UTC | #4
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
Marc Zyngier Dec. 4, 2018, 1:45 p.m. UTC | #5
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.
Kishon Vijay Abraham I Dec. 7, 2018, 8:12 a.m. UTC | #6
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
Marc Zyngier Dec. 7, 2018, 9:45 a.m. UTC | #7
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.
Kishon Vijay Abraham I Dec. 7, 2018, 10:13 a.m. UTC | #8
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
Lorenzo Pieralisi Dec. 11, 2018, 12:35 p.m. UTC | #9
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
Kishon Vijay Abraham I Dec. 12, 2018, 5:54 a.m. UTC | #10
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 mbox series

Patch

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);