| Message ID | 20210325090026.8843-6-kishon@ti.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: Add legacy interrupt support in Keystone | expand |
Hi Kishon, [...] > + if (!legacy_irq_domain) { > + dev_err(dev, "Failed to add irq domain for legacy irqs\n"); > + return -EINVAL; > + } [...] It would be "IRQ" and "IRQs" in the message above. [...] > - ret = ks_pcie_config_legacy_irq(ks_pcie); > - if (ret) > - return ret; > + if (!ks_pcie->is_am6) { > + pp->bridge->child_ops = &ks_child_pcie_ops; > + ret = ks_pcie_config_legacy_irq(ks_pcie); > + if (ret) > + return ret; > + } else { > + ret = ks_pcie_am654_config_legacy_irq(ks_pcie); > + if (ret) > + return ret; > + } [...] What if we change this to the following: if (!ks_pcie->is_am6) { pp->bridge->child_ops = &ks_child_pcie_ops; ret = ks_pcie_config_legacy_irq(ks_pcie); } else { ret = ks_pcie_am654_config_legacy_irq(ks_pcie); } if (ret) return ret; Not sure if this is something you would prefer, but it seems that either of the functions can set "ret", so checking immediately after would be the same as checking in either of the branches. But, this is a matter of style, so it would be up to you - not sure what do you prefer. Krzysztof
On Thu, 25 Mar 2021 09:00:25 +0000, Kishon Vijay Abraham I <kishon@ti.com> wrote: > > Add PCI legacy interrupt support for AM654. AM654 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 AM654 has provided IRQ_EOI register. When the SW writes to IRQ_EOI > register after handling the interrupt, the IP checks the state of > legacy interrupt and re-triggers pulse interrupt invoking the handler > again. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/pci/controller/dwc/pci-keystone.c | 87 +++++++++++++++++++++-- > 1 file changed, 82 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index dfa9a7fcf9b7..84a25207d0d3 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -118,6 +118,7 @@ struct keystone_pcie { > /* PCI Device ID */ > u32 device_id; > struct device_node *legacy_intc_np; > + struct irq_domain *legacy_irq_domain; > > int msi_host_irq; > int num_lanes; > @@ -289,6 +290,29 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie) > return IRQ_HANDLED; > } > > +static void ks_pcie_am654_legacy_irq_handler(struct irq_desc *desc) > +{ > + struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + int virq, i; > + u32 reg; > + > + chained_irq_enter(chip, desc); > + > + for (i = 0; i < PCI_NUM_INTX; i++) { > + reg = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(i)); > + if (!(reg & INTx_EN)) > + continue; > + > + virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, i); > + generic_handle_irq(virq); I'm on my way to kill irq_linear_revmap(), so I'd rather you didn't add more instances. Consider using irq_find_mapping() instead. > + ks_pcie_app_writel(ks_pcie, IRQ_STATUS(i), INTx_EN); > + ks_pcie_app_writel(ks_pcie, IRQ_EOI, i); What are these writes for? The first one feels like an Ack, and the second one has EOI written over it. If that's what they are, llease move these to a proper irq_chip structure and use the appropriate flow handler, instead of dummy_irq_chip and handle_simple_irq. Thanks, M.
diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c index dfa9a7fcf9b7..84a25207d0d3 100644 --- a/drivers/pci/controller/dwc/pci-keystone.c +++ b/drivers/pci/controller/dwc/pci-keystone.c @@ -118,6 +118,7 @@ struct keystone_pcie { /* PCI Device ID */ u32 device_id; struct device_node *legacy_intc_np; + struct irq_domain *legacy_irq_domain; int msi_host_irq; int num_lanes; @@ -289,6 +290,29 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie) return IRQ_HANDLED; } +static void ks_pcie_am654_legacy_irq_handler(struct irq_desc *desc) +{ + struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc); + struct irq_chip *chip = irq_desc_get_chip(desc); + int virq, i; + u32 reg; + + chained_irq_enter(chip, desc); + + for (i = 0; i < PCI_NUM_INTX; i++) { + reg = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(i)); + if (!(reg & INTx_EN)) + continue; + + virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, i); + generic_handle_irq(virq); + ks_pcie_app_writel(ks_pcie, IRQ_STATUS(i), INTx_EN); + ks_pcie_app_writel(ks_pcie, IRQ_EOI, i); + } + + chained_irq_exit(chip, desc); +} + void ks_pcie_irq_eoi(struct irq_data *data) { struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); @@ -728,6 +752,54 @@ static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie) return ret; } +static int ks_pcie_am654_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 ks_pcie_am654_irq_domain_ops = { + .map = ks_pcie_am654_intx_map, +}; + +static int ks_pcie_am654_config_legacy_irq(struct keystone_pcie *ks_pcie) +{ + struct device *dev = ks_pcie->pci->dev; + struct irq_domain *legacy_irq_domain; + struct device_node *np = ks_pcie->np; + struct device_node *intc_np; + int ret = 0; + int irq; + int i; + + intc_np = of_get_child_by_name(np, "interrupt-controller"); + if (!intc_np) { + dev_warn(dev, "legacy interrupt-controller node is absent\n"); + return -EINVAL; + } + + irq = irq_of_parse_and_map(intc_np, 0); + if (!irq) + return -EINVAL; + + irq_set_chained_handler_and_data(irq, ks_pcie_am654_legacy_irq_handler, ks_pcie); + legacy_irq_domain = irq_domain_add_linear(intc_np, PCI_NUM_INTX, + &ks_pcie_am654_irq_domain_ops, ks_pcie); + if (!legacy_irq_domain) { + dev_err(dev, "Failed to add irq domain for legacy irqs\n"); + return -EINVAL; + } + ks_pcie->legacy_irq_domain = legacy_irq_domain; + + for (i = 0; i < PCI_NUM_INTX; i++) + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(i), INTx_EN); + + return ret; +} + static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) { struct device *dev = ks_pcie->pci->dev; @@ -835,12 +907,17 @@ static int __init ks_pcie_host_init(struct pcie_port *pp) int ret; pp->bridge->ops = &ks_pcie_ops; - if (!ks_pcie->is_am6) - pp->bridge->child_ops = &ks_child_pcie_ops; - ret = ks_pcie_config_legacy_irq(ks_pcie); - if (ret) - return ret; + if (!ks_pcie->is_am6) { + pp->bridge->child_ops = &ks_child_pcie_ops; + ret = ks_pcie_config_legacy_irq(ks_pcie); + if (ret) + return ret; + } else { + ret = ks_pcie_am654_config_legacy_irq(ks_pcie); + if (ret) + return ret; + } ret = ks_pcie_config_msi_irq(ks_pcie); if (ret)
Add PCI legacy interrupt support for AM654. AM654 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 AM654 has provided IRQ_EOI register. When the SW writes to IRQ_EOI register after handling the interrupt, the IP checks the state of legacy interrupt and re-triggers pulse interrupt invoking the handler again. Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> --- drivers/pci/controller/dwc/pci-keystone.c | 87 +++++++++++++++++++++-- 1 file changed, 82 insertions(+), 5 deletions(-)