| Message ID | 1438585198-8764-2-git-send-email-zhaoshenglong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 3, 2015 at 8:59 AM, Shannon Zhao <zhaoshenglong@huawei.com> wrote: > From: Shannon Zhao <shannon.zhao@linaro.org> > > Since PL061 currently only supports AMBA driver, to support using GPIO > PL061 by DT or ACPI, it needs to add support for platform driver. > A DT binding is provided with this patch, ACPI support is added in a > separate one. > > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> I already stated in 0/2 what the problem is with this, and it's obviously adding a huge codechunk, very similar to the AMBA probe path. We need to investigate if ACPI can just spawn AMBA/PrimeCell type devices instead. Yours, Linus Walleij
On Mon, 3 Aug 2015, at 01:40 PM, Linus Walleij wrote: > On Mon, Aug 3, 2015 at 8:59 AM, Shannon Zhao <zhaoshenglong@huawei.com> > wrote: > > > From: Shannon Zhao <shannon.zhao@linaro.org> > > > > Since PL061 currently only supports AMBA driver, to support using GPIO > > PL061 by DT or ACPI, it needs to add support for platform driver. > > A DT binding is provided with this patch, ACPI support is added in a > > separate one. > > > > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> > > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > > I already stated in 0/2 what the problem is with this, and it's obviously > adding > a huge codechunk, very similar to the AMBA probe path. > > We need to investigate if ACPI can just spawn AMBA/PrimeCell type devices > instead. > AMBA DT support is a hideous hack, we had kind of hoped to avoid doing that in ACPI. If you notice ARM already updated pl011 driver to support platform device type probing this seems to be the way to go. Graeme
On Mon, Aug 03, 2015 at 04:13:31PM +0100, Graeme Gregory wrote: > > > On Mon, 3 Aug 2015, at 01:40 PM, Linus Walleij wrote: > > On Mon, Aug 3, 2015 at 8:59 AM, Shannon Zhao <zhaoshenglong@huawei.com> > > wrote: > > > > > From: Shannon Zhao <shannon.zhao@linaro.org> > > > > > > Since PL061 currently only supports AMBA driver, to support using GPIO > > > PL061 by DT or ACPI, it needs to add support for platform driver. > > > A DT binding is provided with this patch, ACPI support is added in a > > > separate one. > > > > > > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> > > > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > > > > I already stated in 0/2 what the problem is with this, and it's obviously > > adding > > a huge codechunk, very similar to the AMBA probe path. > > > > We need to investigate if ACPI can just spawn AMBA/PrimeCell type devices > > instead. > > > AMBA DT support is a hideous hack, we had kind of hoped to avoid doing > that in ACPI. > > If you notice ARM already updated pl011 driver to support platform > device type probing this seems to be the way to go. As the AMBA bus maintainer, NAK on that.
On Mon, Aug 3, 2015 at 5:13 PM, Graeme Gregory <gg@slimlogic.co.uk> wrote: > On Mon, 3 Aug 2015, at 01:40 PM, Linus Walleij wrote: >> On Mon, Aug 3, 2015 at 8:59 AM, Shannon Zhao <zhaoshenglong@huawei.com> >> wrote: >> >> > From: Shannon Zhao <shannon.zhao@linaro.org> >> > >> > Since PL061 currently only supports AMBA driver, to support using GPIO >> > PL061 by DT or ACPI, it needs to add support for platform driver. >> > A DT binding is provided with this patch, ACPI support is added in a >> > separate one. >> > >> > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> >> >> I already stated in 0/2 what the problem is with this, and it's obviously >> adding >> a huge codechunk, very similar to the AMBA probe path. >> >> We need to investigate if ACPI can just spawn AMBA/PrimeCell type devices >> instead. > > AMBA DT support is a hideous hack, we had kind of hoped to avoid doing > that in ACPI. Define what you mean with "hideous hack". I think it is elegant. It combines the DT aspect of telling where the device is with the Plug-and-play aspect of reading the 0xB105F00D magic in the PrimeCells very nicely. With an optional override mechanism. It's nice. The runtime PM stuff and pclk handling that Russell and also Ulf has worked on is very helpful and takes out a lot of codelines in a lot of drivers. Also very nice. > If you notice ARM already updated pl011 driver to support platform > device type probing this seems to be the way to go. Just because there is a precedent does not mean it is good. Continuing with this adds 100+ lines of hairy initialization to every AMBA driver. What needs to happen is stop this and make ACPI spawn AMBA devices for PrimeCells, and move the serial port over to that. A compromise may be to have something that probes a list of these platform devices in drivers/acpi/ and spawn a corresponding AMBA device for it. It will still save codelines in all other subsystems and lower maintenance for the subsystem maintainers. Yours, Linus Walleij
On Thu, 13 Aug 2015, at 01:14 PM, Linus Walleij wrote: > On Mon, Aug 3, 2015 at 5:13 PM, Graeme Gregory <gg@slimlogic.co.uk> > wrote: > > On Mon, 3 Aug 2015, at 01:40 PM, Linus Walleij wrote: > >> On Mon, Aug 3, 2015 at 8:59 AM, Shannon Zhao <zhaoshenglong@huawei.com> > >> wrote: > >> > >> > From: Shannon Zhao <shannon.zhao@linaro.org> > >> > > >> > Since PL061 currently only supports AMBA driver, to support using GPIO > >> > PL061 by DT or ACPI, it needs to add support for platform driver. > >> > A DT binding is provided with this patch, ACPI support is added in a > >> > separate one. > >> > > >> > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> > >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > >> > >> I already stated in 0/2 what the problem is with this, and it's obviously > >> adding > >> a huge codechunk, very similar to the AMBA probe path. > >> > >> We need to investigate if ACPI can just spawn AMBA/PrimeCell type devices > >> instead. > > > > AMBA DT support is a hideous hack, we had kind of hoped to avoid doing > > that in ACPI. > > Define what you mean with "hideous hack". > drivers/of/platform.c search for #ifdef CONFIG_ARM_AMBA > I think it is elegant. It combines the DT aspect of telling where the > device is with the Plug-and-play aspect of reading the 0xB105F00D > magic in the PrimeCells very nicely. With an optional override > mechanism. It's nice. > > The runtime PM stuff and pclk handling that Russell and also Ulf > has worked on is very helpful and takes out a lot of codelines > in a lot of drivers. Also very nice. > I have no objection to AMBA devices that are children of an AMBA bus object. Its the hack above that is nasty! It is also un-scalable as it sets the precedent that the platform.c in both DT and ACPI should learn about all the different types of device probing. > > If you notice ARM already updated pl011 driver to support platform > > device type probing this seems to be the way to go. > > Just because there is a precedent does not mean it is good. > > Continuing with this adds 100+ lines of hairy initialization to > every AMBA driver. > > What needs to happen is stop this and make ACPI spawn > AMBA devices for PrimeCells, and move the serial port over > to that. > > A compromise may be to have something that probes a list > of these platform devices in drivers/acpi/ and spawn a > corresponding AMBA device for it. It will still save codelines in > all other subsystems and lower maintenance for the > subsystem maintainers. > I am currently working on exactly what you describe above. Graeme
On Thu, Aug 13, 2015 at 01:25:13PM +0100, Graeme Gregory wrote: > > > On Thu, 13 Aug 2015, at 01:14 PM, Linus Walleij wrote: > > On Mon, Aug 3, 2015 at 5:13 PM, Graeme Gregory <gg@slimlogic.co.uk> > > wrote: > > > On Mon, 3 Aug 2015, at 01:40 PM, Linus Walleij wrote: > > >> On Mon, Aug 3, 2015 at 8:59 AM, Shannon Zhao <zhaoshenglong@huawei.com> > > >> wrote: > > >> > > >> > From: Shannon Zhao <shannon.zhao@linaro.org> > > >> > > > >> > Since PL061 currently only supports AMBA driver, to support using GPIO > > >> > PL061 by DT or ACPI, it needs to add support for platform driver. > > >> > A DT binding is provided with this patch, ACPI support is added in a > > >> > separate one. > > >> > > > >> > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> > > >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > > >> > > >> I already stated in 0/2 what the problem is with this, and it's obviously > > >> adding > > >> a huge codechunk, very similar to the AMBA probe path. > > >> > > >> We need to investigate if ACPI can just spawn AMBA/PrimeCell type devices > > >> instead. > > > > > > AMBA DT support is a hideous hack, we had kind of hoped to avoid doing > > > that in ACPI. > > > > Define what you mean with "hideous hack". > > > drivers/of/platform.c search for #ifdef CONFIG_ARM_AMBA The way that AMBA support was added to OF was not something I was involved in. Don't make the mistake of saying "we're only supporting platform devices in ACPI because the OF implementation is messed up". > > I think it is elegant. It combines the DT aspect of telling where the > > device is with the Plug-and-play aspect of reading the 0xB105F00D > > magic in the PrimeCells very nicely. With an optional override > > mechanism. It's nice. It's also the _right_ way to deal with buses with different properties from platform devices, especially where some can be probed and identified via hardware-encoded IDs. > I have no objection to AMBA devices that are children of an AMBA bus > object. Its the hack above that is nasty! It is also un-scalable as it > sets the precedent that the platform.c in both DT and ACPI should learn > about all the different types of device probing. What's actually the nasty thing is that DT and ACPI are wedded to platform devices, which pushes people down the path of more platform device usage, when in fact they should be creating their own bus classes. > > > If you notice ARM already updated pl011 driver to support platform > > > device type probing this seems to be the way to go. > > > > Just because there is a precedent does not mean it is good. Exactly. > > Continuing with this adds 100+ lines of hairy initialization to > > every AMBA driver. And that's utterly insane. > > What needs to happen is stop this and make ACPI spawn > > AMBA devices for PrimeCells, and move the serial port over > > to that. > > > > A compromise may be to have something that probes a list > > of these platform devices in drivers/acpi/ and spawn a > > corresponding AMBA device for it. It will still save codelines in > > all other subsystems and lower maintenance for the > > subsystem maintainers. Even that isn't nice... but I guess as the madness will be contained within ACPI, it'll be ACPI's problem to maintain, so up to them how to deal with it provided the code is contained within ACPI. :)
diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c index 0475613..64c10eb 100644 --- a/drivers/gpio/gpio-pl061.c +++ b/drivers/gpio/gpio-pl061.c @@ -24,6 +24,7 @@ #include <linux/slab.h> #include <linux/pinctrl/consumer.h> #include <linux/pm.h> +#include <linux/platform_device.h> #define GPIODIR 0x400 #define GPIOIS 0x404 @@ -241,6 +242,94 @@ static struct irq_chip pl061_irqchip = { .irq_set_type = pl061_irq_type, }; +static int pl061_platform_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct pl061_platform_data *pdata = dev_get_platdata(dev); + struct pl061_gpio *chip; + int ret, irq, i, irq_base; + struct resource *mem; + + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!mem) + return -EINVAL; + + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); + if (chip == NULL) + return -ENOMEM; + + if (pdata) { + chip->gc.base = pdata->gpio_base; + irq_base = pdata->irq_base; + if (irq_base <= 0) { + dev_err(&pdev->dev, "invalid IRQ base in pdata\n"); + return -ENODEV; + } + } else { + chip->gc.base = -1; + irq_base = 0; + } + + chip->base = devm_ioremap(dev, mem->start, resource_size(mem)); + if (IS_ERR(chip->base)) + return PTR_ERR(chip->base); + + spin_lock_init(&chip->lock); + if (of_property_read_bool(dev->of_node, "gpio-ranges")) + chip->uses_pinctrl = true; + + chip->gc.request = pl061_gpio_request; + chip->gc.free = pl061_gpio_free; + chip->gc.direction_input = pl061_direction_input; + chip->gc.direction_output = pl061_direction_output; + chip->gc.get = pl061_get_value; + chip->gc.set = pl061_set_value; + chip->gc.ngpio = PL061_GPIO_NR; + chip->gc.label = dev_name(dev); + chip->gc.dev = dev; + chip->gc.owner = THIS_MODULE; + + ret = gpiochip_add(&chip->gc); + if (ret) + return ret; + + /* + * irq_chip support + */ + writeb(0, chip->base + GPIOIE); /* disable irqs */ + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + dev_err(&pdev->dev, "invalid IRQ\n"); + return -ENODEV; + } + + ret = gpiochip_irqchip_add(&chip->gc, &pl061_irqchip, + irq_base, handle_simple_irq, + IRQ_TYPE_NONE); + if (ret) { + dev_info(&pdev->dev, "could not add irqchip\n"); + return ret; + } + gpiochip_set_chained_irqchip(&chip->gc, &pl061_irqchip, + irq, pl061_irq_handler); + + for (i = 0; i < PL061_GPIO_NR; i++) { + if (pdata) { + if (pdata->directions & (BIT(i))) + pl061_direction_output(&chip->gc, i, + pdata->values & (BIT(i))); + else + pl061_direction_input(&chip->gc, i); + } + } + + platform_set_drvdata(pdev, chip); + dev_info(&pdev->dev, "PL061 GPIO chip @%pa registered\n", + &mem->start); + + return 0; +} + static int pl061_probe(struct amba_device *adev, const struct amba_id *id) { struct device *dev = &adev->dev; @@ -376,6 +465,23 @@ static const struct dev_pm_ops pl061_dev_pm_ops = { }; #endif +static const struct of_device_id pl061_of_match[] = { + { .compatible = "arm,pl061", }, + {}, +}; +MODULE_DEVICE_TABLE(of, pl061_of_match); + +static struct platform_driver pl061_gpio_platform_driver = { + .driver = { + .name = "pl061_gpio", + .of_match_table = pl061_of_match, +#ifdef CONFIG_PM + .pm = &pl061_dev_pm_ops, +#endif + }, + .probe = pl061_platform_probe, +}; + static struct amba_id pl061_ids[] = { { .id = 0x00041061, @@ -399,6 +505,8 @@ static struct amba_driver pl061_gpio_driver = { static int __init pl061_gpio_init(void) { + if (platform_driver_register(&pl061_gpio_platform_driver)) + pr_warn("could not register PL061 platform driver\n"); return amba_driver_register(&pl061_gpio_driver); } module_init(pl061_gpio_init);