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@xxxxxx> 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@xxxxxxx>
>>>> ---
>>>> 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.
--
Jazz is not dead. It just smells funny...
