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

[Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org>]

On 08.04.2016 11:18, Grygorii Strashko wrote:
> On 04/07/2016 08:11 PM, Marcin Niestroj wrote:
>>
>> On 07.04.2016 12:48, Grygorii Strashko wrote:
>>> On 04/06/2016 10:28 PM, Marcin Niestroj wrote:
>>>> Hi,
>>>>
>>>> On 06.04.2016 21:11, Tony Lindgren wrote:
>>>>> * Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org> [160406 09:34]:
>>>>>> 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
>>>>>> in SLEEP mode (RTC-only) to notify about power-button presses.
>>>>>> Handling
>>>>>> power-button presses enables to recover from RTC-only power states
>>>>>> correctly.
>>>>>>
>>>>>> Implementation uses gpiochip to utilize standard bindings. However,
>>>>>> configuration is possible only using device-tree (no runtime changes).
>>>>>
>>>>> Hey looks good to me, adding linux-omap list to Cc.
>>>>>
>>>>> Can you please check that the "depends on GPIOLIB" does not disable
>>>>> the RTC driver for omap1?
>>>>
>>>> Looks ok for omap1. ARCH_OMAP1 selects ARCH_REQUIRE_GPIOLIB, which
>>>> selects GPIOLIB.
>>>>
>>>> Best regards,
>>>> Marcin
>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Tony
>>>>>
>>>>>> Signed-off-by: Marcin Niestroj <m.niestroj-z3quKL4iOrmQ6ZAhV5LmOA@public.gmane.org>
>>>>>> ---
>>>>>>    Documentation/devicetree/bindings/rtc/rtc-omap.txt |  18 ++-
>>>>>>    drivers/rtc/Kconfig                                |   2 +-
>>>>>>    drivers/rtc/rtc-omap.c                             | 137
>>>>>> ++++++++++++++++++++-
>>>>>>    3 files changed, 154 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/rtc/rtc-omap.txt
>>>>>> b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
>>>>>> index bf7d11a..4a7738e 100644
>>>>>> --- a/Documentation/devicetree/bindings/rtc/rtc-omap.txt
>>>>>> +++ b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
>>>>>> @@ -18,8 +18,12 @@ Optional properties:
>>>>>>      through pmic_power_en
>>>>>>    - clocks: Any internal or external clocks feeding in to rtc
>>>>>>    - clock-names: Corresponding names of the clocks
>>>>>> +- gpio-controller: Mark as gpio controller when using ext_wakeup
>>>>>> +- #gpio-cells: Should be set to 2
>>>>>> +- ngpios: Number of ext_wakeup sources supported by processor (board)
>>>>>> +- ext-wakeup-gpios: List of ext_wakeup sources to configure
>>>>>>
>>>>>> -Example:
>>>>>> +Examples:
>>>>>>
>>>>>>    rtc@1c23000 {
>>>>>>        compatible = "ti,da830-rtc";
>>>>>> @@ -31,3 +35,15 @@ rtc@1c23000 {
>>>>>>        clocks = <&clk_32k_rtc>, <&clk_32768_ck>;
>>>>>>        clock-names = "ext-clk", "int-clk";
>>>>>>    };
>>>>>> +
>>>>>> +rtc: rtc@44e3e000 {
>>>>>> +    compatible = "ti,am3352-rtc", "ti,da830-rtc";
>>>>>> +    reg = <0x44e3e000 0x1000>;
>>>>>> +    interrupts = <75
>>>>>> +              76>;
>>>>>> +    system-power-controller;
>>>>>> +    gpio-controller;
>>>>>> +    #gpio-cells = <2>;
>>>>>> +    ngpios = <1>;
>>>>>> +    ext-wakeup-gpios = <&rtc 0 GPIO_ACTIVE_LOW>;
>>>>>> +};
>>>
>>> I'm not sure that rtc can request gpios by itself in this
>>> way (if i rememberer this can break modules isnmod/rmmod).
>>>
>>> cc: linux-gpio
>>>
>>> The gpio-hog is more correct way follow if I'm not mistaken)
>>> - see gpiochip_request_own_desc().
>>
>> You are right. It is not possible to rmmod module, as it is "in use"
>> all the time. I'll try with gpio_requst_own_desc().
>>
>>>
>>> Another question, in commit message you refer to  power-button,
>>> but i do not see anything related in binding description.
>>
>> We don't have power-button connected right to the processor. It is
>> connected to PMIC. During runtime we receive IRQs about power-button
>> from PMIC using i2c bus. The only purpose of this patch is to
>> configure processor's ext_wakeup line, which is triggered during
>> RTC-only mode (for example when power-button is pressed), causing
>> device wakeup. On the other hand, it is not possible to use ext_wakeup
>> during runtime, as we are only able to read it's status, but it
>> cannot trigger any interrupts.
>
> Sry, but I don't like this approach - it could make sense if RTC
> EXT_WAKEUP will be at least partially mapped on gpiolib interface.
> But your gpiochip is fake, you do not/can't use GPIO hogging mechanism
> and you're even parsing DT on your own (in V3).

With gpio hogging we can't pass polarity to the driver. It is hidden
in gpiolib.

>
> In general it's more like irqchip than gpiochip, but RTC can
> trigger IRQ on ext_wakeup :(
>
> As per above, your first version of the patch looks more sensible to me.
>
> Additional note. Shouldn't EXT_WAKEUP_STATUS be cleared after wake up?
> This is requested by am57x trm at least: "SW must clear the events before
> PMIC_PWR_ENABLE can go from 1 - 0. "

I haven't seen that in am335x TRM. In current implementation if
EXT_WAKEUP_STATUS is set, we read it and write it back together with 
EXT_WAKEUP_EN and EXT_WAKEUP_POL, which results in clearing of this
event.

>
>>
>>>
>>> Shouldn't some gpio-key node be here, which will consume rtc-gpio?
>>>
>>>
>>>>>> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
>>>>>> index 3e84315..f013346 100644
>>>>>> --- a/drivers/rtc/Kconfig
>>>>>> +++ b/drivers/rtc/Kconfig
>
> [...]
>
>

-- 
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