lkml.org 
[lkml]   [2019]   [Feb]   [8]   [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 Bjorn,

On 08/02/19 2:22 AM, Bjorn Helgaas wrote:
> In the subject, "legacy_irq_handler" looks like it's supposed to be a
> function name, but it's not. Maybe something like:
>
> PCI: keystone: Check INTA/B/C/D IRQ_STATUS in ks_pcie_legacy_irq_handler()
>
> 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.
>
> s/The legacy interrupt handler/ks_pcie_handle_legacy_irq()/ ?
> s/to a/to an/
> s/inorder/in order/
> s/generic_handle_irq/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.
>
> s/platform which/platform, which/
> s/separate interrupt line/separate interrupt lines/
> s/AM654 which/AM654, which/
> s/PCIe wrapper/PCIe wrapper,/
> s/interrupt line for all/interrupt line shared by all/
>
>
>> 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.
>
> s/irq_of_parse_and_map/irq_of_parse_and_map()
> s/irq will/IRQ will/
>
>> 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++) {
>> + reg = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(irq_no));
>> + if (!(reg & INTx_EN))
>> + continue;
>> + ks_pcie_handle_legacy_irq(ks_pcie, irq_no);
>
> It's too bad that reading IRQ_STATUS and writing IRQ_EOI are now in
> separate functions. It's nice to have them together for code auditing
> purposes.
>
> Could maybe accumulate a mask of which INTx bits are set and call
> ks_pcie_handle_legacy_irq() only once with that mask? Of course, then
> you'd need another loop in ks_pcie_handle_legacy_irq().

Patch "5" of this series "PCI: keystone: Cleanup ks_pcie_msi_irq_handler and
ks_pcie_legacy_irq_handler" does more cleanup in irq handler and there
ks_pcie_handle_legacy_irq is removed and everything is done in a single function.

I have to anyway revisit legacy irq handler and have some of the register
writes move to proper irq_chip callbacks as Lorenzo commented in [1]

[1] -> https://lkml.org/lkml/2019/1/24/333

Thanks
Kishon

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