From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759390AbbCDJAp (ORCPT ); Wed, 4 Mar 2015 04:00:45 -0500 Received: from mail-we0-f170.google.com ([74.125.82.170]:43081 "EHLO mail-we0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753962AbbCDJAm (ORCPT ); Wed, 4 Mar 2015 04:00:42 -0500 Message-ID: <54F6C9B4.9020500@gmail.com> Date: Wed, 04 Mar 2015 10:00:36 +0100 From: Sebastian Hesselbarth User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 To: Jisheng Zhang , Alexey Brodkin CC: "linux-kernel@vger.kernel.org" , Vineet Gupta , Thomas Gleixner , Jason Cooper Subject: Re: [PATCH] irq-dw-apb-ictl: convert to platform device References: <1425394619-1461-1-git-send-email-abrodkin@synopsys.com> <20150304111112.56814be0@xhacker> In-Reply-To: <20150304111112.56814be0@xhacker> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04.03.2015 04:11, Jisheng Zhang wrote: > On Tue, 3 Mar 2015 06:56:59 -0800 > Alexey Brodkin wrote: >> This allows utilization of probe deferral if master interrupt controller >> was not yet probed. >> >> In my case there's an DW APB GPIO configured as interrupt controller >> which is a master for DW APB INTC. >> >> Interrupt controller sub-node of DW APB GPIO gets parsed only on DW APB >> GPIO real probe - before that slave interrupt controllers cannot resolve >> its master connection which lead to failures on probes of devices that >> use DW APB INTC as its interrupt controller. Alexey, thanks for the input, but the point is not to break existing users which your patch clearly does. Just to get this straight for me, you are using DW_APB_INTC as a _slave_ interrupt controller of DW_APB_GPIO _master_ interrupt controller? Is this a fictional setup to test your IP or is it a real world example? IMHO, if the setup is real, i.e. an APB_INTC hooked up to a GPIO upstream-wise, you should think of a new driver for an externally connected but APB bus accessible (???) interrupt controller. If it is rather some extension of the GPIO controller you should amend dw_apb_gpio.c instead. Sorry, but I cannot really wrap my head around that specific setup... Sebastian >> Now when this driver is converted to a normal platform device it may use >> probe deferral and so all dependences between devices and their >> interrupt controllers are resolved automatically on boot. >> >> Signed-off-by: Alexey Brodkin >> Cc: Vineet Gupta >> Cc: Thomas Gleixner >> Cc: Jason Cooper >> Cc: Jisheng Zhang >> --- >> drivers/irqchip/irq-dw-apb-ictl.c | 112 >> +++++++++++++++++++++++--------------- 1 file changed, 67 insertions(+), 45 >> deletions(-) >> >> diff --git a/drivers/irqchip/irq-dw-apb-ictl.c >> b/drivers/irqchip/irq-dw-apb-ictl.c index 53bb732..0b05c58 100644 >> --- a/drivers/irqchip/irq-dw-apb-ictl.c >> +++ b/drivers/irqchip/irq-dw-apb-ictl.c >> @@ -11,13 +11,15 @@ >> * warranty of any kind, whether express or implied. >> */ >> >> -#include > > this headfile need to be kept for readl_relaxed/writel_relaxed > >> -#include >> #include >> -#include >> #include >> +#include >> +#include > > sort alphabetically > >> >> -#include "irqchip.h" >> +struct dw_apb_intl_priv { >> + struct device *dev; >> + struct irq_domain *irq_domain; >> +}; >> >> #define APB_INT_ENABLE_L 0x00 >> #define APB_INT_ENABLE_H 0x04 >> @@ -65,41 +67,38 @@ static void dw_apb_ictl_resume(struct irq_data *d) >> #define dw_apb_ictl_resume NULL >> #endif /* CONFIG_PM */ >> >> -static int __init dw_apb_ictl_init(struct device_node *np, >> - struct device_node *parent) >> +static int dw_apb_ictl_probe(struct platform_device *pdev) >> { >> unsigned int clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN; >> - struct resource r; >> - struct irq_domain *domain; >> + struct device *dev = &pdev->dev; >> + struct device_node *np = dev->of_node; >> struct irq_chip_generic *gc; >> + struct dw_apb_intl_priv *p; >> + struct resource *res; >> void __iomem *iobase; >> int ret, nrirqs, irq; >> u32 reg; >> >> - /* Map the parent interrupt for the chained handler */ >> - irq = irq_of_parse_and_map(np, 0); >> - if (irq <= 0) { >> - pr_err("%s: unable to parse irq\n", np->full_name); >> - return -EINVAL; >> - } >> + if (!np) >> + return -ENODEV; >> >> - ret = of_address_to_resource(np, 0, &r); >> - if (ret) { >> - pr_err("%s: unable to get resource\n", np->full_name); >> - return ret; >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) { >> + if (irq != -EPROBE_DEFER) >> + dev_err(dev, "no irq resource %d\n", irq); >> + return irq; >> } >> >> - if (!request_mem_region(r.start, resource_size(&r), >> np->full_name)) { >> - pr_err("%s: unable to request mem region\n", >> np->full_name); >> + p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL); >> + if (!p) >> return -ENOMEM; >> - } >> >> - iobase = ioremap(r.start, resource_size(&r)); >> - if (!iobase) { >> - pr_err("%s: unable to map resource\n", np->full_name); >> - ret = -ENOMEM; >> - goto err_release; >> - } >> + p->dev = &pdev->dev; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + iobase = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(iobase)) >> + return PTR_ERR(iobase); >> >> /* >> * DW IP can be configured to allow 2-64 irqs. We can determine >> @@ -120,25 +119,27 @@ static int __init dw_apb_ictl_init(struct device_node >> *np, else >> nrirqs = fls(readl_relaxed(iobase + APB_INT_ENABLE_L)); >> >> - domain = irq_domain_add_linear(np, nrirqs, >> - &irq_generic_chip_ops, NULL); >> - if (!domain) { >> + p->irq_domain = irq_domain_add_linear(np, nrirqs, >> + &irq_generic_chip_ops, NULL); >> + if (!p->irq_domain) { >> pr_err("%s: unable to add irq domain\n", np->full_name); >> - ret = -ENOMEM; >> - goto err_unmap; >> + return -ENOMEM; >> } >> >> - ret = irq_alloc_domain_generic_chips(domain, 32, (nrirqs > 32) ? >> 2 : 1, >> - np->name, handle_level_irq, >> clr, 0, >> + ret = irq_alloc_domain_generic_chips(p->irq_domain, 32, >> + (nrirqs > 32) ? 2 : 1, >> np->name, >> + handle_level_irq, clr, 0, >> IRQ_GC_MASK_CACHE_PER_TYPE | >> IRQ_GC_INIT_MASK_CACHE); >> if (ret) { >> - pr_err("%s: unable to alloc irq domain gc\n", >> np->full_name); >> - goto err_unmap; >> + dev_info(p->dev, "irq_alloc_domain_generic_chips >> failed\n"); >> + irq_domain_remove(p->irq_domain); >> + p->irq_domain = NULL; >> + return -ENOMEM; >> } >> >> - gc = irq_get_domain_generic_chip(domain, 0); >> - gc->private = domain; >> + gc = irq_get_domain_generic_chip(p->irq_domain, 0); >> + gc->private = p->irq_domain; >> gc->reg_base = iobase; >> >> gc->chip_types[0].regs.mask = APB_INT_MASK_L; >> @@ -159,12 +160,33 @@ static int __init dw_apb_ictl_init(struct device_node >> *np, irq_set_chained_handler(irq, dw_apb_ictl_handler); >> >> return 0; >> +} >> >> -err_unmap: >> - iounmap(iobase); >> -err_release: >> - release_mem_region(r.start, resource_size(&r)); >> - return ret; >> +static int dw_apb_ictl_remove(struct platform_device *pdev) >> +{ >> + struct dw_apb_intl_priv *p = platform_get_drvdata(pdev); >> + >> + irq_domain_remove(p->irq_domain); >> + return 0; >> } >> -IRQCHIP_DECLARE(dw_apb_ictl, >> - "snps,dw-apb-ictl", dw_apb_ictl_init); > > So, the ictl won't be initialized in init_IRQ(), it will cause problems. > >> + >> +static const struct of_device_id dw_apb_ictl_dt_ids[] = { >> + { .compatible = "snps,dw-apb-ictl", }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, dw_apb_ictl_dt_ids); >> + >> +static struct platform_driver dw_apb_ictl_device_driver = { >> + .probe = dw_apb_ictl_probe, >> + .remove = dw_apb_ictl_remove, >> + .driver = { >> + .name = "synopsys_dw_apb_irq", >> + .of_match_table = of_match_ptr(dw_apb_ictl_dt_ids), >> + } >> +}; >> + >> +module_platform_driver(dw_apb_ictl_device_driver); > > This is too late. If the ictl is needed during early boot, for example > on Marvell BG2Q DMP board, we still need to calibrate the delay loop, the > kernel will hang at "Calibrating delay loop..." > > Thanks, > Jisheng >