lkml.org 
[lkml]   [2019]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 2/9] PCI: keystone: Modify legacy_irq_handler to check the IRQ_STATUS of INTA/B/C/D
From
Date
Hi Lorenzo,

On 07/02/19 9:45 PM, Lorenzo Pieralisi wrote:
> On Thu, Feb 07, 2019 at 04:39:17PM +0530, Kishon Vijay Abraham I wrote:
>> The legacy interrupt handler directly checks the IRQ_STATUS register
>> corresponding to a interrupt line inorder to invoke generic_handle_irq.
>>
>> While this is okay for K2G platform which has separate interrupt line for
>> each of the 4 legacy interrupts, AM654 which uses the same PCIe wrapper
>> has a single interrupt line for all the legacy interrupts. So for AM654
>> the interrupt handler won't be able to directly check the IRQ_STATUS
>> register corresponding to the interrupt line.
>>
>> Also the legacy interrupt handler uses 'virq' obtained from
>> irq_of_parse_and_map to find the correct interrupt line which raised the
>> interrupt. There is no guarantee that virq assigned for contiguous hardware
>> irq will be contiguous and the interrupt handler might end up checking
>> the wrong IRQ_STATUS register.
>>
>> In order to overcome the above issues, read the IRQ_STATUS register of
>> all the 4 legacy interrupts to determine which interrupt was raised.
>>
>> Link: https://lkml.kernel.org/r/bb081d21-7c03-0357-4294-7e92d95d838c@arm.com
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>> drivers/pci/controller/dwc/pci-keystone.c | 22 ++++++++++++----------
>> 1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>> index 5286a480f76b..4cf9849d5a1d 100644
>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>> @@ -214,16 +214,11 @@ static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie,
>> {
>> struct dw_pcie *pci = ks_pcie->pci;
>> struct device *dev = pci->dev;
>> - u32 pending;
>> int virq;
>>
>> - pending = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(offset));
>> -
>> - if (BIT(0) & pending) {
>> - virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
>> - dev_dbg(dev, ": irq: irq_offset %d, virq %d\n", offset, virq);
>> - generic_handle_irq(virq);
>> - }
>> + virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
>> + dev_dbg(dev, ": irq: irq_offset %d, virq %d\n", offset, virq);
>> + generic_handle_irq(virq);
>>
>> /* EOI the INTx interrupt */
>> ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset);
>> @@ -607,8 +602,9 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
>> struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
>> struct dw_pcie *pci = ks_pcie->pci;
>> struct device *dev = pci->dev;
>> - u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
>> struct irq_chip *chip = irq_desc_get_chip(desc);
>> + unsigned int irq_no;
>> + u32 reg;
>>
>> dev_dbg(dev, ": Handling legacy irq %d\n", irq);
>>
>> @@ -618,7 +614,13 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
>> * ack operation.
>> */
>> chained_irq_enter(chip, desc);
>> - ks_pcie_handle_legacy_irq(ks_pcie, irq_offset);
>> + for (irq_no = 0; irq_no < PCI_NUM_INTX; irq_no++) {
>
> I understand the aim of this code but now on platforms where there
> is a 1:1 relationship between Linux IRQ and INTX this loop has
> steps carried out for nothing.
>
> If I understand the code correctly what this code does is force
> looping over INTX status regs regardless of what linux IRQ number was
> actually active.

right. This driver is used by 2 platforms K2G and AM654 (The patches are there
on the list). K2G has 4 interrupt lines for each of the 4 legacy interrups
while AM654 has a single interrupt line. One of the purpose of this patch is to
have a single legacy interrupt handler for both K2G and AM654.
>
> You could do something faster by creating a matrix LinuxIRQ x INTx to
> detect what INTx status register should actually be checked.
>
> This seems overkill to me but it is not that complicated to implement
> and may clarify the code (and avoid reading up to three registers for
> nothing on the IRQ code path, which can make things faster too).

Agreed. But that's not possible for AM654 which has a single interrupt line and
all the registers has to be read to identify the interrupt source.

Thanks
Kishon

\
 
 \ /
  Last update: 2019-02-08 05:33    [W:0.186 / U:0.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site