| Message ID | 20240729080101.3859701-2-msp@baylibre.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | firmware: ti_sci: Partial-IO support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
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
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
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
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 >
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
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
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
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
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
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
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: |
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(+)