| Message ID | 1421406010-14851-2-git-send-email-robert.jarzmik@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 16 Jan 2015, Robert Jarzmik wrote: > Lubbock () board is the IO motherboard of the Intel PXA25x Development > Platform, which supports the Lubbock pxa25x soc board. > > Historically, this support was in arch/arm/mach-pxa/lubbock.c. When > gpio-pxa was moved to drivers/pxa, it became a driver, and its > initialization and probing happened at postcore initcall. The lubbock > code used to install the chained lubbock interrupt handler at init_irq() > time. > > The consequence of the gpio-pxa change is that the installed chained irq > handler lubbock_irq_handler() was overwritten in pxa_gpio_probe(_dt)(), > removing : > - the handler > - the falling edge detection setting of GPIO0, which revealed the > interrupt request from the lubbock IO board. > > As a fix, move the gpio0 chained handler setup to a place where we have > the guarantee that pxa_gpio_probe() was called before, so that lubbock > handler becomes the true IRQ chained handler of GPIO0, demuxing the > lubbock IO board interrupts. How is this guaranteed? > This patch moves all that handling to a mfd driver. It's only purpose > for the time being is the interrupt handling, but in the future it > should encompass all the motherboard CPLDs handling : > - leds > - switches > - hexleds > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > --- > Since v1: change the name from cottula to lubbock_io > Dmitry pointed out the Cottula was the pxa25x family name, > lubbock was the pxa25x development board name. Therefore the > name was changed to lubbock_io (lubbock IO board) > change the resources to bi-irq ioresource > Discussion between Arnd and Robert to change the gpio > request by a irq request. > Since v2: take into account Mark's review > Use irq flags from resources (DT case and pdata case). > Change of name from lubbock_io to lubbock-io > --- > drivers/mfd/Kconfig | 10 +++ > drivers/mfd/Makefile | 1 + > drivers/mfd/lubbock.c | 196 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 207 insertions(+) > create mode 100644 drivers/mfd/lubbock.c > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 2e6b731..4d8939f 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -91,6 +91,16 @@ config MFD_AXP20X > components like regulators or the PEK (Power Enable Key) under the > corresponding menus. > > +config MFD_LUBBOCK > + bool "Lubbock Motherboard" > + def_bool ARCH_LUBBOCK > + select MFD_CORE > + help > + This driver supports the Lubbock multifunction chip found on the > + pxa25x development platform system (named Lubbock). This IO board > + supports the interrupts handling, ethernet controller, flash chips, > + etc ... > + > config MFD_CROS_EC > tristate "ChromeOS Embedded Controller" > select MFD_CORE > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 53467e2..aff1f4f 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -9,6 +9,7 @@ obj-$(CONFIG_MFD_88PM805) += 88pm805.o 88pm80x.o > obj-$(CONFIG_MFD_SM501) += sm501.o > obj-$(CONFIG_MFD_ASIC3) += asic3.o tmio_core.o > obj-$(CONFIG_MFD_BCM590XX) += bcm590xx.o > +obj-$(CONFIG_MFD_LUBBOCK) += lubbock.o > obj-$(CONFIG_MFD_CROS_EC) += cros_ec.o > obj-$(CONFIG_MFD_CROS_EC_I2C) += cros_ec_i2c.o > obj-$(CONFIG_MFD_CROS_EC_SPI) += cros_ec_spi.o > diff --git a/drivers/mfd/lubbock.c b/drivers/mfd/lubbock.c > new file mode 100644 > index 0000000..6025135 > --- /dev/null > +++ b/drivers/mfd/lubbock.c > @@ -0,0 +1,196 @@ > +/* > + * Intel Cotulla MFD - lubbock motherboard > + * > + * Copyright (C) 2014 Robert Jarzmik > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * Lubbock motherboard driver, supporting lubbock (aka. pxa25x) soc board. Please use uppercase characters i.e. Lubbock, PXA25X, SoC, etc. > + * Superfluous '\n'. > + */ > + > +#include <linux/bitops.h> > +#include <linux/gpio.h> > +#include <linux/gpio/consumer.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/irq.h> > +#include <linux/irqdomain.h> > +#include <linux/mfd/core.h> > +#include <linux/module.h> Can this be built as a module? If so, why isn't it a tristate? > +#include <linux/of_platform.h> > + > +#define COT_IRQ_MASK_EN 0xc0 > +#define COT_IRQ_SET_CLR 0xd0 > + > +#define LUBBOCK_NB_IRQ 8 > + > +struct lubbock { > + void __iomem *base; Random spacing. > + int irq; > + unsigned int irq_mask; > + struct gpio_desc *gpio0; > + struct irq_domain *irqdomain; > +}; > + > +static irqreturn_t lubbock_irq_handler(int in_irq, void *d) > +{ > + struct lubbock *cot = d; > + unsigned long pending; > + unsigned int bit; > + > + pending = readl(cot->base + COT_IRQ_SET_CLR) & cot->irq_mask; > + for_each_set_bit(bit, &pending, LUBBOCK_NB_IRQ) > + generic_handle_irq(irq_find_mapping(cot->irqdomain, bit)); I'd like to see a '\n' here. > + return IRQ_HANDLED; > +} > + > +static void lubbock_irq_mask_ack(struct irq_data *d) > +{ > + struct lubbock *cot = irq_data_get_irq_chip_data(d); > + unsigned int lubbock_irq = irqd_to_hwirq(d); > + unsigned int set, bit = BIT(lubbock_irq); > + > + cot->irq_mask &= ~bit; > + writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN); > + set = readl(cot->base + COT_IRQ_SET_CLR); > + writel(set & ~bit, cot->base + COT_IRQ_SET_CLR); > +} > + > +static void lubbock_irq_unmask(struct irq_data *d) > +{ > + struct lubbock *cot = irq_data_get_irq_chip_data(d); > + unsigned int lubbock_irq = irqd_to_hwirq(d); > + unsigned int bit = BIT(lubbock_irq); > + > + cot->irq_mask |= bit; > + writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN); > +} > + > +static struct irq_chip lubbock_irq_chip = { > + .name = "lubbock", > + .irq_mask_ack = lubbock_irq_mask_ack, > + .irq_unmask = lubbock_irq_unmask, > + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE, > +}; > + > +static int lubbock_irq_domain_map(struct irq_domain *d, unsigned int irq, > + irq_hw_number_t hwirq) > +{ > + struct lubbock *cot = d->host_data; > + > + irq_set_chip_and_handler(irq, &lubbock_irq_chip, handle_level_irq); > + irq_set_chip_data(irq, cot); Again, I'd prefer some separation between code and the return. (same in all cases below). > + return 0; > +} > + > +static const struct irq_domain_ops lubbock_irq_domain_ops = { > + .xlate = irq_domain_xlate_twocell, > + .map = lubbock_irq_domain_map, > +}; > + > +static int lubbock_resume(struct platform_device *pdev) > +{ > + struct lubbock *cot = platform_get_drvdata(pdev); > + > + writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN); > + return 0; > +} > + > +static int lubbock_probe(struct platform_device *pdev) > +{ > + struct resource *res; > + struct lubbock *cot; > + int ret; > + unsigned int base_irq = 0; > + unsigned long irqflags; > + > + cot = devm_kzalloc(&pdev->dev, sizeof(*cot), GFP_KERNEL); > + if (!cot) > + return -ENOMEM; '\n' here. > + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); platform_get_irq()? > + if (res) { > + cot->irq = (unsigned int)res->start; > + irqflags = res->flags; > + } > + if (!cot->irq) > + return -ENODEV; > + > + res = platform_get_resource(pdev, IORESOURCE_IRQ, 1); platform_get_irq()? > + if (res) > + base_irq = (unsigned int)res->start; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + cot->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(cot->base)) > + return PTR_ERR(cot->base); > + > + platform_set_drvdata(pdev, cot); > + > + writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN); > + writel(0, cot->base + COT_IRQ_SET_CLR); '\n' > + ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler, > + irqflags, dev_name(&pdev->dev), cot); > + if (ret == -ENOSYS) > + return -EPROBE_DEFER; I haven't seen anyone do this after devm_request_irq() before. Why is it required here? > + if (ret) { > + dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n", > + ret); I'm not keen on this type of formatting. Besides the system will print out the returned error on failure. > + return ret; > + } > + irq_set_irq_wake(cot->irq, 1); > + > + cot->irqdomain = > + irq_domain_add_linear(pdev->dev.of_node, LUBBOCK_NB_IRQ, > + &lubbock_irq_domain_ops, cot); As a personal preference, I would prefer to see: cot->irqdomain = irq_domain_add_linear(pdev->dev.of_node, LUBBOCK_NB_IRQ, &lubbock_irq_domain_ops, cot); > + if (!cot->irqdomain) > + return -ENODEV; > + > + ret = 0; 'ret' will be zero here, or we would have returned already. > + if (base_irq) > + ret = irq_create_strict_mappings(cot->irqdomain, base_irq, 0, > + LUBBOCK_NB_IRQ); > + if (ret) { > + dev_err(&pdev->dev, "Couldn't create the irq mapping %d..%d\n", > + base_irq, base_irq + LUBBOCK_NB_IRQ); > + return ret; > + } Is this solely the check from irq_create_strict_mappings(), therefore it should be inside the previous if () { ... }. > + dev_info(&pdev->dev, "base=%p, irq=%d, base_irq=%d\n", > + cot->base, cot->irq, base_irq); Please remove this line. > + return 0; > +} > + > +static int lubbock_remove(struct platform_device *pdev) > +{ > + struct lubbock *cot = platform_get_drvdata(pdev); > + > + irq_set_chip_and_handler(cot->irq, NULL, NULL); > + return 0; > +} > + > +static const struct of_device_id lubbock_id_table[] = { > + { .compatible = "intel,lubbock-io", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, lubbock_id_table); > + > +static struct platform_driver lubbock_driver = { > + .driver = { > + .name = "lubbock_io", > + .of_match_table = of_match_ptr(lubbock_id_table), > + }, > + .probe = lubbock_probe, > + .remove = lubbock_remove, > + .resume = lubbock_resume, > +}; > + > +module_platform_driver(lubbock_driver); > + > +MODULE_DESCRIPTION("Lubbock driver"); "Lubbock MFD Driver"? > +MODULE_AUTHOR("Robert Jarzmik"); Email. > +MODULE_LICENSE("GPL");
Lee Jones <lee.jones@linaro.org> writes: > On Fri, 16 Jan 2015, Robert Jarzmik wrote: > >> As a fix, move the gpio0 chained handler setup to a place where we have >> the guarantee that pxa_gpio_probe() was called before, so that lubbock >> handler becomes the true IRQ chained handler of GPIO0, demuxing the >> lubbock IO board interrupts. > > How is this guaranteed? In the following chunk : ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler, irqflags, dev_name(&pdev->dev), cot); if (ret == -ENOSYS) return -EPROBE_DEFER; See __setup_irq(), and see what happens if the irq chip is not set (which happens on pxa platform when the gpio driver is not registered). >> + * Lubbock motherboard driver, supporting lubbock (aka. pxa25x) soc board. > > Please use uppercase characters i.e. Lubbock, PXA25X, SoC, etc. OK, your tree, your rules. > Superfluous '\n'. Yep. > Can this be built as a module? Not in its current form. > If so, why isn't it a tristate? See above. Now the question is : should it be buildable as a module ? I was thinking it shouldn't because without this driver lubbock becomes a bit useless (most of its peripherals are on the motherboard). >> +struct lubbock { >> + void __iomem *base; > > Random spacing. Right. >> + pending = readl(cot->base + COT_IRQ_SET_CLR) & cot->irq_mask; >> + for_each_set_bit(bit, &pending, LUBBOCK_NB_IRQ) >> + generic_handle_irq(irq_find_mapping(cot->irqdomain, bit)); > > I'd like to see a '\n' here. OK. > Again, I'd prefer some separation between code and the return. > > (same in all cases below). OK. > >> + cot = devm_kzalloc(&pdev->dev, sizeof(*cot), GFP_KERNEL); >> + if (!cot) >> + return -ENOMEM; > > '\n' here. OK. >> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > platform_get_irq()? No. I need the flags. >> + if (res) { >> + cot->irq = (unsigned int)res->start; >> + irqflags = res->flags; >> + } >> + if (!cot->irq) >> + return -ENODEV; >> + >> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 1); > > platform_get_irq()? Yes, certainly. >> + writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN); >> + writel(0, cot->base + COT_IRQ_SET_CLR); > > '\n' OK. >> + ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler, >> + irqflags, dev_name(&pdev->dev), cot); >> + if (ret == -ENOSYS) >> + return -EPROBE_DEFER; > > I haven't seen anyone do this after devm_request_irq() before. > Why is it required here? Explained above. > >> + if (ret) { >> + dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n", >> + ret); > > I'm not keen on this type of formatting. Besides the system will > print out the returned error on failure. Well, it will print -EINVAL or -ENODEV. When I'll receive an request on the driver with -ENODEV, how will I know it will come from this request_irq() or another part of the code ... Well I can remove it if you want, but I think it's an error. >> + cot->irqdomain = >> + irq_domain_add_linear(pdev->dev.of_node, LUBBOCK_NB_IRQ, >> + &lubbock_irq_domain_ops, cot); > > As a personal preference, I would prefer to see: > > cot->irqdomain = irq_domain_add_linear(pdev->dev.of_node, > LUBBOCK_NB_IRQ, > &lubbock_irq_domain_ops, cot); Your tree, your rules. OK. > >> + if (!cot->irqdomain) >> + return -ENODEV; >> + >> + ret = 0; > > 'ret' will be zero here, or we would have returned already. Good catch. OK. >> + if (base_irq) >> + ret = irq_create_strict_mappings(cot->irqdomain, base_irq, 0, >> + LUBBOCK_NB_IRQ); >> + if (ret) { >> + dev_err(&pdev->dev, "Couldn't create the irq mapping %d..%d\n", >> + base_irq, base_irq + LUBBOCK_NB_IRQ); >> + return ret; >> + } > > Is this solely the check from irq_create_strict_mappings(), therefore > it should be inside the previous if () { ... }. As you wish. >> + dev_info(&pdev->dev, "base=%p, irq=%d, base_irq=%d\n", >> + cot->base, cot->irq, base_irq); > > Please remove this line. OK. >> +MODULE_DESCRIPTION("Lubbock driver"); > > "Lubbock MFD Driver"? Yes. > >> +MODULE_AUTHOR("Robert Jarzmik"); > > Email. Sure Thanks for the review.
On Mon, 19 Jan 2015, Robert Jarzmik wrote: > Lee Jones <lee.jones@linaro.org> writes: > > On Fri, 16 Jan 2015, Robert Jarzmik wrote: [...] > >> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > > > platform_get_irq()? > No. I need the flags. Where are they used? [...] > >> + if (ret) { > >> + dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n", > >> + ret); > > > > I'm not keen on this type of formatting. Besides the system will > > print out the returned error on failure. > Well, it will print -EINVAL or -ENODEV. When I'll receive an request on the > driver with -ENODEV, how will I know it will come from this request_irq() or > another part of the code ... Well I can remove it if you want, but I think it's > an error. I'm not asking you to remove the entire message, just the junk at the end.
On Tue, Jan 20, 2015 at 10:29:19AM +0000, Lee Jones wrote: > On Mon, 19 Jan 2015, Robert Jarzmik wrote: > > >> + if (ret) { > > >> + dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n", > > >> + ret); > > > > > > I'm not keen on this type of formatting. Besides the system will > > > print out the returned error on failure. > > Well, it will print -EINVAL or -ENODEV. When I'll receive an request on the > > driver with -ENODEV, how will I know it will come from this request_irq() or > > another part of the code ... Well I can remove it if you want, but I think it's > > an error. > > I'm not asking you to remove the entire message, just the junk at the > end. No. Leave it. If request_irq() returns -ENODEV or -ENXIO, you'll just get the "Couldn't request main irq" message but without the error code printed. What I'd suggest (and always have done) is: dev_err(&pdev->dev, "couldn't request main irq%d: %d\n", irq, ret); but I guess printing the IRQ number no longer makes sense with todays dynamic mapping of logical IRQ numbers, as it is no longer meaningful.
Russell King - ARM Linux <linux@arm.linux.org.uk> writes: > What I'd suggest (and always have done) is: > > dev_err(&pdev->dev, "couldn't request main irq%d: %d\n", > irq, ret); I like it, it's even more compact, I'll use it for next patch version. > but I guess printing the IRQ number no longer makes sense with todays > dynamic mapping of logical IRQ numbers, as it is no longer meaningful. Yes ... we're not yet there with pxa gpio interrupts, maybe it will come eventually one day. For Lee: > > > platform_get_irq()? > > No. I need the flags. > Where are they used? A couple of lines below, using local "irqflags" variable : ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler, irqflags, dev_name(&pdev->dev), cot); Cheers.
On Wed, 21 Jan 2015, Robert Jarzmik wrote: > > > > platform_get_irq()? > > > No. I need the flags. > > Where are they used? > A couple of lines below, using local "irqflags" variable : > ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler, > irqflags, dev_name(&pdev->dev), cot); No, I mean where are they _used_?
Lee Jones <lee.jones@linaro.org> writes: > On Wed, 21 Jan 2015, Robert Jarzmik wrote: >> > > > platform_get_irq()? >> > > No. I need the flags. >> > Where are they used? >> A couple of lines below, using local "irqflags" variable : >> ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler, >> irqflags, dev_name(&pdev->dev), cot); > > No, I mean where are they _used_? I don't get your point. For you, isn't it "using" irqflags by passing them as an argument to devm_request_irq() ??? And if you think ahead, then there are used all around in kernel/irq/manage.c, as you probably know. Cheers.
On Wed, Jan 21, 2015 at 08:46:29AM +0100, Robert Jarzmik wrote: > Russell King - ARM Linux <linux@arm.linux.org.uk> writes: > > > What I'd suggest (and always have done) is: > > > > dev_err(&pdev->dev, "couldn't request main irq%d: %d\n", > > irq, ret); > I like it, it's even more compact, I'll use it for next patch version. BTW, this is an example why I have the policy of always ensuring that the kernel messages print sufficient diagnostics. Right now, I have a problem - since I rebooted my firewall a few nights ago, I now get on one of my machines: rt6_redirect: source isn't a valid nexthop for redirect target and it spews that for a few minutes every 26 hours or so. No further information, and it leaves you wondering "well, what was the invalid next hop? What was the source?" Pretty much the only way to try and find out is to leave a tcpdump or wireshark running for 24 hours to try and get a dump - which is not that easy if you don't have lots of disk space. So, right now, I have no way to diagnose the above. If it printed that information, then I'd be able to see what the addresses were, and I'd probably be able to come up with a tcpdump filter which didn't involve logging all IPv6 traffic. Kernel messages need to be smart. If not, they might as well just be "The kernel encountered a problem. Abort, Retry or Fail?"
On Wed, Jan 21, 2015 at 08:16:45AM +0000, Lee Jones wrote: > On Wed, 21 Jan 2015, Robert Jarzmik wrote: > > > > > platform_get_irq()? > > > > No. I need the flags. > > > Where are they used? > > A couple of lines below, using local "irqflags" variable : > > ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler, > > irqflags, dev_name(&pdev->dev), cot); > > No, I mean where are they _used_? They're used in request_irq, and request_irq uses it to set the interrupt type (edge or level) as specified in the IRQ resource.
On Wed, 21 Jan 2015, Robert Jarzmik wrote: > Lee Jones <lee.jones@linaro.org> writes: > > > On Wed, 21 Jan 2015, Robert Jarzmik wrote: > >> > > > platform_get_irq()? > >> > > No. I need the flags. > >> > Where are they used? > >> A couple of lines below, using local "irqflags" variable : > >> ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler, > >> irqflags, dev_name(&pdev->dev), cot); > > > > No, I mean where are they _used_? > I don't get your point. > > For you, isn't it "using" irqflags by passing them as an argument to devm_request_irq() No, this is where the argument is being _passed_. > ??? And if you think ahead, then there are used all around in > kernel/irq/manage.c, as you probably know. Perhaps some context would help. Looking at one of the other patches in the series it appears the flag you're trying to capture is IORESOURCE_IRQ_LOWEDGE. When I grep for where this is being _used_ (think 'consumed, rather than passed. I only see a single entry in drivers/pnp/interface.c. That's what got me thinking... are you sure you're a) making use of this flag and b) assuming the answer to 'a' is "no" and if everyting's working correctly in the code's current state, are you sure you need them at all? I think to set an edge trigger on an IRQ, you should instead do so via irq_set_irq_type(), or have a missed a line or two?
On Wed, Jan 21, 2015 at 12:35:51PM +0000, Lee Jones wrote: > I think to set an edge trigger on an IRQ, you should instead do so via > irq_set_irq_type(), or have a missed a line or two? Setting the IRQ type in request_irq() is perfectly acceptable. include/linux/interrupt.h: /* * These correspond to the IORESOURCE_IRQ_* defines in * linux/ioport.h to select the interrupt line behaviour. When * requesting an interrupt without specifying a IRQF_TRIGGER, the * setting should be assumed to be "as already configured", which * may be as per machine or firmware initialisation. */ #define IRQF_TRIGGER_NONE 0x00000000 #define IRQF_TRIGGER_RISING 0x00000001 #define IRQF_TRIGGER_FALLING 0x00000002 #define IRQF_TRIGGER_HIGH 0x00000004 #define IRQF_TRIGGER_LOW 0x00000008
> ----- Mail original ----- > De: "Lee Jones" <lee.jones@linaro.org> First of all, this is my web mail interface, so please be kind with my mail formatting ... > Looking at one of the other patches in the series it appears the flag > you're trying to capture is IORESOURCE_IRQ_LOWEDGE. When I grep for > where this is being _used_ (think 'consumed, rather than passed. I > only see a single entry in drivers/pnp/interface.c. Look at this call chain : request_irq() setup_irq() __setup_irq() __irq_set_trigger() pxa_gpio_irq_type() (aka. chip->irq_set_type) => hardware register manipulation Moreover, you should be aware of the bijection existing between : - IORESOURCE_IRQ_LOWEDGE and IRQF_TRIGGER_RISING - it's HIGHEDGE twin - it is noted in : include/linux/interrupt.h > That's what got me thinking... are you sure you're a) making use of this flag Yes, I'm quite sure. > b) assuming the answer to 'a' is "no" I won't :) > I think to set an edge trigger on an IRQ, you should instead do so via > irq_set_irq_type(), or have a missed a line or two? __setup_irq(), that's the entry point. Cheers. -- Robert
On Wed, 21 Jan 2015, robert.jarzmik@free.fr wrote: > > ----- Mail original ----- > > De: "Lee Jones" <lee.jones@linaro.org> > First of all, this is my web mail interface, so please be kind with > my mail formatting ... I have no idea what you want me to change or do differently? Perhaps it might be more prudent for you to switch to a quality mailer? > > Looking at one of the other patches in the series it appears the flag > > you're trying to capture is IORESOURCE_IRQ_LOWEDGE. When I grep for > > where this is being _used_ (think 'consumed, rather than passed. I > > only see a single entry in drivers/pnp/interface.c. > Look at this call chain : > request_irq() > setup_irq() > __setup_irq() > __irq_set_trigger() > pxa_gpio_irq_type() (aka. chip->irq_set_type) > => hardware register manipulation > > Moreover, you should be aware of the bijection existing between : > - IORESOURCE_IRQ_LOWEDGE and IRQF_TRIGGER_RISING > - it's HIGHEDGE twin > - it is noted in : include/linux/interrupt.h > > > That's what got me thinking... are you sure you're a) making use of > this flag > Yes, I'm quite sure. > > > b) assuming the answer to 'a' is "no" > I won't :) > > > I think to set an edge trigger on an IRQ, you should instead do so via > > irq_set_irq_type(), or have a missed a line or two? > __setup_irq(), that's the entry point. Very well, Russell and yourself have convinced me. If you fixup the remainder of comments, I'm happy.
(adding netdev) On Wed, 2015-01-21 at 09:44 +0000, Russell King - ARM Linux wrote: > On Wed, Jan 21, 2015 at 08:46:29AM +0100, Robert Jarzmik wrote: > > Russell King - ARM Linux <linux@arm.linux.org.uk> writes: > > > > > What I'd suggest (and always have done) is: > > > > > > dev_err(&pdev->dev, "couldn't request main irq%d: %d\n", > > > irq, ret); > > I like it, it's even more compact, I'll use it for next patch version. > > BTW, this is an example why I have the policy of always ensuring that > the kernel messages print sufficient diagnostics. Right now, I have > a problem - since I rebooted my firewall a few nights ago, I now get > on one of my machines: > > rt6_redirect: source isn't a valid nexthop for redirect target > > and it spews that for a few minutes every 26 hours or so. No further > information, and it leaves you wondering "well, what was the invalid > next hop? What was the source?" > > Pretty much the only way to try and find out is to leave a tcpdump or > wireshark running for 24 hours to try and get a dump - which is not > that easy if you don't have lots of disk space. So, right now, I have > no way to diagnose the above. > > If it printed that information, then I'd be able to see what the > addresses were, and I'd probably be able to come up with a tcpdump > filter which didn't involve logging all IPv6 traffic. > > Kernel messages need to be smart. If not, they might as well just be > "The kernel encountered a problem. Abort, Retry or Fail?" >
On Wed, Jan 21, 2015 at 08:05:21AM -0800, Joe Perches wrote:
> (adding netdev)
I wasn't actually reporting that as an issue; I was using it as an
example. It's from a very old kernel (2.6.27.21) which I run on one
of my old x86 machines.
On Wed, 2015-01-21 at 16:11 +0000, Russell King - ARM Linux wrote: > On Wed, Jan 21, 2015 at 08:05:21AM -0800, Joe Perches wrote: > > (adding netdev) > > I wasn't actually reporting that as an issue; I was using it as an > example. It's from a very old kernel (2.6.27.21) which I run on one > of my old x86 machines. It's still the same code. If the message can be improved, why not do it?
On Wed, Jan 21, 2015 at 08:40:44AM -0800, Joe Perches wrote: > On Wed, 2015-01-21 at 16:11 +0000, Russell King - ARM Linux wrote: > > On Wed, Jan 21, 2015 at 08:05:21AM -0800, Joe Perches wrote: > > > (adding netdev) > > > > I wasn't actually reporting that as an issue; I was using it as an > > example. It's from a very old kernel (2.6.27.21) which I run on one > > of my old x86 machines. > > It's still the same code. > If the message can be improved, why not do it? I assume you're taking the responsibility to test anything that comes out of this then? I'm not; I tried updating the kernel on the machine to 2.6.32 many years ago and that was a no-go because of userspace (udev) incompatibilities.
Lee Jones <lee.jones@linaro.org> writes: > Very well, Russell and yourself have convinced me. If you fixup the > remainder of comments, I'm happy. Cool. Let me a couple of days to gather my wits, cross-check I have not forgotten a comment, make some testing on the board and then post v4. Cheers.
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 2e6b731..4d8939f 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -91,6 +91,16 @@ config MFD_AXP20X components like regulators or the PEK (Power Enable Key) under the corresponding menus. +config MFD_LUBBOCK + bool "Lubbock Motherboard" + def_bool ARCH_LUBBOCK + select MFD_CORE + help + This driver supports the Lubbock multifunction chip found on the + pxa25x development platform system (named Lubbock). This IO board + supports the interrupts handling, ethernet controller, flash chips, + etc ... + config MFD_CROS_EC tristate "ChromeOS Embedded Controller" select MFD_CORE diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 53467e2..aff1f4f 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -9,6 +9,7 @@ obj-$(CONFIG_MFD_88PM805) += 88pm805.o 88pm80x.o obj-$(CONFIG_MFD_SM501) += sm501.o obj-$(CONFIG_MFD_ASIC3) += asic3.o tmio_core.o obj-$(CONFIG_MFD_BCM590XX) += bcm590xx.o +obj-$(CONFIG_MFD_LUBBOCK) += lubbock.o obj-$(CONFIG_MFD_CROS_EC) += cros_ec.o obj-$(CONFIG_MFD_CROS_EC_I2C) += cros_ec_i2c.o obj-$(CONFIG_MFD_CROS_EC_SPI) += cros_ec_spi.o diff --git a/drivers/mfd/lubbock.c b/drivers/mfd/lubbock.c new file mode 100644 index 0000000..6025135 --- /dev/null +++ b/drivers/mfd/lubbock.c @@ -0,0 +1,196 @@ +/* + * Intel Cotulla MFD - lubbock motherboard + * + * Copyright (C) 2014 Robert Jarzmik + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * Lubbock motherboard driver, supporting lubbock (aka. pxa25x) soc board. + * + */ + +#include <linux/bitops.h> +#include <linux/gpio.h> +#include <linux/gpio/consumer.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/irq.h> +#include <linux/irqdomain.h> +#include <linux/mfd/core.h> +#include <linux/module.h> +#include <linux/of_platform.h> + +#define COT_IRQ_MASK_EN 0xc0 +#define COT_IRQ_SET_CLR 0xd0 + +#define LUBBOCK_NB_IRQ 8 + +struct lubbock { + void __iomem *base; + int irq; + unsigned int irq_mask; + struct gpio_desc *gpio0; + struct irq_domain *irqdomain; +}; + +static irqreturn_t lubbock_irq_handler(int in_irq, void *d) +{ + struct lubbock *cot = d; + unsigned long pending; + unsigned int bit; + + pending = readl(cot->base + COT_IRQ_SET_CLR) & cot->irq_mask; + for_each_set_bit(bit, &pending, LUBBOCK_NB_IRQ) + generic_handle_irq(irq_find_mapping(cot->irqdomain, bit)); + return IRQ_HANDLED; +} + +static void lubbock_irq_mask_ack(struct irq_data *d) +{ + struct lubbock *cot = irq_data_get_irq_chip_data(d); + unsigned int lubbock_irq = irqd_to_hwirq(d); + unsigned int set, bit = BIT(lubbock_irq); + + cot->irq_mask &= ~bit; + writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN); + set = readl(cot->base + COT_IRQ_SET_CLR); + writel(set & ~bit, cot->base + COT_IRQ_SET_CLR); +} + +static void lubbock_irq_unmask(struct irq_data *d) +{ + struct lubbock *cot = irq_data_get_irq_chip_data(d); + unsigned int lubbock_irq = irqd_to_hwirq(d); + unsigned int bit = BIT(lubbock_irq); + + cot->irq_mask |= bit; + writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN); +} + +static struct irq_chip lubbock_irq_chip = { + .name = "lubbock", + .irq_mask_ack = lubbock_irq_mask_ack, + .irq_unmask = lubbock_irq_unmask, + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE, +}; + +static int lubbock_irq_domain_map(struct irq_domain *d, unsigned int irq, + irq_hw_number_t hwirq) +{ + struct lubbock *cot = d->host_data; + + irq_set_chip_and_handler(irq, &lubbock_irq_chip, handle_level_irq); + irq_set_chip_data(irq, cot); + return 0; +} + +static const struct irq_domain_ops lubbock_irq_domain_ops = { + .xlate = irq_domain_xlate_twocell, + .map = lubbock_irq_domain_map, +}; + +static int lubbock_resume(struct platform_device *pdev) +{ + struct lubbock *cot = platform_get_drvdata(pdev); + + writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN); + return 0; +} + +static int lubbock_probe(struct platform_device *pdev) +{ + struct resource *res; + struct lubbock *cot; + int ret; + unsigned int base_irq = 0; + unsigned long irqflags; + + cot = devm_kzalloc(&pdev->dev, sizeof(*cot), GFP_KERNEL); + if (!cot) + return -ENOMEM; + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); + if (res) { + cot->irq = (unsigned int)res->start; + irqflags = res->flags; + } + if (!cot->irq) + return -ENODEV; + + res = platform_get_resource(pdev, IORESOURCE_IRQ, 1); + if (res) + base_irq = (unsigned int)res->start; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + cot->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(cot->base)) + return PTR_ERR(cot->base); + + platform_set_drvdata(pdev, cot); + + writel(cot->irq_mask, cot->base + COT_IRQ_MASK_EN); + writel(0, cot->base + COT_IRQ_SET_CLR); + ret = devm_request_irq(&pdev->dev, cot->irq, lubbock_irq_handler, + irqflags, dev_name(&pdev->dev), cot); + if (ret == -ENOSYS) + return -EPROBE_DEFER; + if (ret) { + dev_err(&pdev->dev, "Couldn't request main irq : ret = %d\n", + ret); + return ret; + } + irq_set_irq_wake(cot->irq, 1); + + cot->irqdomain = + irq_domain_add_linear(pdev->dev.of_node, LUBBOCK_NB_IRQ, + &lubbock_irq_domain_ops, cot); + if (!cot->irqdomain) + return -ENODEV; + + ret = 0; + if (base_irq) + ret = irq_create_strict_mappings(cot->irqdomain, base_irq, 0, + LUBBOCK_NB_IRQ); + if (ret) { + dev_err(&pdev->dev, "Couldn't create the irq mapping %d..%d\n", + base_irq, base_irq + LUBBOCK_NB_IRQ); + return ret; + } + + dev_info(&pdev->dev, "base=%p, irq=%d, base_irq=%d\n", + cot->base, cot->irq, base_irq); + + return 0; +} + +static int lubbock_remove(struct platform_device *pdev) +{ + struct lubbock *cot = platform_get_drvdata(pdev); + + irq_set_chip_and_handler(cot->irq, NULL, NULL); + return 0; +} + +static const struct of_device_id lubbock_id_table[] = { + { .compatible = "intel,lubbock-io", }, + { } +}; +MODULE_DEVICE_TABLE(of, lubbock_id_table); + +static struct platform_driver lubbock_driver = { + .driver = { + .name = "lubbock_io", + .of_match_table = of_match_ptr(lubbock_id_table), + }, + .probe = lubbock_probe, + .remove = lubbock_remove, + .resume = lubbock_resume, +}; + +module_platform_driver(lubbock_driver); + +MODULE_DESCRIPTION("Lubbock driver"); +MODULE_AUTHOR("Robert Jarzmik"); +MODULE_LICENSE("GPL");
Lubbock () board is the IO motherboard of the Intel PXA25x Development Platform, which supports the Lubbock pxa25x soc board. Historically, this support was in arch/arm/mach-pxa/lubbock.c. When gpio-pxa was moved to drivers/pxa, it became a driver, and its initialization and probing happened at postcore initcall. The lubbock code used to install the chained lubbock interrupt handler at init_irq() time. The consequence of the gpio-pxa change is that the installed chained irq handler lubbock_irq_handler() was overwritten in pxa_gpio_probe(_dt)(), removing : - the handler - the falling edge detection setting of GPIO0, which revealed the interrupt request from the lubbock IO board. As a fix, move the gpio0 chained handler setup to a place where we have the guarantee that pxa_gpio_probe() was called before, so that lubbock handler becomes the true IRQ chained handler of GPIO0, demuxing the lubbock IO board interrupts. This patch moves all that handling to a mfd driver. It's only purpose for the time being is the interrupt handling, but in the future it should encompass all the motherboard CPLDs handling : - leds - switches - hexleds Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> --- Since v1: change the name from cottula to lubbock_io Dmitry pointed out the Cottula was the pxa25x family name, lubbock was the pxa25x development board name. Therefore the name was changed to lubbock_io (lubbock IO board) change the resources to bi-irq ioresource Discussion between Arnd and Robert to change the gpio request by a irq request. Since v2: take into account Mark's review Use irq flags from resources (DT case and pdata case). Change of name from lubbock_io to lubbock-io --- drivers/mfd/Kconfig | 10 +++ drivers/mfd/Makefile | 1 + drivers/mfd/lubbock.c | 196 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 207 insertions(+) create mode 100644 drivers/mfd/lubbock.c