[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-rtc
Subject:    Re: [PATCH] rtc: stm32: Handle single EXTI IRQ as wake up source
From:       Marek Vasut <marex () denx ! de>
Date:       2023-05-31 10:17:56
Message-ID: dc8dd57f-d149-ec1e-3aaa-17163d1d8bcd () denx ! de
[Download RAW message or body]

On 5/31/23 11:36, Alexandre TORGUE wrote:
> Hi Marek

Hi,

> On 5/30/23 21:54, Marek Vasut wrote:
>> On 5/30/23 17:18, Amelie Delaunay wrote:
>>> On 5/30/23 16:13, Marek Vasut wrote:
>>>> On 5/30/23 16:02, Amelie Delaunay wrote:
>>>>> Hi Marek,
>>>>
>>>> Hello Amelie,
>>>>
>>>>> On 5/18/23 02:33, Marek Vasut wrote:
>>>>>> The arch/arm/boot/dts/stm32mp151.dtsi specifies one interrupt for the
>>>>>> RTC node, while the expectation of the RTC driver is two 
>>>>>> interrupts on
>>>>>> STM32MP15xx SoC, one connected to GIC interrupt controller and 
>>>>>> another
>>>>>> one to EXTI. Per STM32MP15xx reference manual, the two interrupts 
>>>>>> serve
>>>>>> the same purpose, except the EXTI one can also wake the system up. 
>>>>>> The
>>>>>> EXTI driver already internally handles this GIC and EXTI duality and
>>>>>> maps the EXTI interrupt onto GIC during runtime, and only uses the 
>>>>>> EXTI
>>>>>> for wake up functionality.
>>>>>>
>>>>>> Therefore, fix the driver such that if on STM32MP15xx there is 
>>>>>> only one
>>>>>> interrupt specified in the DT, use that interrupt as EXTI 
>>>>>> interrupt and
>>>>>> set it as wake up source.
>>>>>>
>>>>>> This fixes the following warning in the kernel log on STM32MP15xx:
>>>>>> "
>>>>>> stm32_rtc 5c004000.rtc: error -ENXIO: IRQ index 1 not found
>>>>>> stm32_rtc 5c004000.rtc: alarm can't wake up the system: -6
>>>>>> "
>>>>>>
>>>>>> This also fixes the system wake up via built-in RTC using e.g.:
>>>>>> $ rtcwake -s 5 -m mem
>>>>>>
>>>>>> Fixes: b72252b6580c ("rtc: stm32: add stm32mp1 rtc support")
>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>> ---
>>>>>> Cc: Alessandro Zummo <a.zummo@towertech.it>
>>>>>> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
>>>>>> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
>>>>>> Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
>>>>>> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>>> Cc: linux-rtc@vger.kernel.org
>>>>>> Cc: linux-stm32@st-md-mailman.stormreply.com
>>>>>> ---
>>>>>>   drivers/rtc/rtc-stm32.c | 9 +++++----
>>>>>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
>>>>>> index 229cb2847cc48..72994b9f95319 100644
>>>>>> --- a/drivers/rtc/rtc-stm32.c
>>>>>> +++ b/drivers/rtc/rtc-stm32.c
>>>>>> @@ -780,14 +780,15 @@ static int stm32_rtc_probe(struct 
>>>>>> platform_device *pdev)
>>>>>>       ret = device_init_wakeup(&pdev->dev, true);
>>>>>>       if (rtc->data->has_wakeirq) {
>>>>>> -        rtc->wakeirq_alarm = platform_get_irq(pdev, 1);
>>>>>> +        rtc->wakeirq_alarm = platform_get_irq_optional(pdev, 1);
>>>>>>           if (rtc->wakeirq_alarm > 0) {
>>>>>>               ret = dev_pm_set_dedicated_wake_irq(&pdev->dev,
>>>>>>                                   rtc->wakeirq_alarm);
>>>>>> -        } else {
>>>>>> +        } else if (rtc->wakeirq_alarm == -EPROBE_DEFER) {
>>>>>>               ret = rtc->wakeirq_alarm;
>>>>>> -            if (rtc->wakeirq_alarm == -EPROBE_DEFER)
>>>>>> -                goto err;
>>>>>> +            goto err;
>>>>>> +        } else {
>>>>>> +            ret = dev_pm_set_wake_irq(&pdev->dev, rtc->irq_alarm);
>>>>>>           }
>>>>>>       }
>>>>>>       if (ret)
>>>>>
>>>>> In our downstream [1], dedicated wakeup irq management is dropped: 
>>>>> it is neither described in st,stm32-rtc bindings nor used in 
>>>>> STM32Fx, STM32Hx, STM32MP1x device trees.
>>>>> The pointed patch is going to be upstreamed.
>>>>>
>>>>> [1] 
>>>>> https://github.com/STMicroelectronics/linux/commit/5a45e1f100d59c33b6b50fe98c0f62862bd6f3d2
>>>>
>>>> Won't that break compatibility with DTs which do use two interrupts 
>>>> entries ?
>>>
>>> I didn't find any DTs using STM32 RTC with two interrupts. Did I miss 
>>> something?
>>
>> I am not aware of any either (I also did check a couple of 
>> repositories to be sure) . However, the DT is an ABI , so there might 
>> be users we do not know about, who might be unable to update their DTs 
>> , and who would be broken by dropping the support for two interrupts.
> 
> Currently if people use 2 interrupts in their DT with an up to date 
> kernel I don't think it works fine. At the beginning of the MP1 story, 2 
> interrupts were needed to wakeup system from LPSTOP: one for GIC and the 
> other one for EXTI. But since maybe 2 years, EXTI and GIC interrupts are 
> directly linked inside the EXTI driver. So now, devices only need to 
> take one interrupt. With this implementation if one device uses 2 
> interrupts in their DT then the GIC interrupt will be mapped two times. 
> So I think that current implementation in RTC driver is broken and it 
> should be aligned with "new" EXTI implementation. Note also that the use 
> of 2 interrupts has never been documented in dt-bindings documentation.

In that case, maybe add a warning into the driver if you detect two 
interrupts, notify user and avoid any unexpected surprises ?

> Above words are for STM32 MPU products For STM32 MCU products RTC is 
> only mapped to the EXTI (not to the NVIC) so no needs to handle two 
> interrupts.

Like I wrote before, I am fine either way.

[...]
[prev in list] [next in list] [prev in thread] [next in thread]