| Message ID | 1411067086-16613-4-git-send-email-nm@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 18 Sep 2014, Nishanth Menon wrote: > +static irqreturn_t palmas_wake_irq(int irq, void *_palmas) > +{ > + /* > + * Return Not handled so that interrupt is disabled. And how is that interrupt disabled by returning IRQ_NONE? You mean it gets disabled after it got reraised 100000 times and the spurious detector kills it? > + * Level event ensures that the event is eventually handled > + * by the appropriate chip handler already registered Eventually handled? So eventually it's not handled? > + */ > + return IRQ_NONE; > +} > + > int palmas_ext_control_req_config(struct palmas *palmas, > enum palmas_external_requestor_id id, int ext_ctrl, bool enable) > { > @@ -409,6 +420,7 @@ static void palmas_dt_to_pdata(struct i2c_client *i2c, > pdata->mux_from_pdata = 1; > pdata->pad2 = prop; > } > + pdata->wakeirq = irq_of_parse_and_map(node, 1); > > /* The default for this register is all masked */ > ret = of_property_read_u32(node, "ti,power-ctrl", &prop); > @@ -521,6 +533,7 @@ static int palmas_i2c_probe(struct i2c_client *i2c, > i2c_set_clientdata(i2c, palmas); > palmas->dev = &i2c->dev; > palmas->irq = i2c->irq; > + palmas->wakeirq = pdata->wakeirq; > > match = of_match_device(of_palmas_match_tbl, &i2c->dev); > > @@ -587,6 +600,25 @@ static int palmas_i2c_probe(struct i2c_client *i2c, > if (ret < 0) > goto err_i2c; > > + if (!palmas->wakeirq) > + goto no_wake_irq; > + > + ret = devm_request_irq(palmas->dev, palmas->wakeirq, > + palmas_wake_irq, > + pdata->irq_flags, > + dev_name(palmas->dev), > + &palmas); > + if (ret < 0) { > + dev_err(palmas->dev, "Invalid wakeirq(%d) (res: %d), skiping\n", > + palmas->wakeirq, ret); > + palmas->wakeirq = 0; > + } else { > + /* We use wakeirq only during suspend-resume path */ > + device_set_wakeup_capable(palmas->dev, true); > + disable_irq_nosync(palmas->wakeirq); Urgh. Why nosysnc? And why do you want to do that at all? irq_set_status_flags(irq, IRQ_NOAUTOEN); Is what you want to set before requesting the irq. > + } > + > +no_wake_irq: > no_irq: > slave = PALMAS_BASE_TO_SLAVE(PALMAS_PU_PD_OD_BASE); > addr = PALMAS_BASE_TO_REG(PALMAS_PU_PD_OD_BASE, > @@ -706,6 +738,34 @@ static int palmas_i2c_remove(struct i2c_client *i2c) > return 0; > } > > +static int palmas_i2c_suspend(struct i2c_client *i2c, pm_message_t mesg) > +{ > + struct palmas *palmas = i2c_get_clientdata(i2c); > + struct device *dev = &i2c->dev; > + > + if (!palmas->wakeirq) > + return 0; > + > + if (device_may_wakeup(dev)) > + enable_irq(palmas->wakeirq); > + > + return 0; > +} > + > +static int palmas_i2c_resume(struct i2c_client *i2c) > +{ > + struct palmas *palmas = i2c_get_clientdata(i2c); > + struct device *dev = &i2c->dev; > + > + if (!palmas->wakeirq) > + return 0; > + > + if (device_may_wakeup(dev)) > + disable_irq_nosync(palmas->wakeirq); Again, why nosync? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17:57-20140918, Thomas Gleixner wrote: > On Thu, 18 Sep 2014, Nishanth Menon wrote: > > +static irqreturn_t palmas_wake_irq(int irq, void *_palmas) > > +{ > > + /* > > + * Return Not handled so that interrupt is disabled. > > And how is that interrupt disabled by returning IRQ_NONE? You mean it > gets disabled after it got reraised 100000 times and the spurious > detector kills it? No, that does not happen due to the hardware involved: http://fpaste.org/134757/09428214/ (there are still fixes needed to various drivers to completely achieve low power states). Explanation below: > > > + * Level event ensures that the event is eventually handled > > + * by the appropriate chip handler already registered > > Eventually handled? So eventually it's not handled? I had previously tried explaining this in v1: https://patchwork.kernel.org/patch/4743321/ I agree it is a little convoluted, so will try again: mainly because of the hardware entities involved. Two different hardware generate interrupt. A GPIO hardware block handles palmas interrupts when SoC is ON, However with GPIO block active, we cannot achieve low power suspend (deep sleep) for the SoC, so, we switch off all GPIOs and SoC hardware blocks OFF as part of the sequence of going to low power suspend (mem), and depend on the hardware controlling the pins of the SoC to generate wakeup event (wakeirq). wakeirq is provided by drivers/pinctrl/pinctrl-single.c (implementation to handle pad generated interrupt source). wakeirq is generated by the hardware at the pin (we call it control module in TI SoC), this generates an interrupt on level change. Palmas generates level interrupt, and the level is cleared when interrupt source is cleared. At suspend - we enable_irq(wakeirq) - this arms the pin for palmas interrupt to generate an interrupt when level changes. We start the wake sequence in deep sleep (only thing alive is that control module, every thing else, including GPIO block is powered off). On generating a wakeup event (in the example log, I used palmas power button), palmas generates a level event, the transition triggers two things: a) control module generates wakeirq (detecting the level shift) b) wakeirq causes wakeup of SoC from deep sleep. wakeirq wont be generated again by the hardware because pinctrl handles the wakeirq interrupt event in the control module. At resume - we disable_irq wakeirq -which in turn disables the pin for generating interrupts if level ever changes again. GPIO block is restored as part of resume path, and we generate the handler for palmas regular interrupt service which in turn goes and detects the real event and handles it. I suppose I can improve the commit message to elaborate this better? Will that help? > > > + */ > > + return IRQ_NONE; > > +} > > + > > int palmas_ext_control_req_config(struct palmas *palmas, > > enum palmas_external_requestor_id id, int ext_ctrl, bool enable) > > { > > @@ -409,6 +420,7 @@ static void palmas_dt_to_pdata(struct i2c_client *i2c, > > pdata->mux_from_pdata = 1; > > pdata->pad2 = prop; > > } > > + pdata->wakeirq = irq_of_parse_and_map(node, 1); > > > > /* The default for this register is all masked */ > > ret = of_property_read_u32(node, "ti,power-ctrl", &prop); > > @@ -521,6 +533,7 @@ static int palmas_i2c_probe(struct i2c_client *i2c, > > i2c_set_clientdata(i2c, palmas); > > palmas->dev = &i2c->dev; > > palmas->irq = i2c->irq; > > + palmas->wakeirq = pdata->wakeirq; > > > > match = of_match_device(of_palmas_match_tbl, &i2c->dev); > > > > @@ -587,6 +600,25 @@ static int palmas_i2c_probe(struct i2c_client *i2c, > > if (ret < 0) > > goto err_i2c; > > > > + if (!palmas->wakeirq) > > + goto no_wake_irq; > > + > > + ret = devm_request_irq(palmas->dev, palmas->wakeirq, > > + palmas_wake_irq, > > + pdata->irq_flags, > > + dev_name(palmas->dev), > > + &palmas); > > + if (ret < 0) { > > + dev_err(palmas->dev, "Invalid wakeirq(%d) (res: %d), skiping\n", > > + palmas->wakeirq, ret); > > + palmas->wakeirq = 0; > > + } else { > > + /* We use wakeirq only during suspend-resume path */ > > + device_set_wakeup_capable(palmas->dev, true); > > + disable_irq_nosync(palmas->wakeirq); > > Urgh. Why nosysnc? And why do you want to do that at all? > > irq_set_status_flags(irq, IRQ_NOAUTOEN); > > Is what you want to set before requesting the irq. Aaah, OK. thanks on the suggestion. will do that. > > > > + } > > + > > +no_wake_irq: > > no_irq: > > slave = PALMAS_BASE_TO_SLAVE(PALMAS_PU_PD_OD_BASE); > > addr = PALMAS_BASE_TO_REG(PALMAS_PU_PD_OD_BASE, > > @@ -706,6 +738,34 @@ static int palmas_i2c_remove(struct i2c_client *i2c) > > return 0; > > } > > > > +static int palmas_i2c_suspend(struct i2c_client *i2c, pm_message_t mesg) > > +{ > > + struct palmas *palmas = i2c_get_clientdata(i2c); > > + struct device *dev = &i2c->dev; > > + > > + if (!palmas->wakeirq) > > + return 0; > > + > > + if (device_may_wakeup(dev)) > > + enable_irq(palmas->wakeirq); > > + > > + return 0; > > +} > > + > > +static int palmas_i2c_resume(struct i2c_client *i2c) > > +{ > > + struct palmas *palmas = i2c_get_clientdata(i2c); > > + struct device *dev = &i2c->dev; > > + > > + if (!palmas->wakeirq) > > + return 0; > > + > > + if (device_may_wakeup(dev)) > > + disable_irq_nosync(palmas->wakeirq); > > Again, why nosync? true - nosync is not necessary at here. disable_irq is however necessary as we are not interested in wakeup events for level changes. We just use the enable/disable to control when we'd want to arm the pin for waking up from suspend state.
On Thu, 18 Sep 2014, Nishanth Menon wrote: > On 17:57-20140918, Thomas Gleixner wrote: > > I suppose I can improve the commit message to elaborate this better? > Will that help? You also want to improve the comment in the empty handler. > > > > > + */ > > > + return IRQ_NONE; And it still does not explain WHY you think that returning IRQ_NONE is the right thing to do here. You actually handle the interrupt, right? Just because the handler is an NOP does not mean you did not handle it. > > > +static int palmas_i2c_suspend(struct i2c_client *i2c, pm_message_t mesg) > > > +{ > > > + struct palmas *palmas = i2c_get_clientdata(i2c); > > > + struct device *dev = &i2c->dev; > > > + > > > + if (!palmas->wakeirq) > > > + return 0; > > > + > > > + if (device_may_wakeup(dev)) > > > + enable_irq(palmas->wakeirq); > > > + > > > + return 0; > > > +} > > > + > > > +static int palmas_i2c_resume(struct i2c_client *i2c) > > > +{ > > > + struct palmas *palmas = i2c_get_clientdata(i2c); > > > + struct device *dev = &i2c->dev; > > > + > > > + if (!palmas->wakeirq) > > > + return 0; > > > + > > > + if (device_may_wakeup(dev)) > > > + disable_irq_nosync(palmas->wakeirq); > > > > Again, why nosync? > true - nosync is not necessary at here. disable_irq is however necessary > as we are not interested in wakeup events for level changes. > > We just use the enable/disable to control when we'd want to arm the pin > for waking up from suspend state. And what is issuing the call to enable/disable_irq_wake()? So if that interrupt is not marked proper then you can bring your device into a wont resume state easily start suspend enable wakeirq disable_device_irqs() if (!iswakeup_irq()) disable_irq() // does not mask due to lazy masking .... wakeirq fires if (irq_is_disabled()) mask_irq(); transition into suspend Now your pinctrl irq is masked at the HW level and wont wake the machine up ever again. So now looking at that pinctrl irq chip thing, which seems to be designed to handle these kind of wakeups. That thing looks massivly wrong as well, simply because it enforces to use enable_irq/disable_irq(). So because the sole purpose of this chip is to handle the separate wakeup style interrupt, it should actually NOT enable the interrupt in the irq_unmask callback. Simply because during normal operation nothing is interested in the interrupt and any operation which might enable it (including request irq) is just making the system handle completely pointless interrupts and hoops and loops juggling with enable/disable irq. So the right thing here is to have an empty unmask function and do the actual unmask only in the irq_set_wake() callback. mask of course needs to do what it says. The point is, that the following sequence of code will just work w/o generating an interrupt on the wakeirq line outside of the wake enabled context. dev_init() request_wakeirq(); suspend() if (may_wake()) enable_irq_wake(); resume() if (may_wake()) disable_irq_wake(); The other omap drivers using this have the same issue ... And of course they are subtly different. The uart one handles the actual device interrupt, which is violating the general rule of possible interrupt reentrancy in the pm-runtime case if the two interrupts are affine to two different cores. Yes, it's protected by a lock and works by chance .... The mmc one issues a disable_irq_nosync() in the wakeup irq handler itself. WHY does one driver need that and the other does not? You are not even able to come up with a common scheme for OMAP. I don't want to see the mess others are going to create when this stuff becomes more used. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08:37-20140919, Thomas Gleixner wrote: > On Thu, 18 Sep 2014, Nishanth Menon wrote: > > On 17:57-20140918, Thomas Gleixner wrote: > > > > I suppose I can improve the commit message to elaborate this better? > > Will that help? > > You also want to improve the comment in the empty handler. OK. will do the same. Thanks for suggesting. > > > > > > > > + */ > > > > + return IRQ_NONE; > > And it still does not explain WHY you think that returning IRQ_NONE is > the right thing to do here. You actually handle the interrupt, right? > Just because the handler is an NOP does not mean you did not handle > it. Hmm.. My motivation for IRQ_NONE was because this specific handler does not handle the interrupt. Now, from this discussion, I understand that I should rather use IRQ_HANDLED since the event is indeed handled (just not here). Thank you for correcting my understanding. Will update in my next rev (once we solve the following discussion).. > > > > > +static int palmas_i2c_suspend(struct i2c_client *i2c, pm_message_t mesg) > > > > +{ > > > > + struct palmas *palmas = i2c_get_clientdata(i2c); > > > > + struct device *dev = &i2c->dev; > > > > + > > > > + if (!palmas->wakeirq) > > > > + return 0; > > > > + > > > > + if (device_may_wakeup(dev)) > > > > + enable_irq(palmas->wakeirq); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int palmas_i2c_resume(struct i2c_client *i2c) > > > > +{ > > > > + struct palmas *palmas = i2c_get_clientdata(i2c); > > > > + struct device *dev = &i2c->dev; > > > > + > > > > + if (!palmas->wakeirq) > > > > + return 0; > > > > + > > > > + if (device_may_wakeup(dev)) > > > > + disable_irq_nosync(palmas->wakeirq); > > > > > > Again, why nosync? > > true - nosync is not necessary at here. disable_irq is however necessary > > as we are not interested in wakeup events for level changes. > > > > We just use the enable/disable to control when we'd want to arm the pin > > for waking up from suspend state. > > And what is issuing the call to enable/disable_irq_wake()? > > So if that interrupt is not marked proper then you can bring your > device into a wont resume state easily > > start suspend > enable wakeirq > disable_device_irqs() > if (!iswakeup_irq()) > disable_irq() // does not mask due to lazy masking > > .... > wakeirq fires > if (irq_is_disabled()) > mask_irq(); > > transition into suspend > > Now your pinctrl irq is masked at the HW level and wont wake the > machine up ever again. True. > > So now looking at that pinctrl irq chip thing, which seems to be > designed to handle these kind of wakeups. That thing looks massivly > wrong as well, simply because it enforces to use > enable_irq/disable_irq(). > > So because the sole purpose of this chip is to handle the separate > wakeup style interrupt, it should actually NOT enable the interrupt in > the irq_unmask callback. > > Simply because during normal operation nothing is interested in the > interrupt and any operation which might enable it (including request > irq) is just making the system handle completely pointless interrupts > and hoops and loops juggling with enable/disable irq. > > So the right thing here is to have an empty unmask function and do the > actual unmask only in the irq_set_wake() callback. mask of course > needs to do what it says. The point is, that the following sequence of > code will just work w/o generating an interrupt on the wakeirq line > outside of the wake enabled context. > > dev_init() > request_wakeirq(); > > suspend() > if (may_wake()) > enable_irq_wake(); > > resume() > if (may_wake()) > disable_irq_wake(); > > The other omap drivers using this have the same issue ... And of > course they are subtly different. > > The uart one handles the actual device interrupt, which is violating > the general rule of possible interrupt reentrancy in the pm-runtime > case if the two interrupts are affine to two different cores. Yes, > it's protected by a lock and works by chance .... > > The mmc one issues a disable_irq_nosync() in the wakeup irq handler > itself. > > WHY does one driver need that and the other does not? You are not even > able to come up with a common scheme for OMAP. I don't want to see the > mess others are going to create when this stuff becomes more used. > > Thanks, > > tglx I think I understand your concern - I request Tony to comment about this. I mean, I can try and hook things like uart in other drivers (like https://patchwork.kernel.org/patch/4759171/ ), but w.r.t overall generic usage guideline wise, I would prefer Tony to comment.
On Fri, 19 Sep 2014, Nishanth Menon wrote: > On 08:37-20140919, Thomas Gleixner wrote: > > The other omap drivers using this have the same issue ... And of > > course they are subtly different. > > > > The uart one handles the actual device interrupt, which is violating > > the general rule of possible interrupt reentrancy in the pm-runtime > > case if the two interrupts are affine to two different cores. Yes, > > it's protected by a lock and works by chance .... > > > > The mmc one issues a disable_irq_nosync() in the wakeup irq handler > > itself. > > > > WHY does one driver need that and the other does not? You are not even > > able to come up with a common scheme for OMAP. I don't want to see the > > mess others are going to create when this stuff becomes more used. > > > > Thanks, > > > > tglx > > I think I understand your concern - I request Tony to comment about > this. I mean, I can try and hook things like uart in other drivers > (like https://patchwork.kernel.org/patch/4759171/ ), but w.r.t overall > generic usage guideline wise, I would prefer Tony to comment. No, the uart and that i2c thing are just wrong. Assume the following device irq affine to cpu0 wakeup irq affine to cpu1 CPU 0 CPU 1 runtime suspend enable_wake(wakeup irq); wakeup interrupt is raised device interrupt is raised dev_handler(device) dev_handler(device) It might work due to locking, but it is nevertheless wrong. Interrupt handlers for devices are guaranteed not to be reentrant. And this brilliant stuff simply violates that guarantee. So, no. It's wrong even if it happens to work by chance. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Thomas Gleixner <tglx@linutronix.de> [140919 10:37]: > On Fri, 19 Sep 2014, Nishanth Menon wrote: > > On 08:37-20140919, Thomas Gleixner wrote: > > > The other omap drivers using this have the same issue ... And of > > > course they are subtly different. > > > > > > The uart one handles the actual device interrupt, which is violating > > > the general rule of possible interrupt reentrancy in the pm-runtime > > > case if the two interrupts are affine to two different cores. Yes, > > > it's protected by a lock and works by chance .... > > > > > > The mmc one issues a disable_irq_nosync() in the wakeup irq handler > > > itself. > > > > > > WHY does one driver need that and the other does not? You are not even > > > able to come up with a common scheme for OMAP. I don't want to see the > > > mess others are going to create when this stuff becomes more used. > > > > > > Thanks, > > > > > > tglx > > > > I think I understand your concern - I request Tony to comment about > > this. I mean, I can try and hook things like uart in other drivers > > (like https://patchwork.kernel.org/patch/4759171/ ), but w.r.t overall > > generic usage guideline wise, I would prefer Tony to comment. > > No, the uart and that i2c thing are just wrong. Assume the following > > device irq affine to cpu0 > wakeup irq affine to cpu1 > > CPU 0 CPU 1 > > runtime suspend > > enable_wake(wakeup irq); > > wakeup interrupt is raised device interrupt is raised > > dev_handler(device) dev_handler(device) > > It might work due to locking, but it is nevertheless wrong. Interrupt > handlers for devices are guaranteed not to be reentrant. And this > brilliant stuff simply violates that guarantee. So, no. It's wrong > even if it happens to work by chance. Hmm yeah that's a good point indeed. From hardware point of view the wake-up events behave like interrupts and could also be used as the only interrupt in some messed up cases. That avoids all kinds of custom APIs from driver point. The re-entrancy problem we've most likely had ever since we enabled the PRCM interrupts, and maybe that's why I did not even consider that part. I think before that we were calling the driver interrupt after waking up from the PM code.. Anyways, how about the following to deal with the re-entrancy problem: 1. The wake-up interrupt handler must have a separate interrupt handler that just calls tasklet_schedule() 2. The device interrupt handler also just calls tasklet_schedule() 3. The tasklet then does pm_runtime_get, handles the registers, and so on. Or would we still have a re-entrancy problem somewhere else with that? Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 19 Sep 2014, Tony Lindgren wrote: > * Thomas Gleixner <tglx@linutronix.de> [140919 10:37]: > >From hardware point of view the wake-up events behave like interrupts > and could also be used as the only interrupt in some messed up cases. > That avoids all kinds of custom APIs from driver point. > > The re-entrancy problem we've most likely had ever since we enabled > the PRCM interrupts, and maybe that's why I did not even consider > that part. I think before that we were calling the driver interrupt > after waking up from the PM code.. > > Anyways, how about the following to deal with the re-entrancy problem: > > 1. The wake-up interrupt handler must have a separate interrupt > handler that just calls tasklet_schedule() > > 2. The device interrupt handler also just calls tasklet_schedule() > > 3. The tasklet then does pm_runtime_get, handles the registers, and > so on. > > Or would we still have a re-entrancy problem somewhere else with > that? Why on earth are you wanting tasklets in there? That's just silly, really. The wakeup handler is supposed to bring the thing out of deep sleep and nothing else. All you want it to do is to mask itself and save the information that the real device irq is pending. A stub handler for the wakeup irq is enough. We can have that in the irq/pm core and all it would do is simply: irqreturn_t handle_jinxed_wakeup_irq(unsigned irq, void *dev_id) { unsigned device_irq = get_dev_irq(dev_id); force_mask(irq); set_irq_pending(device_irq); return HANDLED; } So on resume_device_irqs() the real device interrupt gets reenabled and unmasked (if it was masked) and the interrupt gets resent either in hardware (level or retrigger) or by the software resend mechanism. That completely avoids tasklets, reentrant irq handlers and all other crap which might be required. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Thomas Gleixner <tglx@linutronix.de> [140919 12:47]: > On Fri, 19 Sep 2014, Tony Lindgren wrote: > > * Thomas Gleixner <tglx@linutronix.de> [140919 10:37]: > > >From hardware point of view the wake-up events behave like interrupts > > and could also be used as the only interrupt in some messed up cases. > > That avoids all kinds of custom APIs from driver point. > > > > The re-entrancy problem we've most likely had ever since we enabled > > the PRCM interrupts, and maybe that's why I did not even consider > > that part. I think before that we were calling the driver interrupt > > after waking up from the PM code.. > > > > Anyways, how about the following to deal with the re-entrancy problem: > > > > 1. The wake-up interrupt handler must have a separate interrupt > > handler that just calls tasklet_schedule() > > > > 2. The device interrupt handler also just calls tasklet_schedule() > > > > 3. The tasklet then does pm_runtime_get, handles the registers, and > > so on. > > > > Or would we still have a re-entrancy problem somewhere else with > > that? > > Why on earth are you wanting tasklets in there? That's just silly, > really. Lack of a framework on driver side to cope with this in a generic way? :p > The wakeup handler is supposed to bring the thing out of deep sleep > and nothing else. All you want it to do is to mask itself and save the > information that the real device irq is pending. Yes that is enough. > A stub handler for the wakeup irq is enough. We can have that in the > irq/pm core and all it would do is simply: > > irqreturn_t handle_jinxed_wakeup_irq(unsigned irq, void *dev_id) > { > unsigned device_irq = get_dev_irq(dev_id); > > force_mask(irq); > set_irq_pending(device_irq); > return HANDLED; > } > > So on resume_device_irqs() the real device interrupt gets reenabled > and unmasked (if it was masked) and the interrupt gets resent either > in hardware (level or retrigger) or by the software resend mechanism. > > That completely avoids tasklets, reentrant irq handlers and all other > crap which might be required. Okie dokie, that sounds good to me. Having this generic for the drivers would be excellent, that's the missing piece. Thanks, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 19 Sep 2014, Tony Lindgren wrote: > * Thomas Gleixner <tglx@linutronix.de> [140919 12:47]: > > Why on earth are you wanting tasklets in there? That's just silly, > > really. > > Lack of a framework on driver side to cope with this in a generic > way? :p So instead of creating such a thing we rather have a completely ass backward workaround which spreads itself all over the tree? You SoC folks really need a quarterly sanity check. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Thomas Gleixner <tglx@linutronix.de> [140919 19:08]: > On Fri, 19 Sep 2014, Tony Lindgren wrote: > > * Thomas Gleixner <tglx@linutronix.de> [140919 12:47]: > > > Why on earth are you wanting tasklets in there? That's just silly, > > > really. > > > > Lack of a framework on driver side to cope with this in a generic > > way? :p > > So instead of creating such a thing we rather have a completely ass > backward workaround which spreads itself all over the tree? I'm not kidding, you're the first one who was able to come up with such a simple solution and also describe why it should be done that way. > You SoC folks really need a quarterly sanity check. Probably time for me to file a bios bug on that yeah :) Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c index 28cb048..de7d204 100644 --- a/drivers/mfd/palmas.c +++ b/drivers/mfd/palmas.c @@ -24,6 +24,7 @@ #include <linux/mfd/core.h> #include <linux/mfd/palmas.h> #include <linux/of_device.h> +#include <linux/of_irq.h> static const struct regmap_config palmas_regmap_config[PALMAS_NUM_CLIENTS] = { { @@ -326,6 +327,16 @@ static struct regmap_irq_chip tps65917_irq_chip = { PALMAS_INT1_MASK), }; +static irqreturn_t palmas_wake_irq(int irq, void *_palmas) +{ + /* + * Return Not handled so that interrupt is disabled. + * Level event ensures that the event is eventually handled + * by the appropriate chip handler already registered + */ + return IRQ_NONE; +} + int palmas_ext_control_req_config(struct palmas *palmas, enum palmas_external_requestor_id id, int ext_ctrl, bool enable) { @@ -409,6 +420,7 @@ static void palmas_dt_to_pdata(struct i2c_client *i2c, pdata->mux_from_pdata = 1; pdata->pad2 = prop; } + pdata->wakeirq = irq_of_parse_and_map(node, 1); /* The default for this register is all masked */ ret = of_property_read_u32(node, "ti,power-ctrl", &prop); @@ -521,6 +533,7 @@ static int palmas_i2c_probe(struct i2c_client *i2c, i2c_set_clientdata(i2c, palmas); palmas->dev = &i2c->dev; palmas->irq = i2c->irq; + palmas->wakeirq = pdata->wakeirq; match = of_match_device(of_palmas_match_tbl, &i2c->dev); @@ -587,6 +600,25 @@ static int palmas_i2c_probe(struct i2c_client *i2c, if (ret < 0) goto err_i2c; + if (!palmas->wakeirq) + goto no_wake_irq; + + ret = devm_request_irq(palmas->dev, palmas->wakeirq, + palmas_wake_irq, + pdata->irq_flags, + dev_name(palmas->dev), + &palmas); + if (ret < 0) { + dev_err(palmas->dev, "Invalid wakeirq(%d) (res: %d), skiping\n", + palmas->wakeirq, ret); + palmas->wakeirq = 0; + } else { + /* We use wakeirq only during suspend-resume path */ + device_set_wakeup_capable(palmas->dev, true); + disable_irq_nosync(palmas->wakeirq); + } + +no_wake_irq: no_irq: slave = PALMAS_BASE_TO_SLAVE(PALMAS_PU_PD_OD_BASE); addr = PALMAS_BASE_TO_REG(PALMAS_PU_PD_OD_BASE, @@ -706,6 +738,34 @@ static int palmas_i2c_remove(struct i2c_client *i2c) return 0; } +static int palmas_i2c_suspend(struct i2c_client *i2c, pm_message_t mesg) +{ + struct palmas *palmas = i2c_get_clientdata(i2c); + struct device *dev = &i2c->dev; + + if (!palmas->wakeirq) + return 0; + + if (device_may_wakeup(dev)) + enable_irq(palmas->wakeirq); + + return 0; +} + +static int palmas_i2c_resume(struct i2c_client *i2c) +{ + struct palmas *palmas = i2c_get_clientdata(i2c); + struct device *dev = &i2c->dev; + + if (!palmas->wakeirq) + return 0; + + if (device_may_wakeup(dev)) + disable_irq_nosync(palmas->wakeirq); + + return 0; +} + static const struct i2c_device_id palmas_i2c_id[] = { { "palmas", }, { "twl6035", }, @@ -721,6 +781,8 @@ static struct i2c_driver palmas_i2c_driver = { .of_match_table = of_palmas_match_tbl, .owner = THIS_MODULE, }, + .suspend = palmas_i2c_suspend, + .resume = palmas_i2c_resume, .probe = palmas_i2c_probe, .remove = palmas_i2c_remove, .id_table = palmas_i2c_id, diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h index fb0390a..e8cf4c2 100644 --- a/include/linux/mfd/palmas.h +++ b/include/linux/mfd/palmas.h @@ -75,6 +75,7 @@ struct palmas { /* IRQ Data */ int irq; u32 irq_mask; + int wakeirq; struct mutex irq_lock; struct regmap_irq_chip_data *irq_data; @@ -377,6 +378,7 @@ struct palmas_clk_platform_data { struct palmas_platform_data { int irq_flags; + int wakeirq; int gpio_base; /* bit value to be loaded to the POWER_CTRL register */
With the recent pinctrl-single changes, omaps can treat wake-up events from deeper power states as interrupts. This is to handle the case where the system needs two interrupt sources when SoC is in deep sleep(1 to exit from deep power mode such as sleep, and other from the module handling the actual event during system active state). This is not the same as threaded interrupts as the wakeup interrupt source is used only as part of deeper power saving mode. Let's add support for the optional second interrupt for wake-up events. And then SoC can wakeup and handle the event using it's regular handler. This is similar in approach to commit 2a0b965cfb6e ("serial: omap: Add support for optional wake-up") Signed-off-by: Nishanth Menon <nm@ti.com> --- V3: updates based on Thomas's comments. V2: http://marc.info/?t=140995045000003&r=1&w=2 drivers/mfd/palmas.c | 62 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/mfd/palmas.h | 2 ++ 2 files changed, 64 insertions(+)