[PATCH v2 0/6] firmware: ti_sci: Partial-IO support

[PATCH v2 0/6] firmware: ti_sci: Partial-IO support

[Markus Schneider-Pargmann]

Hi,

v2
--
In v2 I fixed the comments on the first version of this series and
rebased to v6.11-rc1. See below for a more detailed list.

I also dropped the omap serial series for Partial-IO as it can't be
tested with Partial-IO at the moment. The code was tested with other low
power modes but they will be upstreamed later on.

Series
------
Partial-IO is a poweroff SoC state with a few pingroups active for
wakeup. This state can be entered by sending a TI_SCI PREPARE_SLEEP
message.

The message is sent on poweroff if one of the potential wakeup sources
for this power state are wakeup enabled. A list of potential wakeup
sources for the specific SoC is described in the devicetree. The wakeup
sources can be individually enabled/disabled by the user in the running
system.

The series is based on v6.11-rc1.

Partial-IO
----------
This series is part of a bigger topic to support Partial-IO on am62,
am62a and am62p. Partial-IO is a poweroff state in which some pins are
able to wakeup the SoC. In detail MCU m_can and two serial port pins can
trigger the wakeup.
A documentation can also be found in section 6.2.4 in the TRM:
  https://www.ti.com/lit/pdf/spruiv7

This other series is relevant for the support of Partial-IO:

 - can: m_can: Add am62 wakeup support
   https://lore.kernel.org/lkml/20240729074135.3850634-1-msp@baylibre.com/
   https://gitlab.baylibre.com/msp8/linux/-/tree/topic/mcan-wakeup-source/v6.11?ref_type=heads

Testing
-------
A test branch is available here that includes all patches required to
test Partial-IO:

https://gitlab.baylibre.com/msp8/linux/-/tree/integration/am62-lp-sk-partialio/v6.11?ref_type=heads

After enabling Wake-on-LAN the system can be powered off and will enter
the Partial-IO state in which it can be woken up by activity on the
specific pins:
    ethtool -s can0 wol p
    ethtool -s can1 wol p
    poweroff

I tested these patches on am62-lp-sk.

Best,
Markus

Previous versions:
 v1: https://lore.kernel.org/lkml/20240523080225.1288617-1-msp@baylibre.com/

Changes in v2:
 - Rebase to v6.11-rc1
 - dt-binding:
    - Update commit message
    - Add more verbose description of the new binding for a better
      explanation.
 - ti_sci driver:
    - Combine ti_sci_do_send() into ti_sci_do_xfer and only wait on a
      response if a flag is set.
    - On failure to enter Partial-IO, do emergency_restart()
    - Add comments
    - Fix small things

Markus Schneider-Pargmann (5):
  dt-bindings: ti, sci: Add property for partial-io-wakeup-sources
  firmware: ti_sci: Partial-IO support
  arm64: dts: ti: k3-pinctrl: Add WKUP_EN flag
  arm64: dts: ti: k3-am62: Add partial-io wakeup sources
  arm64: dts: ti: k3-am62a: Add partial-io wakeup sources

Vibhore Vardhan (1):
  arm64: dts: ti: k3-am62p: Add partial-io wakeup sources

 .../bindings/arm/keystone/ti,sci.yaml         |  13 ++
 arch/arm64/boot/dts/ti/k3-am62.dtsi           |   4 +
 arch/arm64/boot/dts/ti/k3-am62a.dtsi          |   4 +
 arch/arm64/boot/dts/ti/k3-am62p.dtsi          |   4 +
 arch/arm64/boot/dts/ti/k3-pinctrl.h           |   3 +
 drivers/firmware/ti_sci.c                     | 160 +++++++++++++++---
 drivers/firmware/ti_sci.h                     |  34 ++++
 7 files changed, 203 insertions(+), 19 deletions(-)

-- 
2.45.2

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

[Markus Schneider-Pargmann]

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(+)

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

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

[Krzysztof Kozlowski]

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

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

[Markus Schneider-Pargmann]

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

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

[Krzysztof Kozlowski]

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

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

[Markus Schneider-Pargmann]

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
> 

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

[Krzysztof Kozlowski]

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

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

[Krzysztof Kozlowski]

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

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

[Markus Schneider-Pargmann]

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

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

[Krzysztof Kozlowski]

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

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

[Markus Schneider-Pargmann]

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

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

[Krzysztof Kozlowski]

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

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

[Markus Schneider-Pargmann]

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

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

[Krzysztof Kozlowski]

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

[PATCH v2 2/6] firmware: ti_sci: Partial-IO support

[Markus Schneider-Pargmann]

Add support for Partial-IO poweroff. In Partial-IO pins of a few modules
can generate system wakeups while DDR memory is not powered resulting in
a fresh boot of the system. The modules that can be wakeup sources are
defined by the devicetree.

Only wakeup sources that are actually enabled by the user will be
considered as a an active wakeup source. If none of the wakeup sources
are enabled the system will do a normal poweroff. If at least one wakeup
source is enabled it will instead send a TI_SCI_MSG_PREPARE_SLEEP
message from the sys_off handler. Sending this message will result in an
immediate shutdown of the system. No execution is expected after this
point. The code will enter an infinite loop.

The wakeup source device nodes are gathered during probe. But they are
only resolved to the actual devices in the sys_off handler, if they
exist. If they do not exist, they are ignored.

A short documentation about Partial-IO can be found in section 6.2.4.5
of the TRM at
  https://www.ti.com/lit/pdf/spruiv7

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/firmware/ti_sci.c | 160 +++++++++++++++++++++++++++++++++-----
 drivers/firmware/ti_sci.h |  34 ++++++++
 2 files changed, 175 insertions(+), 19 deletions(-)

diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
index 160968301b1f..ba2e56da0215 100644
--- a/drivers/firmware/ti_sci.c
+++ b/drivers/firmware/ti_sci.c
@@ -99,6 +99,9 @@ struct ti_sci_desc {
  * @node:	list head
  * @host_id:	Host ID
  * @users:	Number of users of this instance
+ * @nr_wakeup_sources: Number of device nodes in wakeup_source_nodes
+ * @wakeup_source_nodes: Array of all device_nodes listed as wakeup sources in
+ *			 the devicetree
  */
 struct ti_sci_info {
 	struct device *dev;
@@ -116,6 +119,9 @@ struct ti_sci_info {
 	u8 host_id;
 	/* protected by ti_sci_list_mutex */
 	int users;
+
+	int nr_wakeup_sources;
+	struct device_node **wakeup_source_nodes;
 };
 
 #define cl_to_ti_sci_info(c)	container_of(c, struct ti_sci_info, cl)
@@ -392,10 +398,13 @@ static void ti_sci_put_one_xfer(struct ti_sci_xfers_info *minfo,
 static inline int ti_sci_do_xfer(struct ti_sci_info *info,
 				 struct ti_sci_xfer *xfer)
 {
+	struct ti_sci_msg_hdr *hdr = (struct ti_sci_msg_hdr *)xfer->tx_message.buf;
 	int ret;
 	int timeout;
 	struct device *dev = info->dev;
 	bool done_state = true;
+	bool response_expected = !!(hdr->flags & (TI_SCI_FLAG_REQ_ACK_ON_PROCESSED |
+						  TI_SCI_FLAG_REQ_ACK_ON_RECEIVED));
 
 	ret = mbox_send_message(info->chan_tx, &xfer->tx_message);
 	if (ret < 0)
@@ -403,25 +412,27 @@ static inline int ti_sci_do_xfer(struct ti_sci_info *info,
 
 	ret = 0;
 
-	if (system_state <= SYSTEM_RUNNING) {
-		/* And we wait for the response. */
-		timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
-		if (!wait_for_completion_timeout(&xfer->done, timeout))
-			ret = -ETIMEDOUT;
-	} else {
-		/*
-		 * If we are !running, we cannot use wait_for_completion_timeout
-		 * during noirq phase, so we must manually poll the completion.
-		 */
-		ret = read_poll_timeout_atomic(try_wait_for_completion, done_state,
-					       done_state, 1,
-					       info->desc->max_rx_timeout_ms * 1000,
-					       false, &xfer->done);
-	}
+	if (response_expected) {
+		if (system_state <= SYSTEM_RUNNING) {
+			/* And we wait for the response. */
+			timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
+			if (!wait_for_completion_timeout(&xfer->done, timeout))
+				ret = -ETIMEDOUT;
+		} else {
+			/*
+			 * If we are !running, we cannot use wait_for_completion_timeout
+			 * during noirq phase, so we must manually poll the completion.
+			 */
+			ret = read_poll_timeout_atomic(try_wait_for_completion, done_state,
+						       done_state, 1,
+						       info->desc->max_rx_timeout_ms * 1000,
+						       false, &xfer->done);
+		}
 
-	if (ret == -ETIMEDOUT)
-		dev_err(dev, "Mbox timedout in resp(caller: %pS)\n",
-			(void *)_RET_IP_);
+		if (ret == -ETIMEDOUT)
+			dev_err(dev, "Mbox timedout in resp(caller: %pS)\n",
+				(void *)_RET_IP_);
+	}
 
 	/*
 	 * NOTE: we might prefer not to need the mailbox ticker to manage the
@@ -3262,6 +3273,82 @@ static int tisci_reboot_handler(struct sys_off_data *data)
 	return NOTIFY_BAD;
 }
 
+/*
+ * Enter Partial-IO, which disables everything including DDR with only a small
+ * logic being active for wakeup.
+ */
+static int tisci_enter_partial_io(struct ti_sci_info *info)
+{
+	struct ti_sci_msg_req_prepare_sleep *req;
+	struct ti_sci_xfer *xfer;
+	struct device *dev = info->dev;
+	int ret = 0;
+
+	xfer = ti_sci_get_one_xfer(info, TI_SCI_MSG_PREPARE_SLEEP,
+				   TI_SCI_FLAG_REQ_GENERIC_NORESPONSE,
+				   sizeof(*req), sizeof(struct ti_sci_msg_hdr));
+	if (IS_ERR(xfer)) {
+		ret = PTR_ERR(xfer);
+		dev_err(dev, "Message alloc failed(%d)\n", ret);
+		return ret;
+	}
+
+	req = (struct ti_sci_msg_req_prepare_sleep *)xfer->xfer_buf;
+	req->mode = TISCI_MSG_VALUE_SLEEP_MODE_PARTIAL_IO;
+	req->ctx_lo = 0;
+	req->ctx_hi = 0;
+	req->debug_flags = 0;
+
+	ret = ti_sci_do_xfer(info, xfer);
+	if (ret) {
+		dev_err(dev, "Mbox send fail %d\n", ret);
+		goto fail;
+	}
+
+fail:
+	ti_sci_put_one_xfer(&info->minfo, xfer);
+
+	return ret;
+}
+
+static int tisci_sys_off_handler(struct sys_off_data *data)
+{
+	struct ti_sci_info *info = data->cb_data;
+	int i;
+	int ret;
+	bool enter_partial_io = false;
+
+	for (i = 0; i != info->nr_wakeup_sources; ++i) {
+		struct platform_device *pdev =
+			of_find_device_by_node(info->wakeup_source_nodes[i]);
+
+		if (!pdev)
+			continue;
+
+		if (device_may_wakeup(&pdev->dev)) {
+			dev_dbg(info->dev, "%pOFp identified as wakeup source\n",
+				info->wakeup_source_nodes[i]);
+			enter_partial_io = true;
+		}
+	}
+
+	if (!enter_partial_io)
+		return NOTIFY_DONE;
+
+	ret = tisci_enter_partial_io(info);
+
+	if (ret) {
+		dev_err(info->dev,
+			"Failed to enter Partial-IO %pe, trying to do an emergency restart\n",
+			ERR_PTR(ret));
+		emergency_restart();
+	}
+
+	while (1);
+
+	return NOTIFY_DONE;
+}
+
 /* Description for K2G */
 static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = {
 	.default_host_id = 2,
@@ -3398,6 +3485,35 @@ static int ti_sci_probe(struct platform_device *pdev)
 		goto out;
 	}
 
+	if (of_property_read_bool(dev->of_node, "ti,partial-io-wakeup-sources")) {
+		info->nr_wakeup_sources =
+			of_count_phandle_with_args(dev->of_node,
+						   "ti,partial-io-wakeup-sources",
+						   NULL);
+		info->wakeup_source_nodes =
+			devm_kzalloc(dev, sizeof(*info->wakeup_source_nodes),
+				     GFP_KERNEL);
+
+		for (i = 0; i != info->nr_wakeup_sources; ++i) {
+			struct device_node *devnode =
+				of_parse_phandle(dev->of_node,
+						 "ti,partial-io-wakeup-sources",
+						 i);
+			info->wakeup_source_nodes[i] = devnode;
+		}
+
+		ret = devm_register_sys_off_handler(dev,
+						    SYS_OFF_MODE_POWER_OFF,
+						    SYS_OFF_PRIO_FIRMWARE,
+						    tisci_sys_off_handler,
+						    info);
+		if (ret) {
+			dev_err(dev, "Failed to register sys_off_handler %pe\n",
+				ERR_PTR(ret));
+			goto out;
+		}
+	}
+
 	dev_info(dev, "ABI: %d.%d (firmware rev 0x%04x '%s')\n",
 		 info->handle.version.abi_major, info->handle.version.abi_minor,
 		 info->handle.version.firmware_revision,
@@ -3407,7 +3523,13 @@ static int ti_sci_probe(struct platform_device *pdev)
 	list_add_tail(&info->node, &ti_sci_list);
 	mutex_unlock(&ti_sci_list_mutex);
 
-	return of_platform_populate(dev->of_node, NULL, NULL, dev);
+	ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
+	if (ret) {
+		dev_err(dev, "platform_populate failed %pe\n", ERR_PTR(ret));
+		goto out;
+	}
+	return 0;
+
 out:
 	if (!IS_ERR(info->chan_tx))
 		mbox_free_channel(info->chan_tx);
diff --git a/drivers/firmware/ti_sci.h b/drivers/firmware/ti_sci.h
index 5846c60220f5..ec8e6bb1791a 100644
--- a/drivers/firmware/ti_sci.h
+++ b/drivers/firmware/ti_sci.h
@@ -35,6 +35,9 @@
 #define TI_SCI_MSG_QUERY_CLOCK_FREQ	0x010d
 #define TI_SCI_MSG_GET_CLOCK_FREQ	0x010e
 
+/* Low Power Mode Requests */
+#define TI_SCI_MSG_PREPARE_SLEEP	0x0300
+
 /* Resource Management Requests */
 #define TI_SCI_MSG_GET_RESOURCE_RANGE	0x1500
 
@@ -545,6 +548,37 @@ struct ti_sci_msg_resp_get_clock_freq {
 	u64 freq_hz;
 } __packed;
 
+#define TISCI_MSG_VALUE_SLEEP_MODE_DEEP_SLEEP				0x0
+#define TISCI_MSG_VALUE_SLEEP_MODE_MCU_ONLY				0x1
+#define TISCI_MSG_VALUE_SLEEP_MODE_STANDBY				0x2
+/*
+ * When sending perpare_sleep with MODE_PARTIAL_IO no response will be sent,
+ * no further steps are required. */
+#define TISCI_MSG_VALUE_SLEEP_MODE_PARTIAL_IO				0x3
+
+/**
+ * struct tisci_msg_req_prepare_sleep - Request for TISCI_MSG_PREPARE_SLEEP.
+ *
+ * @hdr				TISCI header to provide ACK/NAK flags to the host.
+ * @mode			Low power mode to enter.
+ * @ctx_lo			Low 32-bits of physical pointer to address to use for context save.
+ * @ctx_hi			High 32-bits of physical pointer to address to use for context save.
+ * @debug_flags			Flags that can be set to halt the sequence during suspend or
+ *				resume to allow JTAG connection and debug.
+ *
+ * This message is used as the first step of entering a low power mode. It
+ * allows configurable information, including which state to enter to be
+ * easily shared from the application, as this is a non-secure message and
+ * therefore can be sent by anyone.
+ */
+struct ti_sci_msg_req_prepare_sleep {
+	struct ti_sci_msg_hdr	hdr;
+	u8			mode;
+	u32			ctx_lo;
+	u32			ctx_hi;
+	u32			debug_flags;
+} __packed;
+
 #define TI_SCI_IRQ_SECONDARY_HOST_INVALID	0xff
 
 /**
-- 
2.45.2

Re: [PATCH v2 2/6] firmware: ti_sci: Partial-IO support

[Nishanth Menon]

On 10:00-20240729, Markus Schneider-Pargmann wrote:
> Add support for Partial-IO poweroff. In Partial-IO pins of a few modules
> can generate system wakeups while DDR memory is not powered resulting in
> a fresh boot of the system. The modules that can be wakeup sources are
> defined by the devicetree.
> 
> Only wakeup sources that are actually enabled by the user will be
> considered as a an active wakeup source. If none of the wakeup sources
> are enabled the system will do a normal poweroff. If at least one wakeup
> source is enabled it will instead send a TI_SCI_MSG_PREPARE_SLEEP
> message from the sys_off handler. Sending this message will result in an
> immediate shutdown of the system. No execution is expected after this
> point. The code will enter an infinite loop.
> 
> The wakeup source device nodes are gathered during probe. But they are
> only resolved to the actual devices in the sys_off handler, if they
> exist. If they do not exist, they are ignored.
> 
> A short documentation about Partial-IO can be found in section 6.2.4.5
> of the TRM at
>   https://www.ti.com/lit/pdf/spruiv7
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---
>  drivers/firmware/ti_sci.c | 160 +++++++++++++++++++++++++++++++++-----
>  drivers/firmware/ti_sci.h |  34 ++++++++
>  2 files changed, 175 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
> index 160968301b1f..ba2e56da0215 100644
> --- a/drivers/firmware/ti_sci.c
> +++ b/drivers/firmware/ti_sci.c
> @@ -99,6 +99,9 @@ struct ti_sci_desc {
>   * @node:	list head
>   * @host_id:	Host ID
>   * @users:	Number of users of this instance
> + * @nr_wakeup_sources: Number of device nodes in wakeup_source_nodes
> + * @wakeup_source_nodes: Array of all device_nodes listed as wakeup sources in
> + *			 the devicetree
>   */
>  struct ti_sci_info {
>  	struct device *dev;
> @@ -116,6 +119,9 @@ struct ti_sci_info {
>  	u8 host_id;
>  	/* protected by ti_sci_list_mutex */
>  	int users;
> +
> +	int nr_wakeup_sources;
> +	struct device_node **wakeup_source_nodes;
>  };
>  
>  #define cl_to_ti_sci_info(c)	container_of(c, struct ti_sci_info, cl)
> @@ -392,10 +398,13 @@ static void ti_sci_put_one_xfer(struct ti_sci_xfers_info *minfo,
>  static inline int ti_sci_do_xfer(struct ti_sci_info *info,
>  				 struct ti_sci_xfer *xfer)
>  {
> +	struct ti_sci_msg_hdr *hdr = (struct ti_sci_msg_hdr *)xfer->tx_message.buf;
>  	int ret;
>  	int timeout;
>  	struct device *dev = info->dev;
>  	bool done_state = true;
> +	bool response_expected = !!(hdr->flags & (TI_SCI_FLAG_REQ_ACK_ON_PROCESSED |
> +						  TI_SCI_FLAG_REQ_ACK_ON_RECEIVED));

I think a separate patch to introduce a no_response expected patch would
make sense on which we build tisci_sys_off_handler in the next patch?

>  
>  	ret = mbox_send_message(info->chan_tx, &xfer->tx_message);
>  	if (ret < 0)
> @@ -403,25 +412,27 @@ static inline int ti_sci_do_xfer(struct ti_sci_info *info,
>  
>  	ret = 0;
>  
> -	if (system_state <= SYSTEM_RUNNING) {
> -		/* And we wait for the response. */
> -		timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
> -		if (!wait_for_completion_timeout(&xfer->done, timeout))
> -			ret = -ETIMEDOUT;
> -	} else {
> -		/*
> -		 * If we are !running, we cannot use wait_for_completion_timeout
> -		 * during noirq phase, so we must manually poll the completion.
> -		 */
> -		ret = read_poll_timeout_atomic(try_wait_for_completion, done_state,
> -					       done_state, 1,
> -					       info->desc->max_rx_timeout_ms * 1000,
> -					       false, &xfer->done);
> -	}
> +	if (response_expected) {

	How about a goto?

if (!response_expected)
	goto no_response;
> +		if (system_state <= SYSTEM_RUNNING) {
> +			/* And we wait for the response. */
> +			timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
> +			if (!wait_for_completion_timeout(&xfer->done, timeout))
> +				ret = -ETIMEDOUT;
> +		} else {
> +			/*
> +			 * If we are !running, we cannot use wait_for_completion_timeout
> +			 * during noirq phase, so we must manually poll the completion.
> +			 */
> +			ret = read_poll_timeout_atomic(try_wait_for_completion, done_state,
> +						       done_state, 1,
> +						       info->desc->max_rx_timeout_ms * 1000,
> +						       false, &xfer->done);
> +		}
>  
> -	if (ret == -ETIMEDOUT)
> -		dev_err(dev, "Mbox timedout in resp(caller: %pS)\n",
> -			(void *)_RET_IP_);
> +		if (ret == -ETIMEDOUT)
> +			dev_err(dev, "Mbox timedout in resp(caller: %pS)\n",
> +				(void *)_RET_IP_);
> +	}
>  
no_response:

>  	/*
>  	 * NOTE: we might prefer not to need the mailbox ticker to manage the
> @@ -3262,6 +3273,82 @@ static int tisci_reboot_handler(struct sys_off_data *data)
>  	return NOTIFY_BAD;
>  }
>  
[...]

> +static int tisci_sys_off_handler(struct sys_off_data *data)
> +{
> +	struct ti_sci_info *info = data->cb_data;
> +	int i;
> +	int ret;
> +	bool enter_partial_io = false;
> +
> +	for (i = 0; i != info->nr_wakeup_sources; ++i) {
> +		struct platform_device *pdev =
> +			of_find_device_by_node(info->wakeup_source_nodes[i]);
> +
> +		if (!pdev)
> +			continue;
> +
> +		if (device_may_wakeup(&pdev->dev)) {
> +			dev_dbg(info->dev, "%pOFp identified as wakeup source\n",
> +				info->wakeup_source_nodes[i]);
> +			enter_partial_io = true;
> +		}
> +	}
> +
> +	if (!enter_partial_io)
> +		return NOTIFY_DONE;
> +
> +	ret = tisci_enter_partial_io(info);
> +
> +	if (ret) {
> +		dev_err(info->dev,
> +			"Failed to enter Partial-IO %pe, trying to do an emergency restart\n",
> +			ERR_PTR(ret));
> +		emergency_restart();
> +	}
> +
> +	while (1);

Why not fall through OR go through emergency_restart (since there is
no fall through for shutdown path) if it acks, but actually fails to
enter LPM state after a dt described or a default timeout period?

> +
> +	return NOTIFY_DONE;
> +}
> +
>  /* Description for K2G */
>  static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = {
>  	.default_host_id = 2,
> @@ -3398,6 +3485,35 @@ static int ti_sci_probe(struct platform_device *pdev)
>  		goto out;
>  	}
>  
> +	if (of_property_read_bool(dev->of_node, "ti,partial-io-wakeup-sources")) {

You should probably check on TISCI_MSG_QUERY_FW_CAPS[1] if
Partial IO on low power mode is supported as well? if there is a
mismatch, report so?

> +		info->nr_wakeup_sources =
> +			of_count_phandle_with_args(dev->of_node,
> +						   "ti,partial-io-wakeup-sources",
> +						   NULL);
> +		info->wakeup_source_nodes =
> +			devm_kzalloc(dev, sizeof(*info->wakeup_source_nodes),
> +				     GFP_KERNEL);
> +
> +		for (i = 0; i != info->nr_wakeup_sources; ++i) {
> +			struct device_node *devnode =
> +				of_parse_phandle(dev->of_node,
> +						 "ti,partial-io-wakeup-sources",
> +						 i);
> +			info->wakeup_source_nodes[i] = devnode;

Curious: Don't we need to maintain reference counting for the devnode
if CONFIG_OF_DYNAMIC?

	[...]

[1] https://software-dl.ti.com/tisci/esd/latest/2_tisci_msgs/general/core.html#tisci-msg-query-fw-caps

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v2 2/6] firmware: ti_sci: Partial-IO support

[Markus Schneider-Pargmann]

On Tue, Jul 30, 2024 at 07:28:01AM GMT, Nishanth Menon wrote:
> On 10:00-20240729, Markus Schneider-Pargmann wrote:
> > Add support for Partial-IO poweroff. In Partial-IO pins of a few modules
> > can generate system wakeups while DDR memory is not powered resulting in
> > a fresh boot of the system. The modules that can be wakeup sources are
> > defined by the devicetree.
> > 
> > Only wakeup sources that are actually enabled by the user will be
> > considered as a an active wakeup source. If none of the wakeup sources
> > are enabled the system will do a normal poweroff. If at least one wakeup
> > source is enabled it will instead send a TI_SCI_MSG_PREPARE_SLEEP
> > message from the sys_off handler. Sending this message will result in an
> > immediate shutdown of the system. No execution is expected after this
> > point. The code will enter an infinite loop.
> > 
> > The wakeup source device nodes are gathered during probe. But they are
> > only resolved to the actual devices in the sys_off handler, if they
> > exist. If they do not exist, they are ignored.
> > 
> > A short documentation about Partial-IO can be found in section 6.2.4.5
> > of the TRM at
> >   https://www.ti.com/lit/pdf/spruiv7
> > 
> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > ---
> >  drivers/firmware/ti_sci.c | 160 +++++++++++++++++++++++++++++++++-----
> >  drivers/firmware/ti_sci.h |  34 ++++++++
> >  2 files changed, 175 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
> > index 160968301b1f..ba2e56da0215 100644
> > --- a/drivers/firmware/ti_sci.c
> > +++ b/drivers/firmware/ti_sci.c
> > @@ -99,6 +99,9 @@ struct ti_sci_desc {
> >   * @node:	list head
> >   * @host_id:	Host ID
> >   * @users:	Number of users of this instance
> > + * @nr_wakeup_sources: Number of device nodes in wakeup_source_nodes
> > + * @wakeup_source_nodes: Array of all device_nodes listed as wakeup sources in
> > + *			 the devicetree
> >   */
> >  struct ti_sci_info {
> >  	struct device *dev;
> > @@ -116,6 +119,9 @@ struct ti_sci_info {
> >  	u8 host_id;
> >  	/* protected by ti_sci_list_mutex */
> >  	int users;
> > +
> > +	int nr_wakeup_sources;
> > +	struct device_node **wakeup_source_nodes;
> >  };
> >  
> >  #define cl_to_ti_sci_info(c)	container_of(c, struct ti_sci_info, cl)
> > @@ -392,10 +398,13 @@ static void ti_sci_put_one_xfer(struct ti_sci_xfers_info *minfo,
> >  static inline int ti_sci_do_xfer(struct ti_sci_info *info,
> >  				 struct ti_sci_xfer *xfer)
> >  {
> > +	struct ti_sci_msg_hdr *hdr = (struct ti_sci_msg_hdr *)xfer->tx_message.buf;
> >  	int ret;
> >  	int timeout;
> >  	struct device *dev = info->dev;
> >  	bool done_state = true;
> > +	bool response_expected = !!(hdr->flags & (TI_SCI_FLAG_REQ_ACK_ON_PROCESSED |
> > +						  TI_SCI_FLAG_REQ_ACK_ON_RECEIVED));
> 
> I think a separate patch to introduce a no_response expected patch would
> make sense on which we build tisci_sys_off_handler in the next patch?
> 
> >  
> >  	ret = mbox_send_message(info->chan_tx, &xfer->tx_message);
> >  	if (ret < 0)
> > @@ -403,25 +412,27 @@ static inline int ti_sci_do_xfer(struct ti_sci_info *info,
> >  
> >  	ret = 0;
> >  
> > -	if (system_state <= SYSTEM_RUNNING) {
> > -		/* And we wait for the response. */
> > -		timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
> > -		if (!wait_for_completion_timeout(&xfer->done, timeout))
> > -			ret = -ETIMEDOUT;
> > -	} else {
> > -		/*
> > -		 * If we are !running, we cannot use wait_for_completion_timeout
> > -		 * during noirq phase, so we must manually poll the completion.
> > -		 */
> > -		ret = read_poll_timeout_atomic(try_wait_for_completion, done_state,
> > -					       done_state, 1,
> > -					       info->desc->max_rx_timeout_ms * 1000,
> > -					       false, &xfer->done);
> > -	}
> > +	if (response_expected) {
> 
> 	How about a goto?

Yes, thanks, looks cleaner.

> 
> if (!response_expected)
> 	goto no_response;
> > +		if (system_state <= SYSTEM_RUNNING) {
> > +			/* And we wait for the response. */
> > +			timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
> > +			if (!wait_for_completion_timeout(&xfer->done, timeout))
> > +				ret = -ETIMEDOUT;
> > +		} else {
> > +			/*
> > +			 * If we are !running, we cannot use wait_for_completion_timeout
> > +			 * during noirq phase, so we must manually poll the completion.
> > +			 */
> > +			ret = read_poll_timeout_atomic(try_wait_for_completion, done_state,
> > +						       done_state, 1,
> > +						       info->desc->max_rx_timeout_ms * 1000,
> > +						       false, &xfer->done);
> > +		}
> >  
> > -	if (ret == -ETIMEDOUT)
> > -		dev_err(dev, "Mbox timedout in resp(caller: %pS)\n",
> > -			(void *)_RET_IP_);
> > +		if (ret == -ETIMEDOUT)
> > +			dev_err(dev, "Mbox timedout in resp(caller: %pS)\n",
> > +				(void *)_RET_IP_);
> > +	}
> >  
> no_response:
> 
> >  	/*
> >  	 * NOTE: we might prefer not to need the mailbox ticker to manage the
> > @@ -3262,6 +3273,82 @@ static int tisci_reboot_handler(struct sys_off_data *data)
> >  	return NOTIFY_BAD;
> >  }
> >  
> [...]
> 
> > +static int tisci_sys_off_handler(struct sys_off_data *data)
> > +{
> > +	struct ti_sci_info *info = data->cb_data;
> > +	int i;
> > +	int ret;
> > +	bool enter_partial_io = false;
> > +
> > +	for (i = 0; i != info->nr_wakeup_sources; ++i) {
> > +		struct platform_device *pdev =
> > +			of_find_device_by_node(info->wakeup_source_nodes[i]);
> > +
> > +		if (!pdev)
> > +			continue;
> > +
> > +		if (device_may_wakeup(&pdev->dev)) {
> > +			dev_dbg(info->dev, "%pOFp identified as wakeup source\n",
> > +				info->wakeup_source_nodes[i]);
> > +			enter_partial_io = true;
> > +		}
> > +	}
> > +
> > +	if (!enter_partial_io)
> > +		return NOTIFY_DONE;
> > +
> > +	ret = tisci_enter_partial_io(info);
> > +
> > +	if (ret) {
> > +		dev_err(info->dev,
> > +			"Failed to enter Partial-IO %pe, trying to do an emergency restart\n",
> > +			ERR_PTR(ret));
> > +		emergency_restart();
> > +	}
> > +
> > +	while (1);
> 
> Why not fall through OR go through emergency_restart (since there is
> no fall through for shutdown path) if it acks, but actually fails to
> enter LPM state after a dt described or a default timeout period?
> 
> > +
> > +	return NOTIFY_DONE;
> > +}
> > +
> >  /* Description for K2G */
> >  static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = {
> >  	.default_host_id = 2,
> > @@ -3398,6 +3485,35 @@ static int ti_sci_probe(struct platform_device *pdev)
> >  		goto out;
> >  	}
> >  
> > +	if (of_property_read_bool(dev->of_node, "ti,partial-io-wakeup-sources")) {
> 
> You should probably check on TISCI_MSG_QUERY_FW_CAPS[1] if
> Partial IO on low power mode is supported as well? if there is a
> mismatch, report so?

I actually have another series in my queue that introduces this check. I
just implemented this check for Partial-IO yesterday in the patch that
introduces fw capabilities. If you like I can switch these series
around.

> 
> > +		info->nr_wakeup_sources =
> > +			of_count_phandle_with_args(dev->of_node,
> > +						   "ti,partial-io-wakeup-sources",
> > +						   NULL);
> > +		info->wakeup_source_nodes =
> > +			devm_kzalloc(dev, sizeof(*info->wakeup_source_nodes),
> > +				     GFP_KERNEL);
> > +
> > +		for (i = 0; i != info->nr_wakeup_sources; ++i) {
> > +			struct device_node *devnode =
> > +				of_parse_phandle(dev->of_node,
> > +						 "ti,partial-io-wakeup-sources",
> > +						 i);
> > +			info->wakeup_source_nodes[i] = devnode;
> 
> Curious: Don't we need to maintain reference counting for the devnode
> if CONFIG_OF_DYNAMIC?

In case you mean I missed of_node_put(), yes, I did, thank you. I added
it in a ti_sci_remove().

Best
Markus

Re: [PATCH v2 2/6] firmware: ti_sci: Partial-IO support

[Nishanth Menon]

On 15:01-20240730, Markus Schneider-Pargmann wrote:
> > > +
> > > +	return NOTIFY_DONE;
> > > +}
> > > +
> > >  /* Description for K2G */
> > >  static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = {
> > >  	.default_host_id = 2,
> > > @@ -3398,6 +3485,35 @@ static int ti_sci_probe(struct platform_device *pdev)
> > >  		goto out;
> > >  	}
> > >  
> > > +	if (of_property_read_bool(dev->of_node, "ti,partial-io-wakeup-sources")) {
> > 
> > You should probably check on TISCI_MSG_QUERY_FW_CAPS[1] if
> > Partial IO on low power mode is supported as well? if there is a
> > mismatch, report so?
> 
> I actually have another series in my queue that introduces this check. I
> just implemented this check for Partial-IO yesterday in the patch that
> introduces fw capabilities. If you like I can switch these series
> around.

Yes, please introduce it part of the series.

> 
> > 
> > > +		info->nr_wakeup_sources =
> > > +			of_count_phandle_with_args(dev->of_node,
> > > +						   "ti,partial-io-wakeup-sources",
> > > +						   NULL);
> > > +		info->wakeup_source_nodes =
> > > +			devm_kzalloc(dev, sizeof(*info->wakeup_source_nodes),
> > > +				     GFP_KERNEL);
> > > +
> > > +		for (i = 0; i != info->nr_wakeup_sources; ++i) {
> > > +			struct device_node *devnode =
> > > +				of_parse_phandle(dev->of_node,
> > > +						 "ti,partial-io-wakeup-sources",
> > > +						 i);
> > > +			info->wakeup_source_nodes[i] = devnode;
> > 
> > Curious: Don't we need to maintain reference counting for the devnode
> > if CONFIG_OF_DYNAMIC?
> 
> In case you mean I missed of_node_put(), yes, I did, thank you. I added
> it in a ti_sci_remove().

And unless I am mistaken, of_node_get as required as you are
retaining the reference of the node till shutdown / remove is invoked.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v2 2/6] firmware: ti_sci: Partial-IO support

[Markus Schneider-Pargmann]

On Tue, Jul 30, 2024 at 10:07:22AM GMT, Nishanth Menon wrote:
> On 15:01-20240730, Markus Schneider-Pargmann wrote:
> > > > +
> > > > +	return NOTIFY_DONE;
> > > > +}
> > > > +
> > > >  /* Description for K2G */
> > > >  static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = {
> > > >  	.default_host_id = 2,
> > > > @@ -3398,6 +3485,35 @@ static int ti_sci_probe(struct platform_device *pdev)
> > > >  		goto out;
> > > >  	}
> > > >  
> > > > +	if (of_property_read_bool(dev->of_node, "ti,partial-io-wakeup-sources")) {
> > > 
> > > You should probably check on TISCI_MSG_QUERY_FW_CAPS[1] if
> > > Partial IO on low power mode is supported as well? if there is a
> > > mismatch, report so?
> > 
> > I actually have another series in my queue that introduces this check. I
> > just implemented this check for Partial-IO yesterday in the patch that
> > introduces fw capabilities. If you like I can switch these series
> > around.
> 
> Yes, please introduce it part of the series.
> 
> > 
> > > 
> > > > +		info->nr_wakeup_sources =
> > > > +			of_count_phandle_with_args(dev->of_node,
> > > > +						   "ti,partial-io-wakeup-sources",
> > > > +						   NULL);
> > > > +		info->wakeup_source_nodes =
> > > > +			devm_kzalloc(dev, sizeof(*info->wakeup_source_nodes),
> > > > +				     GFP_KERNEL);
> > > > +
> > > > +		for (i = 0; i != info->nr_wakeup_sources; ++i) {
> > > > +			struct device_node *devnode =
> > > > +				of_parse_phandle(dev->of_node,
> > > > +						 "ti,partial-io-wakeup-sources",
> > > > +						 i);
> > > > +			info->wakeup_source_nodes[i] = devnode;
> > > 
> > > Curious: Don't we need to maintain reference counting for the devnode
> > > if CONFIG_OF_DYNAMIC?
> > 
> > In case you mean I missed of_node_put(), yes, I did, thank you. I added
> > it in a ti_sci_remove().
> 
> And unless I am mistaken, of_node_get as required as you are
> retaining the reference of the node till shutdown / remove is invoked.

The function documentation says the refcount is already incremented:

 * Return: The device_node pointer with refcount incremented.  Use
 * of_node_put() on it when done.

Best
Markus

> 
> -- 
> Regards,
> Nishanth Menon
> Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v2 2/6] firmware: ti_sci: Partial-IO support

[Nishanth Menon]

On 14:36-20240731, Markus Schneider-Pargmann wrote:
> > > > > +		for (i = 0; i != info->nr_wakeup_sources; ++i) {
> > > > > +			struct device_node *devnode =
> > > > > +				of_parse_phandle(dev->of_node,
> > > > > +						 "ti,partial-io-wakeup-sources",
> > > > > +						 i);
> > > > > +			info->wakeup_source_nodes[i] = devnode;
> > > > 
> > > > Curious: Don't we need to maintain reference counting for the devnode
> > > > if CONFIG_OF_DYNAMIC?
> > > 
> > > In case you mean I missed of_node_put(), yes, I did, thank you. I added
> > > it in a ti_sci_remove().
> > 
> > And unless I am mistaken, of_node_get as required as you are
> > retaining the reference of the node till shutdown / remove is invoked.
> 
> The function documentation says the refcount is already incremented:
> 
>  * Return: The device_node pointer with refcount incremented.  Use
>  * of_node_put() on it when done.
> 

Yes indeed. I missed it. Thanks for looking it up.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v2 2/6] firmware: ti_sci: Partial-IO support

[Andrew Davis]

On 7/29/24 3:00 AM, Markus Schneider-Pargmann wrote:
> Add support for Partial-IO poweroff. In Partial-IO pins of a few modules
> can generate system wakeups while DDR memory is not powered resulting in
> a fresh boot of the system. The modules that can be wakeup sources are
> defined by the devicetree.
> 
> Only wakeup sources that are actually enabled by the user will be
> considered as a an active wakeup source. If none of the wakeup sources
> are enabled the system will do a normal poweroff. If at least one wakeup
> source is enabled it will instead send a TI_SCI_MSG_PREPARE_SLEEP
> message from the sys_off handler. Sending this message will result in an
> immediate shutdown of the system. No execution is expected after this
> point. The code will enter an infinite loop.
> 
> The wakeup source device nodes are gathered during probe. But they are
> only resolved to the actual devices in the sys_off handler, if they
> exist. If they do not exist, they are ignored.
> 
> A short documentation about Partial-IO can be found in section 6.2.4.5
> of the TRM at
>    https://www.ti.com/lit/pdf/spruiv7
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---
>   drivers/firmware/ti_sci.c | 160 +++++++++++++++++++++++++++++++++-----
>   drivers/firmware/ti_sci.h |  34 ++++++++
>   2 files changed, 175 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
> index 160968301b1f..ba2e56da0215 100644
> --- a/drivers/firmware/ti_sci.c
> +++ b/drivers/firmware/ti_sci.c
> @@ -99,6 +99,9 @@ struct ti_sci_desc {
>    * @node:	list head
>    * @host_id:	Host ID
>    * @users:	Number of users of this instance
> + * @nr_wakeup_sources: Number of device nodes in wakeup_source_nodes
> + * @wakeup_source_nodes: Array of all device_nodes listed as wakeup sources in
> + *			 the devicetree
>    */
>   struct ti_sci_info {
>   	struct device *dev;
> @@ -116,6 +119,9 @@ struct ti_sci_info {
>   	u8 host_id;
>   	/* protected by ti_sci_list_mutex */
>   	int users;
> +
> +	int nr_wakeup_sources;
> +	struct device_node **wakeup_source_nodes;
>   };
>   
>   #define cl_to_ti_sci_info(c)	container_of(c, struct ti_sci_info, cl)
> @@ -392,10 +398,13 @@ static void ti_sci_put_one_xfer(struct ti_sci_xfers_info *minfo,
>   static inline int ti_sci_do_xfer(struct ti_sci_info *info,
>   				 struct ti_sci_xfer *xfer)
>   {
> +	struct ti_sci_msg_hdr *hdr = (struct ti_sci_msg_hdr *)xfer->tx_message.buf;
>   	int ret;
>   	int timeout;
>   	struct device *dev = info->dev;
>   	bool done_state = true;
> +	bool response_expected = !!(hdr->flags & (TI_SCI_FLAG_REQ_ACK_ON_PROCESSED |
> +						  TI_SCI_FLAG_REQ_ACK_ON_RECEIVED));
>   
>   	ret = mbox_send_message(info->chan_tx, &xfer->tx_message);
>   	if (ret < 0)
> @@ -403,25 +412,27 @@ static inline int ti_sci_do_xfer(struct ti_sci_info *info,
>   
>   	ret = 0;
>   
> -	if (system_state <= SYSTEM_RUNNING) {
> -		/* And we wait for the response. */
> -		timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
> -		if (!wait_for_completion_timeout(&xfer->done, timeout))
> -			ret = -ETIMEDOUT;
> -	} else {
> -		/*
> -		 * If we are !running, we cannot use wait_for_completion_timeout
> -		 * during noirq phase, so we must manually poll the completion.
> -		 */
> -		ret = read_poll_timeout_atomic(try_wait_for_completion, done_state,
> -					       done_state, 1,
> -					       info->desc->max_rx_timeout_ms * 1000,
> -					       false, &xfer->done);
> -	}
> +	if (response_expected) {

If a response is not expected why not simply return above and not add even more
indention here? Also, in that case, is the call to mbox_client_txdone() needed?

> +		if (system_state <= SYSTEM_RUNNING) {
> +			/* And we wait for the response. */
> +			timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
> +			if (!wait_for_completion_timeout(&xfer->done, timeout))
> +				ret = -ETIMEDOUT;
> +		} else {
> +			/*
> +			 * If we are !running, we cannot use wait_for_completion_timeout
> +			 * during noirq phase, so we must manually poll the completion.
> +			 */
> +			ret = read_poll_timeout_atomic(try_wait_for_completion, done_state,
> +						       done_state, 1,
> +						       info->desc->max_rx_timeout_ms * 1000,
> +						       false, &xfer->done);
> +		}
>   
> -	if (ret == -ETIMEDOUT)
> -		dev_err(dev, "Mbox timedout in resp(caller: %pS)\n",
> -			(void *)_RET_IP_);
> +		if (ret == -ETIMEDOUT)
> +			dev_err(dev, "Mbox timedout in resp(caller: %pS)\n",
> +				(void *)_RET_IP_);
> +	}
>   
>   	/*
>   	 * NOTE: we might prefer not to need the mailbox ticker to manage the
> @@ -3262,6 +3273,82 @@ static int tisci_reboot_handler(struct sys_off_data *data)
>   	return NOTIFY_BAD;
>   }
>   
> +/*
> + * Enter Partial-IO, which disables everything including DDR with only a small
> + * logic being active for wakeup.
> + */
> +static int tisci_enter_partial_io(struct ti_sci_info *info)
> +{
> +	struct ti_sci_msg_req_prepare_sleep *req;
> +	struct ti_sci_xfer *xfer;
> +	struct device *dev = info->dev;
> +	int ret = 0;
> +
> +	xfer = ti_sci_get_one_xfer(info, TI_SCI_MSG_PREPARE_SLEEP,
> +				   TI_SCI_FLAG_REQ_GENERIC_NORESPONSE,
> +				   sizeof(*req), sizeof(struct ti_sci_msg_hdr));
> +	if (IS_ERR(xfer)) {
> +		ret = PTR_ERR(xfer);
> +		dev_err(dev, "Message alloc failed(%d)\n", ret);
> +		return ret;
> +	}
> +
> +	req = (struct ti_sci_msg_req_prepare_sleep *)xfer->xfer_buf;
> +	req->mode = TISCI_MSG_VALUE_SLEEP_MODE_PARTIAL_IO;
> +	req->ctx_lo = 0;
> +	req->ctx_hi = 0;
> +	req->debug_flags = 0;
> +
> +	ret = ti_sci_do_xfer(info, xfer);
> +	if (ret) {
> +		dev_err(dev, "Mbox send fail %d\n", ret);
> +		goto fail;

Seems unneeded here.

> +	}
> +
> +fail:
> +	ti_sci_put_one_xfer(&info->minfo, xfer);

Is this safe? If we didn't wait for the transfer to complete,
the rx buffer might still be in use.

> +
> +	return ret;
> +}
> +
> +static int tisci_sys_off_handler(struct sys_off_data *data)
> +{
> +	struct ti_sci_info *info = data->cb_data;
> +	int i;
> +	int ret;
> +	bool enter_partial_io = false;
> +
> +	for (i = 0; i != info->nr_wakeup_sources; ++i) {
> +		struct platform_device *pdev =
> +			of_find_device_by_node(info->wakeup_source_nodes[i]);
> +
> +		if (!pdev)
> +			continue;
> +
> +		if (device_may_wakeup(&pdev->dev)) {
> +			dev_dbg(info->dev, "%pOFp identified as wakeup source\n",
> +				info->wakeup_source_nodes[i]);
> +			enter_partial_io = true;
> +		}
> +	}
> +
> +	if (!enter_partial_io)
> +		return NOTIFY_DONE;
> +
> +	ret = tisci_enter_partial_io(info);
> +

extra newline

> +	if (ret) {
> +		dev_err(info->dev,
> +			"Failed to enter Partial-IO %pe, trying to do an emergency restart\n",
> +			ERR_PTR(ret));
> +		emergency_restart();
> +	}
> +
> +	while (1);

Why? If we fail to turn off here, we should try other
methods, not lock the system.

> +
> +	return NOTIFY_DONE;
> +}
> +
>   /* Description for K2G */
>   static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = {
>   	.default_host_id = 2,
> @@ -3398,6 +3485,35 @@ static int ti_sci_probe(struct platform_device *pdev)
>   		goto out;
>   	}
>   
> +	if (of_property_read_bool(dev->of_node, "ti,partial-io-wakeup-sources")) {
> +		info->nr_wakeup_sources =
> +			of_count_phandle_with_args(dev->of_node,
> +						   "ti,partial-io-wakeup-sources",
> +						   NULL);
> +		info->wakeup_source_nodes =
> +			devm_kzalloc(dev, sizeof(*info->wakeup_source_nodes),
> +				     GFP_KERNEL);
> +
> +		for (i = 0; i != info->nr_wakeup_sources; ++i) {
> +			struct device_node *devnode =
> +				of_parse_phandle(dev->of_node,
> +						 "ti,partial-io-wakeup-sources",
> +						 i);
> +			info->wakeup_source_nodes[i] = devnode;
> +		}
> +
> +		ret = devm_register_sys_off_handler(dev,
> +						    SYS_OFF_MODE_POWER_OFF,
> +						    SYS_OFF_PRIO_FIRMWARE,

So this will be done instead of PSCI sys-off? Maybe a better question,
why is this all not done in PSCI off handler in TF-A?

> +						    tisci_sys_off_handler,
> +						    info);
> +		if (ret) {
> +			dev_err(dev, "Failed to register sys_off_handler %pe\n",
> +				ERR_PTR(ret));
> +			goto out;
> +		}
> +	}
> +
>   	dev_info(dev, "ABI: %d.%d (firmware rev 0x%04x '%s')\n",
>   		 info->handle.version.abi_major, info->handle.version.abi_minor,
>   		 info->handle.version.firmware_revision,
> @@ -3407,7 +3523,13 @@ static int ti_sci_probe(struct platform_device *pdev)
>   	list_add_tail(&info->node, &ti_sci_list);
>   	mutex_unlock(&ti_sci_list_mutex);
>   
> -	return of_platform_populate(dev->of_node, NULL, NULL, dev);
> +	ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
> +	if (ret) {
> +		dev_err(dev, "platform_populate failed %pe\n", ERR_PTR(ret));
> +		goto out;
> +	}

Need newline here. Also this change is fine, but seems unrelated and
might need to go in another patch.

Andrew

> +	return 0;
> +
>   out:
>   	if (!IS_ERR(info->chan_tx))
>   		mbox_free_channel(info->chan_tx);
> diff --git a/drivers/firmware/ti_sci.h b/drivers/firmware/ti_sci.h
> index 5846c60220f5..ec8e6bb1791a 100644
> --- a/drivers/firmware/ti_sci.h
> +++ b/drivers/firmware/ti_sci.h
> @@ -35,6 +35,9 @@
>   #define TI_SCI_MSG_QUERY_CLOCK_FREQ	0x010d
>   #define TI_SCI_MSG_GET_CLOCK_FREQ	0x010e
>   
> +/* Low Power Mode Requests */
> +#define TI_SCI_MSG_PREPARE_SLEEP	0x0300
> +
>   /* Resource Management Requests */
>   #define TI_SCI_MSG_GET_RESOURCE_RANGE	0x1500
>   
> @@ -545,6 +548,37 @@ struct ti_sci_msg_resp_get_clock_freq {
>   	u64 freq_hz;
>   } __packed;
>   
> +#define TISCI_MSG_VALUE_SLEEP_MODE_DEEP_SLEEP				0x0
> +#define TISCI_MSG_VALUE_SLEEP_MODE_MCU_ONLY				0x1
> +#define TISCI_MSG_VALUE_SLEEP_MODE_STANDBY				0x2
> +/*
> + * When sending perpare_sleep with MODE_PARTIAL_IO no response will be sent,
> + * no further steps are required. */
> +#define TISCI_MSG_VALUE_SLEEP_MODE_PARTIAL_IO				0x3
> +
> +/**
> + * struct tisci_msg_req_prepare_sleep - Request for TISCI_MSG_PREPARE_SLEEP.
> + *
> + * @hdr				TISCI header to provide ACK/NAK flags to the host.
> + * @mode			Low power mode to enter.
> + * @ctx_lo			Low 32-bits of physical pointer to address to use for context save.
> + * @ctx_hi			High 32-bits of physical pointer to address to use for context save.
> + * @debug_flags			Flags that can be set to halt the sequence during suspend or
> + *				resume to allow JTAG connection and debug.
> + *
> + * This message is used as the first step of entering a low power mode. It
> + * allows configurable information, including which state to enter to be
> + * easily shared from the application, as this is a non-secure message and
> + * therefore can be sent by anyone.
> + */
> +struct ti_sci_msg_req_prepare_sleep {
> +	struct ti_sci_msg_hdr	hdr;
> +	u8			mode;
> +	u32			ctx_lo;
> +	u32			ctx_hi;
> +	u32			debug_flags;
> +} __packed;
> +
>   #define TI_SCI_IRQ_SECONDARY_HOST_INVALID	0xff
>   
>   /**

Re: [PATCH v2 2/6] firmware: ti_sci: Partial-IO support

[Nishanth Menon]

On 10:12-20240730, Andrew Davis wrote:
[...]

> > +	if (response_expected) {
> 
> If a response is not expected why not simply return above and not add even more
> indention here? Also, in that case, is the call to mbox_client_txdone() needed?

Unless I am mistaken, if you ignore the actual shutdown usage, the
mbox_client_txdone will need to be invoked for the tx_tick to be
invoked for the next message in the queue to be submitted


-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v2 2/6] firmware: ti_sci: Partial-IO support

[Krzysztof Kozlowski]

On 29/07/2024 10:00, Markus Schneider-Pargmann wrote:
> +static int tisci_sys_off_handler(struct sys_off_data *data)
> +{
> +	struct ti_sci_info *info = data->cb_data;
> +	int i;
> +	int ret;
> +	bool enter_partial_io = false;
> +
> +	for (i = 0; i != info->nr_wakeup_sources; ++i) {
> +		struct platform_device *pdev =
> +			of_find_device_by_node(info->wakeup_source_nodes[i]);
> +
> +		if (!pdev)
> +			continue;
> +
> +		if (device_may_wakeup(&pdev->dev)) {

...

> +			dev_dbg(info->dev, "%pOFp identified as wakeup source\n",
> +				info->wakeup_source_nodes[i]);
> +			enter_partial_io = true;
> +		}
> +	}
> +
> +	if (!enter_partial_io)
> +		return NOTIFY_DONE;
> +
> +	ret = tisci_enter_partial_io(info);
> +
> +	if (ret) {
> +		dev_err(info->dev,
> +			"Failed to enter Partial-IO %pe, trying to do an emergency restart\n",
> +			ERR_PTR(ret));
> +		emergency_restart();
> +	}
> +
> +	while (1);
> +
> +	return NOTIFY_DONE;
> +}
> +
>  /* Description for K2G */
>  static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = {
>  	.default_host_id = 2,
> @@ -3398,6 +3485,35 @@ static int ti_sci_probe(struct platform_device *pdev)
>  		goto out;
>  	}
>  
> +	if (of_property_read_bool(dev->of_node, "ti,partial-io-wakeup-sources")) {
> +		info->nr_wakeup_sources =
> +			of_count_phandle_with_args(dev->of_node,
> +						   "ti,partial-io-wakeup-sources",
> +						   NULL);

I don't see the point of this. You have quite a static list of devices -
just look at your DTS. Then you don't even do anything useful with the
phandles you got here.

This property looks entirely useless. I already commented on the binding
(which you did not respond to), so let's comment also here:

No, it duplicates wakeup-source and your code shows that it is not even
needed.

Best regards,
Krzysztof

Re: [PATCH v2 2/6] firmware: ti_sci: Partial-IO support

[Markus Schneider-Pargmann]

On Tue, Aug 06, 2024 at 08:26:00AM GMT, Krzysztof Kozlowski wrote:
> On 29/07/2024 10:00, Markus Schneider-Pargmann wrote:
> > +static int tisci_sys_off_handler(struct sys_off_data *data)
> > +{
> > +	struct ti_sci_info *info = data->cb_data;
> > +	int i;
> > +	int ret;
> > +	bool enter_partial_io = false;
> > +
> > +	for (i = 0; i != info->nr_wakeup_sources; ++i) {
> > +		struct platform_device *pdev =
> > +			of_find_device_by_node(info->wakeup_source_nodes[i]);
> > +
> > +		if (!pdev)
> > +			continue;
> > +
> > +		if (device_may_wakeup(&pdev->dev)) {
> 
> ...
> 
> > +			dev_dbg(info->dev, "%pOFp identified as wakeup source\n",
> > +				info->wakeup_source_nodes[i]);
> > +			enter_partial_io = true;
> > +		}
> > +	}
> > +
> > +	if (!enter_partial_io)
> > +		return NOTIFY_DONE;
> > +
> > +	ret = tisci_enter_partial_io(info);
> > +
> > +	if (ret) {
> > +		dev_err(info->dev,
> > +			"Failed to enter Partial-IO %pe, trying to do an emergency restart\n",
> > +			ERR_PTR(ret));
> > +		emergency_restart();
> > +	}
> > +
> > +	while (1);
> > +
> > +	return NOTIFY_DONE;
> > +}
> > +
> >  /* Description for K2G */
> >  static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = {
> >  	.default_host_id = 2,
> > @@ -3398,6 +3485,35 @@ static int ti_sci_probe(struct platform_device *pdev)
> >  		goto out;
> >  	}
> >  
> > +	if (of_property_read_bool(dev->of_node, "ti,partial-io-wakeup-sources")) {
> > +		info->nr_wakeup_sources =
> > +			of_count_phandle_with_args(dev->of_node,
> > +						   "ti,partial-io-wakeup-sources",
> > +						   NULL);
> 
> I don't see the point of this. You have quite a static list of devices -
> just look at your DTS. Then you don't even do anything useful with the
> phandles you got here.

I am gathering the list of phandles in probe.

Then during shutdown I am resolving the phandles to devices if there
are associated devices and check if wakeup is enabled for these devices.
If wakeup is enabled for one of the devices in the list, I put the
SoC into Partial-IO instead of a normal poweroff. This way the
devices which have wakeup enabled can actually wakeup the SoC as
requested by the user by setting wakeup enable.

Best
Markus

> 
> This property looks entirely useless. I already commented on the binding
> (which you did not respond to), so let's comment also here:
> 
> No, it duplicates wakeup-source and your code shows that it is not even
> needed.
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 2/6] firmware: ti_sci: Partial-IO support

[Krzysztof Kozlowski]

On 06/08/2024 09:19, Markus Schneider-Pargmann wrote:
> On Tue, Aug 06, 2024 at 08:26:00AM GMT, Krzysztof Kozlowski wrote:
>> On 29/07/2024 10:00, Markus Schneider-Pargmann wrote:
>>> +static int tisci_sys_off_handler(struct sys_off_data *data)
>>> +{
>>> +	struct ti_sci_info *info = data->cb_data;
>>> +	int i;
>>> +	int ret;
>>> +	bool enter_partial_io = false;
>>> +
>>> +	for (i = 0; i != info->nr_wakeup_sources; ++i) {
>>> +		struct platform_device *pdev =
>>> +			of_find_device_by_node(info->wakeup_source_nodes[i]);
>>> +
>>> +		if (!pdev)
>>> +			continue;
>>> +
>>> +		if (device_may_wakeup(&pdev->dev)) {
>>
>> ...
>>
>>> +			dev_dbg(info->dev, "%pOFp identified as wakeup source\n",
>>> +				info->wakeup_source_nodes[i]);
>>> +			enter_partial_io = true;
>>> +		}
>>> +	}
>>> +
>>> +	if (!enter_partial_io)
>>> +		return NOTIFY_DONE;
>>> +
>>> +	ret = tisci_enter_partial_io(info);
>>> +
>>> +	if (ret) {
>>> +		dev_err(info->dev,
>>> +			"Failed to enter Partial-IO %pe, trying to do an emergency restart\n",
>>> +			ERR_PTR(ret));
>>> +		emergency_restart();
>>> +	}
>>> +
>>> +	while (1);
>>> +
>>> +	return NOTIFY_DONE;
>>> +}
>>> +
>>>  /* Description for K2G */
>>>  static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = {
>>>  	.default_host_id = 2,
>>> @@ -3398,6 +3485,35 @@ static int ti_sci_probe(struct platform_device *pdev)
>>>  		goto out;
>>>  	}
>>>  
>>> +	if (of_property_read_bool(dev->of_node, "ti,partial-io-wakeup-sources")) {
>>> +		info->nr_wakeup_sources =
>>> +			of_count_phandle_with_args(dev->of_node,
>>> +						   "ti,partial-io-wakeup-sources",
>>> +						   NULL);
>>
>> I don't see the point of this. You have quite a static list of devices -
>> just look at your DTS. Then you don't even do anything useful with the
>> phandles you got here.
> 
> I am gathering the list of phandles in probe.

I know what the code is doing.

> 
> Then during shutdown I am resolving the phandles to devices if there
> are associated devices and check if wakeup is enabled for these devices.

I can read the code.

> If wakeup is enabled for one of the devices in the list, I put the
> SoC into Partial-IO instead of a normal poweroff. This way the
> devices which have wakeup enabled can actually wakeup the SoC as
> requested by the user by setting wakeup enable.

I see all this. I said there is no point in doing this. Instead of
repeating the code, you can say what is the point of doing it.

I said once, so repeat one more time - you have *static* list of
devices, therefore any DTS is meaningless. It is just fixed, not flexible.

The code here is still not doing anything useful with the phandles. By
useful I mean - actually enable the wakeup. No, the property is totally
misplaced just to satisfy your code. That's a "no".

Best regards,
Krzysztof

[PATCH v2 3/6] arm64: dts: ti: k3-pinctrl: Add WKUP_EN flag

[Markus Schneider-Pargmann]

WKUP_EN is a flag to enable pin wakeup. Any activity will wakeup the SoC
in that case.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 arch/arm64/boot/dts/ti/k3-pinctrl.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-pinctrl.h b/arch/arm64/boot/dts/ti/k3-pinctrl.h
index 22b8d73cfd32..dd4d53e8420a 100644
--- a/arch/arm64/boot/dts/ti/k3-pinctrl.h
+++ b/arch/arm64/boot/dts/ti/k3-pinctrl.h
@@ -12,6 +12,7 @@
 #define PULLTYPESEL_SHIFT	(17)
 #define RXACTIVE_SHIFT		(18)
 #define DEBOUNCE_SHIFT		(11)
+#define WKUP_EN_SHIFT		(29)
 
 #define PULL_DISABLE		(1 << PULLUDEN_SHIFT)
 #define PULL_ENABLE		(0 << PULLUDEN_SHIFT)
@@ -38,6 +39,8 @@
 #define PIN_DEBOUNCE_CONF5	(5 << DEBOUNCE_SHIFT)
 #define PIN_DEBOUNCE_CONF6	(6 << DEBOUNCE_SHIFT)
 
+#define WKUP_EN			(1 << WKUP_EN_SHIFT)
+
 /* Default mux configuration for gpio-ranges to use with pinctrl */
 #define PIN_GPIO_RANGE_IOPAD	(PIN_INPUT | 7)
 
-- 
2.45.2

Re: [PATCH v2 3/6] arm64: dts: ti: k3-pinctrl: Add WKUP_EN flag

[Nishanth Menon]

On 10:00-20240729, Markus Schneider-Pargmann wrote:
> WKUP_EN is a flag to enable pin wakeup. Any activity will wakeup the SoC
> in that case.
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---
>  arch/arm64/boot/dts/ti/k3-pinctrl.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-pinctrl.h b/arch/arm64/boot/dts/ti/k3-pinctrl.h
> index 22b8d73cfd32..dd4d53e8420a 100644
> --- a/arch/arm64/boot/dts/ti/k3-pinctrl.h
> +++ b/arch/arm64/boot/dts/ti/k3-pinctrl.h
> @@ -12,6 +12,7 @@
>  #define PULLTYPESEL_SHIFT	(17)
>  #define RXACTIVE_SHIFT		(18)
>  #define DEBOUNCE_SHIFT		(11)
> +#define WKUP_EN_SHIFT		(29)
>  
>  #define PULL_DISABLE		(1 << PULLUDEN_SHIFT)
>  #define PULL_ENABLE		(0 << PULLUDEN_SHIFT)
> @@ -38,6 +39,8 @@
>  #define PIN_DEBOUNCE_CONF5	(5 << DEBOUNCE_SHIFT)
>  #define PIN_DEBOUNCE_CONF6	(6 << DEBOUNCE_SHIFT)
>  
> +#define WKUP_EN			(1 << WKUP_EN_SHIFT)
> +

Are we using this?

>  /* Default mux configuration for gpio-ranges to use with pinctrl */
>  #define PIN_GPIO_RANGE_IOPAD	(PIN_INPUT | 7)
>  
> -- 
> 2.45.2
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v2 3/6] arm64: dts: ti: k3-pinctrl: Add WKUP_EN flag

[Markus Schneider-Pargmann]

Hi Nishanth,

On Tue, Jul 30, 2024 at 07:09:58AM GMT, Nishanth Menon wrote:
> On 10:00-20240729, Markus Schneider-Pargmann wrote:
> > WKUP_EN is a flag to enable pin wakeup. Any activity will wakeup the SoC
> > in that case.
> > 
> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > ---
> >  arch/arm64/boot/dts/ti/k3-pinctrl.h | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/ti/k3-pinctrl.h b/arch/arm64/boot/dts/ti/k3-pinctrl.h
> > index 22b8d73cfd32..dd4d53e8420a 100644
> > --- a/arch/arm64/boot/dts/ti/k3-pinctrl.h
> > +++ b/arch/arm64/boot/dts/ti/k3-pinctrl.h
> > @@ -12,6 +12,7 @@
> >  #define PULLTYPESEL_SHIFT	(17)
> >  #define RXACTIVE_SHIFT		(18)
> >  #define DEBOUNCE_SHIFT		(11)
> > +#define WKUP_EN_SHIFT		(29)
> >  
> >  #define PULL_DISABLE		(1 << PULLUDEN_SHIFT)
> >  #define PULL_ENABLE		(0 << PULLUDEN_SHIFT)
> > @@ -38,6 +39,8 @@
> >  #define PIN_DEBOUNCE_CONF5	(5 << DEBOUNCE_SHIFT)
> >  #define PIN_DEBOUNCE_CONF6	(6 << DEBOUNCE_SHIFT)
> >  
> > +#define WKUP_EN			(1 << WKUP_EN_SHIFT)
> > +
> 
> Are we using this?

Yes, this needs to be used in pinctrl for the relevant pins. The users
are not part of this series, as it will probably be in devicetree
overlays or maybe future board files.

This is an example configuration for mcu_mcan0 that is required to
enable support for wakeup from Partial-IO:

  &mcu_pmx0 {
    mcu_mcan0_tx_pins_default: mcu-mcan0-tx-pins-default {
      pinctrl-single,pins = <
        AM62X_IOPAD(0x034, PIN_OUTPUT, 0) /* (D6) MCU_MCAN0_TX */
      >;
    };

    mcu_mcan0_rx_pins_default: mcu-mcan0-rx-pins-default {
      pinctrl-single,pins = <
        AM62X_IOPAD(0x038, PIN_INPUT, 0) /* (B3) MCU_MCAN0_RX */
      >;
    };

    mcu_mcan0_rx_pins_wakeup: mcu-mcan0-rx-pins-wakeup {
      pinctrl-single,pins = <
        AM62X_IOPAD(0x038, PIN_INPUT | WKUP_EN, 0) /* (B3) MCU_MCAN0_RX */
      >;
    };
  };

  &mcu_mcan0 {
    pinctrl-names = "default", "wakeup";
    pinctrl-0 = <&mcu_mcan0_tx_pins_default>, <&mcu_mcan0_rx_pins_default>;
    pinctrl-1 = <&mcu_mcan0_tx_pins_default>, <&mcu_mcan0_rx_pins_wakeup>;
    status = "okay";
  };

Best
Markus

Re: [PATCH v2 3/6] arm64: dts: ti: k3-pinctrl: Add WKUP_EN flag

[Nishanth Menon]

On 14:32-20240730, Markus Schneider-Pargmann wrote:
> Hi Nishanth,
> 
> On Tue, Jul 30, 2024 at 07:09:58AM GMT, Nishanth Menon wrote:
> > On 10:00-20240729, Markus Schneider-Pargmann wrote:
> > > WKUP_EN is a flag to enable pin wakeup. Any activity will wakeup the SoC
> > > in that case.
> > > 
> > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > > ---
> > >  arch/arm64/boot/dts/ti/k3-pinctrl.h | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/ti/k3-pinctrl.h b/arch/arm64/boot/dts/ti/k3-pinctrl.h
> > > index 22b8d73cfd32..dd4d53e8420a 100644
> > > --- a/arch/arm64/boot/dts/ti/k3-pinctrl.h
> > > +++ b/arch/arm64/boot/dts/ti/k3-pinctrl.h
> > > @@ -12,6 +12,7 @@
> > >  #define PULLTYPESEL_SHIFT	(17)
> > >  #define RXACTIVE_SHIFT		(18)
> > >  #define DEBOUNCE_SHIFT		(11)
> > > +#define WKUP_EN_SHIFT		(29)
> > >  
> > >  #define PULL_DISABLE		(1 << PULLUDEN_SHIFT)
> > >  #define PULL_ENABLE		(0 << PULLUDEN_SHIFT)
> > > @@ -38,6 +39,8 @@
> > >  #define PIN_DEBOUNCE_CONF5	(5 << DEBOUNCE_SHIFT)
> > >  #define PIN_DEBOUNCE_CONF6	(6 << DEBOUNCE_SHIFT)
> > >  
> > > +#define WKUP_EN			(1 << WKUP_EN_SHIFT)
> > > +
> > 
> > Are we using this?
> 
> Yes, this needs to be used in pinctrl for the relevant pins. The users
> are not part of this series, as it will probably be in devicetree
> overlays or maybe future board files.
> 
> This is an example configuration for mcu_mcan0 that is required to
> enable support for wakeup from Partial-IO:
> 
>   &mcu_pmx0 {
>     mcu_mcan0_tx_pins_default: mcu-mcan0-tx-pins-default {
>       pinctrl-single,pins = <
>         AM62X_IOPAD(0x034, PIN_OUTPUT, 0) /* (D6) MCU_MCAN0_TX */
>       >;
>     };
> 
>     mcu_mcan0_rx_pins_default: mcu-mcan0-rx-pins-default {
>       pinctrl-single,pins = <
>         AM62X_IOPAD(0x038, PIN_INPUT, 0) /* (B3) MCU_MCAN0_RX */
>       >;
>     };
> 
>     mcu_mcan0_rx_pins_wakeup: mcu-mcan0-rx-pins-wakeup {
>       pinctrl-single,pins = <
>         AM62X_IOPAD(0x038, PIN_INPUT | WKUP_EN, 0) /* (B3) MCU_MCAN0_RX */
>       >;
>     };
>   };
> 
>   &mcu_mcan0 {
>     pinctrl-names = "default", "wakeup";
>     pinctrl-0 = <&mcu_mcan0_tx_pins_default>, <&mcu_mcan0_rx_pins_default>;
>     pinctrl-1 = <&mcu_mcan0_tx_pins_default>, <&mcu_mcan0_rx_pins_wakeup>;
>     status = "okay";
>   };


Please introduce at least 1 user in the series? or if there are
dependencies, then we can hold back this patch till the right users are
available.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v2 3/6] arm64: dts: ti: k3-pinctrl: Add WKUP_EN flag

[Krzysztof Kozlowski]

On 30/07/2024 14:32, Markus Schneider-Pargmann wrote:
> Hi Nishanth,
> 
> On Tue, Jul 30, 2024 at 07:09:58AM GMT, Nishanth Menon wrote:
>> On 10:00-20240729, Markus Schneider-Pargmann wrote:
>>> WKUP_EN is a flag to enable pin wakeup. Any activity will wakeup the SoC
>>> in that case.
>>>
>>> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
>>> ---
>>>  arch/arm64/boot/dts/ti/k3-pinctrl.h | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/ti/k3-pinctrl.h b/arch/arm64/boot/dts/ti/k3-pinctrl.h
>>> index 22b8d73cfd32..dd4d53e8420a 100644
>>> --- a/arch/arm64/boot/dts/ti/k3-pinctrl.h
>>> +++ b/arch/arm64/boot/dts/ti/k3-pinctrl.h
>>> @@ -12,6 +12,7 @@
>>>  #define PULLTYPESEL_SHIFT	(17)
>>>  #define RXACTIVE_SHIFT		(18)
>>>  #define DEBOUNCE_SHIFT		(11)
>>> +#define WKUP_EN_SHIFT		(29)
>>>  
>>>  #define PULL_DISABLE		(1 << PULLUDEN_SHIFT)
>>>  #define PULL_ENABLE		(0 << PULLUDEN_SHIFT)
>>> @@ -38,6 +39,8 @@
>>>  #define PIN_DEBOUNCE_CONF5	(5 << DEBOUNCE_SHIFT)
>>>  #define PIN_DEBOUNCE_CONF6	(6 << DEBOUNCE_SHIFT)
>>>  
>>> +#define WKUP_EN			(1 << WKUP_EN_SHIFT)
>>> +
>>
>> Are we using this?
> 
> Yes, this needs to be used in pinctrl for the relevant pins. The users
> are not part of this series, as it will probably be in devicetree
> overlays or maybe future board files.

You cannot add some code without users claiming that "someone, someday
might need it". It's just dead code.

Best regards,
Krzysztof

[PATCH v2 4/6] arm64: dts: ti: k3-am62: Add partial-io wakeup sources

[Markus Schneider-Pargmann]

In Partial-IO mode there are a number of possible wakeup sources. Add
the list of phandles to these wakeup sources.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 arch/arm64/boot/dts/ti/k3-am62.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am62.dtsi b/arch/arm64/boot/dts/ti/k3-am62.dtsi
index bfb55ca11323..2bae8550c900 100644
--- a/arch/arm64/boot/dts/ti/k3-am62.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62.dtsi
@@ -122,3 +122,7 @@ dss_vp1_clk: clock-divider-oldi {
 #include "k3-am62-main.dtsi"
 #include "k3-am62-mcu.dtsi"
 #include "k3-am62-wakeup.dtsi"
+
+&dmsc {
+	ti,partial-io-wakeup-sources = <&mcu_mcan0>, <&mcu_mcan1>, <&mcu_uart0>, <&wkup_uart0>;
+};
-- 
2.45.2

[PATCH v2 5/6] arm64: dts: ti: k3-am62a: Add partial-io wakeup sources

[Markus Schneider-Pargmann]

In Partial-IO mode there are a number of possible wakeup sources. Add
the list of phandles to these wakeup sources.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 arch/arm64/boot/dts/ti/k3-am62a.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am62a.dtsi b/arch/arm64/boot/dts/ti/k3-am62a.dtsi
index b1b884600293..5c13851c29ec 100644
--- a/arch/arm64/boot/dts/ti/k3-am62a.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62a.dtsi
@@ -123,3 +123,7 @@ cbass_wakeup: bus@b00000 {
 #include "k3-am62a-main.dtsi"
 #include "k3-am62a-mcu.dtsi"
 #include "k3-am62a-wakeup.dtsi"
+
+&dmsc {
+	ti,partial-io-wakeup-sources = <&mcu_mcan0>, <&mcu_mcan1>, <&mcu_uart0>, <&wkup_uart0>;
+};
-- 
2.45.2

[PATCH v2 6/6] arm64: dts: ti: k3-am62p: Add partial-io wakeup sources

[Markus Schneider-Pargmann]

From: Vibhore Vardhan <vibhore@ti.com>

In Partial-IO mode there are a number of possible wakeup sources. Add
the list of phandles to these wakeup sources. Based on the patch for
AM62a.

Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 arch/arm64/boot/dts/ti/k3-am62p.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am62p.dtsi b/arch/arm64/boot/dts/ti/k3-am62p.dtsi
index 75a15c368c11..cd81f6d173f4 100644
--- a/arch/arm64/boot/dts/ti/k3-am62p.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62p.dtsi
@@ -126,3 +126,7 @@ cbass_wakeup: bus@b00000 {
 
 /* Include AM62P specific peripherals */
 #include "k3-am62p-main.dtsi"
+
+&dmsc {
+	ti,partial-io-wakeup-sources = <&mcu_mcan0>, <&mcu_mcan1>, <&mcu_uart0>, <&wkup_uart0>;
+};
-- 
2.45.2

Re: [PATCH v2 0/6] firmware: ti_sci: Partial-IO support

[Krzysztof Kozlowski]

On 29/07/2024 10:00, Markus Schneider-Pargmann wrote:
> Hi,
> 
> v2
> --
> In v2 I fixed the comments on the first version of this series and
> rebased to v6.11-rc1. See below for a more detailed list.

I don't see you even caring to respond to comments, so how do you
imagine comments were fixed?

Best regards,
Krzysztof