| Message ID | 20190221101518.22604-4-kishon@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: DWC/Keystone: MSI configuration cleanup | expand |
On Thu, Feb 21, 2019 at 03:45:12PM +0530, Kishon Vijay Abraham I wrote: > K2G provides separate IRQ lines for each of the four legacy interrupts. > Model this using hierarchy domain instead of linear domain with chained > IRQ handler. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/pci/controller/dwc/pci-keystone.c | 205 +++++++++++++--------- > 1 file changed, 118 insertions(+), 87 deletions(-) Hi Kishon, I CC'ed Marc because you are actually re-writing an interrupt controller driver so I would be happier to merge this refactoring if Marc can have a look and he is satisfied with it - more so because most of the code can be reused by other host bridge drivers with similar behaviour. I will have a look too, unfortunately it is becoming a bit tight for v5.1 but let's see how it goes. Thanks, Lorenzo > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index 47f0dcf638f2..7f1648453f54 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -61,6 +61,7 @@ > > #define IRQ_STATUS(n) (0x184 + ((n) << 4)) > #define IRQ_ENABLE_SET(n) (0x188 + ((n) << 4)) > +#define IRQ_ENABLE_CLR(n) (0x18C + ((n) << 4)) > #define INTx_EN BIT(0) > > #define ERR_IRQ_STATUS 0x1c4 > @@ -87,7 +88,6 @@ struct keystone_pcie { > struct dw_pcie *pci; > /* PCI Device ID */ > u32 device_id; > - int legacy_host_irqs[PCI_NUM_INTX]; > struct device_node *legacy_intc_np; > > int msi_host_irqs[MAX_MSI_HOST_IRQS]; > @@ -96,7 +96,6 @@ struct keystone_pcie { > struct phy **phy; > struct device_link **link; > struct device_node *msi_intc_np; > - struct irq_domain *legacy_irq_domain; > struct device_node *np; > > int error_irq; > @@ -199,26 +198,6 @@ static int ks_pcie_msi_host_init(struct pcie_port *pp) > return dw_pcie_allocate_domains(pp); > } > > -static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie, > - int offset) > -{ > - 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); > - } > - > - /* EOI the INTx interrupt */ > - ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset); > -} > - > static void ks_pcie_enable_error_irq(struct keystone_pcie *ks_pcie) > { > ks_pcie_app_writel(ks_pcie, ERR_IRQ_ENABLE_SET, ERR_IRQ_ALL); > @@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie) > return IRQ_HANDLED; > } > > -static void ks_pcie_ack_legacy_irq(struct irq_data *d) > +void ks_pcie_irq_eoi(struct irq_data *data) > { > + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); > + irq_hw_number_t hwirq = data->hwirq; > + > + ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq); > + irq_chip_eoi_parent(data); > } > > -static void ks_pcie_mask_legacy_irq(struct irq_data *d) > +void ks_pcie_irq_enable(struct irq_data *data) > { > + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); > + irq_hw_number_t hwirq = data->hwirq; > + > + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN); > + irq_chip_enable_parent(data); > } > > -static void ks_pcie_unmask_legacy_irq(struct irq_data *d) > +void ks_pcie_irq_disable(struct irq_data *data) > { > + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); > + irq_hw_number_t hwirq = data->hwirq; > + > + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN); > + irq_chip_disable_parent(data); > } > > static struct irq_chip ks_pcie_legacy_irq_chip = { > - .name = "Keystone-PCI-Legacy-IRQ", > - .irq_ack = ks_pcie_ack_legacy_irq, > - .irq_mask = ks_pcie_mask_legacy_irq, > - .irq_unmask = ks_pcie_unmask_legacy_irq, > + .name = "Keystone-PCI-Legacy-IRQ", > + .irq_enable = ks_pcie_irq_enable, > + .irq_disable = ks_pcie_irq_disable, > + .irq_eoi = ks_pcie_irq_eoi, > + .irq_mask = irq_chip_mask_parent, > + .irq_unmask = irq_chip_unmask_parent, > + .irq_retrigger = irq_chip_retrigger_hierarchy, > + .irq_set_type = irq_chip_set_type_parent, > + .irq_set_affinity = irq_chip_set_affinity_parent, > }; > > -static int ks_pcie_init_legacy_irq_map(struct irq_domain *d, > - unsigned int irq, > - irq_hw_number_t hw_irq) > +static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain, > + unsigned int virq, > + unsigned int nr_irqs, void *data) > { > - irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip, > - handle_level_irq); > - irq_set_chip_data(irq, d->host_data); > + struct keystone_pcie *ks_pcie = domain->host_data; > + struct device_node *np = ks_pcie->legacy_intc_np; > + struct irq_fwspec parent_fwspec, *fwspec = data; > + struct of_phandle_args out_irq; > + int ret, i; > + > + if (nr_irqs != 1) > + return -EINVAL; > + > + ret = of_irq_parse_one(np, fwspec->param[0], &out_irq); > + if (ret < 0) { > + pr_err("Failed to parse interrupt node\n"); > + return ret; > + } > + > + parent_fwspec.fwnode = &out_irq.np->fwnode; > + parent_fwspec.param_count = out_irq.args_count; > + > + for (i = 0; i < out_irq.args_count; i++) > + parent_fwspec.param[i] = out_irq.args[i]; > + > + ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec); > + if (ret < 0) { > + pr_err("Failed to allocate parent irq %u: %d\n", > + parent_fwspec.param[0], ret); > + return ret; > + } > + > + ret = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0], > + &ks_pcie_legacy_irq_chip, ks_pcie); > + if (ret < 0) { > + pr_err("Failed to set hwirq and chip\n"); > + goto err_set_hwirq_and_chip; > + } > > return 0; > + > +err_set_hwirq_and_chip: > + irq_domain_free_irqs_parent(domain, virq, 1); > + > + return ret; > +} > + > +static int ks_pcie_irq_domain_translate(struct irq_domain *domain, > + struct irq_fwspec *fwspec, > + unsigned long *hwirq, > + unsigned int *type) > +{ > + if (is_of_node(fwspec->fwnode)) { > + if (fwspec->param_count != 2) > + return -EINVAL; > + > + if (fwspec->param[0] >= PCI_NUM_INTX) > + return -EINVAL; > + > + *hwirq = fwspec->param[0]; > + *type = fwspec->param[1]; > + > + return 0; > + } > + > + return -EINVAL; > } > > static const struct irq_domain_ops ks_pcie_legacy_irq_domain_ops = { > - .map = ks_pcie_init_legacy_irq_map, > - .xlate = irq_domain_xlate_onetwocell, > + .alloc = ks_pcie_legacy_irq_domain_alloc, > + .free = irq_domain_free_irqs_common, > + .translate = ks_pcie_irq_domain_translate, > }; > > /** > @@ -572,35 +629,6 @@ static void ks_pcie_msi_irq_handler(struct irq_desc *desc) > chained_irq_exit(chip, desc); > } > > -/** > - * ks_pcie_legacy_irq_handler() - Handle legacy interrupt > - * @irq: IRQ line for legacy interrupts > - * @desc: Pointer to irq descriptor > - * > - * Traverse through pending legacy interrupts and invoke handler for each. Also > - * takes care of interrupt controller level mask/ack operation. > - */ > -static void ks_pcie_legacy_irq_handler(struct irq_desc *desc) > -{ > - unsigned int irq = irq_desc_get_irq(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); > - > - dev_dbg(dev, ": Handling legacy irq %d\n", irq); > - > - /* > - * The chained irq handler installation would have replaced normal > - * interrupt driver handler so we need to take care of mask/unmask and > - * ack operation. > - */ > - chained_irq_enter(chip, desc); > - ks_pcie_handle_legacy_irq(ks_pcie, irq_offset); > - chained_irq_exit(chip, desc); > -} > - > static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie) > { > struct device *dev = ks_pcie->pci->dev; > @@ -655,14 +683,33 @@ static int ks_pcie_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 irq_domain *parent_domain; > + struct device_node *parent_node; > struct device_node *intc_np; > - int irq_count, irq, ret = 0, i; > + int irq_count, ret = 0; > > intc_np = of_get_child_by_name(np, "legacy-interrupt-controller"); > if (!intc_np) { > dev_warn(dev, "legacy-interrupt-controller node is absent\n"); > return -EINVAL; > } > + ks_pcie->legacy_intc_np = intc_np; > + > + parent_node = of_irq_find_parent(intc_np); > + if (!parent_node) { > + dev_err(dev, "unable to obtain parent node\n"); > + ret = -ENXIO; > + goto err; > + } > + > + parent_domain = irq_find_host(parent_node); > + if (!parent_domain) { > + dev_err(dev, "unable to obtain parent domain\n"); > + ret = -ENXIO; > + goto err; > + } > + > + of_node_put(parent_node); > > irq_count = of_irq_count(intc_np); > if (!irq_count) { > @@ -671,31 +718,15 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) > goto err; > } > > - for (i = 0; i < irq_count; i++) { > - irq = irq_of_parse_and_map(intc_np, i); > - if (!irq) { > - ret = -EINVAL; > - goto err; > - } > - ks_pcie->legacy_host_irqs[i] = irq; > - > - irq_set_chained_handler_and_data(irq, > - ks_pcie_legacy_irq_handler, > - ks_pcie); > - } > - > legacy_irq_domain = > - irq_domain_add_linear(intc_np, PCI_NUM_INTX, > - &ks_pcie_legacy_irq_domain_ops, NULL); > + irq_domain_add_hierarchy(parent_domain, 0, PCI_NUM_INTX, > + intc_np, > + &ks_pcie_legacy_irq_domain_ops, > + ks_pcie); > if (!legacy_irq_domain) { > dev_err(dev, "Failed to add irq domain for legacy irqs\n"); > ret = -EINVAL; > - goto err; > } > - 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); > > err: > of_node_put(intc_np); > -- > 2.17.1 >
On Thu, 21 Feb 2019 16:24:14 +0000 Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Thu, Feb 21, 2019 at 03:45:12PM +0530, Kishon Vijay Abraham I wrote: > > K2G provides separate IRQ lines for each of the four legacy interrupts. > > Model this using hierarchy domain instead of linear domain with chained > > IRQ handler. > > > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > > --- > > drivers/pci/controller/dwc/pci-keystone.c | 205 +++++++++++++--------- > > 1 file changed, 118 insertions(+), 87 deletions(-) > > Hi Kishon, > > I CC'ed Marc because you are actually re-writing an interrupt controller > driver so I would be happier to merge this refactoring if Marc can have > a look and he is satisfied with it - more so because most of the code can > be reused by other host bridge drivers with similar behaviour. Cheers Lorenzo. It doesn't look too bad, but there is a couple of points I'd like to see clarified. Comments below. > > I will have a look too, unfortunately it is becoming a bit tight for > v5.1 but let's see how it goes. > > Thanks, > Lorenzo > > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > > index 47f0dcf638f2..7f1648453f54 100644 > > --- a/drivers/pci/controller/dwc/pci-keystone.c > > +++ b/drivers/pci/controller/dwc/pci-keystone.c > > @@ -61,6 +61,7 @@ > > > > #define IRQ_STATUS(n) (0x184 + ((n) << 4)) > > #define IRQ_ENABLE_SET(n) (0x188 + ((n) << 4)) > > +#define IRQ_ENABLE_CLR(n) (0x18C + ((n) << 4)) > > #define INTx_EN BIT(0) > > > > #define ERR_IRQ_STATUS 0x1c4 > > @@ -87,7 +88,6 @@ struct keystone_pcie { > > struct dw_pcie *pci; > > /* PCI Device ID */ > > u32 device_id; > > - int legacy_host_irqs[PCI_NUM_INTX]; > > struct device_node *legacy_intc_np; > > > > int msi_host_irqs[MAX_MSI_HOST_IRQS]; > > @@ -96,7 +96,6 @@ struct keystone_pcie { > > struct phy **phy; > > struct device_link **link; > > struct device_node *msi_intc_np; > > - struct irq_domain *legacy_irq_domain; > > struct device_node *np; > > > > int error_irq; > > @@ -199,26 +198,6 @@ static int ks_pcie_msi_host_init(struct pcie_port *pp) > > return dw_pcie_allocate_domains(pp); > > } > > > > -static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie, > > - int offset) > > -{ > > - 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); > > - } > > - > > - /* EOI the INTx interrupt */ > > - ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset); > > -} > > - > > static void ks_pcie_enable_error_irq(struct keystone_pcie *ks_pcie) > > { > > ks_pcie_app_writel(ks_pcie, ERR_IRQ_ENABLE_SET, ERR_IRQ_ALL); > > @@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie) > > return IRQ_HANDLED; > > } > > > > -static void ks_pcie_ack_legacy_irq(struct irq_data *d) > > +void ks_pcie_irq_eoi(struct irq_data *data) > > { > > + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); > > + irq_hw_number_t hwirq = data->hwirq; > > + > > + ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq); > > + irq_chip_eoi_parent(data); > > } > > > > -static void ks_pcie_mask_legacy_irq(struct irq_data *d) > > +void ks_pcie_irq_enable(struct irq_data *data) > > { > > + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); > > + irq_hw_number_t hwirq = data->hwirq; > > + > > + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN); > > + irq_chip_enable_parent(data); > > } > > > > -static void ks_pcie_unmask_legacy_irq(struct irq_data *d) > > +void ks_pcie_irq_disable(struct irq_data *data) > > { > > + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); > > + irq_hw_number_t hwirq = data->hwirq; > > + > > + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN); > > + irq_chip_disable_parent(data); > > } > > > > static struct irq_chip ks_pcie_legacy_irq_chip = { > > - .name = "Keystone-PCI-Legacy-IRQ", > > - .irq_ack = ks_pcie_ack_legacy_irq, > > - .irq_mask = ks_pcie_mask_legacy_irq, > > - .irq_unmask = ks_pcie_unmask_legacy_irq, > > + .name = "Keystone-PCI-Legacy-IRQ", > > + .irq_enable = ks_pcie_irq_enable, > > + .irq_disable = ks_pcie_irq_disable, > > + .irq_eoi = ks_pcie_irq_eoi, > > + .irq_mask = irq_chip_mask_parent, > > + .irq_unmask = irq_chip_unmask_parent, > > + .irq_retrigger = irq_chip_retrigger_hierarchy, > > + .irq_set_type = irq_chip_set_type_parent, > > + .irq_set_affinity = irq_chip_set_affinity_parent, > > }; > > > > -static int ks_pcie_init_legacy_irq_map(struct irq_domain *d, > > - unsigned int irq, > > - irq_hw_number_t hw_irq) > > +static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain, > > + unsigned int virq, > > + unsigned int nr_irqs, void *data) > > { > > - irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip, > > - handle_level_irq); > > - irq_set_chip_data(irq, d->host_data); > > + struct keystone_pcie *ks_pcie = domain->host_data; > > + struct device_node *np = ks_pcie->legacy_intc_np; > > + struct irq_fwspec parent_fwspec, *fwspec = data; > > + struct of_phandle_args out_irq; > > + int ret, i; > > + > > + if (nr_irqs != 1) > > + return -EINVAL; > > + > > + ret = of_irq_parse_one(np, fwspec->param[0], &out_irq); > > + if (ret < 0) { > > + pr_err("Failed to parse interrupt node\n"); > > + return ret; > > + } What it this trying to do? Fishing out the interrupts from DT based on the legacy pin? This looks at best obscure. I wonder why you don't do that at probe time instead. Anyway, this requires documenting. > > + > > + parent_fwspec.fwnode = &out_irq.np->fwnode; > > + parent_fwspec.param_count = out_irq.args_count; > > + > > + for (i = 0; i < out_irq.args_count; i++) > > + parent_fwspec.param[i] = out_irq.args[i]; This feels like a duplicate of of_phandle_args_to_fwspec(). If you need such a helper, please export it from the irqdomain code. > > + > > + ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec); > > + if (ret < 0) { > > + pr_err("Failed to allocate parent irq %u: %d\n", > > + parent_fwspec.param[0], ret); > > + return ret; > > + } > > + > > + ret = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0], > > + &ks_pcie_legacy_irq_chip, ks_pcie); > > + if (ret < 0) { > > + pr_err("Failed to set hwirq and chip\n"); > > + goto err_set_hwirq_and_chip; > > + } > > > > return 0; > > + > > +err_set_hwirq_and_chip: > > + irq_domain_free_irqs_parent(domain, virq, 1); > > + > > + return ret; > > +} > > + > > +static int ks_pcie_irq_domain_translate(struct irq_domain *domain, > > + struct irq_fwspec *fwspec, > > + unsigned long *hwirq, > > + unsigned int *type) > > +{ > > + if (is_of_node(fwspec->fwnode)) { If you don't plan to support ACPI, you can drop this. > > + if (fwspec->param_count != 2) > > + return -EINVAL; From the DT binding: pcie_intc: Interrupt controller device node for Legacy IRQ chip interrupt-cells: should be set to 1 So why do we have 2 cells here? > > + > > + if (fwspec->param[0] >= PCI_NUM_INTX) > > + return -EINVAL; Most of the OF code assumes that the pin number describing the legacy interrupt is 1-based, while you obviously treat it as 0-based. How does it work? > > + > > + *hwirq = fwspec->param[0]; > > + *type = fwspec->param[1]; As far as I remember, PCI legacy interrupts are level triggered, so there should be no need to advertise the trigger (which is consistent with the way the binding is written). > > + > > + return 0; > > + } > > + > > + return -EINVAL; > > } > > > > static const struct irq_domain_ops ks_pcie_legacy_irq_domain_ops = { > > - .map = ks_pcie_init_legacy_irq_map, > > - .xlate = irq_domain_xlate_onetwocell, > > + .alloc = ks_pcie_legacy_irq_domain_alloc, > > + .free = irq_domain_free_irqs_common, > > + .translate = ks_pcie_irq_domain_translate, > > }; > > > > /** > > @@ -572,35 +629,6 @@ static void ks_pcie_msi_irq_handler(struct irq_desc *desc) > > chained_irq_exit(chip, desc); > > } > > > > -/** > > - * ks_pcie_legacy_irq_handler() - Handle legacy interrupt > > - * @irq: IRQ line for legacy interrupts > > - * @desc: Pointer to irq descriptor > > - * > > - * Traverse through pending legacy interrupts and invoke handler for each. Also > > - * takes care of interrupt controller level mask/ack operation. > > - */ > > -static void ks_pcie_legacy_irq_handler(struct irq_desc *desc) > > -{ > > - unsigned int irq = irq_desc_get_irq(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); > > - > > - dev_dbg(dev, ": Handling legacy irq %d\n", irq); > > - > > - /* > > - * The chained irq handler installation would have replaced normal > > - * interrupt driver handler so we need to take care of mask/unmask and > > - * ack operation. > > - */ > > - chained_irq_enter(chip, desc); > > - ks_pcie_handle_legacy_irq(ks_pcie, irq_offset); > > - chained_irq_exit(chip, desc); > > -} > > - > > static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie) > > { > > struct device *dev = ks_pcie->pci->dev; > > @@ -655,14 +683,33 @@ static int ks_pcie_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 irq_domain *parent_domain; > > + struct device_node *parent_node; > > struct device_node *intc_np; > > - int irq_count, irq, ret = 0, i; > > + int irq_count, ret = 0; > > > > intc_np = of_get_child_by_name(np, "legacy-interrupt-controller"); > > if (!intc_np) { > > dev_warn(dev, "legacy-interrupt-controller node is absent\n"); > > return -EINVAL; > > } > > + ks_pcie->legacy_intc_np = intc_np; > > + > > + parent_node = of_irq_find_parent(intc_np); > > + if (!parent_node) { > > + dev_err(dev, "unable to obtain parent node\n"); > > + ret = -ENXIO; > > + goto err; > > + } > > + > > + parent_domain = irq_find_host(parent_node); > > + if (!parent_domain) { > > + dev_err(dev, "unable to obtain parent domain\n"); > > + ret = -ENXIO; > > + goto err; > > + } > > + > > + of_node_put(parent_node); > > > > irq_count = of_irq_count(intc_np); > > if (!irq_count) { > > @@ -671,31 +718,15 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) > > goto err; > > } > > > > - for (i = 0; i < irq_count; i++) { > > - irq = irq_of_parse_and_map(intc_np, i); > > - if (!irq) { > > - ret = -EINVAL; > > - goto err; > > - } > > - ks_pcie->legacy_host_irqs[i] = irq; > > - > > - irq_set_chained_handler_and_data(irq, > > - ks_pcie_legacy_irq_handler, > > - ks_pcie); > > - } > > - > > legacy_irq_domain = > > - irq_domain_add_linear(intc_np, PCI_NUM_INTX, > > - &ks_pcie_legacy_irq_domain_ops, NULL); > > + irq_domain_add_hierarchy(parent_domain, 0, PCI_NUM_INTX, > > + intc_np, > > + &ks_pcie_legacy_irq_domain_ops, > > + ks_pcie); > > if (!legacy_irq_domain) { > > dev_err(dev, "Failed to add irq domain for legacy irqs\n"); > > ret = -EINVAL; > > - goto err; > > } > > - 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); > > > > err: > > of_node_put(intc_np); > > -- > > 2.17.1 > > The structure of the code otherwise looks good. I trust you'd be able to shed some light on the questions I have. Thanks, M. -- Without deviation from the norm, progress is not possible.
Hi, On 23/02/19 5:41 PM, Marc Zyngier wrote: > On Thu, 21 Feb 2019 16:24:14 +0000 > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > >> On Thu, Feb 21, 2019 at 03:45:12PM +0530, Kishon Vijay Abraham I wrote: >>> K2G provides separate IRQ lines for each of the four legacy interrupts. >>> Model this using hierarchy domain instead of linear domain with chained >>> IRQ handler. >>> >>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>> --- >>> drivers/pci/controller/dwc/pci-keystone.c | 205 +++++++++++++--------- >>> 1 file changed, 118 insertions(+), 87 deletions(-) >> >> Hi Kishon, >> >> I CC'ed Marc because you are actually re-writing an interrupt controller >> driver so I would be happier to merge this refactoring if Marc can have >> a look and he is satisfied with it - more so because most of the code can >> be reused by other host bridge drivers with similar behaviour. > > Cheers Lorenzo. > > It doesn't look too bad, but there is a couple of points I'd like to see > clarified. Comments below. > >> >> I will have a look too, unfortunately it is becoming a bit tight for >> v5.1 but let's see how it goes. >> >> Thanks, >> Lorenzo >> >>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c >>> index 47f0dcf638f2..7f1648453f54 100644 >>> --- a/drivers/pci/controller/dwc/pci-keystone.c >>> +++ b/drivers/pci/controller/dwc/pci-keystone.c >>> @@ -61,6 +61,7 @@ >>> >>> #define IRQ_STATUS(n) (0x184 + ((n) << 4)) >>> #define IRQ_ENABLE_SET(n) (0x188 + ((n) << 4)) >>> +#define IRQ_ENABLE_CLR(n) (0x18C + ((n) << 4)) >>> #define INTx_EN BIT(0) >>> >>> #define ERR_IRQ_STATUS 0x1c4 >>> @@ -87,7 +88,6 @@ struct keystone_pcie { >>> struct dw_pcie *pci; >>> /* PCI Device ID */ >>> u32 device_id; >>> - int legacy_host_irqs[PCI_NUM_INTX]; >>> struct device_node *legacy_intc_np; >>> >>> int msi_host_irqs[MAX_MSI_HOST_IRQS]; >>> @@ -96,7 +96,6 @@ struct keystone_pcie { >>> struct phy **phy; >>> struct device_link **link; >>> struct device_node *msi_intc_np; >>> - struct irq_domain *legacy_irq_domain; >>> struct device_node *np; >>> >>> int error_irq; >>> @@ -199,26 +198,6 @@ static int ks_pcie_msi_host_init(struct pcie_port *pp) >>> return dw_pcie_allocate_domains(pp); >>> } >>> >>> -static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie, >>> - int offset) >>> -{ >>> - 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); >>> - } >>> - >>> - /* EOI the INTx interrupt */ >>> - ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset); >>> -} >>> - >>> static void ks_pcie_enable_error_irq(struct keystone_pcie *ks_pcie) >>> { >>> ks_pcie_app_writel(ks_pcie, ERR_IRQ_ENABLE_SET, ERR_IRQ_ALL); >>> @@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie) >>> return IRQ_HANDLED; >>> } >>> >>> -static void ks_pcie_ack_legacy_irq(struct irq_data *d) >>> +void ks_pcie_irq_eoi(struct irq_data *data) >>> { >>> + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); >>> + irq_hw_number_t hwirq = data->hwirq; >>> + >>> + ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq); >>> + irq_chip_eoi_parent(data); >>> } >>> >>> -static void ks_pcie_mask_legacy_irq(struct irq_data *d) >>> +void ks_pcie_irq_enable(struct irq_data *data) >>> { >>> + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); >>> + irq_hw_number_t hwirq = data->hwirq; >>> + >>> + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN); >>> + irq_chip_enable_parent(data); >>> } >>> >>> -static void ks_pcie_unmask_legacy_irq(struct irq_data *d) >>> +void ks_pcie_irq_disable(struct irq_data *data) >>> { >>> + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); >>> + irq_hw_number_t hwirq = data->hwirq; >>> + >>> + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN); >>> + irq_chip_disable_parent(data); >>> } >>> >>> static struct irq_chip ks_pcie_legacy_irq_chip = { >>> - .name = "Keystone-PCI-Legacy-IRQ", >>> - .irq_ack = ks_pcie_ack_legacy_irq, >>> - .irq_mask = ks_pcie_mask_legacy_irq, >>> - .irq_unmask = ks_pcie_unmask_legacy_irq, >>> + .name = "Keystone-PCI-Legacy-IRQ", >>> + .irq_enable = ks_pcie_irq_enable, >>> + .irq_disable = ks_pcie_irq_disable, >>> + .irq_eoi = ks_pcie_irq_eoi, >>> + .irq_mask = irq_chip_mask_parent, >>> + .irq_unmask = irq_chip_unmask_parent, >>> + .irq_retrigger = irq_chip_retrigger_hierarchy, >>> + .irq_set_type = irq_chip_set_type_parent, >>> + .irq_set_affinity = irq_chip_set_affinity_parent, >>> }; >>> >>> -static int ks_pcie_init_legacy_irq_map(struct irq_domain *d, >>> - unsigned int irq, >>> - irq_hw_number_t hw_irq) >>> +static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain, >>> + unsigned int virq, >>> + unsigned int nr_irqs, void *data) >>> { >>> - irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip, >>> - handle_level_irq); >>> - irq_set_chip_data(irq, d->host_data); >>> + struct keystone_pcie *ks_pcie = domain->host_data; >>> + struct device_node *np = ks_pcie->legacy_intc_np; >>> + struct irq_fwspec parent_fwspec, *fwspec = data; >>> + struct of_phandle_args out_irq; >>> + int ret, i; >>> + >>> + if (nr_irqs != 1) >>> + return -EINVAL; >>> + >>> + ret = of_irq_parse_one(np, fwspec->param[0], &out_irq); >>> + if (ret < 0) { >>> + pr_err("Failed to parse interrupt node\n"); >>> + return ret; >>> + } > > What it this trying to do? Fishing out the interrupts from DT based on > the legacy pin? This looks at best obscure. I wonder why you don't do > that at probe time instead. Anyway, this requires documenting. The device-tree of PCIe node looks something like below. interrupt-map = <0 0 0 1 &pcie_intc0 0 IRQ_TYPE_EDGE_RISING>, /* INT A */ <0 0 0 2 &pcie_intc0 1 IRQ_TYPE_EDGE_RISING>, /* INT B */ <0 0 0 3 &pcie_intc0 2 IRQ_TYPE_EDGE_RISING>, /* INT C */ <0 0 0 4 &pcie_intc0 3 IRQ_TYPE_EDGE_RISING>; /* INT D */ pcie_intc0: legacy-interrupt-controller { interrupt-controller; #interrupt-cells = <2>; interrupt-parent = <&gic>; interrupts = <GIC_SPI 48 IRQ_TYPE_EDGE_RISING>, <GIC_SPI 49 IRQ_TYPE_EDGE_RISING>, <GIC_SPI 50 IRQ_TYPE_EDGE_RISING>, <GIC_SPI 51 IRQ_TYPE_EDGE_RISING>; }; INTA corresponds to HWIRQ '0' of the hierarchy irq domain we create in this driver which in turn corresponds to GIC_SPI '48' of GIC. We could create an array of parent_fwspec for each of the four interrupt lines in legacy-interrupt-controller during probe and use it directly here while invoking irq_domain_alloc_irqs_parent. I think you want me to do that instead of using of_irq_parse_one here? > >>> + >>> + parent_fwspec.fwnode = &out_irq.np->fwnode; >>> + parent_fwspec.param_count = out_irq.args_count; >>> + >>> + for (i = 0; i < out_irq.args_count; i++) >>> + parent_fwspec.param[i] = out_irq.args[i]; > > This feels like a duplicate of of_phandle_args_to_fwspec(). If you need > such a helper, please export it from the irqdomain code. sure. > >>> + >>> + ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec); >>> + if (ret < 0) { >>> + pr_err("Failed to allocate parent irq %u: %d\n", >>> + parent_fwspec.param[0], ret); >>> + return ret; >>> + } >>> + >>> + ret = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0], >>> + &ks_pcie_legacy_irq_chip, ks_pcie); >>> + if (ret < 0) { >>> + pr_err("Failed to set hwirq and chip\n"); >>> + goto err_set_hwirq_and_chip; >>> + } >>> >>> return 0; >>> + >>> +err_set_hwirq_and_chip: >>> + irq_domain_free_irqs_parent(domain, virq, 1); >>> + >>> + return ret; >>> +} >>> + >>> +static int ks_pcie_irq_domain_translate(struct irq_domain *domain, >>> + struct irq_fwspec *fwspec, >>> + unsigned long *hwirq, >>> + unsigned int *type) >>> +{ >>> + if (is_of_node(fwspec->fwnode)) { > > If you don't plan to support ACPI, you can drop this. okay. > >>> + if (fwspec->param_count != 2) >>> + return -EINVAL; > > From the DT binding: > > pcie_intc: Interrupt controller device node for Legacy IRQ chip > interrupt-cells: should be set to 1 > > So why do we have 2 cells here? With '1' cells, it's not possible to specify irq trigger type. DT binding has to be fixed. I'll fix this in the next revision of the patch series. > >>> + >>> + if (fwspec->param[0] >= PCI_NUM_INTX) >>> + return -EINVAL; > > Most of the OF code assumes that the pin number describing the legacy > interrupt is 1-based, while you obviously treat it as 0-based. How does > it work? INTA corresponds to '0' of the hierarchy interrupt domain (using interrupt-map). > >>> + >>> + *hwirq = fwspec->param[0]; >>> + *type = fwspec->param[1]; > > As far as I remember, PCI legacy interrupts are level triggered, so > there should be no need to advertise the trigger (which is consistent > with the way the binding is written). It is pulse triggered at subsystem level. Quoting the TRM "The interrupt request signal at the PCIe SS boundary is a pulse signal that is triggered each time an assert interrupt message is received." The PCIe subsystem also has a level signal (interrupt pending signal) but the interrupt request signal is the one that is connected to GIC. Thanks Kishon
On Thu, 07 Mar 2019 09:12:30 +0000, Kishon Vijay Abraham I <kishon@ti.com> wrote: > > Hi, > > On 23/02/19 5:41 PM, Marc Zyngier wrote: > > On Thu, 21 Feb 2019 16:24:14 +0000 > > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > > >> On Thu, Feb 21, 2019 at 03:45:12PM +0530, Kishon Vijay Abraham I wrote: > >>> K2G provides separate IRQ lines for each of the four legacy interrupts. > >>> Model this using hierarchy domain instead of linear domain with chained > >>> IRQ handler. > >>> > >>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > >>> --- > >>> drivers/pci/controller/dwc/pci-keystone.c | 205 +++++++++++++--------- > >>> 1 file changed, 118 insertions(+), 87 deletions(-) > >> > >> Hi Kishon, > >> > >> I CC'ed Marc because you are actually re-writing an interrupt controller > >> driver so I would be happier to merge this refactoring if Marc can have > >> a look and he is satisfied with it - more so because most of the code can > >> be reused by other host bridge drivers with similar behaviour. > > > > Cheers Lorenzo. > > > > It doesn't look too bad, but there is a couple of points I'd like to see > > clarified. Comments below. > > > >> > >> I will have a look too, unfortunately it is becoming a bit tight for > >> v5.1 but let's see how it goes. > >> > >> Thanks, > >> Lorenzo > >> > >>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > >>> index 47f0dcf638f2..7f1648453f54 100644 > >>> --- a/drivers/pci/controller/dwc/pci-keystone.c > >>> +++ b/drivers/pci/controller/dwc/pci-keystone.c > >>> @@ -61,6 +61,7 @@ > >>> > >>> #define IRQ_STATUS(n) (0x184 + ((n) << 4)) > >>> #define IRQ_ENABLE_SET(n) (0x188 + ((n) << 4)) > >>> +#define IRQ_ENABLE_CLR(n) (0x18C + ((n) << 4)) > >>> #define INTx_EN BIT(0) > >>> > >>> #define ERR_IRQ_STATUS 0x1c4 > >>> @@ -87,7 +88,6 @@ struct keystone_pcie { > >>> struct dw_pcie *pci; > >>> /* PCI Device ID */ > >>> u32 device_id; > >>> - int legacy_host_irqs[PCI_NUM_INTX]; > >>> struct device_node *legacy_intc_np; > >>> > >>> int msi_host_irqs[MAX_MSI_HOST_IRQS]; > >>> @@ -96,7 +96,6 @@ struct keystone_pcie { > >>> struct phy **phy; > >>> struct device_link **link; > >>> struct device_node *msi_intc_np; > >>> - struct irq_domain *legacy_irq_domain; > >>> struct device_node *np; > >>> > >>> int error_irq; > >>> @@ -199,26 +198,6 @@ static int ks_pcie_msi_host_init(struct pcie_port *pp) > >>> return dw_pcie_allocate_domains(pp); > >>> } > >>> > >>> -static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie, > >>> - int offset) > >>> -{ > >>> - 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); > >>> - } > >>> - > >>> - /* EOI the INTx interrupt */ > >>> - ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset); > >>> -} > >>> - > >>> static void ks_pcie_enable_error_irq(struct keystone_pcie *ks_pcie) > >>> { > >>> ks_pcie_app_writel(ks_pcie, ERR_IRQ_ENABLE_SET, ERR_IRQ_ALL); > >>> @@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie) > >>> return IRQ_HANDLED; > >>> } > >>> > >>> -static void ks_pcie_ack_legacy_irq(struct irq_data *d) > >>> +void ks_pcie_irq_eoi(struct irq_data *data) > >>> { > >>> + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); > >>> + irq_hw_number_t hwirq = data->hwirq; > >>> + > >>> + ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq); > >>> + irq_chip_eoi_parent(data); > >>> } > >>> > >>> -static void ks_pcie_mask_legacy_irq(struct irq_data *d) > >>> +void ks_pcie_irq_enable(struct irq_data *data) > >>> { > >>> + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); > >>> + irq_hw_number_t hwirq = data->hwirq; > >>> + > >>> + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN); > >>> + irq_chip_enable_parent(data); > >>> } > >>> > >>> -static void ks_pcie_unmask_legacy_irq(struct irq_data *d) > >>> +void ks_pcie_irq_disable(struct irq_data *data) > >>> { > >>> + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); > >>> + irq_hw_number_t hwirq = data->hwirq; > >>> + > >>> + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN); > >>> + irq_chip_disable_parent(data); > >>> } > >>> > >>> static struct irq_chip ks_pcie_legacy_irq_chip = { > >>> - .name = "Keystone-PCI-Legacy-IRQ", > >>> - .irq_ack = ks_pcie_ack_legacy_irq, > >>> - .irq_mask = ks_pcie_mask_legacy_irq, > >>> - .irq_unmask = ks_pcie_unmask_legacy_irq, > >>> + .name = "Keystone-PCI-Legacy-IRQ", > >>> + .irq_enable = ks_pcie_irq_enable, > >>> + .irq_disable = ks_pcie_irq_disable, > >>> + .irq_eoi = ks_pcie_irq_eoi, > >>> + .irq_mask = irq_chip_mask_parent, > >>> + .irq_unmask = irq_chip_unmask_parent, > >>> + .irq_retrigger = irq_chip_retrigger_hierarchy, > >>> + .irq_set_type = irq_chip_set_type_parent, > >>> + .irq_set_affinity = irq_chip_set_affinity_parent, > >>> }; > >>> > >>> -static int ks_pcie_init_legacy_irq_map(struct irq_domain *d, > >>> - unsigned int irq, > >>> - irq_hw_number_t hw_irq) > >>> +static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain, > >>> + unsigned int virq, > >>> + unsigned int nr_irqs, void *data) > >>> { > >>> - irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip, > >>> - handle_level_irq); > >>> - irq_set_chip_data(irq, d->host_data); > >>> + struct keystone_pcie *ks_pcie = domain->host_data; > >>> + struct device_node *np = ks_pcie->legacy_intc_np; > >>> + struct irq_fwspec parent_fwspec, *fwspec = data; > >>> + struct of_phandle_args out_irq; > >>> + int ret, i; > >>> + > >>> + if (nr_irqs != 1) > >>> + return -EINVAL; > >>> + > >>> + ret = of_irq_parse_one(np, fwspec->param[0], &out_irq); > >>> + if (ret < 0) { > >>> + pr_err("Failed to parse interrupt node\n"); > >>> + return ret; > >>> + } > > > > What it this trying to do? Fishing out the interrupts from DT based on > > the legacy pin? This looks at best obscure. I wonder why you don't do > > that at probe time instead. Anyway, this requires documenting. > > The device-tree of PCIe node looks something like below. > > interrupt-map = <0 0 0 1 &pcie_intc0 0 IRQ_TYPE_EDGE_RISING>, /* INT A */ > <0 0 0 2 &pcie_intc0 1 IRQ_TYPE_EDGE_RISING>, /* INT B */ > <0 0 0 3 &pcie_intc0 2 IRQ_TYPE_EDGE_RISING>, /* INT C */ > <0 0 0 4 &pcie_intc0 3 IRQ_TYPE_EDGE_RISING>; /* INT D */ > pcie_intc0: legacy-interrupt-controller { > interrupt-controller; > #interrupt-cells = <2>; > interrupt-parent = <&gic>; > interrupts = <GIC_SPI 48 IRQ_TYPE_EDGE_RISING>, > <GIC_SPI 49 IRQ_TYPE_EDGE_RISING>, > <GIC_SPI 50 IRQ_TYPE_EDGE_RISING>, > <GIC_SPI 51 IRQ_TYPE_EDGE_RISING>; > }; > > INTA corresponds to HWIRQ '0' of the hierarchy irq domain we create in this > driver which in turn corresponds to GIC_SPI '48' of GIC. > > We could create an array of parent_fwspec for each of the four interrupt lines > in legacy-interrupt-controller during probe and use it directly here while > invoking irq_domain_alloc_irqs_parent. I think you want me to do that instead > of using of_irq_parse_one here? Up to you. I don't mind either way, but it is the lack of documentation that annoys me the most here. The more worrying part is that you are using an EDGE signalling in these bindings, and that's wrong. PCI legacy interrupts are level, always. [...] > > From the DT binding: > > > > pcie_intc: Interrupt controller device node for Legacy IRQ chip > > interrupt-cells: should be set to 1 > > > > So why do we have 2 cells here? > > With '1' cells, it's not possible to specify irq trigger type. DT > binding has to be fixed. I'll fix this in the next revision of the > patch series. As I said above, this is wrong. Legacy interrupts are always level, by definition. Why are you making them edge-triggered? > > > >>> + > >>> + if (fwspec->param[0] >= PCI_NUM_INTX) > >>> + return -EINVAL; > > > > Most of the OF code assumes that the pin number describing the legacy > > interrupt is 1-based, while you obviously treat it as 0-based. How does > > it work? > > INTA corresponds to '0' of the hierarchy interrupt domain (using > interrupt-map). I'm not disputing this. It is more that the DT code does number the pin from 1, if memory serves well. Can you please verify this (see of_irq_parse_pci). > > > >>> + > >>> + *hwirq = fwspec->param[0]; > >>> + *type = fwspec->param[1]; > > > > As far as I remember, PCI legacy interrupts are level triggered, so > > there should be no need to advertise the trigger (which is consistent > > with the way the binding is written). > > It is pulse triggered at subsystem level. Quoting the TRM > "The interrupt request signal at the PCIe SS boundary is a pulse signal that is > triggered each time an assert interrupt message is received." The PCIe > subsystem also has a level signal (interrupt pending signal) but the interrupt > request signal is the one that is connected to GIC. This looks completely wrong. MSIs are always edge, and legacy always level, end of story. If your HW is any different, then it doesn't seem to be compliant with PCI. Thanks, M. -- Jazz is not dead, it just smell funny.
Hi Marc, On 07/03/19 5:32 PM, Marc Zyngier wrote: > On Thu, 07 Mar 2019 09:12:30 +0000, > Kishon Vijay Abraham I <kishon@ti.com> wrote: >> >> Hi, >> >> On 23/02/19 5:41 PM, Marc Zyngier wrote: >>> On Thu, 21 Feb 2019 16:24:14 +0000 >>> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: >>> >>>> On Thu, Feb 21, 2019 at 03:45:12PM +0530, Kishon Vijay Abraham I wrote: >>>>> K2G provides separate IRQ lines for each of the four legacy interrupts. >>>>> Model this using hierarchy domain instead of linear domain with chained >>>>> IRQ handler. >>>>> >>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>>> --- >>>>> drivers/pci/controller/dwc/pci-keystone.c | 205 +++++++++++++--------- >>>>> 1 file changed, 118 insertions(+), 87 deletions(-) >>>> >>>> Hi Kishon, >>>> >>>> I CC'ed Marc because you are actually re-writing an interrupt controller >>>> driver so I would be happier to merge this refactoring if Marc can have >>>> a look and he is satisfied with it - more so because most of the code can >>>> be reused by other host bridge drivers with similar behaviour. >>> >>> Cheers Lorenzo. >>> >>> It doesn't look too bad, but there is a couple of points I'd like to see >>> clarified. Comments below. >>> >>>> >>>> I will have a look too, unfortunately it is becoming a bit tight for >>>> v5.1 but let's see how it goes. >>>> >>>> Thanks, >>>> Lorenzo >>>> >>>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c >>>>> index 47f0dcf638f2..7f1648453f54 100644 >>>>> --- a/drivers/pci/controller/dwc/pci-keystone.c >>>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c >>>>> @@ -61,6 +61,7 @@ >>>>> >>>>> #define IRQ_STATUS(n) (0x184 + ((n) << 4)) >>>>> #define IRQ_ENABLE_SET(n) (0x188 + ((n) << 4)) >>>>> +#define IRQ_ENABLE_CLR(n) (0x18C + ((n) << 4)) >>>>> #define INTx_EN BIT(0) >>>>> >>>>> #define ERR_IRQ_STATUS 0x1c4 >>>>> @@ -87,7 +88,6 @@ struct keystone_pcie { >>>>> struct dw_pcie *pci; >>>>> /* PCI Device ID */ >>>>> u32 device_id; >>>>> - int legacy_host_irqs[PCI_NUM_INTX]; >>>>> struct device_node *legacy_intc_np; >>>>> >>>>> int msi_host_irqs[MAX_MSI_HOST_IRQS]; >>>>> @@ -96,7 +96,6 @@ struct keystone_pcie { >>>>> struct phy **phy; >>>>> struct device_link **link; >>>>> struct device_node *msi_intc_np; >>>>> - struct irq_domain *legacy_irq_domain; >>>>> struct device_node *np; >>>>> >>>>> int error_irq; >>>>> @@ -199,26 +198,6 @@ static int ks_pcie_msi_host_init(struct pcie_port *pp) >>>>> return dw_pcie_allocate_domains(pp); >>>>> } >>>>> >>>>> -static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie, >>>>> - int offset) >>>>> -{ >>>>> - 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); >>>>> - } >>>>> - >>>>> - /* EOI the INTx interrupt */ >>>>> - ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset); >>>>> -} >>>>> - >>>>> static void ks_pcie_enable_error_irq(struct keystone_pcie *ks_pcie) >>>>> { >>>>> ks_pcie_app_writel(ks_pcie, ERR_IRQ_ENABLE_SET, ERR_IRQ_ALL); >>>>> @@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie) >>>>> return IRQ_HANDLED; >>>>> } >>>>> >>>>> -static void ks_pcie_ack_legacy_irq(struct irq_data *d) >>>>> +void ks_pcie_irq_eoi(struct irq_data *data) >>>>> { >>>>> + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); >>>>> + irq_hw_number_t hwirq = data->hwirq; >>>>> + >>>>> + ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq); >>>>> + irq_chip_eoi_parent(data); >>>>> } >>>>> >>>>> -static void ks_pcie_mask_legacy_irq(struct irq_data *d) >>>>> +void ks_pcie_irq_enable(struct irq_data *data) >>>>> { >>>>> + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); >>>>> + irq_hw_number_t hwirq = data->hwirq; >>>>> + >>>>> + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN); >>>>> + irq_chip_enable_parent(data); >>>>> } >>>>> >>>>> -static void ks_pcie_unmask_legacy_irq(struct irq_data *d) >>>>> +void ks_pcie_irq_disable(struct irq_data *data) >>>>> { >>>>> + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); >>>>> + irq_hw_number_t hwirq = data->hwirq; >>>>> + >>>>> + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN); >>>>> + irq_chip_disable_parent(data); >>>>> } >>>>> >>>>> static struct irq_chip ks_pcie_legacy_irq_chip = { >>>>> - .name = "Keystone-PCI-Legacy-IRQ", >>>>> - .irq_ack = ks_pcie_ack_legacy_irq, >>>>> - .irq_mask = ks_pcie_mask_legacy_irq, >>>>> - .irq_unmask = ks_pcie_unmask_legacy_irq, >>>>> + .name = "Keystone-PCI-Legacy-IRQ", >>>>> + .irq_enable = ks_pcie_irq_enable, >>>>> + .irq_disable = ks_pcie_irq_disable, >>>>> + .irq_eoi = ks_pcie_irq_eoi, >>>>> + .irq_mask = irq_chip_mask_parent, >>>>> + .irq_unmask = irq_chip_unmask_parent, >>>>> + .irq_retrigger = irq_chip_retrigger_hierarchy, >>>>> + .irq_set_type = irq_chip_set_type_parent, >>>>> + .irq_set_affinity = irq_chip_set_affinity_parent, >>>>> }; >>>>> >>>>> -static int ks_pcie_init_legacy_irq_map(struct irq_domain *d, >>>>> - unsigned int irq, >>>>> - irq_hw_number_t hw_irq) >>>>> +static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain, >>>>> + unsigned int virq, >>>>> + unsigned int nr_irqs, void *data) >>>>> { >>>>> - irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip, >>>>> - handle_level_irq); >>>>> - irq_set_chip_data(irq, d->host_data); >>>>> + struct keystone_pcie *ks_pcie = domain->host_data; >>>>> + struct device_node *np = ks_pcie->legacy_intc_np; >>>>> + struct irq_fwspec parent_fwspec, *fwspec = data; >>>>> + struct of_phandle_args out_irq; >>>>> + int ret, i; >>>>> + >>>>> + if (nr_irqs != 1) >>>>> + return -EINVAL; >>>>> + >>>>> + ret = of_irq_parse_one(np, fwspec->param[0], &out_irq); >>>>> + if (ret < 0) { >>>>> + pr_err("Failed to parse interrupt node\n"); >>>>> + return ret; >>>>> + } >>> >>> What it this trying to do? Fishing out the interrupts from DT based on >>> the legacy pin? This looks at best obscure. I wonder why you don't do >>> that at probe time instead. Anyway, this requires documenting. >> >> The device-tree of PCIe node looks something like below. >> >> interrupt-map = <0 0 0 1 &pcie_intc0 0 IRQ_TYPE_EDGE_RISING>, /* INT A */ >> <0 0 0 2 &pcie_intc0 1 IRQ_TYPE_EDGE_RISING>, /* INT B */ >> <0 0 0 3 &pcie_intc0 2 IRQ_TYPE_EDGE_RISING>, /* INT C */ >> <0 0 0 4 &pcie_intc0 3 IRQ_TYPE_EDGE_RISING>; /* INT D */ >> pcie_intc0: legacy-interrupt-controller { >> interrupt-controller; >> #interrupt-cells = <2>; >> interrupt-parent = <&gic>; >> interrupts = <GIC_SPI 48 IRQ_TYPE_EDGE_RISING>, >> <GIC_SPI 49 IRQ_TYPE_EDGE_RISING>, >> <GIC_SPI 50 IRQ_TYPE_EDGE_RISING>, >> <GIC_SPI 51 IRQ_TYPE_EDGE_RISING>; >> }; >> >> INTA corresponds to HWIRQ '0' of the hierarchy irq domain we create in this >> driver which in turn corresponds to GIC_SPI '48' of GIC. >> >> We could create an array of parent_fwspec for each of the four interrupt lines >> in legacy-interrupt-controller during probe and use it directly here while >> invoking irq_domain_alloc_irqs_parent. I think you want me to do that instead >> of using of_irq_parse_one here? > > Up to you. I don't mind either way, but it is the lack of > documentation that annoys me the most here. Okay, I'll add a comment before invoking of_irq_parse_one. > > The more worrying part is that you are using an EDGE signalling in > these bindings, and that's wrong. PCI legacy interrupts are level, > always. Agreed. But the HW uses pulse signal whenever it sees an Assert_Intx message. > > [...] > >>> From the DT binding: >>> >>> pcie_intc: Interrupt controller device node for Legacy IRQ chip >>> interrupt-cells: should be set to 1 >>> >>> So why do we have 2 cells here? >> >> With '1' cells, it's not possible to specify irq trigger type. DT >> binding has to be fixed. I'll fix this in the next revision of the >> patch series. > > As I said above, this is wrong. Legacy interrupts are always level, by > definition. Why are you making them edge-triggered > >>> >>>>> + >>>>> + if (fwspec->param[0] >= PCI_NUM_INTX) >>>>> + return -EINVAL; >>> >>> Most of the OF code assumes that the pin number describing the legacy >>> interrupt is 1-based, while you obviously treat it as 0-based. How does >>> it work? >> >> INTA corresponds to '0' of the hierarchy interrupt domain (using >> interrupt-map). > > I'm not disputing this. It is more that the DT code does number the > pin from 1, if memory serves well. Can you please verify this (see > of_irq_parse_pci). Yes it does number from '1'. However I use interrupt-map property to map from legacy interrupt pin to a interrupt number of the legacy interrupt controller which starts from '0'. > >>> >>>>> + >>>>> + *hwirq = fwspec->param[0]; >>>>> + *type = fwspec->param[1]; >>> >>> As far as I remember, PCI legacy interrupts are level triggered, so >>> there should be no need to advertise the trigger (which is consistent >>> with the way the binding is written). >> >> It is pulse triggered at subsystem level. Quoting the TRM >> "The interrupt request signal at the PCIe SS boundary is a pulse signal that is >> triggered each time an assert interrupt message is received." The PCIe >> subsystem also has a level signal (interrupt pending signal) but the interrupt >> request signal is the one that is connected to GIC. > > This looks completely wrong. MSIs are always edge, and legacy always > level, end of story. If your HW is any different, then it doesn't seem > to be compliant with PCI. Agree that Legacy interrupts ought to be level-triggered. I checked this with HW team and I've copy-pasted the response. "The legacy interrupts from the PCIe controller are still level. The PCIe controller will hold the legacy interrupt lines high until a de-assert message is sent back. The pulse interrupt is only to send the interrupt from the PCIe sub-system to the SoC interrupt controller. This should not impact the operation of the legacy interrupt on the PCIe side." Looks like the interrupts are propagated multiple levels and only the last level to the interrupt controller is pulse. Thanks Kishon
On Mon, 18 Mar 2019 11:03:07 +0530 Kishon Vijay Abraham I <kishon@ti.com> wrote: > Hi Marc, > > On 07/03/19 5:32 PM, Marc Zyngier wrote: > > On Thu, 07 Mar 2019 09:12:30 +0000, > > Kishon Vijay Abraham I <kishon@ti.com> wrote: > >> > >> Hi, > >> > >> On 23/02/19 5:41 PM, Marc Zyngier wrote: > >>> On Thu, 21 Feb 2019 16:24:14 +0000 > >>> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > >>> > >>>> On Thu, Feb 21, 2019 at 03:45:12PM +0530, Kishon Vijay Abraham I wrote: > >>>>> K2G provides separate IRQ lines for each of the four legacy interrupts. > >>>>> Model this using hierarchy domain instead of linear domain with chained > >>>>> IRQ handler. > >>>>> > >>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > >>>>> --- > >>>>> drivers/pci/controller/dwc/pci-keystone.c | 205 +++++++++++++--------- > >>>>> 1 file changed, 118 insertions(+), 87 deletions(-) > >>>> > >>>> Hi Kishon, > >>>> > >>>> I CC'ed Marc because you are actually re-writing an interrupt controller > >>>> driver so I would be happier to merge this refactoring if Marc can have > >>>> a look and he is satisfied with it - more so because most of the code can > >>>> be reused by other host bridge drivers with similar behaviour. > >>> > >>> Cheers Lorenzo. > >>> > >>> It doesn't look too bad, but there is a couple of points I'd like to see > >>> clarified. Comments below. > >>> > >>>> > >>>> I will have a look too, unfortunately it is becoming a bit tight for > >>>> v5.1 but let's see how it goes. > >>>> > >>>> Thanks, > >>>> Lorenzo > >>>> > >>>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > >>>>> index 47f0dcf638f2..7f1648453f54 100644 > >>>>> --- a/drivers/pci/controller/dwc/pci-keystone.c > >>>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c > >>>>> @@ -61,6 +61,7 @@ > >>>>> > >>>>> #define IRQ_STATUS(n) (0x184 + ((n) << 4)) > >>>>> #define IRQ_ENABLE_SET(n) (0x188 + ((n) << 4)) > >>>>> +#define IRQ_ENABLE_CLR(n) (0x18C + ((n) << 4)) > >>>>> #define INTx_EN BIT(0) > >>>>> > >>>>> #define ERR_IRQ_STATUS 0x1c4 > >>>>> @@ -87,7 +88,6 @@ struct keystone_pcie { > >>>>> struct dw_pcie *pci; > >>>>> /* PCI Device ID */ > >>>>> u32 device_id; > >>>>> - int legacy_host_irqs[PCI_NUM_INTX]; > >>>>> struct device_node *legacy_intc_np; > >>>>> > >>>>> int msi_host_irqs[MAX_MSI_HOST_IRQS]; > >>>>> @@ -96,7 +96,6 @@ struct keystone_pcie { > >>>>> struct phy **phy; > >>>>> struct device_link **link; > >>>>> struct device_node *msi_intc_np; > >>>>> - struct irq_domain *legacy_irq_domain; > >>>>> struct device_node *np; > >>>>> > >>>>> int error_irq; > >>>>> @@ -199,26 +198,6 @@ static int ks_pcie_msi_host_init(struct pcie_port *pp) > >>>>> return dw_pcie_allocate_domains(pp); > >>>>> } > >>>>> > >>>>> -static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie, > >>>>> - int offset) > >>>>> -{ > >>>>> - 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); > >>>>> - } > >>>>> - > >>>>> - /* EOI the INTx interrupt */ > >>>>> - ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset); > >>>>> -} > >>>>> - > >>>>> static void ks_pcie_enable_error_irq(struct keystone_pcie *ks_pcie) > >>>>> { > >>>>> ks_pcie_app_writel(ks_pcie, ERR_IRQ_ENABLE_SET, ERR_IRQ_ALL); > >>>>> @@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie) > >>>>> return IRQ_HANDLED; > >>>>> } > >>>>> > >>>>> -static void ks_pcie_ack_legacy_irq(struct irq_data *d) > >>>>> +void ks_pcie_irq_eoi(struct irq_data *data) > >>>>> { > >>>>> + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); > >>>>> + irq_hw_number_t hwirq = data->hwirq; > >>>>> + > >>>>> + ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq); > >>>>> + irq_chip_eoi_parent(data); > >>>>> } > >>>>> > >>>>> -static void ks_pcie_mask_legacy_irq(struct irq_data *d) > >>>>> +void ks_pcie_irq_enable(struct irq_data *data) > >>>>> { > >>>>> + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); > >>>>> + irq_hw_number_t hwirq = data->hwirq; > >>>>> + > >>>>> + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN); > >>>>> + irq_chip_enable_parent(data); > >>>>> } > >>>>> > >>>>> -static void ks_pcie_unmask_legacy_irq(struct irq_data *d) > >>>>> +void ks_pcie_irq_disable(struct irq_data *data) > >>>>> { > >>>>> + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); > >>>>> + irq_hw_number_t hwirq = data->hwirq; > >>>>> + > >>>>> + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN); > >>>>> + irq_chip_disable_parent(data); > >>>>> } > >>>>> > >>>>> static struct irq_chip ks_pcie_legacy_irq_chip = { > >>>>> - .name = "Keystone-PCI-Legacy-IRQ", > >>>>> - .irq_ack = ks_pcie_ack_legacy_irq, > >>>>> - .irq_mask = ks_pcie_mask_legacy_irq, > >>>>> - .irq_unmask = ks_pcie_unmask_legacy_irq, > >>>>> + .name = "Keystone-PCI-Legacy-IRQ", > >>>>> + .irq_enable = ks_pcie_irq_enable, > >>>>> + .irq_disable = ks_pcie_irq_disable, > >>>>> + .irq_eoi = ks_pcie_irq_eoi, > >>>>> + .irq_mask = irq_chip_mask_parent, > >>>>> + .irq_unmask = irq_chip_unmask_parent, > >>>>> + .irq_retrigger = irq_chip_retrigger_hierarchy, > >>>>> + .irq_set_type = irq_chip_set_type_parent, > >>>>> + .irq_set_affinity = irq_chip_set_affinity_parent, > >>>>> }; > >>>>> > >>>>> -static int ks_pcie_init_legacy_irq_map(struct irq_domain *d, > >>>>> - unsigned int irq, > >>>>> - irq_hw_number_t hw_irq) > >>>>> +static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain, > >>>>> + unsigned int virq, > >>>>> + unsigned int nr_irqs, void *data) > >>>>> { > >>>>> - irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip, > >>>>> - handle_level_irq); > >>>>> - irq_set_chip_data(irq, d->host_data); > >>>>> + struct keystone_pcie *ks_pcie = domain->host_data; > >>>>> + struct device_node *np = ks_pcie->legacy_intc_np; > >>>>> + struct irq_fwspec parent_fwspec, *fwspec = data; > >>>>> + struct of_phandle_args out_irq; > >>>>> + int ret, i; > >>>>> + > >>>>> + if (nr_irqs != 1) > >>>>> + return -EINVAL; > >>>>> + > >>>>> + ret = of_irq_parse_one(np, fwspec->param[0], &out_irq); > >>>>> + if (ret < 0) { > >>>>> + pr_err("Failed to parse interrupt node\n"); > >>>>> + return ret; > >>>>> + } > >>> > >>> What it this trying to do? Fishing out the interrupts from DT based on > >>> the legacy pin? This looks at best obscure. I wonder why you don't do > >>> that at probe time instead. Anyway, this requires documenting. > >> > >> The device-tree of PCIe node looks something like below. > >> > >> interrupt-map = <0 0 0 1 &pcie_intc0 0 IRQ_TYPE_EDGE_RISING>, /* INT A */ > >> <0 0 0 2 &pcie_intc0 1 IRQ_TYPE_EDGE_RISING>, /* INT B */ > >> <0 0 0 3 &pcie_intc0 2 IRQ_TYPE_EDGE_RISING>, /* INT C */ > >> <0 0 0 4 &pcie_intc0 3 IRQ_TYPE_EDGE_RISING>; /* INT D */ > >> pcie_intc0: legacy-interrupt-controller { > >> interrupt-controller; > >> #interrupt-cells = <2>; > >> interrupt-parent = <&gic>; > >> interrupts = <GIC_SPI 48 IRQ_TYPE_EDGE_RISING>, > >> <GIC_SPI 49 IRQ_TYPE_EDGE_RISING>, > >> <GIC_SPI 50 IRQ_TYPE_EDGE_RISING>, > >> <GIC_SPI 51 IRQ_TYPE_EDGE_RISING>; > >> }; > >> > >> INTA corresponds to HWIRQ '0' of the hierarchy irq domain we create in this > >> driver which in turn corresponds to GIC_SPI '48' of GIC. > >> > >> We could create an array of parent_fwspec for each of the four interrupt lines > >> in legacy-interrupt-controller during probe and use it directly here while > >> invoking irq_domain_alloc_irqs_parent. I think you want me to do that instead > >> of using of_irq_parse_one here? > > > > Up to you. I don't mind either way, but it is the lack of > > documentation that annoys me the most here. > > Okay, I'll add a comment before invoking of_irq_parse_one. > > > > The more worrying part is that you are using an EDGE signalling in > > these bindings, and that's wrong. PCI legacy interrupts are level, > > always. > > Agreed. But the HW uses pulse signal whenever it sees an Assert_Intx message. I'm not convinced... See below. > > > > [...] > > > >>> From the DT binding: > >>> > >>> pcie_intc: Interrupt controller device node for Legacy IRQ chip > >>> interrupt-cells: should be set to 1 > >>> > >>> So why do we have 2 cells here? > >> > >> With '1' cells, it's not possible to specify irq trigger type. DT > >> binding has to be fixed. I'll fix this in the next revision of the > >> patch series. > > > > As I said above, this is wrong. Legacy interrupts are always level, by > > definition. Why are you making them edge-triggered > > > >>> > >>>>> + > >>>>> + if (fwspec->param[0] >= PCI_NUM_INTX) > >>>>> + return -EINVAL; > >>> > >>> Most of the OF code assumes that the pin number describing the legacy > >>> interrupt is 1-based, while you obviously treat it as 0-based. How does > >>> it work? > >> > >> INTA corresponds to '0' of the hierarchy interrupt domain (using > >> interrupt-map). > > > > I'm not disputing this. It is more that the DT code does number the > > pin from 1, if memory serves well. Can you please verify this (see > > of_irq_parse_pci). > > Yes it does number from '1'. However I use interrupt-map property to map from > legacy interrupt pin to a interrupt number of the legacy interrupt controller > which starts from '0'. If you've verified that it actually works, fine bby me. > > > >>> > >>>>> + > >>>>> + *hwirq = fwspec->param[0]; > >>>>> + *type = fwspec->param[1]; > >>> > >>> As far as I remember, PCI legacy interrupts are level triggered, so > >>> there should be no need to advertise the trigger (which is consistent > >>> with the way the binding is written). > >> > >> It is pulse triggered at subsystem level. Quoting the TRM > >> "The interrupt request signal at the PCIe SS boundary is a pulse signal that is > >> triggered each time an assert interrupt message is received." The PCIe > >> subsystem also has a level signal (interrupt pending signal) but the interrupt > >> request signal is the one that is connected to GIC. > > > > This looks completely wrong. MSIs are always edge, and legacy always > > level, end of story. If your HW is any different, then it doesn't seem > > to be compliant with PCI. > > Agree that Legacy interrupts ought to be level-triggered. I checked this with > HW team and I've copy-pasted the response. > > "The legacy interrupts from the PCIe controller are still level. The PCIe > controller will hold the legacy interrupt lines high until a de-assert message > is sent back. The pulse interrupt is only to send the interrupt from the PCIe > sub-system to the SoC interrupt controller. This should not impact the > operation of the legacy interrupt on the PCIe side." > > Looks like the interrupts are propagated multiple levels and only the last > level to the interrupt controller is pulse. Let's take an example: My device (let's assume a network controller of some sort) generates an interrupt. Level is high, and you observe an edge. The driver handles the interrupt, but due to a race, the line stays high (the device has received a new packet). How is a *new* edge generated? The line never transited to low, and we have more packet to process. With a level triggered interrupt, you just take the interrupt again. With an edge interrupt, you need to actively resample the level and retrigger the interrupt. How is this done? Similar issues exist with interrupt sharing, which is a fairly common thing. Thanks, M. -- Without deviation from the norm, progress is not possible.
Hi Marc, On 18/03/19 4:46 PM, Marc Zyngier wrote: > On Mon, 18 Mar 2019 11:03:07 +0530 > Kishon Vijay Abraham I <kishon@ti.com> wrote: > >> Hi Marc, >> >> On 07/03/19 5:32 PM, Marc Zyngier wrote: >>> On Thu, 07 Mar 2019 09:12:30 +0000, >>> Kishon Vijay Abraham I <kishon@ti.com> wrote: >>>> >>>> Hi, >>>> >>>> On 23/02/19 5:41 PM, Marc Zyngier wrote: >>>>> On Thu, 21 Feb 2019 16:24:14 +0000 >>>>> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: >>>>> >>>>>> On Thu, Feb 21, 2019 at 03:45:12PM +0530, Kishon Vijay Abraham I wrote: >>>>>>> K2G provides separate IRQ lines for each of the four legacy interrupts. >>>>>>> Model this using hierarchy domain instead of linear domain with chained >>>>>>> IRQ handler. >>>>>>> >>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>>>>> --- >>>>>>> drivers/pci/controller/dwc/pci-keystone.c | 205 +++++++++++++--------- >>>>>>> 1 file changed, 118 insertions(+), 87 deletions(-) >>>>>> >>>>>> Hi Kishon, >>>>>> >>>>>> I CC'ed Marc because you are actually re-writing an interrupt controller >>>>>> driver so I would be happier to merge this refactoring if Marc can have >>>>>> a look and he is satisfied with it - more so because most of the code can >>>>>> be reused by other host bridge drivers with similar behaviour. >>>>> >>>>> Cheers Lorenzo. >>>>> >>>>> It doesn't look too bad, but there is a couple of points I'd like to see >>>>> clarified. Comments below. >>>>> >>>>>> >>>>>> I will have a look too, unfortunately it is becoming a bit tight for >>>>>> v5.1 but let's see how it goes. >>>>>> >>>>>> Thanks, >>>>>> Lorenzo >>>>>> >>>>>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c >>>>>>> index 47f0dcf638f2..7f1648453f54 100644 >>>>>>> --- a/drivers/pci/controller/dwc/pci-keystone.c >>>>>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c >>>>>>> @@ -61,6 +61,7 @@ >>>>>>> >>>>>>> #define IRQ_STATUS(n) (0x184 + ((n) << 4)) >>>>>>> #define IRQ_ENABLE_SET(n) (0x188 + ((n) << 4)) >>>>>>> +#define IRQ_ENABLE_CLR(n) (0x18C + ((n) << 4)) >>>>>>> #define INTx_EN BIT(0) >>>>>>> >>>>>>> #define ERR_IRQ_STATUS 0x1c4 >>>>>>> @@ -87,7 +88,6 @@ struct keystone_pcie { >>>>>>> struct dw_pcie *pci; >>>>>>> /* PCI Device ID */ >>>>>>> u32 device_id; >>>>>>> - int legacy_host_irqs[PCI_NUM_INTX]; >>>>>>> struct device_node *legacy_intc_np; >>>>>>> >>>>>>> int msi_host_irqs[MAX_MSI_HOST_IRQS]; >>>>>>> @@ -96,7 +96,6 @@ struct keystone_pcie { >>>>>>> struct phy **phy; >>>>>>> struct device_link **link; >>>>>>> struct device_node *msi_intc_np; >>>>>>> - struct irq_domain *legacy_irq_domain; >>>>>>> struct device_node *np; >>>>>>> >>>>>>> int error_irq; >>>>>>> @@ -199,26 +198,6 @@ static int ks_pcie_msi_host_init(struct pcie_port *pp) >>>>>>> return dw_pcie_allocate_domains(pp); >>>>>>> } >>>>>>> >>>>>>> -static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie, >>>>>>> - int offset) >>>>>>> -{ >>>>>>> - 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); >>>>>>> - } >>>>>>> - >>>>>>> - /* EOI the INTx interrupt */ >>>>>>> - ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset); >>>>>>> -} >>>>>>> - >>>>>>> static void ks_pcie_enable_error_irq(struct keystone_pcie *ks_pcie) >>>>>>> { >>>>>>> ks_pcie_app_writel(ks_pcie, ERR_IRQ_ENABLE_SET, ERR_IRQ_ALL); >>>>>>> @@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie) >>>>>>> return IRQ_HANDLED; >>>>>>> } >>>>>>> >>>>>>> -static void ks_pcie_ack_legacy_irq(struct irq_data *d) >>>>>>> +void ks_pcie_irq_eoi(struct irq_data *data) >>>>>>> { >>>>>>> + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); >>>>>>> + irq_hw_number_t hwirq = data->hwirq; >>>>>>> + >>>>>>> + ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq); >>>>>>> + irq_chip_eoi_parent(data); >>>>>>> } >>>>>>> >>>>>>> -static void ks_pcie_mask_legacy_irq(struct irq_data *d) >>>>>>> +void ks_pcie_irq_enable(struct irq_data *data) >>>>>>> { >>>>>>> + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); >>>>>>> + irq_hw_number_t hwirq = data->hwirq; >>>>>>> + >>>>>>> + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN); >>>>>>> + irq_chip_enable_parent(data); >>>>>>> } >>>>>>> >>>>>>> -static void ks_pcie_unmask_legacy_irq(struct irq_data *d) >>>>>>> +void ks_pcie_irq_disable(struct irq_data *data) >>>>>>> { >>>>>>> + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); >>>>>>> + irq_hw_number_t hwirq = data->hwirq; >>>>>>> + >>>>>>> + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN); >>>>>>> + irq_chip_disable_parent(data); >>>>>>> } >>>>>>> >>>>>>> static struct irq_chip ks_pcie_legacy_irq_chip = { >>>>>>> - .name = "Keystone-PCI-Legacy-IRQ", >>>>>>> - .irq_ack = ks_pcie_ack_legacy_irq, >>>>>>> - .irq_mask = ks_pcie_mask_legacy_irq, >>>>>>> - .irq_unmask = ks_pcie_unmask_legacy_irq, >>>>>>> + .name = "Keystone-PCI-Legacy-IRQ", >>>>>>> + .irq_enable = ks_pcie_irq_enable, >>>>>>> + .irq_disable = ks_pcie_irq_disable, >>>>>>> + .irq_eoi = ks_pcie_irq_eoi, >>>>>>> + .irq_mask = irq_chip_mask_parent, >>>>>>> + .irq_unmask = irq_chip_unmask_parent, >>>>>>> + .irq_retrigger = irq_chip_retrigger_hierarchy, >>>>>>> + .irq_set_type = irq_chip_set_type_parent, >>>>>>> + .irq_set_affinity = irq_chip_set_affinity_parent, >>>>>>> }; >>>>>>> >>>>>>> -static int ks_pcie_init_legacy_irq_map(struct irq_domain *d, >>>>>>> - unsigned int irq, >>>>>>> - irq_hw_number_t hw_irq) >>>>>>> +static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain, >>>>>>> + unsigned int virq, >>>>>>> + unsigned int nr_irqs, void *data) >>>>>>> { >>>>>>> - irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip, >>>>>>> - handle_level_irq); >>>>>>> - irq_set_chip_data(irq, d->host_data); >>>>>>> + struct keystone_pcie *ks_pcie = domain->host_data; >>>>>>> + struct device_node *np = ks_pcie->legacy_intc_np; >>>>>>> + struct irq_fwspec parent_fwspec, *fwspec = data; >>>>>>> + struct of_phandle_args out_irq; >>>>>>> + int ret, i; >>>>>>> + >>>>>>> + if (nr_irqs != 1) >>>>>>> + return -EINVAL; >>>>>>> + >>>>>>> + ret = of_irq_parse_one(np, fwspec->param[0], &out_irq); >>>>>>> + if (ret < 0) { >>>>>>> + pr_err("Failed to parse interrupt node\n"); >>>>>>> + return ret; >>>>>>> + } >>>>> >>>>> What it this trying to do? Fishing out the interrupts from DT based on >>>>> the legacy pin? This looks at best obscure. I wonder why you don't do >>>>> that at probe time instead. Anyway, this requires documenting. >>>> >>>> The device-tree of PCIe node looks something like below. >>>> >>>> interrupt-map = <0 0 0 1 &pcie_intc0 0 IRQ_TYPE_EDGE_RISING>, /* INT A */ >>>> <0 0 0 2 &pcie_intc0 1 IRQ_TYPE_EDGE_RISING>, /* INT B */ >>>> <0 0 0 3 &pcie_intc0 2 IRQ_TYPE_EDGE_RISING>, /* INT C */ >>>> <0 0 0 4 &pcie_intc0 3 IRQ_TYPE_EDGE_RISING>; /* INT D */ >>>> pcie_intc0: legacy-interrupt-controller { >>>> interrupt-controller; >>>> #interrupt-cells = <2>; >>>> interrupt-parent = <&gic>; >>>> interrupts = <GIC_SPI 48 IRQ_TYPE_EDGE_RISING>, >>>> <GIC_SPI 49 IRQ_TYPE_EDGE_RISING>, >>>> <GIC_SPI 50 IRQ_TYPE_EDGE_RISING>, >>>> <GIC_SPI 51 IRQ_TYPE_EDGE_RISING>; >>>> }; >>>> >>>> INTA corresponds to HWIRQ '0' of the hierarchy irq domain we create in this >>>> driver which in turn corresponds to GIC_SPI '48' of GIC. >>>> >>>> We could create an array of parent_fwspec for each of the four interrupt lines >>>> in legacy-interrupt-controller during probe and use it directly here while >>>> invoking irq_domain_alloc_irqs_parent. I think you want me to do that instead >>>> of using of_irq_parse_one here? >>> >>> Up to you. I don't mind either way, but it is the lack of >>> documentation that annoys me the most here. >> >> Okay, I'll add a comment before invoking of_irq_parse_one. >>> >>> The more worrying part is that you are using an EDGE signalling in >>> these bindings, and that's wrong. PCI legacy interrupts are level, >>> always. >> >> Agreed. But the HW uses pulse signal whenever it sees an Assert_Intx message. > > I'm not convinced... See below. > >>> >>> [...] >>> >>>>> From the DT binding: >>>>> >>>>> pcie_intc: Interrupt controller device node for Legacy IRQ chip >>>>> interrupt-cells: should be set to 1 >>>>> >>>>> So why do we have 2 cells here? >>>> >>>> With '1' cells, it's not possible to specify irq trigger type. DT >>>> binding has to be fixed. I'll fix this in the next revision of the >>>> patch series. >>> >>> As I said above, this is wrong. Legacy interrupts are always level, by >>> definition. Why are you making them edge-triggered >>> >>>>> >>>>>>> + >>>>>>> + if (fwspec->param[0] >= PCI_NUM_INTX) >>>>>>> + return -EINVAL; >>>>> >>>>> Most of the OF code assumes that the pin number describing the legacy >>>>> interrupt is 1-based, while you obviously treat it as 0-based. How does >>>>> it work? >>>> >>>> INTA corresponds to '0' of the hierarchy interrupt domain (using >>>> interrupt-map). >>> >>> I'm not disputing this. It is more that the DT code does number the >>> pin from 1, if memory serves well. Can you please verify this (see >>> of_irq_parse_pci). >> >> Yes it does number from '1'. However I use interrupt-map property to map from >> legacy interrupt pin to a interrupt number of the legacy interrupt controller >> which starts from '0'. > > If you've verified that it actually works, fine bby me. > >>> >>>>> >>>>>>> + >>>>>>> + *hwirq = fwspec->param[0]; >>>>>>> + *type = fwspec->param[1]; >>>>> >>>>> As far as I remember, PCI legacy interrupts are level triggered, so >>>>> there should be no need to advertise the trigger (which is consistent >>>>> with the way the binding is written). >>>> >>>> It is pulse triggered at subsystem level. Quoting the TRM >>>> "The interrupt request signal at the PCIe SS boundary is a pulse signal that is >>>> triggered each time an assert interrupt message is received." The PCIe >>>> subsystem also has a level signal (interrupt pending signal) but the interrupt >>>> request signal is the one that is connected to GIC. >>> >>> This looks completely wrong. MSIs are always edge, and legacy always >>> level, end of story. If your HW is any different, then it doesn't seem >>> to be compliant with PCI. >> >> Agree that Legacy interrupts ought to be level-triggered. I checked this with >> HW team and I've copy-pasted the response. >> >> "The legacy interrupts from the PCIe controller are still level. The PCIe >> controller will hold the legacy interrupt lines high until a de-assert message >> is sent back. The pulse interrupt is only to send the interrupt from the PCIe >> sub-system to the SoC interrupt controller. This should not impact the >> operation of the legacy interrupt on the PCIe side." >> >> Looks like the interrupts are propagated multiple levels and only the last >> level to the interrupt controller is pulse. > > Let's take an example: My device (let's assume a network controller of > some sort) generates an interrupt. Level is high, and you observe an > edge. The driver handles the interrupt, but due to a race, the line > stays high (the device has received a new packet). > > How is a *new* edge generated? The line never transited to low, and we > have more packet to process. With a level triggered interrupt, you just > take the interrupt again. With an edge interrupt, you need to actively > resample the level and retrigger the interrupt. How is this done? There might be an actual issue in the HW while converting from level to edge where new edge interrupt might not be generated in the case you just mentioned. I am checking this with HW folks. Since this might take time, I'll remove this patch and resend only the MSI cleanup in order to make progress. Thanks Kishon
On Tue, 19 Mar 2019 16:22:39 +0530 Kishon Vijay Abraham I <kishon@ti.com> wrote: Hi Kishon, > Hi Marc, > > On 18/03/19 4:46 PM, Marc Zyngier wrote: > > On Mon, 18 Mar 2019 11:03:07 +0530 > > Kishon Vijay Abraham I <kishon@ti.com> wrote: > > > >> Hi Marc, > >> > >> On 07/03/19 5:32 PM, Marc Zyngier wrote: > >>> On Thu, 07 Mar 2019 09:12:30 +0000, > >>> Kishon Vijay Abraham I <kishon@ti.com> wrote: [...] > >>>> It is pulse triggered at subsystem level. Quoting the TRM > >>>> "The interrupt request signal at the PCIe SS boundary is a pulse signal that is > >>>> triggered each time an assert interrupt message is received." The PCIe > >>>> subsystem also has a level signal (interrupt pending signal) but the interrupt > >>>> request signal is the one that is connected to GIC. > >>> > >>> This looks completely wrong. MSIs are always edge, and legacy always > >>> level, end of story. If your HW is any different, then it doesn't seem > >>> to be compliant with PCI. > >> > >> Agree that Legacy interrupts ought to be level-triggered. I checked this with > >> HW team and I've copy-pasted the response. > >> > >> "The legacy interrupts from the PCIe controller are still level. The PCIe > >> controller will hold the legacy interrupt lines high until a de-assert message > >> is sent back. The pulse interrupt is only to send the interrupt from the PCIe > >> sub-system to the SoC interrupt controller. This should not impact the > >> operation of the legacy interrupt on the PCIe side." > >> > >> Looks like the interrupts are propagated multiple levels and only the last > >> level to the interrupt controller is pulse. > > > > Let's take an example: My device (let's assume a network controller of > > some sort) generates an interrupt. Level is high, and you observe an > > edge. The driver handles the interrupt, but due to a race, the line > > stays high (the device has received a new packet). > > > > How is a *new* edge generated? The line never transited to low, and we > > have more packet to process. With a level triggered interrupt, you just > > take the interrupt again. With an edge interrupt, you need to actively > > resample the level and retrigger the interrupt. How is this done? > > There might be an actual issue in the HW while converting from level to edge > where new edge interrupt might not be generated in the case you just mentioned. > I am checking this with HW folks. There is (non-PCI) HW that does that. On EOI, they force the interrupt controller to resample the level, which results in a new edge to be generated if the level is high. This of course require some HW, usually a register taking the ID of the line that needs resampling. I'd be surprised if there wasn't something along those lines in your HW, at least as a debug mechanism. > Since this might take time, I'll remove this patch and resend only the MSI > cleanup in order to make progress. OK. Thanks, M. -- Without deviation from the norm, progress is not possible.
diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c index 47f0dcf638f2..7f1648453f54 100644 --- a/drivers/pci/controller/dwc/pci-keystone.c +++ b/drivers/pci/controller/dwc/pci-keystone.c @@ -61,6 +61,7 @@ #define IRQ_STATUS(n) (0x184 + ((n) << 4)) #define IRQ_ENABLE_SET(n) (0x188 + ((n) << 4)) +#define IRQ_ENABLE_CLR(n) (0x18C + ((n) << 4)) #define INTx_EN BIT(0) #define ERR_IRQ_STATUS 0x1c4 @@ -87,7 +88,6 @@ struct keystone_pcie { struct dw_pcie *pci; /* PCI Device ID */ u32 device_id; - int legacy_host_irqs[PCI_NUM_INTX]; struct device_node *legacy_intc_np; int msi_host_irqs[MAX_MSI_HOST_IRQS]; @@ -96,7 +96,6 @@ struct keystone_pcie { struct phy **phy; struct device_link **link; struct device_node *msi_intc_np; - struct irq_domain *legacy_irq_domain; struct device_node *np; int error_irq; @@ -199,26 +198,6 @@ static int ks_pcie_msi_host_init(struct pcie_port *pp) return dw_pcie_allocate_domains(pp); } -static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie, - int offset) -{ - 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); - } - - /* EOI the INTx interrupt */ - ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset); -} - static void ks_pcie_enable_error_irq(struct keystone_pcie *ks_pcie) { ks_pcie_app_writel(ks_pcie, ERR_IRQ_ENABLE_SET, ERR_IRQ_ALL); @@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie) return IRQ_HANDLED; } -static void ks_pcie_ack_legacy_irq(struct irq_data *d) +void ks_pcie_irq_eoi(struct irq_data *data) { + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); + irq_hw_number_t hwirq = data->hwirq; + + ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq); + irq_chip_eoi_parent(data); } -static void ks_pcie_mask_legacy_irq(struct irq_data *d) +void ks_pcie_irq_enable(struct irq_data *data) { + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); + irq_hw_number_t hwirq = data->hwirq; + + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN); + irq_chip_enable_parent(data); } -static void ks_pcie_unmask_legacy_irq(struct irq_data *d) +void ks_pcie_irq_disable(struct irq_data *data) { + struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data); + irq_hw_number_t hwirq = data->hwirq; + + ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN); + irq_chip_disable_parent(data); } static struct irq_chip ks_pcie_legacy_irq_chip = { - .name = "Keystone-PCI-Legacy-IRQ", - .irq_ack = ks_pcie_ack_legacy_irq, - .irq_mask = ks_pcie_mask_legacy_irq, - .irq_unmask = ks_pcie_unmask_legacy_irq, + .name = "Keystone-PCI-Legacy-IRQ", + .irq_enable = ks_pcie_irq_enable, + .irq_disable = ks_pcie_irq_disable, + .irq_eoi = ks_pcie_irq_eoi, + .irq_mask = irq_chip_mask_parent, + .irq_unmask = irq_chip_unmask_parent, + .irq_retrigger = irq_chip_retrigger_hierarchy, + .irq_set_type = irq_chip_set_type_parent, + .irq_set_affinity = irq_chip_set_affinity_parent, }; -static int ks_pcie_init_legacy_irq_map(struct irq_domain *d, - unsigned int irq, - irq_hw_number_t hw_irq) +static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain, + unsigned int virq, + unsigned int nr_irqs, void *data) { - irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip, - handle_level_irq); - irq_set_chip_data(irq, d->host_data); + struct keystone_pcie *ks_pcie = domain->host_data; + struct device_node *np = ks_pcie->legacy_intc_np; + struct irq_fwspec parent_fwspec, *fwspec = data; + struct of_phandle_args out_irq; + int ret, i; + + if (nr_irqs != 1) + return -EINVAL; + + ret = of_irq_parse_one(np, fwspec->param[0], &out_irq); + if (ret < 0) { + pr_err("Failed to parse interrupt node\n"); + return ret; + } + + parent_fwspec.fwnode = &out_irq.np->fwnode; + parent_fwspec.param_count = out_irq.args_count; + + for (i = 0; i < out_irq.args_count; i++) + parent_fwspec.param[i] = out_irq.args[i]; + + ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec); + if (ret < 0) { + pr_err("Failed to allocate parent irq %u: %d\n", + parent_fwspec.param[0], ret); + return ret; + } + + ret = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0], + &ks_pcie_legacy_irq_chip, ks_pcie); + if (ret < 0) { + pr_err("Failed to set hwirq and chip\n"); + goto err_set_hwirq_and_chip; + } return 0; + +err_set_hwirq_and_chip: + irq_domain_free_irqs_parent(domain, virq, 1); + + return ret; +} + +static int ks_pcie_irq_domain_translate(struct irq_domain *domain, + struct irq_fwspec *fwspec, + unsigned long *hwirq, + unsigned int *type) +{ + if (is_of_node(fwspec->fwnode)) { + if (fwspec->param_count != 2) + return -EINVAL; + + if (fwspec->param[0] >= PCI_NUM_INTX) + return -EINVAL; + + *hwirq = fwspec->param[0]; + *type = fwspec->param[1]; + + return 0; + } + + return -EINVAL; } static const struct irq_domain_ops ks_pcie_legacy_irq_domain_ops = { - .map = ks_pcie_init_legacy_irq_map, - .xlate = irq_domain_xlate_onetwocell, + .alloc = ks_pcie_legacy_irq_domain_alloc, + .free = irq_domain_free_irqs_common, + .translate = ks_pcie_irq_domain_translate, }; /** @@ -572,35 +629,6 @@ static void ks_pcie_msi_irq_handler(struct irq_desc *desc) chained_irq_exit(chip, desc); } -/** - * ks_pcie_legacy_irq_handler() - Handle legacy interrupt - * @irq: IRQ line for legacy interrupts - * @desc: Pointer to irq descriptor - * - * Traverse through pending legacy interrupts and invoke handler for each. Also - * takes care of interrupt controller level mask/ack operation. - */ -static void ks_pcie_legacy_irq_handler(struct irq_desc *desc) -{ - unsigned int irq = irq_desc_get_irq(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); - - dev_dbg(dev, ": Handling legacy irq %d\n", irq); - - /* - * The chained irq handler installation would have replaced normal - * interrupt driver handler so we need to take care of mask/unmask and - * ack operation. - */ - chained_irq_enter(chip, desc); - ks_pcie_handle_legacy_irq(ks_pcie, irq_offset); - chained_irq_exit(chip, desc); -} - static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie) { struct device *dev = ks_pcie->pci->dev; @@ -655,14 +683,33 @@ static int ks_pcie_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 irq_domain *parent_domain; + struct device_node *parent_node; struct device_node *intc_np; - int irq_count, irq, ret = 0, i; + int irq_count, ret = 0; intc_np = of_get_child_by_name(np, "legacy-interrupt-controller"); if (!intc_np) { dev_warn(dev, "legacy-interrupt-controller node is absent\n"); return -EINVAL; } + ks_pcie->legacy_intc_np = intc_np; + + parent_node = of_irq_find_parent(intc_np); + if (!parent_node) { + dev_err(dev, "unable to obtain parent node\n"); + ret = -ENXIO; + goto err; + } + + parent_domain = irq_find_host(parent_node); + if (!parent_domain) { + dev_err(dev, "unable to obtain parent domain\n"); + ret = -ENXIO; + goto err; + } + + of_node_put(parent_node); irq_count = of_irq_count(intc_np); if (!irq_count) { @@ -671,31 +718,15 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) goto err; } - for (i = 0; i < irq_count; i++) { - irq = irq_of_parse_and_map(intc_np, i); - if (!irq) { - ret = -EINVAL; - goto err; - } - ks_pcie->legacy_host_irqs[i] = irq; - - irq_set_chained_handler_and_data(irq, - ks_pcie_legacy_irq_handler, - ks_pcie); - } - legacy_irq_domain = - irq_domain_add_linear(intc_np, PCI_NUM_INTX, - &ks_pcie_legacy_irq_domain_ops, NULL); + irq_domain_add_hierarchy(parent_domain, 0, PCI_NUM_INTX, + intc_np, + &ks_pcie_legacy_irq_domain_ops, + ks_pcie); if (!legacy_irq_domain) { dev_err(dev, "Failed to add irq domain for legacy irqs\n"); ret = -EINVAL; - goto err; } - 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); err: of_node_put(intc_np);
K2G provides separate IRQ lines for each of the four legacy interrupts. Model this using hierarchy domain instead of linear domain with chained IRQ handler. Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> --- drivers/pci/controller/dwc/pci-keystone.c | 205 +++++++++++++--------- 1 file changed, 118 insertions(+), 87 deletions(-) -- 2.17.1