| Message ID | 1363103673-24720-1-git-send-email-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
On Tue, Mar 12, 2013 at 04:54:33PM +0100, Uwe Kleine-König wrote: > +#include <asm/irq.h> > +#include <asm/io.h> linux/io.h > + unsigned int irqs, i, irq_base; > + > + irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id()); > + if (IS_ERR_VALUE(irq_base)) { Erm... irq_alloc_descs() returns a negative number on error. if ((int)irq_base < 0) or make irq_base an int, and use: if (irq_base < 0)
On Tue, Mar 12, 2013 at 04:01:01PM +0000, Russell King - ARM Linux wrote: > On Tue, Mar 12, 2013 at 04:54:33PM +0100, Uwe Kleine-König wrote: > > +#include <asm/irq.h> > > +#include <asm/io.h> > > linux/io.h > > > + unsigned int irqs, i, irq_base; > > + > > + irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id()); > > + if (IS_ERR_VALUE(irq_base)) { > > Erm... irq_alloc_descs() returns a negative number on error. > > if ((int)irq_base < 0) > > or make irq_base an int, and use: > > if (irq_base < 0) Just for me: So the check using IS_ERR_VALUE is as wrong as the other occurences in arch/arm that you just kicked out or is it just ugly? I thought about it and went the "make irq_base int" route (and even fixed the asm includes) even before I sent my patch. Just failed to pass the right hash to format-patch after rebasing :-| Best regards Uwe
On Tue, 12 Mar 2013, Uwe Kleine-König wrote: > +static struct nvic_chip_data nvic_data __read_mostly; What the heck is this? You have a static struct which you set in irqdata.chip_data? > +static inline void __iomem *nvic_dist_base(struct irq_data *d) > +{ > + struct nvic_chip_data *nvic_data = irq_data_get_irq_chip_data(d); And then you do the dance of reading the pointer to that static struct out of irq_data and dereferencing it? What's the point of this? > + return nvic_data->dist_base; > +} > + > +static void nvic_mask_irq(struct irq_data *d) > +{ > + u32 mask = 1 << (d->hwirq % 32); > + > + writel_relaxed(mask, nvic_dist_base(d) + NVIC_ICER + d->irq / 32 * 4); You're really missing the point of irq_data.chip_data If you set proper irq chip data per bank then this whole stuff is reduced to: struct mydata *md = irq_data_get_irq_chip_data(d); u32 mask = 1 << (d->irq - md->irq_base); writel_relaxed(mask, md->iobase + NVIC_ICER); Can you spot the difference and what that means in terms of instruction cycles? > +} > + > +static void nvic_unmask_irq(struct irq_data *d) > +{ > + u32 mask = 1 << (d->hwirq % 32); > + > + writel_relaxed(mask, nvic_dist_base(d) + NVIC_ISER + d->hwirq / 32 * 4); > +} > + > +void nvic_eoi(struct irq_data *d) static ? > +{ > + /* > + * This is a no-op as end of interrupt is signaled by the exception > + * return sequence. > + */ > +} > + > +static struct irq_chip nvic_chip = { > + .name = "NVIC", > + .irq_mask = nvic_mask_irq, > + .irq_unmask = nvic_unmask_irq, > + .irq_eoi = nvic_eoi, > +}; > + > +static void __init nvic_init_bases(struct device_node *node, > + void __iomem *dist_base) Please make this static void __init nvic_init_bases(struct device_node *node, void __iomem *dist_base) That's way easier to parse. Applies to all other multiline stuff as well. > +{ > + unsigned int irqs, i, irq_base; > + > + nvic_data.dist_base = dist_base; > + > + irqs = ((readl_relaxed(dist_base + NVIC_INTR_CTRL) & 0x0f) + 1) * 32; > + if (irqs > 496) > + irqs = 496; Magic constants. Please use proper defines. Aside of that without a proper comment the logic looks ass backwards. > + irqs = ((readl_relaxed(dist_base + NVIC_INTR_CTRL) & 0x0f) + 1) * 32; Without looking at the datasheet I assume that the chip tells you the number of interrupt banks where a result of 0 means 1 bank and so forth. How should a reviewer understand why the number of banks is limited to 31 without a comment? Do you really expect that a reviewer who stumbles over that goes to search for the datasheet and verify what you hacked up? > + irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id()); irq_alloc_desc_from(16, irqs - 16, ...) Also why are you allocating irqs-16 instead of the full number of irqs? > + if (IS_ERR_VALUE(irq_base)) { See Russells reply > + WARN(1, "Cannot allocate irq_descs\n"); What's the point of that warn? The call path is always the same. So you are spamming dmesg with a pointless backtrace. And on top of that you do: > + irq_base = 16; So you cannot allocate irq_descs and then you set base to 16 and pray that everything works? > + } > + nvic_data.domain = irq_domain_add_legacy(node, irqs - 16, irq_base, 0, > + &irq_domain_simple_ops, NULL); > + if (WARN_ON(!nvic_data.domain)) > + return; See above. Also you are leaking irqdescs though it's questionable whether the machine can continue at all. And of course the init call itself will return sucess. > + /* > + * Set priority on all interrupts. > + */ > + for (i = 0; i < irqs; i += 4) > + writel_relaxed(0, dist_base + NVIC_IPRI + i); > + > + /* > + * Disable all interrupts > + */ > + for (i = 0; i < irqs; i += 32) > + writel_relaxed(~0, dist_base + NVIC_ICER + i * 4 / 32); > + > + /* > + * Setup the Linux IRQ subsystem. > + */ > + for (i = 0; i < irqs; i++) { > + irq_set_chip_and_handler(irq_base + i, &nvic_chip, > + handle_fasteoi_irq); Above you do: irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id()); So you allocate irqs-16 interrupt descriptors and then you initialize 16 more than you allocated. Brilliant stuff that. > + irq_set_chip_data(irq_base + i, &nvic_data); > + set_irq_flags(irq_base + i, IRQF_VALID | IRQF_PROBE); > + } > +} > + > +static int __init nvic_of_init(struct device_node *node, > + struct device_node *parent) > +{ > + void __iomem *dist_base; > + > + if (WARN_ON(!node)) Sigh. Though the real question is: can this actually happen? > + return -ENODEV; > + > + dist_base = of_iomap(node, 0); > + WARN(!dist_base, "unable to map nvic dist registers\n"); Brilliant. You can't map stuff and then you continue just for fun or what? > + nvic_init_bases(node, dist_base); Great. You have failure pathes in nvic_init_bases() and then you return unconditionally success: > + return 0; > +} Thanks, tglx
On Tue, Mar 12, 2013 at 08:27:02PM +0100, Uwe Kleine-König wrote: > On Tue, Mar 12, 2013 at 04:01:01PM +0000, Russell King - ARM Linux wrote: > > On Tue, Mar 12, 2013 at 04:54:33PM +0100, Uwe Kleine-König wrote: > > > +#include <asm/irq.h> > > > +#include <asm/io.h> > > > > linux/io.h > > > > > + unsigned int irqs, i, irq_base; > > > + > > > + irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id()); > > > + if (IS_ERR_VALUE(irq_base)) { > > > > Erm... irq_alloc_descs() returns a negative number on error. > > > > if ((int)irq_base < 0) > > > > or make irq_base an int, and use: > > > > if (irq_base < 0) > Just for me: So the check using IS_ERR_VALUE is as wrong as the other > occurences in arch/arm that you just kicked out or is it just ugly? See my recent patch removing all but one. What we're suffering from here is a mentality problem - one which seems to be basically this: If a macro exists which looks like it does the job I need, I must use it. I won't look at the function and check its range of values that it returns, I'll just use it and hope it's the right thing. The IS_ERR_VALUE() patch and my IS_ERR_OR_NULL() patches, I've spent on each one less than a minute, greping, reading the function, checking its range of return values, sometimes longer if I need to look at other functions, and worked out what the valid range of return values are. However, the general pattern in the kernel is this: For any function that returns an int, values of success will be positive. Values indicating errors will be negative. There are very few int-returning functions which violate that. There is one big, well known exception, and that's in the mmap() stuff, where there's a need to return valid values in the range (0..TASK_SIZE) but differentiate them from -ve errnos. This is where IS_ERR_VALUE() came from, and why it was created. See 07ab67c8d0d7c (Fix get_unmapped_area sanity tests). Today, it seems that IS_ERR_VALUE() is now being used just as a subsitute for testing for < 0... and it needs to stop. See above - unless there's a *good* reason, treat +ve values as success, -ve values as failure from functions returning int. Always design functions in the kernel like that. Again - unless there's a *good* reason like needing to return 0..TASK_SIZE.
Hello Thomas, On Tue, Mar 12, 2013 at 08:57:34PM +0100, Thomas Gleixner wrote: > On Tue, 12 Mar 2013, Uwe Kleine-König wrote: > > +static struct nvic_chip_data nvic_data __read_mostly; > > What the heck is this? You have a static struct which you set in > irqdata.chip_data? I copied that idea from the gic driver and didn't question it because I thought it's too early to allocate memory when it's needed. Or do you just wonder about the usage of this static variable? > > +static inline void __iomem *nvic_dist_base(struct irq_data *d) > > +{ > > + struct nvic_chip_data *nvic_data = irq_data_get_irq_chip_data(d); > > And then you do the dance of reading the pointer to that static struct > out of irq_data and dereferencing it? > > What's the point of this? The idea was to keep the functions generic anyhow. > > + return nvic_data->dist_base; > > +} > > + > > +static void nvic_mask_irq(struct irq_data *d) > > +{ > > + u32 mask = 1 << (d->hwirq % 32); > > + > > + writel_relaxed(mask, nvic_dist_base(d) + NVIC_ICER + d->irq / 32 * 4); > > You're really missing the point of irq_data.chip_data > > If you set proper irq chip data per bank then this whole stuff is > reduced to: > > struct mydata *md = irq_data_get_irq_chip_data(d); > u32 mask = 1 << (d->irq - md->irq_base); > > writel_relaxed(mask, md->iobase + NVIC_ICER); > > Can you spot the difference and what that means in terms of > instruction cycles? Yeah I see. The cost is increased memory usage. You'd probably say that the amount needed here is too small to care about. Still many decisions like this sum up and make the 4 MiB of RAM I have a tight fit. > > +} > > + > > +static void nvic_unmask_irq(struct irq_data *d) > > +{ > > + u32 mask = 1 << (d->hwirq % 32); > > + > > + writel_relaxed(mask, nvic_dist_base(d) + NVIC_ISER + d->hwirq / 32 * 4); > > +} > > + > > +void nvic_eoi(struct irq_data *d) > > static ? yes. > > +{ > > + /* > > + * This is a no-op as end of interrupt is signaled by the exception > > + * return sequence. > > + */ > > +} > > + > > +static struct irq_chip nvic_chip = { > > + .name = "NVIC", > > + .irq_mask = nvic_mask_irq, > > + .irq_unmask = nvic_unmask_irq, > > + .irq_eoi = nvic_eoi, > > +}; > > + > > +static void __init nvic_init_bases(struct device_node *node, > > + void __iomem *dist_base) > > Please make this > > static void __init nvic_init_bases(struct device_node *node, > void __iomem *dist_base) > > That's way easier to parse. Applies to all other multiline stuff as > well. My version is like vim does the layout for me. It's the first time someone opposes to it. The reason I prefer using a fixed indention is that I don't need to touch the latter lines when for example the function name or the function's type change. Hmm, I can fix that if you insist. > > +{ > > + unsigned int irqs, i, irq_base; > > + > > + nvic_data.dist_base = dist_base; > > + > > + irqs = ((readl_relaxed(dist_base + NVIC_INTR_CTRL) & 0x0f) + 1) * 32; > > + if (irqs > 496) > > + irqs = 496; > > Magic constants. Please use proper defines. > > Aside of that without a proper comment the logic looks ass backwards. Ok. > > + irqs = ((readl_relaxed(dist_base + NVIC_INTR_CTRL) & 0x0f) + 1) * 32; > > Without looking at the datasheet I assume that the chip tells you the > number of interrupt banks where a result of 0 means 1 bank and so > forth. > > How should a reviewer understand why the number of banks is limited to > 31 without a comment? Do you really expect that a reviewer who > stumbles over that goes to search for the datasheet and verify what > you hacked up? Will add a comment. > > + irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id()); > > irq_alloc_desc_from(16, irqs - 16, ...) > > Also why are you allocating irqs-16 instead of the full number of > irqs? I already have a comment there in my tree. > > + if (IS_ERR_VALUE(irq_base)) { > > See Russells reply > > > + WARN(1, "Cannot allocate irq_descs\n"); > > What's the point of that warn? The call path is always the same. So > you are spamming dmesg with a pointless backtrace. And on top of that > you do: There is one warning per call to nvic_init_bases. So I don't expect more than one message in dmesg. > > > + irq_base = 16; > > So you cannot allocate irq_descs and then you set base to 16 and pray > that everything works? If something goes wrong here the machine is probably silent about it. So continuing after a prayer might (or might not?) be an option. > > + } > > + nvic_data.domain = irq_domain_add_legacy(node, irqs - 16, irq_base, 0, > > + &irq_domain_simple_ops, NULL); > > + if (WARN_ON(!nvic_data.domain)) > > + return; > > See above. Also you are leaking irqdescs though it's questionable > whether the machine can continue at all. And of course the init call > itself will return sucess. > > > + /* > > + * Set priority on all interrupts. > > + */ > > + for (i = 0; i < irqs; i += 4) > > + writel_relaxed(0, dist_base + NVIC_IPRI + i); > > + > > + /* > > + * Disable all interrupts > > + */ > > + for (i = 0; i < irqs; i += 32) > > + writel_relaxed(~0, dist_base + NVIC_ICER + i * 4 / 32); > > + > > + /* > > + * Setup the Linux IRQ subsystem. > > + */ > > + for (i = 0; i < irqs; i++) { > > + irq_set_chip_and_handler(irq_base + i, &nvic_chip, > > + handle_fasteoi_irq); > > Above you do: > irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id()); > > So you allocate irqs-16 interrupt descriptors and then you initialize > 16 more than you allocated. right. > Brilliant stuff that. > > > + irq_set_chip_data(irq_base + i, &nvic_data); > > + set_irq_flags(irq_base + i, IRQF_VALID | IRQF_PROBE); > > + } > > +} > > + > > +static int __init nvic_of_init(struct device_node *node, > > + struct device_node *parent) > > +{ > > + void __iomem *dist_base; > > + > > + if (WARN_ON(!node)) > > Sigh. > > Though the real question is: can this actually happen? It didn't happen for me. What do you suggest? dropping WARN_ON? > > + return -ENODEV; > > + > > + dist_base = of_iomap(node, 0); > > + WARN(!dist_base, "unable to map nvic dist registers\n"); > > Brilliant. You can't map stuff and then you continue just for fun or > what? What do you suggest? returning -ESOMETHING? > > > + nvic_init_bases(node, dist_base); > > Great. You have failure pathes in nvic_init_bases() and then you > return unconditionally success: Most of your critics also apply to irq-gic.c. I will follow up with a patch for that when you are happy with my work for the nvic. Best regards and thanks for your feed-back, Uwe
On Tue, 12 Mar 2013, Uwe Kleine-König wrote: > Hello Thomas, > > On Tue, Mar 12, 2013 at 08:57:34PM +0100, Thomas Gleixner wrote: > > On Tue, 12 Mar 2013, Uwe Kleine-König wrote: > > > +static struct nvic_chip_data nvic_data __read_mostly; > > > > What the heck is this? You have a static struct which you set in > > irqdata.chip_data? > I copied that idea from the gic driver and didn't question it because I > thought it's too early to allocate memory when it's needed. Or do you > just wonder about the usage of this static variable? Right, copying stuff from some other file is always a great excuse for disabling the brain while coding. Why the heck do you think it's safe to call irq_alloc_descs() from that code then? Just because it did not explode in your face? > > > +static inline void __iomem *nvic_dist_base(struct irq_data *d) > > > +{ > > > + struct nvic_chip_data *nvic_data = irq_data_get_irq_chip_data(d); > > > > And then you do the dance of reading the pointer to that static struct > > out of irq_data and dereferencing it? > > > > What's the point of this? > The idea was to keep the functions generic anyhow. Generic waste or what? If you dereference a static variable indirectly via five indirections that makes the code obvious and the function generic. I see ... NOT > > > + return nvic_data->dist_base; > > > +} > > > + > > > +static void nvic_mask_irq(struct irq_data *d) > > > +{ > > > + u32 mask = 1 << (d->hwirq % 32); > > > + > > > + writel_relaxed(mask, nvic_dist_base(d) + NVIC_ICER + d->irq / 32 * 4); > > > > You're really missing the point of irq_data.chip_data > > > > If you set proper irq chip data per bank then this whole stuff is > > reduced to: > > > > struct mydata *md = irq_data_get_irq_chip_data(d); > > u32 mask = 1 << (d->irq - md->irq_base); > > > > writel_relaxed(mask, md->iobase + NVIC_ICER); > > > > Can you spot the difference and what that means in terms of > > instruction cycles? > Yeah I see. The cost is increased memory usage. You'd probably say that > the amount needed here is too small to care about. Still many decisions > like this sum up and make the 4 MiB of RAM I have a tight fit. You did not answer my question completely. You buy less memory usage for an increased instruction foot print along with modulo/multiply/divide complexity in the interrupt hot path. > > > +static void __init nvic_init_bases(struct device_node *node, > > > + void __iomem *dist_base) > > > > Please make this > > > > static void __init nvic_init_bases(struct device_node *node, > > void __iomem *dist_base) > > > > That's way easier to parse. Applies to all other multiline stuff as > > well. > My version is like vim does the layout for me. It's the first time > someone opposes to it. The reason I prefer using a fixed indention is > that I don't need to touch the latter lines when for example the > function name or the function's type change. I don't care about vim and your preferences. I care about the readability and that's key for reviewing patch and reading code. Can you spot the difference of: static void __init nvic_init_bases(struct device_node *node, void __iomem *dist_base) and static void __init nvic_init_bases(struct device_node *node, void __iomem *dist_base) Or irq_set_chip_and_handler(irq_base + i, &nvic_chip, handle_fasteoi_irq); and irq_set_chip_and_handler(irq_base + i, &nvic_chip, handle_fasteoi_irq); The alignment of the arguments after the opening bracket makes it entirely clear while your vim/lazyness style forces the reader to decode it separately. > Hmm, I can fix that if you insist. You can Hmm as long as you want, except if you provide me an argument why your magic vim setting is superiour. > > > + irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id()); > > > > irq_alloc_desc_from(16, irqs - 16, ...) > > > > Also why are you allocating irqs-16 instead of the full number of > > irqs? > I already have a comment there in my tree. Brilliant answer. I ask you a question and you tell me that you have a comment in your tree ???? Stop that nonsense, I don't care about your magic tree, but I care about answers to a review question. > > > + if (IS_ERR_VALUE(irq_base)) { > > > > See Russells reply > > > > > + WARN(1, "Cannot allocate irq_descs\n"); > > > > What's the point of that warn? The call path is always the same. So > > you are spamming dmesg with a pointless backtrace. And on top of that > > you do: > There is one warning per call to nvic_init_bases. So I don't expect more > than one message in dmesg. You're missing the point again. It does not matter whether you expect one or more. The point is, the call chain is known already. So why is dumping a stack trace usefull? It's entirely sufficient to have a pr_warn() or whatever, simply because this is the important information which might scroll out of the observers window with a stack trace which is completely useless. A stack trace is only helpfull when the code in question can be called from a gazillion of call sites. If the call chain is clear, it's pointless. > > > > > + irq_base = 16; > > > > So you cannot allocate irq_descs and then you set base to 16 and pray > > that everything works? > If something goes wrong here the machine is probably silent about it. So > continuing after a prayer might (or might not?) be an option. What are you smoking? Either the machine can work with the preallocated 16 irqs and allow you to retrieve additional debugging info or it does not. It's that easy. > > > + } > > > + nvic_data.domain = irq_domain_add_legacy(node, irqs - 16, irq_base, 0, > > > + &irq_domain_simple_ops, NULL); > > > + if (WARN_ON(!nvic_data.domain)) > > > + return; > > > > See above. Also you are leaking irqdescs though it's questionable > > whether the machine can continue at all. And of course the init call > > itself will return sucess. -ENOANSWER > > > + /* > > > + * Set priority on all interrupts. > > > + */ > > > + for (i = 0; i < irqs; i += 4) > > > + writel_relaxed(0, dist_base + NVIC_IPRI + i); > > > + > > > + /* > > > + * Disable all interrupts > > > + */ > > > + for (i = 0; i < irqs; i += 32) > > > + writel_relaxed(~0, dist_base + NVIC_ICER + i * 4 / 32); > > > + > > > + /* > > > + * Setup the Linux IRQ subsystem. > > > + */ > > > + for (i = 0; i < irqs; i++) { > > > + irq_set_chip_and_handler(irq_base + i, &nvic_chip, > > > + handle_fasteoi_irq); > > > > Above you do: > > irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id()); > > > > So you allocate irqs-16 interrupt descriptors and then you initialize > > 16 more than you allocated. > right. > > > Brilliant stuff that. -ENOANSWER > > > + irq_set_chip_data(irq_base + i, &nvic_data); > > > + set_irq_flags(irq_base + i, IRQF_VALID | IRQF_PROBE); > > > + } > > > +} > > > + > > > +static int __init nvic_of_init(struct device_node *node, > > > + struct device_node *parent) > > > +{ > > > + void __iomem *dist_base; > > > + > > > + if (WARN_ON(!node)) > > > > Sigh. > > > > Though the real question is: can this actually happen? > It didn't happen for me. Great argument for writing silly code. > What do you suggest? dropping WARN_ON? Did you actually try to understand any of my review questions? > > > + return -ENODEV; > > > + > > > + dist_base = of_iomap(node, 0); > > > + WARN(!dist_base, "unable to map nvic dist registers\n"); > > > > Brilliant. You can't map stuff and then you continue just for fun or > > what? > What do you suggest? returning -ESOMETHING? -EMORON perhaps? Come on, do I need to make any further suggestions? See above, either the machine can survive the failure or it cannot. > > > + nvic_init_bases(node, dist_base); > > > > Great. You have failure pathes in nvic_init_bases() and then you > > return unconditionally success: -ENOANSWER > Most of your critics also apply to irq-gic.c. That's completely irrelevant. > I will follow up with a patch for that when you are happy with my > work for the nvic. Do you really think that copying crappy code, making that copied code worse and on top of that nerve racking the reviewer makes you the person of choice to fixup the initial crap ? Thanks, tglx
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index a350969..18657fd 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -10,6 +10,10 @@ config ARM_GIC config GIC_NON_BANKED bool +config ARM_NVIC + bool + select IRQ_DOMAIN + config ARM_VIC bool select IRQ_DOMAIN diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index 98e3b87..7227c5f 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -7,5 +7,6 @@ obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o obj-$(CONFIG_ARCH_SUNXI) += irq-sunxi.o obj-$(CONFIG_ARCH_SPEAR3XX) += spear-shirq.o obj-$(CONFIG_ARM_GIC) += irq-gic.o +obj-$(CONFIG_ARM_NVIC) += irq-nvic.o obj-$(CONFIG_ARM_VIC) += irq-vic.o obj-$(CONFIG_VERSATILE_FPGA_IRQ) += irq-versatile-fpga.o diff --git a/drivers/irqchip/irq-nvic.c b/drivers/irqchip/irq-nvic.c new file mode 100644 index 0000000..ddfb3d8 --- /dev/null +++ b/drivers/irqchip/irq-nvic.c @@ -0,0 +1,136 @@ +/* + * drivers/irq/irq-nvic.c + * + * Copyright (C) 2008 ARM Limited, All Rights Reserved. + * Copyright (C) 2013 Pengutronix + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * Support for the Nested Vectored Interrupt Controller found on the + * ARMv7-M CPUs (Cortex-M3/M4) + */ +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/compiler.h> +#include <linux/smp.h> +#include <linux/export.h> +#include <linux/err.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/irqdomain.h> + +#include <asm/irq.h> +#include <asm/io.h> +#include <asm/mach/irq.h> + +#include "irqchip.h" + +#define NVIC_INTR_CTRL (0x004) +#define NVIC_ISER (0x100) +#define NVIC_ICER (0x180) +#define NVIC_IPRI (0x400) + +struct nvic_chip_data { + void __iomem *dist_base; + struct irq_domain *domain; +}; + +static struct nvic_chip_data nvic_data __read_mostly; + +static inline void __iomem *nvic_dist_base(struct irq_data *d) +{ + struct nvic_chip_data *nvic_data = irq_data_get_irq_chip_data(d); + return nvic_data->dist_base; +} + +static void nvic_mask_irq(struct irq_data *d) +{ + u32 mask = 1 << (d->hwirq % 32); + + writel_relaxed(mask, nvic_dist_base(d) + NVIC_ICER + d->irq / 32 * 4); +} + +static void nvic_unmask_irq(struct irq_data *d) +{ + u32 mask = 1 << (d->hwirq % 32); + + writel_relaxed(mask, nvic_dist_base(d) + NVIC_ISER + d->hwirq / 32 * 4); +} + +void nvic_eoi(struct irq_data *d) +{ + /* + * This is a no-op as end of interrupt is signaled by the exception + * return sequence. + */ +} + +static struct irq_chip nvic_chip = { + .name = "NVIC", + .irq_mask = nvic_mask_irq, + .irq_unmask = nvic_unmask_irq, + .irq_eoi = nvic_eoi, +}; + +static void __init nvic_init_bases(struct device_node *node, + void __iomem *dist_base) +{ + unsigned int irqs, i, irq_base; + + nvic_data.dist_base = dist_base; + + irqs = ((readl_relaxed(dist_base + NVIC_INTR_CTRL) & 0x0f) + 1) * 32; + if (irqs > 496) + irqs = 496; + + irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id()); + if (IS_ERR_VALUE(irq_base)) { + WARN(1, "Cannot allocate irq_descs\n"); + irq_base = 16; + } + nvic_data.domain = irq_domain_add_legacy(node, irqs - 16, irq_base, 0, + &irq_domain_simple_ops, NULL); + if (WARN_ON(!nvic_data.domain)) + return; + + /* + * Set priority on all interrupts. + */ + for (i = 0; i < irqs; i += 4) + writel_relaxed(0, dist_base + NVIC_IPRI + i); + + /* + * Disable all interrupts + */ + for (i = 0; i < irqs; i += 32) + writel_relaxed(~0, dist_base + NVIC_ICER + i * 4 / 32); + + /* + * Setup the Linux IRQ subsystem. + */ + for (i = 0; i < irqs; i++) { + irq_set_chip_and_handler(irq_base + i, &nvic_chip, + handle_fasteoi_irq); + irq_set_chip_data(irq_base + i, &nvic_data); + set_irq_flags(irq_base + i, IRQF_VALID | IRQF_PROBE); + } +} + +static int __init nvic_of_init(struct device_node *node, + struct device_node *parent) +{ + void __iomem *dist_base; + + if (WARN_ON(!node)) + return -ENODEV; + + dist_base = of_iomap(node, 0); + WARN(!dist_base, "unable to map nvic dist registers\n"); + + nvic_init_bases(node, dist_base); + + return 0; +} +IRQCHIP_DECLARE(cortex_m3_nvic, "arm,cortex-m3-nvic", nvic_of_init);