Re: [PATCH] rtc: omap: Support ext_wakeup configuration

Re: [PATCH] rtc: omap: Support ext_wakeup configuration

[Tony Lindgren]

Hi,

* Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org> [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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtc: omap: Support ext_wakeup configuration

[Grygorii Strashko]

Hi Tony,

On 04/05/2016 01:40 AM, Tony Lindgren wrote:
> Hi,
> 
> * Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org> [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.


-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtc: omap: Support ext_wakeup configuration

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [160408 03:41]:
> Hi Tony,
> 
> On 04/05/2016 01:40 AM, Tony Lindgren wrote:
> > Hi,
> > 
> > * Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org> [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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtc: omap: Support ext_wakeup configuration

[Grygorii Strashko]

On 04/08/2016 06:14 PM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [160408 03:41]:
>> Hi Tony,
>>
>> On 04/05/2016 01:40 AM, Tony Lindgren wrote:
>>> Hi,
>>>
>>> * Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org> [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.

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtc: omap: Support ext_wakeup configuration

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [160408 10:17]:
> On 04/08/2016 06:14 PM, Tony Lindgren wrote:
> > * Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [160408 03:41]:
> >> Hi Tony,
> >>
> >> On 04/05/2016 01:40 AM, Tony Lindgren wrote:
> >>> Hi,
> >>>
> >>> * Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org> [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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtc: omap: Support ext_wakeup configuration

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160408 10:52]:
> * Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtc: omap: Support ext_wakeup configuration

[Marcin Niestroj]

Hi,

On 08.04.2016 19:16, Grygorii Strashko wrote:
> On 04/08/2016 06:14 PM, Tony Lindgren wrote:
>> * Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [160408 03:41]:
>>> Hi Tony,
>>>
>>> On 04/05/2016 01:40 AM, Tony Lindgren wrote:
>>>> Hi,
>>>>
>>>> * Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org> [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.

-- 
Marcin Niestroj
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtc: omap: Support ext_wakeup configuration

[Grygorii Strashko]

On 04/12/2016 08:25 PM, Marcin Niestroj wrote:
> Hi,
> 
> On 08.04.2016 19:16, Grygorii Strashko wrote:
>> On 04/08/2016 06:14 PM, Tony Lindgren wrote:
>>> * Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [160408 03:41]:
>>>> Hi Tony,
>>>>
>>>> On 04/05/2016 01:40 AM, Tony Lindgren wrote:
>>>>> Hi,
>>>>>
>>>>> * Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org> [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.
> 

Below code is just a POC that pinctrl can be used here.
It does one time configuration at boot, but more sates can be added
- sleep, poweroff:

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index 13ac882..9580c82 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -1248,11 +1248,40 @@
 
                rtc: rtc@48838000 {
                        compatible = "ti,am3352-rtc";
-                       reg = <0x48838000 0x100>;
+                       reg = <0x48838000 0x98>;
                        interrupts = <GIC_SPI 217 IRQ_TYPE_LEVEL_HIGH>,
                                     <GIC_SPI 217 IRQ_TYPE_LEVEL_HIGH>;
                        ti,hwmods = "rtcss";
                        clocks = <&sys_32k_ck>;
+                       #address-cells = <1>;
+                       #size-cells = <1>;
+                       ranges;
+                       
+                       rtc_pmx: pinmux@48838098 {
+                               compatible = "pinctrl-single";
+                               reg = <0x48838098 0x8>;
+                               #address-cells = <1>;
+                               #size-cells = <0>;
+                               pinctrl-single,register-width = <32>;
+                               pinctrl-single,function-mask = <0x007fffff>;
+
+                               pinctrl-names = "default"/*, "sleep"*/;
+                               pinctrl-0 = <&rtc_default>;
+/*                             pinctrl-1 = <&rtc_sleep>;*/
+
+                               rtc_default: rtc_default {
+                                       pinctrl-single,pins = <
+                                               0x0 0x000B0111
+                                               0x4 0x00000004
+                                       >;
+                               };
+/*                             rtc_sleep: rtc_sleep {
+                                       pinctrl-single,pins = <
+                                               0x0 0x00000000
+                                               0x4 0x00000000
+                                       >;
+                               };*/
+                       };
                };
 
                /* OCP2SCP1 */
diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index ec2e9c5..28c67a4 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -428,8 +428,10 @@ static void omap_rtc_power_off(void)
 
        rtc->type->unlock(rtc);
        /* enable pmic_power_en control */
+/*
        val = rtc_readl(rtc, OMAP_RTC_PMIC_REG);
        rtc_writel(rtc, OMAP_RTC_PMIC_REG, val | OMAP_RTC_PMIC_POWER_EN_EN);
+*/
 
        /* set alarm two seconds from now */
        omap_rtc_read_time_raw(rtc, &tm);
@@ -650,6 +652,8 @@ static int omap_rtc_probe(struct platform_device *pdev)
                          reg | OMAP_RTC_OSC_SEL_32KCLK_SRC);
        }
 
+       of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+
        rtc->type->lock(rtc);
 
        device_init_wakeup(&pdev->dev, true);
 

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtc: omap: Support ext_wakeup configuration

[Nishanth Menon]

On 04/15/2016 11:46 AM, Grygorii Strashko wrote:
[...]
> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
> index 13ac882..9580c82 100644
> --- a/arch/arm/boot/dts/dra7.dtsi

I see this is an example of what could be done on am335x - but lets
not do this on dra7/am57xx please?


-- 
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtc: omap: Support ext_wakeup configuration

[Grygorii Strashko]

Hi Nishanth,

On 04/15/2016 07:52 PM, Nishanth Menon wrote:
> On 04/15/2016 11:46 AM, Grygorii Strashko wrote:
> [...]
>> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
>> index 13ac882..9580c82 100644
>> --- a/arch/arm/boot/dts/dra7.dtsi
>
> I see this is an example of what could be done on am335x - but lets
> not do this on dra7/am57xx please?
>
>

Don't panic. It's just an example ;)
Late Friday is here, and it was the first available board.

Sry, that I made you worry. ;)

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtc: omap: Support ext_wakeup configuration

[Marcin Niestroj]

Hi,

On 15.04.2016 18:46, Grygorii Strashko wrote:
> On 04/12/2016 08:25 PM, Marcin Niestroj wrote:
>> Hi,
>>
>> On 08.04.2016 19:16, Grygorii Strashko wrote:
>>> On 04/08/2016 06:14 PM, Tony Lindgren wrote:
>>>> * Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> [160408 03:41]:
>>>>> Hi Tony,
>>>>>
>>>>> On 04/05/2016 01:40 AM, Tony Lindgren wrote:
>>>>>> Hi,
>>>>>>
>>>>>> * Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org> [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.
>>
>
> Below code is just a POC that pinctrl can be used here.
> It does one time configuration at boot, but more sates can be added
> - sleep, poweroff:
>
> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
> index 13ac882..9580c82 100644
> --- a/arch/arm/boot/dts/dra7.dtsi
> +++ b/arch/arm/boot/dts/dra7.dtsi
> @@ -1248,11 +1248,40 @@
>
>                 rtc: rtc@48838000 {
>                         compatible = "ti,am3352-rtc";
> -                       reg = <0x48838000 0x100>;
> +                       reg = <0x48838000 0x98>;
>                         interrupts = <GIC_SPI 217 IRQ_TYPE_LEVEL_HIGH>,
>                                      <GIC_SPI 217 IRQ_TYPE_LEVEL_HIGH>;
>                         ti,hwmods = "rtcss";
>                         clocks = <&sys_32k_ck>;
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +                       ranges;
> +
> +                       rtc_pmx: pinmux@48838098 {
> +                               compatible = "pinctrl-single";
> +                               reg = <0x48838098 0x8>;
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +                               pinctrl-single,register-width = <32>;
> +                               pinctrl-single,function-mask = <0x007fffff>;
> +
> +                               pinctrl-names = "default"/*, "sleep"*/;
> +                               pinctrl-0 = <&rtc_default>;
> +/*                             pinctrl-1 = <&rtc_sleep>;*/
> +
> +                               rtc_default: rtc_default {
> +                                       pinctrl-single,pins = <
> +                                               0x0 0x000B0111
> +                                               0x4 0x00000004
> +                                       >;
> +                               };
> +/*                             rtc_sleep: rtc_sleep {
> +                                       pinctrl-single,pins = <
> +                                               0x0 0x00000000
> +                                               0x4 0x00000000
> +                                       >;
> +                               };*/
> +                       };
>                 };
>
>                 /* OCP2SCP1 */
> diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
> index ec2e9c5..28c67a4 100644
> --- a/drivers/rtc/rtc-omap.c
> +++ b/drivers/rtc/rtc-omap.c
> @@ -428,8 +428,10 @@ static void omap_rtc_power_off(void)
>
>         rtc->type->unlock(rtc);
>         /* enable pmic_power_en control */
> +/*
>         val = rtc_readl(rtc, OMAP_RTC_PMIC_REG);
>         rtc_writel(rtc, OMAP_RTC_PMIC_REG, val | OMAP_RTC_PMIC_POWER_EN_EN);
> +*/
>
>         /* set alarm two seconds from now */
>         omap_rtc_read_time_raw(rtc, &tm);
> @@ -650,6 +652,8 @@ static int omap_rtc_probe(struct platform_device *pdev)
>                           reg | OMAP_RTC_OSC_SEL_32KCLK_SRC);
>         }
>
> +       of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> +
>         rtc->type->lock(rtc);
>
>         device_init_wakeup(&pdev->dev, true);
>
>

I made few changes to the POC you provided.

In below code we check reg resource size in order to know if we
should update RTC_PMIC or it is pinctrl responsibility. It's a little
hack, but we make sure, that every device with not modified device-tree
will work as previously. If we want to add support for ext_wakeup, we
just change reg resouce size in device-tree in order to not overlap
requested memory regions in rtc-omap and pinctrl.

I used am335x-chilisom instead of dra7 and updated
pinctrl-single,funcion-mask so the EXT_WAKEUP_STATUS is always cleared
after boot.

So the question now is: is this acceptable? Do we want to continue with
this approach?

diff --git a/arch/arm/boot/dts/am335x-chilisom.dtsi 
b/arch/arm/boot/dts/am335x-chilisom.dtsi
index bc0880d..525ed4f 100644
--- a/arch/arm/boot/dts/am335x-chilisom.dtsi
+++ b/arch/arm/boot/dts/am335x-chilisom.dtsi
@@ -124,6 +124,31 @@

  &rtc {
         system-power-controller;
+
+       reg = <0x44e3e000 0x98>;
+       #address-cells = <1>;
+       #size-cells = <1>; 

+       ranges; 

+ 

+       rtc_pinmux: pinmux@0x44e3e098 { 

+               compatible = "pinctrl-single"; 

+               reg = <0x44e3e098 0x8>; 

+               #address-cells = <1>; 

+               #size-cells = <1>; 

+ 

+               pinctrl-single,register-width = <32>; 

+               pinctrl-single,function-mask = <0x10fff>; 

+ 

+               pinctrl-names = "default"; 

+               pinctrl-0 = <&rtc_pins_default>; 

+ 

+               rtc_pins_default: rtc_pins_default { 

+                       pinctrl-single,pins = < 

+                               0x0 0x00010011 

+                               0x4 0x00000000 

+                       >; 

+               }; 

+       }; 

  }; 

 

  /* NAND Flash */
diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index ec2e9c5..db2512c 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -134,6 +134,7 @@ struct omap_rtc_device_type {
  struct omap_rtc {
         struct rtc_device *rtc;
         void __iomem *base;
+       resource_size_t res_size;
         struct clk *clk;
         int irq_alarm;
         int irq_timer;
@@ -428,8 +429,11 @@ static void omap_rtc_power_off(void)

         rtc->type->unlock(rtc);
         /* enable pmic_power_en control */
-       val = rtc_readl(rtc, OMAP_RTC_PMIC_REG);
-       rtc_writel(rtc, OMAP_RTC_PMIC_REG, val | OMAP_RTC_PMIC_POWER_EN_EN);
+       if (rtc->res_size > OMAP_RTC_PMIC_REG) {
+               val = rtc_readl(rtc, OMAP_RTC_PMIC_REG);
+               rtc_writel(rtc, OMAP_RTC_PMIC_REG,
+                       val | OMAP_RTC_PMIC_POWER_EN_EN);
+       }

         /* set alarm two seconds from now */
         omap_rtc_read_time_raw(rtc, &tm);
@@ -567,6 +571,7 @@ static int omap_rtc_probe(struct platform_device *pdev)
                 clk_prepare_enable(rtc->clk);

         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       rtc->res_size = resource_size(res);
         rtc->base = devm_ioremap_resource(&pdev->dev, res);
         if (IS_ERR(rtc->base))
                 return PTR_ERR(rtc->base);
@@ -681,6 +686,9 @@ static int omap_rtc_probe(struct platform_device *pdev)
                 }
         }

+       /* handle pinmux nodes which configure ext_wakeup inputs */
+       of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+
         return 0;

  err:

-- 
Marcin Niestroj
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtc: omap: Support ext_wakeup configuration

[Tony Lindgren]

* Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org> [160426 00:40]:
> In below code we check reg resource size in order to know if we
> should update RTC_PMIC or it is pinctrl responsibility. It's a little
> hack, but we make sure, that every device with not modified device-tree
> will work as previously. If we want to add support for ext_wakeup, we
> just change reg resouce size in device-tree in order to not overlap
> requested memory regions in rtc-omap and pinctrl.
> 
> I used am335x-chilisom instead of dra7 and updated
> pinctrl-single,funcion-mask so the EXT_WAKEUP_STATUS is always cleared
> after boot.
> 
> So the question now is: is this acceptable? Do we want to continue with
> this approach?

Well the concern I have here is that we not use pinctrl-single
as a separate driver if any of the registers are shared with
the RTC driver. If the registers are shared, the pinctrl
functionality should be implemented in the RTC driver.

The pinctrl driver can also implement a chained IRQ later
on for consumer device drivers to use with the wakeirq API.
Maybe eventually we'll have real chained interrupt for the
RTC to use from Linux side of the C-M3 driver.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtc: omap: Support ext_wakeup configuration

[Marcin Niestroj]

Hi,

Sorry for such delay in response. Please see my comments/questions on
using pinctrl below.

On 26.04.2016 17:39, Tony Lindgren wrote:
> * Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org> [160426 00:40]:
>> In below code we check reg resource size in order to know if we
>> should update RTC_PMIC or it is pinctrl responsibility. It's a little
>> hack, but we make sure, that every device with not modified device-tree
>> will work as previously. If we want to add support for ext_wakeup, we
>> just change reg resouce size in device-tree in order to not overlap
>> requested memory regions in rtc-omap and pinctrl.
>>
>> I used am335x-chilisom instead of dra7 and updated
>> pinctrl-single,funcion-mask so the EXT_WAKEUP_STATUS is always cleared
>> after boot.
>>
>> So the question now is: is this acceptable? Do we want to continue with
>> this approach?
>
> Well the concern I have here is that we not use pinctrl-single
> as a separate driver if any of the registers are shared with
> the RTC driver. If the registers are shared, the pinctrl
> functionality should be implemented in the RTC driver.

We can use pinctrl generic params for:
  * enable wakeup inputs - with 'input-enable'
  * input debounce enable/configuration - with 'input-debounce'

However I don't see any generic way for setting wakeup input polarity.
So I guess we should add some driver specific devicetree binding to
handle hat. I also didn't find any other driver that implements it.

In case of pinctrl-single we have just written custom register values to
set polarity.

So the question is: how should we proceed? Is pinctrl still and option?
If yes, what is your idea about configuring input polarity?

>
> The pinctrl driver can also implement a chained IRQ later
> on for consumer device drivers to use with the wakeirq API.
> Maybe eventually we'll have real chained interrupt for the
> RTC to use from Linux side of the C-M3 driver.
>
> Regards,
>
> Tony
>


-- 
Marcin Niestroj

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH] rtc: omap: Support ext_wakeup configuration

