| Message ID | 1393876300-3061-1-git-send-email-swarren@wwwdotorg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 3 Mar 2014, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> > > Some devices have configurable IRQ output polarities. Software might > use IRQ_TYPE_* to determine how to configure such a device's IRQ > output polarity in order to match how the IRQ controller input is > configured. If the board or SoC inverts the signal between the > device's IRQ output and controller's IRQ output, software must be > aware of this fact, in order to program the IRQ output to the correct > (i.e. opposite) polarity. This flag provides that information. So what you're saying is: Device IRQ output --> [Optional Inverter Logic] --> IRQ controller input. And you're storing the information about the presence of the inverter logic in the irq itself, but the core does not make any use of it and you let the device driver deal with the outcome. This sucks as all affected drivers have to implement the same sanity logic for this. Why don't you implement a core function which tells the driver which polarity to select? That requires a few more changes, but I think it's worth it for other reasons. Right now the set_type logic requires the irq chip drivers to implement sanity checking and default selections for TYPE_NONE. We can be more clever about that and add this information to the irq chip flags. enum { IRQ_CHIP_TYPES_MASK = 0x0f, IRQ_CHIP_DEFAULT_MASK = 0xf0, IRQ_CHIP_EXISTING_FLAGS .... } Now the irq_chip setup tells the core which types are available and which one is the default fallback for TYPE_NONE. So the core can do the sanity checks and we can kill quite some repeated stuff from the irq chip implementations. For the inverted logic case you can handle the inversion in the core as well, i.e. if a driver requests IRQ_TYPE_LEVEL_HIGH you select IRQ_TYPE_LEVEL_LOW for the chip, if possible. For the case where the irq chip can only handle a single polarity you can provide a core function to figure out to which polarity the driver should set the device IRQ output line. int irq_get_device_irq_polarity(int irq, int device_types) { /* * Handle the inversion logic and select a proper * device irq polarity from irq_chip(@irq)->flags and * @device_types. * * Return a proper error code if no match. */ } Let's look at an example: irq_chip.flags = IRQ_TYPE_LEVEL_HIGH; device_types = IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW; Now for the non inverted case, this returns IRQ_TYPE_LEVEL_HIGH, for the inverted case it returns IRQ_TYPE_LEVEL_LOW. In both cases the irq_set_type() logic handles this correctly: Non-Inverted case: irq_set_type(irq, IRQ_TYPE_LEVEL_HIGH); -> Success Inverted case: irq_set_type(irq, IRQ_TYPE_LEVEL_LOW); invert -> IRQ_TYPE_LEVEL_HIGH -> Success To make this work for interrupt chips which have no set_type callback we can do the following in irq_set_type(): if (irq_is_inverted(irq)) type = invert(type); ret = irq_check_type(chip, &type); if (ret < 0 || !chip->irq_settype) return ret; return chip->irq_settype(); And irq_check_type() does: if (!(chip->flags & IRQ_CHIP_TYPES_MASK)) return chip->irq_settype ? 0 : -ENOTSUP; if (*type == IRQ_TYPE_NONE) *type == (chip->flags & IRQ_CHIP_DEFAULT_MASK) >> 4; return type_supported(chip->flags, *type); Thanks, tglx
On Tue, 4 Mar 2014, Thomas Gleixner wrote: > On Mon, 3 Mar 2014, Stephen Warren wrote: > > > From: Stephen Warren <swarren@nvidia.com> > > > > Some devices have configurable IRQ output polarities. Software might > > use IRQ_TYPE_* to determine how to configure such a device's IRQ > > output polarity in order to match how the IRQ controller input is > > configured. If the board or SoC inverts the signal between the > > device's IRQ output and controller's IRQ output, software must be > > aware of this fact, in order to program the IRQ output to the correct > > (i.e. opposite) polarity. This flag provides that information. > > So what you're saying is: > > Device IRQ output --> [Optional Inverter Logic] --> IRQ controller input. > > And you're storing the information about the presence of the inverter > logic in the irq itself, but the core does not make any use of it and > you let the device driver deal with the outcome. > > This sucks as all affected drivers have to implement the same sanity > logic for this. > > Why don't you implement a core function which tells the driver which > polarity to select? That requires a few more changes, but I think it's > worth it for other reasons. > > Right now the set_type logic requires the irq chip drivers to > implement sanity checking and default selections for TYPE_NONE. We can > be more clever about that and add this information to the irq chip > flags. > > enum { > IRQ_CHIP_TYPES_MASK = 0x0f, > IRQ_CHIP_DEFAULT_MASK = 0xf0, > IRQ_CHIP_EXISTING_FLAGS .... > } We need to extend the mask to indicate whether the chip supports BOTH_EDGES. A chip can support FALLING and RISING, but not both at the same time. For the set_type side the current BOTH = FALLING | RISING is fine, but for checking the supported type it's not sufficient. Thanks, tglx
On 03/04/2014 03:04 AM, Thomas Gleixner wrote: > On Mon, 3 Mar 2014, Stephen Warren wrote: > >> From: Stephen Warren <swarren@nvidia.com> >> >> Some devices have configurable IRQ output polarities. Software might >> use IRQ_TYPE_* to determine how to configure such a device's IRQ >> output polarity in order to match how the IRQ controller input is >> configured. If the board or SoC inverts the signal between the >> device's IRQ output and controller's IRQ output, software must be >> aware of this fact, in order to program the IRQ output to the correct >> (i.e. opposite) polarity. This flag provides that information. > > So what you're saying is: > > Device IRQ output --> [Optional Inverter Logic] --> IRQ controller input. > > And you're storing the information about the presence of the inverter > logic in the irq itself, but the core does not make any use of it and > you let the device driver deal with the outcome. > > This sucks as all affected drivers have to implement the same sanity > logic for this. > > Why don't you implement a core function which tells the driver which > polarity to select? That requires a few more changes, but I think it's > worth it for other reasons. > > Right now the set_type logic requires the irq chip drivers to > implement sanity checking and default selections for TYPE_NONE. We can > be more clever about that and add this information to the irq chip > flags. I don't see any such checking in drivers/irqchip/irq-gic.c; it rejects any type other than IRQ_TYPE_LEVEL_HIGH or IRQ_TYPE_EDGE_RISING, and I don't see any mention of TYPE_NONE in that file. Is the driver incomplete? Instead of adding all this extra logic to the core, what do you think of simply telling each driver that has a configurable interrupt output polarity exactly which polarity to use. This information would come from device tree or platform data. This is what I implemented in V1/V2 of this series: http://www.spinics.net/lists/devicetree/msg23648.html Is that any better, or do you definitely want the IRQ core to manage this?
On Tue, 4 Mar 2014, Stephen Warren wrote: > On 03/04/2014 03:04 AM, Thomas Gleixner wrote: > > On Mon, 3 Mar 2014, Stephen Warren wrote: > > > >> From: Stephen Warren <swarren@nvidia.com> > >> > >> Some devices have configurable IRQ output polarities. Software might > >> use IRQ_TYPE_* to determine how to configure such a device's IRQ > >> output polarity in order to match how the IRQ controller input is > >> configured. If the board or SoC inverts the signal between the > >> device's IRQ output and controller's IRQ output, software must be > >> aware of this fact, in order to program the IRQ output to the correct > >> (i.e. opposite) polarity. This flag provides that information. > > > > So what you're saying is: > > > > Device IRQ output --> [Optional Inverter Logic] --> IRQ controller input. > > > > And you're storing the information about the presence of the inverter > > logic in the irq itself, but the core does not make any use of it and > > you let the device driver deal with the outcome. > > > > This sucks as all affected drivers have to implement the same sanity > > logic for this. > > > > Why don't you implement a core function which tells the driver which > > polarity to select? That requires a few more changes, but I think it's > > worth it for other reasons. > > > > Right now the set_type logic requires the irq chip drivers to > > implement sanity checking and default selections for TYPE_NONE. We can > > be more clever about that and add this information to the irq chip > > flags. > > I don't see any such checking in drivers/irqchip/irq-gic.c; it rejects > any type other than IRQ_TYPE_LEVEL_HIGH or IRQ_TYPE_EDGE_RISING, and I > don't see any mention of TYPE_NONE in that file. Is the driver incomplete? No. The IRQ_TYPE_NONE is a hysterical leftover. > Instead of adding all this extra logic to the core, what do you think of > simply telling each driver that has a configurable interrupt output > polarity exactly which polarity to use. This information would come from > device tree or platform data. This is what I implemented in V1/V2 of > this series: > > http://www.spinics.net/lists/devicetree/msg23648.html > > Is that any better, or do you definitely want the IRQ core to manage this? Oh, yes. Simply because any driver which is not aware of that inversion will trip over the issue that it requests by the best of its knowledge IRQ_TYPE_LEVEL_HIGH while it should actually request IRQ_TYPE_LEVEL_LOW due to the inversion. Are you really going to make all possibly affected drivers aware of that? Good luck! That's why we want to move such stuff to core code. Assume the following scenario: driverX which works perfectly fine on SoCA is reused for SoCB where the inverter sits between the device and the SoCB irq controller. With your DT scheme the whole thing falls flat on the nose simply because neither the driverX nor the core code is aware of the incompability. So driverX is happily using the IRQ_TYPE_LEVEL_HIGH setting which was used when the driver was written and the core works nicely with that because the irq chips supports IRQ_TYPE_LEVEL_HIGH. Just the fact that the inverter is in the hardware results in an infinite interrupt storm. Are you going to handle the bug reports and "remote" debug sessions for this kind of crap? Fuck no. You don't want to deal with this and that's why it is way better to build that kind of support into the core where everything which trips over the issue tells the random driver user/developer what's wrong. Dammit. I explained you very detailed WHY this is useful aside of the inverted logic situation. Is it that hard to understand? This usecase clearly shows a shortcoming at the core level along with a potential for consolidation. So why are you trying to convince me that this can be solved with some DT/device drivers hackery? When will you SoC folks ever understand that nothing on your SoCs/board is special? I'm telling that you for years but you simply refuse to get a gripe. Thanks, tglx
diff --git a/include/linux/irq.h b/include/linux/irq.h index 7dc10036eff5..535f3937e99e 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -73,6 +73,11 @@ typedef void (*irq_preflow_handler_t)(struct irq_data *data); * IRQ_IS_POLLED - Always polled by another interrupt. Exclude * it from the spurious interrupt detection * mechanism and from core side polling. + * IRQ_SRC_DST_INVERTED - An inverter exists between the source's IRQ + * output, and the IRQ controller's input. + * For devices with programmable output polarity + * this helps the driver match the device output + * to the controller's input polarity. */ enum { IRQ_TYPE_NONE = 0x00000000, @@ -98,6 +103,7 @@ enum { IRQ_NOTHREAD = (1 << 16), IRQ_PER_CPU_DEVID = (1 << 17), IRQ_IS_POLLED = (1 << 18), + IRQ_SRC_DST_INVERTED = (1 << 19), }; #define IRQF_MODIFY_MASK \ @@ -218,6 +224,11 @@ static inline u32 irqd_get_trigger_type(struct irq_data *d) return d->state_use_accessors & IRQD_TRIGGER_MASK; } +static inline u32 irqd_get_src_dst_inverted(struct irq_data *d) +{ + return d->state_use_accessors & IRQ_SRC_DST_INVERTED; +} + /* * Must only be called inside irq_chip.irq_set_type() functions. */ @@ -538,6 +549,7 @@ extern int irq_set_chip(unsigned int irq, struct irq_chip *chip); extern int irq_set_handler_data(unsigned int irq, void *data); extern int irq_set_chip_data(unsigned int irq, void *data); extern int irq_set_irq_type(unsigned int irq, unsigned int type); +extern int irq_set_src_dst_inverted(unsigned int irq, unsigned int inverted); extern int irq_set_msi_desc(unsigned int irq, struct msi_desc *entry); extern int irq_set_msi_desc_off(unsigned int irq_base, unsigned int irq_offset, struct msi_desc *entry); diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index dc04c166c54d..ff9e13fa1170 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -70,6 +70,30 @@ int irq_set_irq_type(unsigned int irq, unsigned int type) EXPORT_SYMBOL(irq_set_irq_type); /** + * irq_set_src_dst_inverted(unsigned int irq, u32 inverted) + * @irq: irq number + * @inverted: IRQ_SRC_DST_INVERTED value - see include/linux/irq.h + */ +int irq_set_src_dst_inverted(unsigned int irq, unsigned int inverted) +{ + unsigned long flags; + struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL); + int ret = 0; + + if (!desc) + return -EINVAL; + + inverted &= IRQ_SRC_DST_INVERTED; + if (inverted) + irqd_set(&desc->irq_data, inverted); + else + irqd_clear(&desc->irq_data, inverted); + irq_put_desc_busunlock(desc, flags); + return ret; +} +EXPORT_SYMBOL(irq_set_src_dst_inverted); + +/** * irq_set_handler_data - set irq handler data for an irq * @irq: Interrupt number * @data: Pointer to interrupt specific data diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index f14033700c25..f9cee747424e 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -471,6 +471,7 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data) struct irq_domain *domain; irq_hw_number_t hwirq; unsigned int type = IRQ_TYPE_NONE; + unsigned int inverted = 0; unsigned int virq; domain = irq_data->np ? irq_find_host(irq_data->np) : irq_default_domain; @@ -487,6 +488,8 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data) if (domain->ops->xlate(domain, irq_data->np, irq_data->args, irq_data->args_count, &hwirq, &type)) return 0; + inverted = type & IRQ_SRC_DST_INVERTED; + type &= IRQ_TYPE_SENSE_MASK; } /* Create mapping */ @@ -498,6 +501,7 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data) if (type != IRQ_TYPE_NONE && type != irq_get_trigger_type(virq)) irq_set_irq_type(virq, type); + irq_set_src_dst_inverted(virq, inverted); return virq; } EXPORT_SYMBOL_GPL(irq_create_of_mapping);