Message-ID: <4f2ec575-4c2c-49d5-b1c0-85d5b6ce9f2e@kernel.org>
Date: Thu, 5 Sep 2024 13:41:47 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Markus Schneider-Pargmann <msp@...libre.com>
Cc: Nishanth Menon <nm@...com>, Tero Kristo <kristo@...nel.org>,
 Santosh Shilimkar <ssantosh@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Vignesh Raghavendra <vigneshr@...com>,
 Vibhore Vardhan <vibhore@...com>, Kevin Hilman <khilman@...libre.com>,
 Dhruva Gole <d-gole@...com>, linux-arm-kernel@...ts.infradead.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/6] dt-bindings: ti, sci: Add property for
 partial-io-wakeup-sources

On 05/09/2024 13:17, Markus Schneider-Pargmann wrote:
> On Thu, Sep 05, 2024 at 12:41:05PM GMT, Krzysztof Kozlowski wrote:
>> On 05/09/2024 11:49, Markus Schneider-Pargmann wrote:
>>> On Thu, Sep 05, 2024 at 11:25:48AM GMT, Krzysztof Kozlowski wrote:
>>>> On 05/09/2024 11:15, Krzysztof Kozlowski wrote:
>>>>> On 05/09/2024 11:08, Markus Schneider-Pargmann wrote:
>>>>>> On Tue, Aug 06, 2024 at 10:03:00AM GMT, Krzysztof Kozlowski wrote:
>>>>>>> On 06/08/2024 09:11, Markus Schneider-Pargmann wrote:
>>>>>>>> Hi Krzysztof,
>>>>>>>>
>>>>>>>> On Tue, Aug 06, 2024 at 08:18:01AM GMT, Krzysztof Kozlowski wrote:
>>>>>>>>> On 29/07/2024 10:00, Markus Schneider-Pargmann wrote:
>>>>>>>>>> Partial-IO is a very low power mode in which nearly everything is
>>>>>>>>>> powered off. Only pins of a few hardware units are kept sensitive and
>>>>>>>>>> are capable to wakeup the SoC. The device nodes are marked as
>>>>>>>>>> 'wakeup-source' but so are a lot of other device nodes as well that are
>>>>>>>>>> not able to do a wakeup from Partial-IO. This creates the need to
>>>>>>>>>> describe the device nodes that are capable of wakeup from Partial-IO.
>>>>>>>>>>
>>>>>>>>>> This patch adds a property with a list of these nodes defining which
>>>>>>>>>> devices can be used as wakeup sources in Partial-IO.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> <form letter>
>>>>>>>>> This is a friendly reminder during the review process.
>>>>>>>>>
>>>>>>>>> It seems my or other reviewer's previous comments were not fully
>>>>>>>>> addressed. Maybe the feedback got lost between the quotes, maybe you
>>>>>>>>> just forgot to apply it. Please go back to the previous discussion and
>>>>>>>>> either implement all requested changes or keep discussing them.
>>>>>>>>>
>>>>>>>>> Thank you.
>>>>>>>>> </form letter>
>>>>>>>>
>>>>>>>> I tried to address your comment from last version by explaining more
>>>>>>>> thoroughly what the binding is for as it seemed that my previous
>>>>>>>> explanation wasn't really good.
>>>>>>>>
>>>>>>>> You are suggesting to use 'wakeup-source' exclusively. Unfortunately
>>>>>>>> wakeup-source is a boolean property which covers two states. I have at
>>>>>>>> least three states I need to describe:
>>>>>>>>
>>>>>>>>  - wakeup-source for suspend to memory and other low power modes
>>>>>>>>  - wakeup-source for Partial-IO
>>>>>>>>  - no wakeup-source
>>>>>>>
>>>>>>> Maybe we need generic property or maybe custom TI would be fine, but in
>>>>>>> any case - whether device is wakeup and what sort of wakeup it is, is a
>>>>>>> property of the device.
>>>>>>
>>>>>> To continue on this, I currently only know of this Partial-IO mode that
>>>>>> would require a special flag like this. So I think a custom TI property
>>>>>> would work. For example a bool property like
>>>>>>
>>>>>>   ti,partial-io-wakeup-source;
>>>>>>
>>>>>> in the device nodes for which it is relevant? This would be in addition
>>>>>> to the 'wakeup-source' property.
>>>>>
>>>>> Rather oneOf. I don't think having two properties in a node brings any
>>>>> more information.
>>>>>
>>>>> I would suggest finding one more user of this and making the
>>>>> wakeup-source an enum - either string or integer with defines in a header.
>>>>
>>>> I am going through this thread again to write something in DT BoF but
>>>> this is confusing:
>>>>
>>>> "Partial-IO is a very low power mode"
>>>> "not able to do a wakeup from Partial-IO."
>>>> "wakeup-source for Partial-IO"
>>>>
>>>> Are you waking up from Partial-IO or are you waking up into Partial-IO?
>>>>
>>>> And why the devices which are configured as wakeup-source cannot wake up
>>>> from or for Partial-IO?
>>>
>>> Sorry if this is confusing. Let me try again.
>>>
>>> Partial-IO is a very low power mode. Only a small IO unit is switched on
>>> to be sensitive on a small set of pins for any IO activity. The rest of
>>> the SoC is powered off, including DDR. Any activity on these pins
>>> switches on the power for the remaining SoC. This leads to a fresh boot,
>>> not a resume of any kind. On am62 the pins that are sensitive and
>>> therefore wakeup-source from this Partial-IO mode, are the pins of a few
>>> CAN and UARTs from the MCU and Wkup section of the SoC.
>>>
>>> These CAN and UART wakeup-sources are also wakeup-sources for other low
>>> power suspend to ram modes. But wakeup-sources for suspend to ram modes
>>> are typically not a wakeup-source for Partial-IO as they are not powered
>>> in Partial-IO.
>>>
>>> I hope this explains it better.
>>
>> Yeah, it's kind of obvious now that just use wakeup-source. Your
>> hardware does not have two different methods of waking up. System is
>> sleeping - either S2R or partial-IO or whatever - and you want it to be
>> woken up.
>>
>> Entire property is unnecessary... and as I said before - you added it
>> only for your driver. If same feedback is repeated and repeated, there
>> is something in it...
> 
> It's not obvious for me. Maybe you can elaborate a bit.
> 
> The hardware has a different set of wakeup sources depending on the
> power mode it is in and I would like to describe these different sets of
> wakeup sources in the devicetree as for me this is a hardware property,
> not a driver thing.

I stated the argument to which you did not respond: it will not matter
for the device whether this is wakeup-source = S2R or wakeup-source =
TI-Partial-IO or whatever.

Each device is or is not wakeup source.

And just because your device has some registers or some configuration
does not mean this property is suitable for DT.

Best regards,
Krzysztof

Powered by blists - more mailing lists