[Tony Lindgren]

* Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org> [160615 01:40]:
> Hi,
> 
> Sorry for such delay in response. Please see my comments/questions on
> using pinctrl below.
> 
> On 26.04.2016 17:39, Tony Lindgren wrote:
> > * Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org> [160426 00:40]:
> > > In below code we check reg resource size in order to know if we
> > > should update RTC_PMIC or it is pinctrl responsibility. It's a little
> > > hack, but we make sure, that every device with not modified device-tree
> > > will work as previously. If we want to add support for ext_wakeup, we
> > > just change reg resouce size in device-tree in order to not overlap
> > > requested memory regions in rtc-omap and pinctrl.
> > > 
> > > I used am335x-chilisom instead of dra7 and updated
> > > pinctrl-single,funcion-mask so the EXT_WAKEUP_STATUS is always cleared
> > > after boot.
> > > 
> > > So the question now is: is this acceptable? Do we want to continue with
> > > this approach?
> > 
> > Well the concern I have here is that we not use pinctrl-single
> > as a separate driver if any of the registers are shared with
> > the RTC driver. If the registers are shared, the pinctrl
> > functionality should be implemented in the RTC driver.
> 
> We can use pinctrl generic params for:
>  * enable wakeup inputs - with 'input-enable'
>  * input debounce enable/configuration - with 'input-debounce'
> 
> However I don't see any generic way for setting wakeup input polarity.
> So I guess we should add some driver specific devicetree binding to
> handle hat. I also didn't find any other driver that implements it.
> 
> In case of pinctrl-single we have just written custom register values to
> set polarity.
> 
> So the question is: how should we proceed? Is pinctrl still and option?
> If yes, what is your idea about configuring input polarity?

At least I don't have any better ideas. You can define the values
in the driver specific binding, I'd use something like GPIO_ACTIVE_LOW
even though it's a broken GPIO and broken interrupt :)

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html