Hi Marc, On 09/08/21 3:09 pm, Marc Zyngier wrote: > On Mon, 09 Aug 2021 05:50:10 +0100, > Kishon Vijay Abraham I <kishon@xxxxxx> wrote: >> >> Hi Marc, >> >> On 04/08/21 8:43 pm, Marc Zyngier wrote: >>> On Wed, 04 Aug 2021 14:29:11 +0100, >>> Kishon Vijay Abraham I <kishon@xxxxxx> wrote: >>>> >>>> Add PCI legacy interrupt support for J721E. J721E has a single HW >>>> interrupt line for all the four legacy interrupts INTA/INTB/INTC/INTD. >>>> The HW interrupt line connected to GIC is a pulse interrupt whereas >>>> the legacy interrupts by definition is level interrupt. In order to >>>> provide level interrupt functionality to edge interrupt line, PCIe >>>> in J721E has provided IRQ_EOI register. >>>> >>>> However due to Errata ID #i2094 ([1]), EOI feature is not enabled in HW >>>> and only a single pulse interrupt will be generated for every >>>> ASSERT_INTx/DEASSERT_INTx. >>> >>> So my earlier remark stands. If you get a single edge, how do you >>> handle a level that is still high after having handled the interrupt >>> on hardware that has this bug? >> >> Right, this hardware (J721E) has a bug but was fixed in J7200 (Patch 3/3 >> handles that). > > But how do you make it work with J721E? Is it even worth supporting if > (as I expect) it is unreliable? I've seen at-least the NVMe devices triggers the interrupts again and the data transfer completes. But I agree, this is unreliable. > >>> >>>> >>>> [1] -> J721E DRA829/TDA4VM Processors Silicon Revision 1.1/1.0 SPRZ455A – >>>> DECEMBER 2020 – REVISED AUGUST 2021 >>>> (https://www.ti.com/lit/er/sprz455a/sprz455a.pdf) >>>> >>>> Signed-off-by: Kishon Vijay Abraham I <kishon@xxxxxx> >>>> --- >>>> drivers/pci/controller/cadence/pci-j721e.c | 85 ++++++++++++++++++++++ >>>> 1 file changed, 85 insertions(+) >>>> >>>> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c >>>> index 2ec037c43bd5..c2e7a78dc31f 100644 >>>> --- a/drivers/pci/controller/cadence/pci-j721e.c >>>> +++ b/drivers/pci/controller/cadence/pci-j721e.c >>>> @@ -29,6 +29,13 @@ >>>> #define LINK_DOWN BIT(1) >>>> #define J7200_LINK_DOWN BIT(10) >>>> >>>> +#define EOI_REG 0x10 >>>> + >>>> +#define ENABLE_REG_SYS_0 0x100 >>>> +#define STATUS_REG_SYS_0 0x500 >>>> +#define STATUS_CLR_REG_SYS_0 0x700 >>>> +#define INTx_EN(num) (1 << (num)) >>>> + >>>> #define J721E_PCIE_USER_CMD_STATUS 0x4 >>>> #define LINK_TRAINING_ENABLE BIT(0) >>>> >>>> @@ -59,6 +66,7 @@ struct j721e_pcie { >>>> void __iomem *user_cfg_base; >>>> void __iomem *intd_cfg_base; >>>> u32 linkdown_irq_regfield; >>>> + struct irq_domain *legacy_irq_domain; >>>> }; >>>> >>>> enum j721e_pcie_mode { >>>> @@ -121,6 +129,79 @@ static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie) >>>> j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_2, reg); >>>> } >>>> >>>> +static void j721e_pcie_v1_legacy_irq_handler(struct irq_desc *desc) >>>> +{ >>>> + struct j721e_pcie *pcie = irq_desc_get_handler_data(desc); >>>> + struct irq_chip *chip = irq_desc_get_chip(desc); >>>> + int i, virq; >>>> + u32 reg; >>>> + >>>> + chained_irq_enter(chip, desc); >>>> + >>>> + for (i = 0; i < PCI_NUM_INTX; i++) { >>>> + reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_0); >>>> + if (!(reg & INTx_EN(i))) >>>> + continue; >>> >>> Why do you need to perform multiple reads? Surely reg contains all the >>> bits you need, doesn't it? >> >> Right, will fix it up. >>> >>>> + >>>> + virq = irq_find_mapping(pcie->legacy_irq_domain, 3 - i); >>>> + generic_handle_irq(virq); >>> >>> Please combine both lines into a single generic_handle_domain_irq() >>> call. >> >> Okay. >>> >>>> + j721e_pcie_intd_writel(pcie, STATUS_CLR_REG_SYS_0, INTx_EN(i)); >>> >>> What is the purpose of this write? It feels like this should be a >>> irq_eoi callback. >> >> It's an IRQ ACK, since in this platform the level to edge is not >> implemented properly in HW. > > An Ack for an edge interrupt would need to happen before you handle > the interrupt, or you'd wrongly acknowledge edges that fire between > the handling of the interrupt and the Ack, and that would never be > handled. > > If it really is an Ack, it needs to be moved *before* the handling, > preferably in an irq_ack callback. Otherwise, it is an EOI, and it > belongs to irq_eoi. > >>> >>>> + } >>>> + >>>> + chained_irq_exit(chip, desc); >>>> +} >>>> + >>>> +static int j721e_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); >>> >>> An INTx interrupt is a level interrupt. Please use the corresponding flow. >> >> Okay. >>> >>>> + irq_set_chip_data(irq, domain->host_data); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static const struct irq_domain_ops j721e_pcie_intx_domain_ops = { >>>> + .map = j721e_pcie_intx_map, >>>> +}; >>>> + >>>> +static int j721e_pcie_config_legacy_irq(struct j721e_pcie *pcie) >>>> +{ >>>> + struct irq_domain *legacy_irq_domain; >>>> + struct device *dev = pcie->dev; >>>> + struct device_node *node = dev->of_node; >>>> + struct device_node *intc_node; >>>> + int irq, i; >>>> + u32 reg; >>>> + >>>> + intc_node = of_get_child_by_name(node, "interrupt-controller"); >>>> + if (!intc_node) { >>>> + dev_dbg(dev, "interrupt-controller node is absent. Legacy INTR not supported\n"); >>>> + return 0; >>>> + } >>>> + >>>> + irq = irq_of_parse_and_map(intc_node, 0); >>>> + if (!irq) { >>>> + dev_err(dev, "Failed to parse and map legacy irq\n"); >>>> + return -EINVAL; >>>> + } >>>> + irq_set_chained_handler_and_data(irq, j721e_pcie_v1_legacy_irq_handler, pcie); >>>> + >>>> + legacy_irq_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX, >>>> + &j721e_pcie_intx_domain_ops, pcie); >>>> + if (!legacy_irq_domain) { >>>> + dev_err(dev, "Failed to add irq domain for legacy irqs\n"); >>>> + return -EINVAL; >>>> + } >>>> + pcie->legacy_irq_domain = legacy_irq_domain; >>>> + >>>> + for (i = 0; i < PCI_NUM_INTX; i++) { >>>> + reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_0); >>>> + reg |= INTx_EN(i); >>>> + j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_0, reg); >>>> + } >>> >>> This should be moved to the irq_unmask() callback. >> >> Should we also have a corresponding irq_mask()? Then would require us >> implement reference counting since legacy interrupts are shared. > > The core code should already deal with this, I expect. It isn't like > shared interrupts are something new. And yes, you should have both > mask and unmask. Thanks for clarifying. Best Regards, Kishon
