diff mbox series

[v2,1/6] dt-bindings: ti, sci: Add property for partial-io-wakeup-sources

Message ID 20240729080101.3859701-2-msp@baylibre.com (mailing list archive)
State New, archived
Headers show
Series firmware: ti_sci: Partial-IO support | expand

Commit Message

Markus Schneider-Pargmann July 29, 2024, 8 a.m. UTC
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.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 .../devicetree/bindings/arm/keystone/ti,sci.yaml    | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Krzysztof Kozlowski Aug. 6, 2024, 6:18 a.m. UTC | #1
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>

Best regards,
Krzysztof
Markus Schneider-Pargmann Aug. 6, 2024, 7:11 a.m. UTC | #2
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

If something is a wakeup-source for Partial-IO it usually is a
wakeup-source for suspend to memory as well but not the other way
around.

This is the reason why I added a property that lists the devicenodes
that are capable of wakeup from Partial-IO.

Best
Markus
Krzysztof Kozlowski Aug. 6, 2024, 8:03 a.m. UTC | #3
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.

> 
> If something is a wakeup-source for Partial-IO it usually is a
> wakeup-source for suspend to memory as well but not the other way
> around.

I understand, makes sense. The trouble is that your driver code does not
indicate any of this.

> 
> This is the reason why I added a property that lists the devicenodes
> that are capable of wakeup from Partial-IO.

This property looks purely to satisfy your driver design.

Best regards,
Krzysztof
Markus Schneider-Pargmann Sept. 5, 2024, 9:08 a.m. UTC | #4
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.

Best
Markus

> 
> > 
> > If something is a wakeup-source for Partial-IO it usually is a
> > wakeup-source for suspend to memory as well but not the other way
> > around.
> 
> I understand, makes sense. The trouble is that your driver code does not
> indicate any of this.
> 
> > 
> > This is the reason why I added a property that lists the devicenodes
> > that are capable of wakeup from Partial-IO.
> 
> This property looks purely to satisfy your driver design.
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Sept. 5, 2024, 9:15 a.m. UTC | #5
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.

Best regards,
Krzysztof
Krzysztof Kozlowski Sept. 5, 2024, 9:25 a.m. UTC | #6
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?

Best regards,
Krzysztof
Markus Schneider-Pargmann Sept. 5, 2024, 9:49 a.m. UTC | #7
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.

Best
Markus
Krzysztof Kozlowski Sept. 5, 2024, 10:41 a.m. UTC | #8
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...

Best regards,
Krzysztof
Markus Schneider-Pargmann Sept. 5, 2024, 11:17 a.m. UTC | #9
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.

Best
Markus
Krzysztof Kozlowski Sept. 5, 2024, 11:41 a.m. UTC | #10
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
Markus Schneider-Pargmann Sept. 27, 2024, 9:35 a.m. UTC | #11
Hi Krzysztof,

On Thu, Sep 05, 2024 at 01:41:47PM GMT, Krzysztof Kozlowski wrote:
> 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.

I came up with a different (better) way to model this in the devicetree.
This group of units that are powered in Partial-IO are all powered by
just one regulator that is always on. I can simply describe this in the
devicetree by defining the regulator and consumer relationship:

Defining the regulator as described in the board schematic:

  vddshv_canuart: regulator-7 {
         /* TPS22965DSGT */
         compatible = "regulator-fixed";
         regulator-name = "vddshv_canuart";
         regulator-min-microvolt = <3300000>;
         regulator-max-microvolt = <3300000>;
         vin-supply = <&vcc_3v3_main>;
         regulator-always-on;
         regulator-boot-on;
  };

Adding vio-supply to mcan and uarts, note, this binding does not exist
yet:

  &mcu_mcan0 {
         vio-supply = <&vddshv_canuart>;
  };

  &mcu_mcan1 {
         vio-supply = <&vddshv_canuart>;
  };

  &mcu_uart0 {
         vio-supply = <&vddshv_canuart>;
  };

  &wkup_uart0 {
         vio-supply = <&vddshv_canuart>;
  };

Best
Markus
Krzysztof Kozlowski Sept. 28, 2024, 12:13 p.m. UTC | #12
On 27/09/2024 11:35, Markus Schneider-Pargmann wrote:
>>>
>>> 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.
> 
> I came up with a different (better) way to model this in the devicetree.
> This group of units that are powered in Partial-IO are all powered by
> just one regulator that is always on. I can simply describe this in the
> devicetree by defining the regulator and consumer relationship:
> 
> Defining the regulator as described in the board schematic:
> 
>   vddshv_canuart: regulator-7 {
>          /* TPS22965DSGT */
>          compatible = "regulator-fixed";
>          regulator-name = "vddshv_canuart";
>          regulator-min-microvolt = <3300000>;
>          regulator-max-microvolt = <3300000>;
>          vin-supply = <&vcc_3v3_main>;
>          regulator-always-on;
>          regulator-boot-on;
>   };
> 
> Adding vio-supply to mcan and uarts, note, this binding does not exist
> yet:
> 
>   &mcu_mcan0 {
>          vio-supply = <&vddshv_canuart>;
>   };
> 
>   &mcu_mcan1 {
>          vio-supply = <&vddshv_canuart>;
>   };
> 
>   &mcu_uart0 {
>          vio-supply = <&vddshv_canuart>;
>   };
> 
>   &wkup_uart0 {
>          vio-supply = <&vddshv_canuart>;
>   };
> 

I am happy that problem is solved, but it really, really puzzles me how
above fits wakeup-mode-problem at all. This is so different that I doubt
you came up with proper hardware description.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml b/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml
index 25a2b42105e5..7d6152710573 100644
--- a/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml
+++ b/Documentation/devicetree/bindings/arm/keystone/ti,sci.yaml
@@ -61,6 +61,19 @@  properties:
   mboxes:
     minItems: 2
 
+  ti,partial-io-wakeup-sources:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: |
+      Partial-IO is a low power mode in which nearly everything is powered
+      off. Only pins associated with a few hardware units are capable to
+      wakeup the system from this mode. It is a very small subset of all
+      device nodes that have the 'wakeup-source' property.
+      ti,partial-io-wakeup-sources is the list of device nodes that can
+      wakeup the system from Partial-IO.
+
+      This low power mode depends on the capabilities of the SoC and
+      the firmware.
+
   ti,host-id:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: |