| Message ID | 1484640029-22870-5-git-send-email-preid@electromag.com.au |
|---|---|
| State | Superseded |
| Headers | show |
On 2017-01-17 09:00, Phil Reid wrote: > Unfortunately some hardware device will assert their irq line immediately > on power on and provide no mechanism to mask the irq. As the i2c muxes > provide no method to mask irq line this provides a work around by keeping > the parent irq masked until enough device drivers have loaded to service > all pending interrupts. > > For example the the ltc1760 assert its SMBALERT irq immediately on power > on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first > device is registered irq are enabled and fire continuously as the second > device driver has not yet loaded. Setting this parameter to <1 1> will > delay the irq being enabled until both devices are ready. Hang on, does this suggestion I made make any sense at all? Maybe it does, but does the pca954x driver even get notified of any but the first irq client that unmasks interrupts on a mux segment? How can it count the number of active irq clients if not? I'm truly sorry for the trouble I'm causing by not just saying how it should be done from the start, but I feel like I've been thrown in at the deep end when it comes to interrupt controllers... Cheers, peda > Signed-off-by: Phil Reid <preid@electromag.com.au> > --- > Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > index aa09704..ac71be6 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > @@ -19,6 +19,8 @@ Optional Properties: > - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all > children in idle state. This is necessary for example, if there are several > multiplexers on the bus and the devices behind them use same I2C addresses. > + - nxp,irq-mask-enable: array; Defines the minimum number of chips that must > + register an irq for each channel before the parent irq line in enabled. > - interrupt-parent: Phandle for the interrupt controller that services > interrupts for this device. > - interrupts: Interrupt mapping for IRQ. > @@ -36,6 +38,7 @@ Example: > #size-cells = <0>; > reg = <0x74>; > > + nxp,irq-mask-enable = <0 0 0 0 1 0 0 0>; > interrupt-parent = <&ipic>; > interrupts = <17 IRQ_TYPE_LEVEL_LOW>; > interrupt-controller; > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17/01/2017 16:57, Peter Rosin wrote: > On 2017-01-17 09:00, Phil Reid wrote: >> Unfortunately some hardware device will assert their irq line immediately >> on power on and provide no mechanism to mask the irq. As the i2c muxes >> provide no method to mask irq line this provides a work around by keeping >> the parent irq masked until enough device drivers have loaded to service >> all pending interrupts. >> >> For example the the ltc1760 assert its SMBALERT irq immediately on power >> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first >> device is registered irq are enabled and fire continuously as the second >> device driver has not yet loaded. Setting this parameter to <1 1> will >> delay the irq being enabled until both devices are ready. > G'day Peter, > Hang on, does this suggestion I made make any sense at all? Maybe it does, > but does the pca954x driver even get notified of any but the first irq client > that unmasks interrupts on a mux segment? How can it count the number of > active irq clients if not? Good question. So what I did to test is setup my 2 ltc1760s to use the same irq on the pca954x. Using the latest patch series. Adding a log message into the irq_unmask function got the following. dev_err(&data->client->dev, "irq_unmask %d %x %d", pos, data->irq_mask, data->irq_enabled); dmesg | grep irq_unmask [ 4.392098] pca954x 4-0070: irq_unmask 0 1 1 But Looks like both got registered ok to the same irq. cat /proc/interrupts 161: 0 0 i2c-mux-pca954x 0 Level 15-000a, 16-000a So from this testing, it doesn't look like it gets called multiple times. So back to the bitmask for the dt do you think. I think the interrupt enablelogic is correct now. > > I'm truly sorry for the trouble I'm causing by not just saying how it should > be done from the start, but I feel like I've been thrown in at the deep end > when it comes to interrupt controllers... No problem. I'm learning a couple things as we go. Should help me out on other drivers :)
On 2017-01-17 10:28, Phil Reid wrote: > On 17/01/2017 16:57, Peter Rosin wrote: >> On 2017-01-17 09:00, Phil Reid wrote: >>> Unfortunately some hardware device will assert their irq line immediately >>> on power on and provide no mechanism to mask the irq. As the i2c muxes >>> provide no method to mask irq line this provides a work around by keeping >>> the parent irq masked until enough device drivers have loaded to service >>> all pending interrupts. >>> >>> For example the the ltc1760 assert its SMBALERT irq immediately on power >>> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first >>> device is registered irq are enabled and fire continuously as the second >>> device driver has not yet loaded. Setting this parameter to <1 1> will >>> delay the irq being enabled until both devices are ready. >> > G'day Peter, > > >> Hang on, does this suggestion I made make any sense at all? Maybe it does, >> but does the pca954x driver even get notified of any but the first irq client >> that unmasks interrupts on a mux segment? How can it count the number of >> active irq clients if not? > > Good question. > > So what I did to test is setup my 2 ltc1760s to use the same irq on the pca954x. > Using the latest patch series. > > Adding a log message into the irq_unmask function got the following. > dev_err(&data->client->dev, "irq_unmask %d %x %d", pos, data->irq_mask, data->irq_enabled); > > dmesg | grep irq_unmask > [ 4.392098] pca954x 4-0070: irq_unmask 0 1 1 > > > But Looks like both got registered ok to the same irq. > cat /proc/interrupts > 161: 0 0 i2c-mux-pca954x 0 Level 15-000a, 16-000a > > So from this testing, it doesn't look like it gets called multiple times. As I suspected, thanks for verifying! > So back to the bitmask for the dt do you think. Looking at kernel/irq/chip.c:irq_enable (and struct irq_chip docs), I get the feeling that if you provide the irq_enable operation (and maybe irq_disable too?), that might get us more info? > I think the interrupt enablelogic is correct now. > >> >> I'm truly sorry for the trouble I'm causing by not just saying how it should >> be done from the start, but I feel like I've been thrown in at the deep end >> when it comes to interrupt controllers... > > No problem. I'm learning a couple things as we go. > Should help me out on other drivers :) Yes, I'm also picking up a few bits here and there... -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-01-17 10:43, Peter Rosin wrote: > On 2017-01-17 10:28, Phil Reid wrote: >> On 17/01/2017 16:57, Peter Rosin wrote: >>> On 2017-01-17 09:00, Phil Reid wrote: >>>> Unfortunately some hardware device will assert their irq line immediately >>>> on power on and provide no mechanism to mask the irq. As the i2c muxes >>>> provide no method to mask irq line this provides a work around by keeping >>>> the parent irq masked until enough device drivers have loaded to service >>>> all pending interrupts. >>>> >>>> For example the the ltc1760 assert its SMBALERT irq immediately on power >>>> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first >>>> device is registered irq are enabled and fire continuously as the second >>>> device driver has not yet loaded. Setting this parameter to <1 1> will >>>> delay the irq being enabled until both devices are ready. >>> >> G'day Peter, >> >> >>> Hang on, does this suggestion I made make any sense at all? Maybe it does, >>> but does the pca954x driver even get notified of any but the first irq client >>> that unmasks interrupts on a mux segment? How can it count the number of >>> active irq clients if not? >> >> Good question. >> >> So what I did to test is setup my 2 ltc1760s to use the same irq on the pca954x. >> Using the latest patch series. >> >> Adding a log message into the irq_unmask function got the following. >> dev_err(&data->client->dev, "irq_unmask %d %x %d", pos, data->irq_mask, data->irq_enabled); >> >> dmesg | grep irq_unmask >> [ 4.392098] pca954x 4-0070: irq_unmask 0 1 1 >> >> >> But Looks like both got registered ok to the same irq. >> cat /proc/interrupts >> 161: 0 0 i2c-mux-pca954x 0 Level 15-000a, 16-000a >> >> So from this testing, it doesn't look like it gets called multiple times. > > As I suspected, thanks for verifying! > >> So back to the bitmask for the dt do you think. > > Looking at kernel/irq/chip.c:irq_enable (and struct irq_chip docs), I get the > feeling that if you provide the irq_enable operation (and maybe irq_disable too?), > that might get us more info? No, I no longer think that. I think we need to get at the irq descriptor "depth". But that feels like poking at the wrong level. Crap. And to answer the question if I think we should go back to a dt bitmask, then no I do not think that. The array describes what we want to do, it's the linux implementation that gives us difficulties. Agreed? >> I think the interrupt enablelogic is correct now. >> >>> >>> I'm truly sorry for the trouble I'm causing by not just saying how it should >>> be done from the start, but I feel like I've been thrown in at the deep end >>> when it comes to interrupt controllers... >> >> No problem. I'm learning a couple things as we go. >> Should help me out on other drivers :) > > Yes, I'm also picking up a few bits here and there... > > -- > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17/01/2017 18:14, Peter Rosin wrote: > On 2017-01-17 10:43, Peter Rosin wrote: >> On 2017-01-17 10:28, Phil Reid wrote: >>> On 17/01/2017 16:57, Peter Rosin wrote: >>>> On 2017-01-17 09:00, Phil Reid wrote: >>>>> Unfortunately some hardware device will assert their irq line immediately >>>>> on power on and provide no mechanism to mask the irq. As the i2c muxes >>>>> provide no method to mask irq line this provides a work around by keeping >>>>> the parent irq masked until enough device drivers have loaded to service >>>>> all pending interrupts. >>>>> >>>>> For example the the ltc1760 assert its SMBALERT irq immediately on power >>>>> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first >>>>> device is registered irq are enabled and fire continuously as the second >>>>> device driver has not yet loaded. Setting this parameter to <1 1> will >>>>> delay the irq being enabled until both devices are ready. >>>> >>> G'day Peter, >>> >>> >>>> Hang on, does this suggestion I made make any sense at all? Maybe it does, >>>> but does the pca954x driver even get notified of any but the first irq client >>>> that unmasks interrupts on a mux segment? How can it count the number of >>>> active irq clients if not? >>> >>> Good question. >>> >>> So what I did to test is setup my 2 ltc1760s to use the same irq on the pca954x. >>> Using the latest patch series. >>> >>> Adding a log message into the irq_unmask function got the following. >>> dev_err(&data->client->dev, "irq_unmask %d %x %d", pos, data->irq_mask, data->irq_enabled); >>> >>> dmesg | grep irq_unmask >>> [ 4.392098] pca954x 4-0070: irq_unmask 0 1 1 >>> >>> >>> But Looks like both got registered ok to the same irq. >>> cat /proc/interrupts >>> 161: 0 0 i2c-mux-pca954x 0 Level 15-000a, 16-000a >>> >>> So from this testing, it doesn't look like it gets called multiple times. >> >> As I suspected, thanks for verifying! >> >>> So back to the bitmask for the dt do you think. >> >> Looking at kernel/irq/chip.c:irq_enable (and struct irq_chip docs), I get the >> feeling that if you provide the irq_enable operation (and maybe irq_disable too?), >> that might get us more info? > > No, I no longer think that. I think we need to get at the irq descriptor "depth". > But that feels like poking at the wrong level. Crap. > > And to answer the question if I think we should go back to a dt bitmask, then > no I do not think that. The array describes what we want to do, it's the linux > implementation that gives us difficulties. Agreed? G'day Peter, Yes agreed, that makes sense. I had a play with using the irq_enable call back. But that doesn't seem to offer any more feedback then unmask. Called just once on the first irq that registers. The only way I can see to count the number of registered handlers on a shared irq is to count the action handlers on the irq_desc. The only irq_chip handler that seems guaranteed to be called on irq request is irq_bus_lock, irq_bus_sync_unlock callbacks, but these are called elsewhere at various times. We have a dt spec that supports this future proofing of the irq count. But as you say the kernel code prevents us at the moment. The count of 1 bitmask implementation works for my particular situation at the moment. Perhaps the > 1 case can be tackled in the future when someone has a use case? Thoughts? > >>> I think the interrupt enablelogic is correct now. >>> >>>> >>>> I'm truly sorry for the trouble I'm causing by not just saying how it should >>>> be done from the start, but I feel like I've been thrown in at the deep end >>>> when it comes to interrupt controllers... >>> >>> No problem. I'm learning a couple things as we go. >>> Should help me out on other drivers :) >> >> Yes, I'm also picking up a few bits here and there...
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index aa09704..ac71be6 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt @@ -19,6 +19,8 @@ Optional Properties: - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all children in idle state. This is necessary for example, if there are several multiplexers on the bus and the devices behind them use same I2C addresses. + - nxp,irq-mask-enable: array; Defines the minimum number of chips that must + register an irq for each channel before the parent irq line in enabled. - interrupt-parent: Phandle for the interrupt controller that services interrupts for this device. - interrupts: Interrupt mapping for IRQ. @@ -36,6 +38,7 @@ Example: #size-cells = <0>; reg = <0x74>; + nxp,irq-mask-enable = <0 0 0 0 1 0 0 0>; interrupt-parent = <&ipic>; interrupts = <17 IRQ_TYPE_LEVEL_LOW>; interrupt-controller;
Unfortunately some hardware device will assert their irq line immediately on power on and provide no mechanism to mask the irq. As the i2c muxes provide no method to mask irq line this provides a work around by keeping the parent irq masked until enough device drivers have loaded to service all pending interrupts. For example the the ltc1760 assert its SMBALERT irq immediately on power on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first device is registered irq are enabled and fire continuously as the second device driver has not yet loaded. Setting this parameter to <1 1> will delay the irq being enabled until both devices are ready. Signed-off-by: Phil Reid <preid@electromag.com.au> --- Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 +++ 1 file changed, 3 insertions(+)