Re: [PATCH 2/6] PCI: iproc: Add INTx support with better modeling

[Ray Jui]

On 6/12/2018 1:52 AM, poza@xxxxxxxxxxxxxx wrote:
> On 2018-05-30 03:28, Ray Jui wrote:
> > Add PCIe legacy interrupt INTx support to the iProc PCIe driver by
> > modeling it with its own IRQ domain. All 4 interrupts INTA, INTB, INTC,
> > INTD share the same interrupt line connected to the GIC in the system,
> > while the status of each INTx can be obtained through the INTX CSR
> > register
> >
> > Signed-off-by: Ray Jui <ray.jui@xxxxxxxxxxxx>
> > ---
> > Âdrivers/pci/host/pcie-iproc-platform.c |Â 2 +
> > Âdrivers/pci/host/pcie-iproc.cÂÂÂÂÂÂÂÂÂ | 95 
> > +++++++++++++++++++++++++++++++++-
> > Âdrivers/pci/host/pcie-iproc.hÂÂÂÂÂÂÂÂÂ |Â 6 +++
> > Â3 files changed, 101 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-iproc-platform.c
> > b/drivers/pci/host/pcie-iproc-platform.c
> > index e764a2a..7a51e6c 100644
> > --- a/drivers/pci/host/pcie-iproc-platform.c
> > +++ b/drivers/pci/host/pcie-iproc-platform.c
> > @@ -70,6 +70,8 @@ static int iproc_pcie_pltfm_probe(struct
> > platform_device *pdev)
> > ÂÂÂÂ }
> > ÂÂÂÂ pcie->base_addr = reg.start;
> >
> > +ÂÂÂ pcie->irq = platform_get_irq(pdev, 0);
> > +
> > ÂÂÂÂ if (of_property_read_bool(np, "brcm,pcie-ob")) {
> > ÂÂÂÂÂÂÂÂ u32 val;
> >
> > diff --git a/drivers/pci/host/pcie-iproc.c 
> > b/drivers/pci/host/pcie-iproc.c
> > index 14f374d..0bd2c14 100644
> > --- a/drivers/pci/host/pcie-iproc.c
> > +++ b/drivers/pci/host/pcie-iproc.c
> > @@ -14,6 +14,7 @@
> > Â#include <linux/delay.h>
> > Â#include <linux/interrupt.h>
> > Â#include <linux/irqchip/arm-gic-v3.h>
> > +#include <linux/irqchip/chained_irq.h>
> > Â#include <linux/platform_device.h>
> > Â#include <linux/of_address.h>
> > Â#include <linux/of_pci.h>
> > @@ -264,6 +265,7 @@ enum iproc_pcie_reg {
> >
> > ÂÂÂÂ /* enable INTx */
> > ÂÂÂÂ IPROC_PCIE_INTX_EN,
> > +ÂÂÂ IPROC_PCIE_INTX_CSR,
> >
> > ÂÂÂÂ /* outbound address mapping */
> > ÂÂÂÂ IPROC_PCIE_OARR0,
> > @@ -305,6 +307,7 @@ static const u16 iproc_pcie_reg_paxb_bcma[] = {
> > ÂÂÂÂ [IPROC_PCIE_CFG_ADDR]ÂÂÂÂÂÂÂ = 0x1f8,
> > ÂÂÂÂ [IPROC_PCIE_CFG_DATA]ÂÂÂÂÂÂÂ = 0x1fc,
> > ÂÂÂÂ [IPROC_PCIE_INTX_EN]ÂÂÂÂÂÂÂ = 0x330,
> > +ÂÂÂ [IPROC_PCIE_INTX_CSR]ÂÂÂÂÂÂÂ = 0x334,
> > ÂÂÂÂ [IPROC_PCIE_LINK_STATUS]ÂÂÂ = 0xf0c,
> > Â};
> >
> > @@ -316,6 +319,7 @@ static const u16 iproc_pcie_reg_paxb[] = {
> > ÂÂÂÂ [IPROC_PCIE_CFG_ADDR]ÂÂÂÂÂÂÂ = 0x1f8,
> > ÂÂÂÂ [IPROC_PCIE_CFG_DATA]ÂÂÂÂÂÂÂ = 0x1fc,
> > ÂÂÂÂ [IPROC_PCIE_INTX_EN]ÂÂÂÂÂÂÂ = 0x330,
> > +ÂÂÂ [IPROC_PCIE_INTX_CSR]ÂÂÂÂÂÂÂ = 0x334,
> > ÂÂÂÂ [IPROC_PCIE_OARR0]ÂÂÂÂÂÂÂ = 0xd20,
> > ÂÂÂÂ [IPROC_PCIE_OMAP0]ÂÂÂÂÂÂÂ = 0xd40,
> > ÂÂÂÂ [IPROC_PCIE_OARR1]ÂÂÂÂÂÂÂ = 0xd28,
> > @@ -332,6 +336,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = {
> > ÂÂÂÂ [IPROC_PCIE_CFG_ADDR]ÂÂÂÂÂÂÂ = 0x1f8,
> > ÂÂÂÂ [IPROC_PCIE_CFG_DATA]ÂÂÂÂÂÂÂ = 0x1fc,
> > ÂÂÂÂ [IPROC_PCIE_INTX_EN]ÂÂÂÂÂÂÂ = 0x330,
> > +ÂÂÂ [IPROC_PCIE_INTX_CSR]ÂÂÂÂÂÂÂ = 0x334,
> > ÂÂÂÂ [IPROC_PCIE_OARR0]ÂÂÂÂÂÂÂ = 0xd20,
> > ÂÂÂÂ [IPROC_PCIE_OMAP0]ÂÂÂÂÂÂÂ = 0xd40,
> > ÂÂÂÂ [IPROC_PCIE_OARR1]ÂÂÂÂÂÂÂ = 0xd28,
> > @@ -782,9 +787,90 @@ static int iproc_pcie_check_link(struct 
> > iproc_pcie *pcie)
> > ÂÂÂÂ return link_is_active ? 0 : -ENODEV;
> > Â}
> >
> > -static void iproc_pcie_enable(struct iproc_pcie *pcie)
> > +static int iproc_pcie_intx_map(struct irq_domain *domain, unsigned 
> > int irq,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ irq_hw_number_t hwirq)
> > Â{
> > +ÂÂÂ irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
> > +ÂÂÂ irq_set_chip_data(irq, domain->host_data);
> > +
> > +ÂÂÂ return 0;
> > +}
> > +
> > +static const struct irq_domain_ops intx_domain_ops = {
> > +ÂÂÂ .map = iproc_pcie_intx_map,
> > +};
> > +
> > +static void iproc_pcie_isr(struct irq_desc *desc)
> > +{
> > +ÂÂÂ struct irq_chip *chip = irq_desc_get_chip(desc);
> > +ÂÂÂ struct iproc_pcie *pcie;
> > +ÂÂÂ struct device *dev;
> > +ÂÂÂ unsigned long status;
> > +ÂÂÂ u32 bit, virq;
> > +
> > +ÂÂÂ chained_irq_enter(chip, desc);
> > +ÂÂÂ pcie = irq_desc_get_handler_data(desc);
> > +ÂÂÂ dev = pcie->dev;
> > +
> > +ÂÂÂ /* go through INTx A, B, C, D until all interrupts are handled */
> > +ÂÂÂ while ((status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR) &
> > +ÂÂÂÂÂÂÂ SYS_RC_INTX_MASK) != 0) {
> > +ÂÂÂÂÂÂÂ for_each_set_bit(bit, &status, PCI_NUM_INTX) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ virq = irq_find_mapping(pcie->irq_domain, bit + 1);
> > +ÂÂÂÂÂÂÂÂÂÂÂ if (virq)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ generic_handle_irq(virq);
> > +ÂÂÂÂÂÂÂÂÂÂÂ else
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dev_err(dev, "unexpected INTx%u\n", bit);
> > +ÂÂÂÂÂÂÂ }
> > +ÂÂÂ }
> > +
>
> Are these level or edge interrupts ? although I see DT says: IRQ_TYPE_NONE

DT entries should be fixed to trigger on level HIGH like Florian and I 
discussed on the mailing list yesterday. It looks like you are missing a 
lot of discussions on the mailing list. Could you please help to make 
sure you read all the existing discussions when commenting on a patch?

> do you not need to clear interrupt status bits in IPROC_PCIE_INTX_CSR ?

RO