| Message ID | 1459785403-1725-1-git-send-email-m.niestroj@grinn-global.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi, * Marcin Niestroj <m.niestroj@grinn-global.com> [160404 08:57]: > Support configuration of ext_wakeup sources. This patch makes it > possible to enable ext_wakeup and set it's polarity, depending on board > configuration. AM335x's dedicated PMIC (tps65217) uses ext_wakeup to > notify about power-button presses. Handling power-button presses enables > to recover from RTC-only power states correctly. I suggest you just set this pin up as a minimal gpiochip. That way we can use the standard binding :) And if we get lucky, this pin can also trigger during runtime. Regards, Tony
Hi Tony, On 04/05/2016 01:40 AM, Tony Lindgren wrote: > Hi, > > * Marcin Niestroj <m.niestroj@grinn-global.com> [160404 08:57]: >> Support configuration of ext_wakeup sources. This patch makes it >> possible to enable ext_wakeup and set it's polarity, depending on board >> configuration. AM335x's dedicated PMIC (tps65217) uses ext_wakeup to >> notify about power-button presses. Handling power-button presses enables >> to recover from RTC-only power states correctly. > > I suggest you just set this pin up as a minimal gpiochip. That way > we can use the standard binding :) And if we get lucky, this pin can > also trigger during runtime. > Following my comments on v2 of this patch I propose to rollback to this version of the patch. It seems doesn't fit in gpiochip, it's more likely irqchip, but since rtc can't generate IRQ there are nor reasons for these genetic experiments and RTC's specific bindings looks more suitable, at least for me.
* Grygorii Strashko <grygorii.strashko@ti.com> [160408 03:41]: > Hi Tony, > > On 04/05/2016 01:40 AM, Tony Lindgren wrote: > > Hi, > > > > * Marcin Niestroj <m.niestroj@grinn-global.com> [160404 08:57]: > >> Support configuration of ext_wakeup sources. This patch makes it > >> possible to enable ext_wakeup and set it's polarity, depending on board > >> configuration. AM335x's dedicated PMIC (tps65217) uses ext_wakeup to > >> notify about power-button presses. Handling power-button presses enables > >> to recover from RTC-only power states correctly. > > > > I suggest you just set this pin up as a minimal gpiochip. That way > > we can use the standard binding :) And if we get lucky, this pin can > > also trigger during runtime. > > > > Following my comments on v2 of this patch I propose to rollback to > this version of the patch. > > It seems doesn't fit in gpiochip, it's more likely irqchip, but since > rtc can't generate IRQ there are nor reasons for these genetic > experiments and RTC's specific bindings looks more suitable, at least for me. Hmm well gpiochips typically are irqchip too. IMO the generic binding here sounds like "gpio-wakeup" as it's an input with polarity and with an optional interrupt. Plain irqchip would work too in this case if there are no other GPIO specific features. Some other RTCs may have more GPIO like features. In any case, setting the ext_wakeup up as an irqchip means that the RTC controller can be used as a dedicated wakeirq with Linux :) Are you guys sure there's no wake pin events during runtime? AFAIK the RTCs just typically produce an interrupt when programmed to do so, they don't know the state of the SoC. Regards, Tony
On 04/08/2016 06:14 PM, Tony Lindgren wrote: > * Grygorii Strashko <grygorii.strashko@ti.com> [160408 03:41]: >> Hi Tony, >> >> On 04/05/2016 01:40 AM, Tony Lindgren wrote: >>> Hi, >>> >>> * Marcin Niestroj <m.niestroj@grinn-global.com> [160404 08:57]: >>>> Support configuration of ext_wakeup sources. This patch makes it >>>> possible to enable ext_wakeup and set it's polarity, depending on board >>>> configuration. AM335x's dedicated PMIC (tps65217) uses ext_wakeup to >>>> notify about power-button presses. Handling power-button presses enables >>>> to recover from RTC-only power states correctly. >>> >>> I suggest you just set this pin up as a minimal gpiochip. That way >>> we can use the standard binding :) And if we get lucky, this pin can >>> also trigger during runtime. >>> >> >> Following my comments on v2 of this patch I propose to rollback to >> this version of the patch. >> >> It seems doesn't fit in gpiochip, it's more likely irqchip, but since >> rtc can't generate IRQ there are nor reasons for these genetic >> experiments and RTC's specific bindings looks more suitable, at least for me. > > Hmm well gpiochips typically are irqchip too. IMO the generic binding > here sounds like "gpio-wakeup" as it's an input with polarity and with > an optional interrupt. > > Plain irqchip would work too in this case if there are no other GPIO > specific features. Some other RTCs may have more GPIO like features. > > In any case, setting the ext_wakeup up as an irqchip means that the > RTC controller can be used as a dedicated wakeirq with Linux :) It can't :( It can't generate IRQ when state of ext_wakeup line has been changed. > > Are you guys sure there's no wake pin events during runtime? AFAIK > the RTCs just typically produce an interrupt when programmed to > do so, they don't know the state of the SoC. > I do not think that It can be fit in gpiochip or irqchip - in my opinion right way is to proceed with bindings proposed by Marcin in this patch v1. But, probably, it could fit in pinctrl: - this is pin's configuration - this is one-time configuration - or - configuration which can be applied before suspend and resorted after suspend. if you still wanna try some generic framework.
* Grygorii Strashko <grygorii.strashko@ti.com> [160408 10:17]: > On 04/08/2016 06:14 PM, Tony Lindgren wrote: > > * Grygorii Strashko <grygorii.strashko@ti.com> [160408 03:41]: > >> Hi Tony, > >> > >> On 04/05/2016 01:40 AM, Tony Lindgren wrote: > >>> Hi, > >>> > >>> * Marcin Niestroj <m.niestroj@grinn-global.com> [160404 08:57]: > >>>> Support configuration of ext_wakeup sources. This patch makes it > >>>> possible to enable ext_wakeup and set it's polarity, depending on board > >>>> configuration. AM335x's dedicated PMIC (tps65217) uses ext_wakeup to > >>>> notify about power-button presses. Handling power-button presses enables > >>>> to recover from RTC-only power states correctly. > >>> > >>> I suggest you just set this pin up as a minimal gpiochip. That way > >>> we can use the standard binding :) And if we get lucky, this pin can > >>> also trigger during runtime. > >>> > >> > >> Following my comments on v2 of this patch I propose to rollback to > >> this version of the patch. > >> > >> It seems doesn't fit in gpiochip, it's more likely irqchip, but since > >> rtc can't generate IRQ there are nor reasons for these genetic > >> experiments and RTC's specific bindings looks more suitable, at least for me. > > > > Hmm well gpiochips typically are irqchip too. IMO the generic binding > > here sounds like "gpio-wakeup" as it's an input with polarity and with > > an optional interrupt. > > > > Plain irqchip would work too in this case if there are no other GPIO > > specific features. Some other RTCs may have more GPIO like features. > > > > In any case, setting the ext_wakeup up as an irqchip means that the > > RTC controller can be used as a dedicated wakeirq with Linux :) > > It can't :( It can't generate IRQ when state of ext_wakeup line has > been changed. Hmm to me it certainly seems it should be capable of generating and RTC interrupt as we have EXT_WAKEUP_STATUS register containing four bits, one for each wakeup line. We also have EXT_WAKEUP_DB_EN for debounce, EXT_WAKEUP_POL for polarity, and EXT_WAKEUP_EN to enable in addition to status.. These certainly make it look like a gpiochip or irqchip. > > Are you guys sure there's no wake pin events during runtime? AFAIK > > the RTCs just typically produce an interrupt when programmed to > > do so, they don't know the state of the SoC. > > > > I do not think that It can be fit in gpiochip or irqchip - > in my opinion right way is to proceed with bindings proposed by > Marcin in this patch v1. > > But, probably, it could fit in pinctrl: > - this is pin's configuration > - this is one-time configuration > - or - configuration which can be applied before suspend > and resorted after suspend. > > if you still wanna try some generic framework. Yeah the pin certainly should be configured by pinctrl if it is muxable. However, the functionality of the pin is clearly tied to the RTC driver. My guess the reason it does not generate interrupts is that it's status needs to be cleared each time it triggers? That fits the irq mask/unmask for generic wakeirq like we do for the padconf. Sounds like some experiments and reasoning is needed :) I guess the first test to run is tools/testing/selftests/timers/rtctest.c to make sure the RTC interrupts trigger properly. Regards, Tony
* Tony Lindgren <tony@atomide.com> [160408 10:52]: > * Grygorii Strashko <grygorii.strashko@ti.com> [160408 10:17]: > > > > It can't :( It can't generate IRQ when state of ext_wakeup line has > > been changed. > > Hmm to me it certainly seems it should be capable of generating and RTC > interrupt as we have EXT_WAKEUP_STATUS register containing four bits, > one for each wakeup line. So I played a bit with RTC last night and the RTC_PMIC_REG options.. And I came to the same conclusion as Grygorii pretty much. It looks like an interrupt, except it can't generate any interrupts. Sorry for not believing it earlier. It might still be a good idea to confirm this with some hardware folks at TI, maybe newer versions could add support for an interrupt. The EXT_WAKEUP configuration only seems to affect pmic_pwr_enable pin if configured right. Depending on the PMIC, we could use a shared interrupt for handling the ext wakeup line changes in the RTC driver. The ext wakeup lines are not really GPIOs either as we can't monitor the wakeup line states, they are just trigger and then hold the state until cleared. If we ever need to provide the state of the wakeup lines to the other drivers, we could still use GPIO, input, or pinctrl. All these allow adding interrupt support for the cases that allow sharing the interrupt with PMIC. I guess we should just pick the framework that already has support for configuring the debounce time for a pin. Regards, Tony
Hi, On 08.04.2016 19:16, Grygorii Strashko wrote: > On 04/08/2016 06:14 PM, Tony Lindgren wrote: >> * Grygorii Strashko <grygorii.strashko@ti.com> [160408 03:41]: >>> Hi Tony, >>> >>> On 04/05/2016 01:40 AM, Tony Lindgren wrote: >>>> Hi, >>>> >>>> * Marcin Niestroj <m.niestroj@grinn-global.com> [160404 08:57]: >>>>> Support configuration of ext_wakeup sources. This patch makes it >>>>> possible to enable ext_wakeup and set it's polarity, depending on board >>>>> configuration. AM335x's dedicated PMIC (tps65217) uses ext_wakeup to >>>>> notify about power-button presses. Handling power-button presses enables >>>>> to recover from RTC-only power states correctly. >>>> >>>> I suggest you just set this pin up as a minimal gpiochip. That way >>>> we can use the standard binding :) And if we get lucky, this pin can >>>> also trigger during runtime. >>>> >>> >>> Following my comments on v2 of this patch I propose to rollback to >>> this version of the patch. >>> >>> It seems doesn't fit in gpiochip, it's more likely irqchip, but since >>> rtc can't generate IRQ there are nor reasons for these genetic >>> experiments and RTC's specific bindings looks more suitable, at least for me. >> >> Hmm well gpiochips typically are irqchip too. IMO the generic binding >> here sounds like "gpio-wakeup" as it's an input with polarity and with >> an optional interrupt. >> >> Plain irqchip would work too in this case if there are no other GPIO >> specific features. Some other RTCs may have more GPIO like features. >> >> In any case, setting the ext_wakeup up as an irqchip means that the >> RTC controller can be used as a dedicated wakeirq with Linux :) > > It can't :( It can't generate IRQ when state of ext_wakeup line has > been changed. > >> >> Are you guys sure there's no wake pin events during runtime? AFAIK >> the RTCs just typically produce an interrupt when programmed to >> do so, they don't know the state of the SoC. >> > > I do not think that It can be fit in gpiochip or irqchip - > in my opinion right way is to proceed with bindings proposed by > Marcin in this patch v1. > > But, probably, it could fit in pinctrl: > - this is pin's configuration > - this is one-time configuration > - or - configuration which can be applied before suspend > and resorted after suspend. > > if you still wanna try some generic framework. > pinctrl bindings looks ok for our use case (enable/disable input, configure debounce), except I don't see any way to pass polarity (active low/high) to the driver.
diff --git a/Documentation/devicetree/bindings/rtc/rtc-omap.txt b/Documentation/devicetree/bindings/rtc/rtc-omap.txt index bf7d11a..1c05311b 100644 --- a/Documentation/devicetree/bindings/rtc/rtc-omap.txt +++ b/Documentation/devicetree/bindings/rtc/rtc-omap.txt @@ -18,8 +18,20 @@ Optional properties: through pmic_power_en - clocks: Any internal or external clocks feeding in to rtc - clock-names: Corresponding names of the clocks +- ti,ext-wakeup-en: Enables ext_wakeup[n] sources. Value determines which + ext_wakeup is enabled by bitwise OR of corresponding bits. + Examples: + <1> - enabled ext_wakeup[0], + <2> - enabled ext_wakeup[1], + <3> - enabled ext_wakeup[0] and enabled ext_wakeup[1], + ... +- ti,ext-wakeup-pol: Sets polarity of ext_wakeup[n] sources, similarly as + above. Set bit means active-low, unset bit means + active-high. Examples: + <1> - only ext_wakeup0 is active-low + <3> - only ext_wakeup0 and ext_wakeup1 are active-low -Example: +Examples: rtc@1c23000 { compatible = "ti,da830-rtc"; @@ -31,3 +43,13 @@ rtc@1c23000 { clocks = <&clk_32k_rtc>, <&clk_32768_ck>; clock-names = "ext-clk", "int-clk"; }; + +rtc@44e3e000 { + compatible = "ti,am3352-rtc", "ti,da830-rtc"; + reg = <0x44e3e000 0x1000>; + interrupts = <75 + 76>; + system-power-controller; + ti,ext-wakeup-en = <1>; + ti,ext-wakeup-pol = <1>; +}; diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c index ec2e9c5..1a46f5d 100644 --- a/drivers/rtc/rtc-omap.c +++ b/drivers/rtc/rtc-omap.c @@ -114,7 +114,11 @@ #define OMAP_RTC_IRQWAKEEN_ALARM_WAKEEN BIT(1) /* OMAP_RTC_PMIC bit fields: */ -#define OMAP_RTC_PMIC_POWER_EN_EN BIT(16) +#define OMAP_RTC_PMIC_POWER_EN_EN BIT(16) +#define OMAP_RTC_PMIC_EXT_WAKEUP_EN(x) (x) +#define OMAP_RTC_PMIC_EXT_WAKEUP_POL(x) ((x) << 4) +#define OMAP_RTC_PMIC_EXT_WAKEUP_EN_MASK 0x0F +#define OMAP_RTC_PMIC_EXT_WAKEUP_POL_MASK 0xF0 /* OMAP_RTC_KICKER values */ #define KICK0_VALUE 0x83e70b13 @@ -533,6 +537,11 @@ static int omap_rtc_probe(struct platform_device *pdev) const struct platform_device_id *id_entry; const struct of_device_id *of_id; int ret; + u32 ext_wakeup_en; + u32 ext_wakeup_pol; + bool has_ext_wakeup_en; + bool has_ext_wakeup_pol; + u32 val; rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL); if (!rtc) @@ -544,6 +553,30 @@ static int omap_rtc_probe(struct platform_device *pdev) rtc->is_pmic_controller = rtc->type->has_pmic_mode && of_property_read_bool(pdev->dev.of_node, "system-power-controller"); + if (of_property_read_u32(pdev->dev.of_node, + "ti,ext-wakeup-en", + &ext_wakeup_en) >= 0) { + if (ext_wakeup_en < BIT(4)) + has_ext_wakeup_en = true; + else + dev_err(&pdev->dev, + "wrong ext-wakeup-en value\n"); + } else { + dev_err(&pdev->dev, + "no ext-wakeup-en\n"); + } + if (of_property_read_u32(pdev->dev.of_node, + "ti,ext-wakeup-pol", + &ext_wakeup_pol) >= 0) { + if (ext_wakeup_pol < BIT(4)) + has_ext_wakeup_pol = true; + else + dev_err(&pdev->dev, + "wrong ext-wakeup-pol value\n"); + } else { + dev_err(&pdev->dev, + "no ext-wakeup-pol\n"); + } } else { id_entry = platform_get_device_id(pdev); rtc->type = (void *)id_entry->driver_data; @@ -650,6 +683,17 @@ static int omap_rtc_probe(struct platform_device *pdev) reg | OMAP_RTC_OSC_SEL_32KCLK_SRC); } + if (has_ext_wakeup_en || has_ext_wakeup_pol) { + val = rtc_readl(rtc, OMAP_RTC_PMIC_REG); + if (has_ext_wakeup_en) + val = (val & ~OMAP_RTC_PMIC_EXT_WAKEUP_EN_MASK) + | OMAP_RTC_PMIC_EXT_WAKEUP_EN(ext_wakeup_en); + if (has_ext_wakeup_pol) + val = (val & ~OMAP_RTC_PMIC_EXT_WAKEUP_POL_MASK) + | OMAP_RTC_PMIC_EXT_WAKEUP_POL(ext_wakeup_pol); + rtc_writel(rtc, OMAP_RTC_PMIC_REG, val); + } + rtc->type->lock(rtc); device_init_wakeup(&pdev->dev, true);
Support configuration of ext_wakeup sources. This patch makes it possible to enable ext_wakeup and set it's polarity, depending on board configuration. AM335x's dedicated PMIC (tps65217) uses ext_wakeup to notify about power-button presses. Handling power-button presses enables to recover from RTC-only power states correctly. Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com> --- Documentation/devicetree/bindings/rtc/rtc-omap.txt | 24 ++++++++++- drivers/rtc/rtc-omap.c | 46 +++++++++++++++++++++- 2 files changed, 68 insertions(+), 2 deletions(-)