| Message ID | 20190325093947.32633-3-kishon@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for PCIe RC and EP mode in TI's AM654 SoC | expand |
Hi Kishon, On Mon, Mar 25, 2019 at 03:09:23PM +0530, Kishon Vijay Abraham I wrote: > pci-keystone driver uses irq_of_parse_and_map to get irq number of > error_irq. Use platform_get_irq instead and move platform_get_irq() > and request_irq() of error_irq from ks_pcie_add_pcie_port to ks_pcie_probe > since error_irq is common to both RC mode and EP mode. Does this have any DT implications? It's not obvious that platform_get_irq() and irq_of_parse_and_map() work similarly or that they get the same result from DT. I'm sure they *do*, but it would be nice to have some hints in the commit log about why that's the case. > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/pci/controller/dwc/pci-keystone.c | 43 +++++++++-------------- > 1 file changed, 17 insertions(+), 26 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index 07f55b355d75..e50f8773e768 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -98,8 +98,6 @@ struct keystone_pcie { > struct irq_domain *legacy_irq_domain; > struct device_node *np; > > - int error_irq; > - > /* Application register space */ > void __iomem *va_app_base; /* DT 1st resource */ > struct resource app; > @@ -743,12 +741,6 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) > return ret; > } > > -static void ks_pcie_setup_interrupts(struct keystone_pcie *ks_pcie) > -{ > - if (ks_pcie->error_irq > 0) > - ks_pcie_enable_error_irq(ks_pcie); > -} > - > /* > * When a PCI device does not exist during config cycles, keystone host gets a > * bus error instead of returning 0xffffffff. This handler always returns 0 > @@ -810,7 +802,6 @@ static int __init ks_pcie_host_init(struct pcie_port *pp) > > ks_pcie_stop_link(pci); > ks_pcie_setup_rc_app_regs(ks_pcie); > - ks_pcie_setup_interrupts(ks_pcie); > writew(PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8), > pci->dbi_base + PCI_IO_BASE); > > @@ -854,23 +845,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie, > struct device *dev = &pdev->dev; > int ret; > > - /* > - * Index 0 is the platform interrupt for error interrupt > - * from RC. This is optional. > - */ > - ks_pcie->error_irq = irq_of_parse_and_map(ks_pcie->np, 0); > - if (ks_pcie->error_irq <= 0) > - dev_info(dev, "no error IRQ defined\n"); > - else { > - ret = request_irq(ks_pcie->error_irq, ks_pcie_err_irq_handler, > - IRQF_SHARED, "pcie-error-irq", ks_pcie); > - if (ret < 0) { > - dev_err(dev, "failed to request error IRQ %d\n", > - ks_pcie->error_irq); > - return ret; > - } > - } > - > pp->ops = &ks_pcie_host_ops; > ret = ks_pcie_dw_host_init(ks_pcie); > if (ret) { > @@ -946,6 +920,7 @@ static int __init ks_pcie_probe(struct platform_device *pdev) > u32 num_lanes; > char name[10]; > int ret; > + int irq; > int i; > > ks_pcie = devm_kzalloc(dev, sizeof(*ks_pcie), GFP_KERNEL); > @@ -965,6 +940,20 @@ static int __init ks_pcie_probe(struct platform_device *pdev) > return ret; > } > > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(dev, "missing IRQ resource: %d\n", irq); > + return irq; > + } > + > + ret = request_irq(irq, ks_pcie_err_irq_handler, IRQF_SHARED, > + "ks-pcie-error-irq", ks_pcie); > + if (ret < 0) { > + dev_err(dev, "failed to request error IRQ %d\n", > + irq); > + return ret; > + } > + > ret = of_property_read_u32(np, "num-lanes", &num_lanes); > if (ret) > num_lanes = 1; > @@ -1020,6 +1009,8 @@ static int __init ks_pcie_probe(struct platform_device *pdev) > if (ret < 0) > goto err_get_sync; > > + ks_pcie_enable_error_irq(ks_pcie); > + > return 0; > > err_get_sync: > -- > 2.17.1 >
On Mon, Mar 25, 2019 at 03:09:23PM +0530, Kishon Vijay Abraham I wrote: > pci-keystone driver uses irq_of_parse_and_map to get irq number of > error_irq. Use platform_get_irq instead and move platform_get_irq() > and request_irq() of error_irq from ks_pcie_add_pcie_port to ks_pcie_probe > since error_irq is common to both RC mode and EP mode. Part of my confusion here is that there's no hint about why you need to use platform_get_irq() instead of irq_of_parse_and_map(). > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/pci/controller/dwc/pci-keystone.c | 43 +++++++++-------------- > 1 file changed, 17 insertions(+), 26 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index 07f55b355d75..e50f8773e768 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -98,8 +98,6 @@ struct keystone_pcie { > struct irq_domain *legacy_irq_domain; > struct device_node *np; > > - int error_irq; > - > /* Application register space */ > void __iomem *va_app_base; /* DT 1st resource */ > struct resource app; > @@ -743,12 +741,6 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) > return ret; > } > > -static void ks_pcie_setup_interrupts(struct keystone_pcie *ks_pcie) > -{ > - if (ks_pcie->error_irq > 0) > - ks_pcie_enable_error_irq(ks_pcie); > -} > - > /* > * When a PCI device does not exist during config cycles, keystone host gets a > * bus error instead of returning 0xffffffff. This handler always returns 0 > @@ -810,7 +802,6 @@ static int __init ks_pcie_host_init(struct pcie_port *pp) > > ks_pcie_stop_link(pci); > ks_pcie_setup_rc_app_regs(ks_pcie); > - ks_pcie_setup_interrupts(ks_pcie); > writew(PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8), > pci->dbi_base + PCI_IO_BASE); > > @@ -854,23 +845,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie, > struct device *dev = &pdev->dev; > int ret; > > - /* > - * Index 0 is the platform interrupt for error interrupt > - * from RC. This is optional. > - */ > - ks_pcie->error_irq = irq_of_parse_and_map(ks_pcie->np, 0); > - if (ks_pcie->error_irq <= 0) > - dev_info(dev, "no error IRQ defined\n"); > - else { > - ret = request_irq(ks_pcie->error_irq, ks_pcie_err_irq_handler, > - IRQF_SHARED, "pcie-error-irq", ks_pcie); > - if (ret < 0) { > - dev_err(dev, "failed to request error IRQ %d\n", > - ks_pcie->error_irq); > - return ret; > - } > - } > - > pp->ops = &ks_pcie_host_ops; > ret = ks_pcie_dw_host_init(ks_pcie); > if (ret) { > @@ -946,6 +920,7 @@ static int __init ks_pcie_probe(struct platform_device *pdev) > u32 num_lanes; > char name[10]; > int ret; > + int irq; > int i; > > ks_pcie = devm_kzalloc(dev, sizeof(*ks_pcie), GFP_KERNEL); > @@ -965,6 +940,20 @@ static int __init ks_pcie_probe(struct platform_device *pdev) > return ret; > } > > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(dev, "missing IRQ resource: %d\n", irq); > + return irq; > + } > + > + ret = request_irq(irq, ks_pcie_err_irq_handler, IRQF_SHARED, > + "ks-pcie-error-irq", ks_pcie); > + if (ret < 0) { > + dev_err(dev, "failed to request error IRQ %d\n", > + irq); > + return ret; > + } > + > ret = of_property_read_u32(np, "num-lanes", &num_lanes); > if (ret) > num_lanes = 1; > @@ -1020,6 +1009,8 @@ static int __init ks_pcie_probe(struct platform_device *pdev) > if (ret < 0) > goto err_get_sync; > > + ks_pcie_enable_error_irq(ks_pcie); > + > return 0; > > err_get_sync: > -- > 2.17.1 >
Hi Bjorn, On 13/04/19 7:33 PM, Bjorn Helgaas wrote: > Hi Kishon, > > On Mon, Mar 25, 2019 at 03:09:23PM +0530, Kishon Vijay Abraham I wrote: >> pci-keystone driver uses irq_of_parse_and_map to get irq number of >> error_irq. Use platform_get_irq instead and move platform_get_irq() >> and request_irq() of error_irq from ks_pcie_add_pcie_port to ks_pcie_probe >> since error_irq is common to both RC mode and EP mode. > > Does this have any DT implications? No. platform_get_irq() uses of_irq_get(), which in-turn uses of_irq_parse_one() and irq_create_of_mapping() while irq_of_parse_and_map() directly uses of_irq_parse_one() and irq_create_of_mapping(). The only difference is platform_get_irq() falls back to using platform_get_resource() if of_irq_get fails. I thought it's better to use platform_get_irq() for platform devices. Thanks Kishon > > It's not obvious that platform_get_irq() and irq_of_parse_and_map() work > similarly or that they get the same result from DT. > > I'm sure they *do*, but it would be nice to have some hints in the commit > log about why that's the case. > >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> --- >> drivers/pci/controller/dwc/pci-keystone.c | 43 +++++++++-------------- >> 1 file changed, 17 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c >> index 07f55b355d75..e50f8773e768 100644 >> --- a/drivers/pci/controller/dwc/pci-keystone.c >> +++ b/drivers/pci/controller/dwc/pci-keystone.c >> @@ -98,8 +98,6 @@ struct keystone_pcie { >> struct irq_domain *legacy_irq_domain; >> struct device_node *np; >> >> - int error_irq; >> - >> /* Application register space */ >> void __iomem *va_app_base; /* DT 1st resource */ >> struct resource app; >> @@ -743,12 +741,6 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) >> return ret; >> } >> >> -static void ks_pcie_setup_interrupts(struct keystone_pcie *ks_pcie) >> -{ >> - if (ks_pcie->error_irq > 0) >> - ks_pcie_enable_error_irq(ks_pcie); >> -} >> - >> /* >> * When a PCI device does not exist during config cycles, keystone host gets a >> * bus error instead of returning 0xffffffff. This handler always returns 0 >> @@ -810,7 +802,6 @@ static int __init ks_pcie_host_init(struct pcie_port *pp) >> >> ks_pcie_stop_link(pci); >> ks_pcie_setup_rc_app_regs(ks_pcie); >> - ks_pcie_setup_interrupts(ks_pcie); >> writew(PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8), >> pci->dbi_base + PCI_IO_BASE); >> >> @@ -854,23 +845,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie, >> struct device *dev = &pdev->dev; >> int ret; >> >> - /* >> - * Index 0 is the platform interrupt for error interrupt >> - * from RC. This is optional. >> - */ >> - ks_pcie->error_irq = irq_of_parse_and_map(ks_pcie->np, 0); >> - if (ks_pcie->error_irq <= 0) >> - dev_info(dev, "no error IRQ defined\n"); >> - else { >> - ret = request_irq(ks_pcie->error_irq, ks_pcie_err_irq_handler, >> - IRQF_SHARED, "pcie-error-irq", ks_pcie); >> - if (ret < 0) { >> - dev_err(dev, "failed to request error IRQ %d\n", >> - ks_pcie->error_irq); >> - return ret; >> - } >> - } >> - >> pp->ops = &ks_pcie_host_ops; >> ret = ks_pcie_dw_host_init(ks_pcie); >> if (ret) { >> @@ -946,6 +920,7 @@ static int __init ks_pcie_probe(struct platform_device *pdev) >> u32 num_lanes; >> char name[10]; >> int ret; >> + int irq; >> int i; >> >> ks_pcie = devm_kzalloc(dev, sizeof(*ks_pcie), GFP_KERNEL); >> @@ -965,6 +940,20 @@ static int __init ks_pcie_probe(struct platform_device *pdev) >> return ret; >> } >> >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) { >> + dev_err(dev, "missing IRQ resource: %d\n", irq); >> + return irq; >> + } >> + >> + ret = request_irq(irq, ks_pcie_err_irq_handler, IRQF_SHARED, >> + "ks-pcie-error-irq", ks_pcie); >> + if (ret < 0) { >> + dev_err(dev, "failed to request error IRQ %d\n", >> + irq); >> + return ret; >> + } >> + >> ret = of_property_read_u32(np, "num-lanes", &num_lanes); >> if (ret) >> num_lanes = 1; >> @@ -1020,6 +1009,8 @@ static int __init ks_pcie_probe(struct platform_device *pdev) >> if (ret < 0) >> goto err_get_sync; >> >> + ks_pcie_enable_error_irq(ks_pcie); >> + >> return 0; >> >> err_get_sync: >> -- >> 2.17.1 >>
diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c index 07f55b355d75..e50f8773e768 100644 --- a/drivers/pci/controller/dwc/pci-keystone.c +++ b/drivers/pci/controller/dwc/pci-keystone.c @@ -98,8 +98,6 @@ struct keystone_pcie { struct irq_domain *legacy_irq_domain; struct device_node *np; - int error_irq; - /* Application register space */ void __iomem *va_app_base; /* DT 1st resource */ struct resource app; @@ -743,12 +741,6 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie) return ret; } -static void ks_pcie_setup_interrupts(struct keystone_pcie *ks_pcie) -{ - if (ks_pcie->error_irq > 0) - ks_pcie_enable_error_irq(ks_pcie); -} - /* * When a PCI device does not exist during config cycles, keystone host gets a * bus error instead of returning 0xffffffff. This handler always returns 0 @@ -810,7 +802,6 @@ static int __init ks_pcie_host_init(struct pcie_port *pp) ks_pcie_stop_link(pci); ks_pcie_setup_rc_app_regs(ks_pcie); - ks_pcie_setup_interrupts(ks_pcie); writew(PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8), pci->dbi_base + PCI_IO_BASE); @@ -854,23 +845,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie, struct device *dev = &pdev->dev; int ret; - /* - * Index 0 is the platform interrupt for error interrupt - * from RC. This is optional. - */ - ks_pcie->error_irq = irq_of_parse_and_map(ks_pcie->np, 0); - if (ks_pcie->error_irq <= 0) - dev_info(dev, "no error IRQ defined\n"); - else { - ret = request_irq(ks_pcie->error_irq, ks_pcie_err_irq_handler, - IRQF_SHARED, "pcie-error-irq", ks_pcie); - if (ret < 0) { - dev_err(dev, "failed to request error IRQ %d\n", - ks_pcie->error_irq); - return ret; - } - } - pp->ops = &ks_pcie_host_ops; ret = ks_pcie_dw_host_init(ks_pcie); if (ret) { @@ -946,6 +920,7 @@ static int __init ks_pcie_probe(struct platform_device *pdev) u32 num_lanes; char name[10]; int ret; + int irq; int i; ks_pcie = devm_kzalloc(dev, sizeof(*ks_pcie), GFP_KERNEL); @@ -965,6 +940,20 @@ static int __init ks_pcie_probe(struct platform_device *pdev) return ret; } + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + dev_err(dev, "missing IRQ resource: %d\n", irq); + return irq; + } + + ret = request_irq(irq, ks_pcie_err_irq_handler, IRQF_SHARED, + "ks-pcie-error-irq", ks_pcie); + if (ret < 0) { + dev_err(dev, "failed to request error IRQ %d\n", + irq); + return ret; + } + ret = of_property_read_u32(np, "num-lanes", &num_lanes); if (ret) num_lanes = 1; @@ -1020,6 +1009,8 @@ static int __init ks_pcie_probe(struct platform_device *pdev) if (ret < 0) goto err_get_sync; + ks_pcie_enable_error_irq(ks_pcie); + return 0; err_get_sync:
pci-keystone driver uses irq_of_parse_and_map to get irq number of error_irq. Use platform_get_irq instead and move platform_get_irq() and request_irq() of error_irq from ks_pcie_add_pcie_port to ks_pcie_probe since error_irq is common to both RC mode and EP mode. Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> --- drivers/pci/controller/dwc/pci-keystone.c | 43 +++++++++-------------- 1 file changed, 17 insertions(+), 26 deletions(-)