| Message ID | 20190730125157.884-7-andrew.murray@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: etm4x: save/restore ETMv4 context across CPU low power states | expand |
Hi Andrew, On 30/07/2019 13:51, Andrew Murray wrote: > Some coresight components, because of choices made during hardware > integration, require their state to be saved and restored across CPU low > power states. > > The software has no reliable method of detecting when save/restore is > required thus let's add a binding to inform the kernel. > > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > --- > Documentation/devicetree/bindings/arm/coresight.txt | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt > index fcc3bacfd8bc..7cbdb7893af8 100644 > --- a/Documentation/devicetree/bindings/arm/coresight.txt > +++ b/Documentation/devicetree/bindings/arm/coresight.txt > @@ -92,6 +92,9 @@ its hardware characteristcs. > * arm,cp14: must be present if the system accesses ETM/PTM management > registers via co-processor 14. > > + * arm,coresight-needs-save-restore: boolean. Indicates that software > + should save/restore state across power down. > + Do you think we could be a bit more descriptive here about when people could add it to the DT ? Here we don't mention when someone should use this property and it may be added to platforms where it may be absolutely unnecessary. How about : "Indicates that the hardware implementation may not honor the Powerup request from the software and thus might loose the register context on CPU power down (e.g, during CPUIdle). Software must save/restore the context during a CPU power transition cycle." Cheers Suzuki
On Fri, Aug 02, 2019 at 11:40:54AM +0100, Suzuki K Poulose wrote: > Hi Andrew, > > On 30/07/2019 13:51, Andrew Murray wrote: > > Some coresight components, because of choices made during hardware > > integration, require their state to be saved and restored across CPU low > > power states. > > > > The software has no reliable method of detecting when save/restore is > > required thus let's add a binding to inform the kernel. > > > > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > > --- > > Documentation/devicetree/bindings/arm/coresight.txt | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt > > index fcc3bacfd8bc..7cbdb7893af8 100644 > > --- a/Documentation/devicetree/bindings/arm/coresight.txt > > +++ b/Documentation/devicetree/bindings/arm/coresight.txt > > @@ -92,6 +92,9 @@ its hardware characteristcs. > > * arm,cp14: must be present if the system accesses ETM/PTM management > > registers via co-processor 14. > > + * arm,coresight-needs-save-restore: boolean. Indicates that software > > + should save/restore state across power down. > > + > > Do you think we could be a bit more descriptive here about when people could add > it to the DT ? Here we don't mention when someone should use this property and > it may be added to platforms where it may be absolutely unnecessary. How about : > > "Indicates that the hardware implementation may not honor the Powerup request > from the software and thus might loose the register context on CPU power > down (e.g, during CPUIdle). Software must save/restore the context during a > CPU power transition cycle." How about the following: "Indicates that the hardware will loose register context on CPU power down (e.g. CPUIdle), despite the TRCPDCR.PU bit being set." I'm keen to avoid making suggestions about what the kernel will do when it sees this flag and thus prefer to focus on describing what the hardware does. So I dropped your last sentence. However the name of the flag still implies policy which I don't like. I also changed the 'may not honor' wording, I'm not sure if this is really the case or if the spec is open to interpretation. It would great for this wording to also apply to other CS components though I haven't investigated if these have a PU bit or something different. Thanks, Andrew Murray > > Cheers > Suzuki
On Fri, 2 Aug 2019 at 08:37, Andrew Murray <andrew.murray@arm.com> wrote: > > On Fri, Aug 02, 2019 at 11:40:54AM +0100, Suzuki K Poulose wrote: > > Hi Andrew, > > > > On 30/07/2019 13:51, Andrew Murray wrote: > > > Some coresight components, because of choices made during hardware > > > integration, require their state to be saved and restored across CPU low > > > power states. > > > > > > The software has no reliable method of detecting when save/restore is > > > required thus let's add a binding to inform the kernel. > > > > > > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > > > --- > > > Documentation/devicetree/bindings/arm/coresight.txt | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt > > > index fcc3bacfd8bc..7cbdb7893af8 100644 > > > --- a/Documentation/devicetree/bindings/arm/coresight.txt > > > +++ b/Documentation/devicetree/bindings/arm/coresight.txt > > > @@ -92,6 +92,9 @@ its hardware characteristcs. > > > * arm,cp14: must be present if the system accesses ETM/PTM management > > > registers via co-processor 14. > > > + * arm,coresight-needs-save-restore: boolean. Indicates that software > > > + should save/restore state across power down. > > > + > > > > Do you think we could be a bit more descriptive here about when people could add > > it to the DT ? Here we don't mention when someone should use this property and > > it may be added to platforms where it may be absolutely unnecessary. How about : > > > > "Indicates that the hardware implementation may not honor the Powerup request > > from the software and thus might loose the register context on CPU power > > down (e.g, during CPUIdle). Software must save/restore the context during a > > CPU power transition cycle." > > How about the following: > > "Indicates that the hardware will loose register context on CPU power down (e.g. > CPUIdle), despite the TRCPDCR.PU bit being set." > > I'm keen to avoid making suggestions about what the kernel will do when it sees > this flag and thus prefer to focus on describing what the hardware does. So I > dropped your last sentence. However the name of the flag still implies policy > which I don't like. > > I also changed the 'may not honor' wording, I'm not sure if this is really the > case or if the spec is open to interpretation. > > It would great for this wording to also apply to other CS components though I > haven't investigated if these have a PU bit or something different. Exactly - the definition needs to be broad enough to apply to other CS components. Mike what do you think would be appropriate for CTIs? > > Thanks, > > Andrew Murray > > > > > Cheers > > Suzuki
On Sun, Aug 04, 2019 at 07:13:45AM -0600, Mathieu Poirier wrote: > On Fri, 2 Aug 2019 at 08:37, Andrew Murray <andrew.murray@arm.com> wrote: > > > > On Fri, Aug 02, 2019 at 11:40:54AM +0100, Suzuki K Poulose wrote: > > > Hi Andrew, > > > > > > On 30/07/2019 13:51, Andrew Murray wrote: > > > > Some coresight components, because of choices made during hardware > > > > integration, require their state to be saved and restored across CPU low > > > > power states. > > > > > > > > The software has no reliable method of detecting when save/restore is > > > > required thus let's add a binding to inform the kernel. > > > > > > > > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > > > > --- > > > > Documentation/devicetree/bindings/arm/coresight.txt | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt > > > > index fcc3bacfd8bc..7cbdb7893af8 100644 > > > > --- a/Documentation/devicetree/bindings/arm/coresight.txt > > > > +++ b/Documentation/devicetree/bindings/arm/coresight.txt > > > > @@ -92,6 +92,9 @@ its hardware characteristcs. > > > > * arm,cp14: must be present if the system accesses ETM/PTM management > > > > registers via co-processor 14. > > > > + * arm,coresight-needs-save-restore: boolean. Indicates that software > > > > + should save/restore state across power down. > > > > + > > > > > > Do you think we could be a bit more descriptive here about when people could add > > > it to the DT ? Here we don't mention when someone should use this property and > > > it may be added to platforms where it may be absolutely unnecessary. How about : > > > > > > "Indicates that the hardware implementation may not honor the Powerup request > > > from the software and thus might loose the register context on CPU power > > > down (e.g, during CPUIdle). Software must save/restore the context during a > > > CPU power transition cycle." > > > > How about the following: > > > > "Indicates that the hardware will loose register context on CPU power down (e.g. > > CPUIdle), despite the TRCPDCR.PU bit being set." > > > > I'm keen to avoid making suggestions about what the kernel will do when it sees > > this flag and thus prefer to focus on describing what the hardware does. So I > > dropped your last sentence. However the name of the flag still implies policy > > which I don't like. > > > > I also changed the 'may not honor' wording, I'm not sure if this is really the > > case or if the spec is open to interpretation. > > > > It would great for this wording to also apply to other CS components though I > > haven't investigated if these have a PU bit or something different. > > Exactly - the definition needs to be broad enough to apply to other CS > components. Mike what do you think would be appropriate for CTIs? How about we keep this short and simple: * arm,coresight-loses-context-with-cpu : boolean. Indicates that the hardware will lose register context on CPU power down (e.g. CPUIdle). I could have added something like "... despite TRCPDCR.PU being set", or to apply more generically: "... despite available register controls being set to prevent such context loss". However whilst these are more informative - they elude to some of reasons as to why context is lost and as we cannot be exhaustive I'd rather not give a limited example. However if a longer explaination is required: * arm,coresight-loses-context-with-cpu : boolean. Indicates that the hardware will lose register context on CPU power down (e.g. CPUIdle). An example of where this may be needed are systems which contain a coresight component and CPU in the same power domain. When the CPU powers down the coresight component also powers down and loses its context. Any objections/preference? :) Thanks, Andrew Murray > > > > > Thanks, > > > > Andrew Murray > > > > > > > > Cheers > > > Suzuki
Hi, On Wed, 14 Aug 2019 at 11:01, Andrew Murray <andrew.murray@arm.com> wrote: > > On Sun, Aug 04, 2019 at 07:13:45AM -0600, Mathieu Poirier wrote: > > On Fri, 2 Aug 2019 at 08:37, Andrew Murray <andrew.murray@arm.com> wrote: > > > > > > On Fri, Aug 02, 2019 at 11:40:54AM +0100, Suzuki K Poulose wrote: > > > > Hi Andrew, > > > > > > > > On 30/07/2019 13:51, Andrew Murray wrote: > > > > > Some coresight components, because of choices made during hardware > > > > > integration, require their state to be saved and restored across CPU low > > > > > power states. > > > > > > > > > > The software has no reliable method of detecting when save/restore is > > > > > required thus let's add a binding to inform the kernel. > > > > > > > > > > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > > > > > --- > > > > > Documentation/devicetree/bindings/arm/coresight.txt | 3 +++ > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt > > > > > index fcc3bacfd8bc..7cbdb7893af8 100644 > > > > > --- a/Documentation/devicetree/bindings/arm/coresight.txt > > > > > +++ b/Documentation/devicetree/bindings/arm/coresight.txt > > > > > @@ -92,6 +92,9 @@ its hardware characteristcs. > > > > > * arm,cp14: must be present if the system accesses ETM/PTM management > > > > > registers via co-processor 14. > > > > > + * arm,coresight-needs-save-restore: boolean. Indicates that software > > > > > + should save/restore state across power down. > > > > > + > > > > > > > > Do you think we could be a bit more descriptive here about when people could add > > > > it to the DT ? Here we don't mention when someone should use this property and > > > > it may be added to platforms where it may be absolutely unnecessary. How about : > > > > > > > > "Indicates that the hardware implementation may not honor the Powerup request > > > > from the software and thus might loose the register context on CPU power > > > > down (e.g, during CPUIdle). Software must save/restore the context during a > > > > CPU power transition cycle." > > > > > > How about the following: > > > > > > "Indicates that the hardware will loose register context on CPU power down (e.g. > > > CPUIdle), despite the TRCPDCR.PU bit being set." > > > > > > I'm keen to avoid making suggestions about what the kernel will do when it sees > > > this flag and thus prefer to focus on describing what the hardware does. So I > > > dropped your last sentence. However the name of the flag still implies policy > > > which I don't like. > > > > > > I also changed the 'may not honor' wording, I'm not sure if this is really the > > > case or if the spec is open to interpretation. > > > > > > It would great for this wording to also apply to other CS components though I > > > haven't investigated if these have a PU bit or something different. > > > > Exactly - the definition needs to be broad enough to apply to other CS > > components. Mike what do you think would be appropriate for CTIs? > CTIs have no power control at all - i.e. no PU bit to request we stay up - and reside in the debug power domain. So they are coupled to the CS/CPU/ETM/ power domains and reliant on outside forces to request power. The expectation is that for a PE bound CTI, if debug is powered then it will be fully powered - so an ETM with PU respected, or the external debug logic with DBGNOPWRDWN respected should be sufficient for CTI to stay alive. > How about we keep this short and simple: > > * arm,coresight-loses-context-with-cpu : boolean. Indicates that the hardware > will lose register context on CPU power down (e.g. CPUIdle). > So the above name is generic enough to encompass the CTI as well. > I could have added something like "... despite TRCPDCR.PU being set", or to > apply more generically: "... despite available register controls being set to > prevent such context loss". However whilst these are more informative - they > elude to some of reasons as to why context is lost and as we cannot be > exhaustive I'd rather not give a limited example. > > However if a longer explaination is required: > > * arm,coresight-loses-context-with-cpu : boolean. Indicates that the hardware > will lose register context on CPU power down (e.g. CPUIdle). An example of > where this may be needed are systems which contain a coresight component and > CPU in the same power domain. When the CPU powers down the coresight > component also powers down and loses its context. > > Any objections/preference? :) > Don't really care about length of explanation - but shouldn't mention ETM specific features. Mike > Thanks, > > Andrew Murray > > > > > > > > > Thanks, > > > > > > Andrew Murray > > > > > > > > > > > Cheers > > > > Suzuki
Hi Mike, On 14/08/2019 12:06, Mike Leach wrote: > Hi, > > On Wed, 14 Aug 2019 at 11:01, Andrew Murray <andrew.murray@arm.com> wrote: >> >> On Sun, Aug 04, 2019 at 07:13:45AM -0600, Mathieu Poirier wrote: >>> On Fri, 2 Aug 2019 at 08:37, Andrew Murray <andrew.murray@arm.com> wrote: >>>> >>>> On Fri, Aug 02, 2019 at 11:40:54AM +0100, Suzuki K Poulose wrote: >>>>> Hi Andrew, >>>>> >>>>> On 30/07/2019 13:51, Andrew Murray wrote: >>>>>> Some coresight components, because of choices made during hardware >>>>>> integration, require their state to be saved and restored across CPU low >>>>>> power states. ... >>>>>> --- a/Documentation/devicetree/bindings/arm/coresight.txt >>>>>> +++ b/Documentation/devicetree/bindings/arm/coresight.txt >>>>>> @@ -92,6 +92,9 @@ its hardware characteristcs. >>>>>> * arm,cp14: must be present if the system accesses ETM/PTM management >>>>>> registers via co-processor 14. >>>>>> + * arm,coresight-needs-save-restore: boolean. Indicates that software >>>>>> + should save/restore state across power down. >>>>>> + >>>>> >>>>> Do you think we could be a bit more descriptive here about when people could add >>>>> it to the DT ? Here we don't mention when someone should use this property and >>>>> it may be added to platforms where it may be absolutely unnecessary. How about : >>>>> >>>>> "Indicates that the hardware implementation may not honor the Powerup request >>>>> from the software and thus might loose the register context on CPU power >>>>> down (e.g, during CPUIdle). Software must save/restore the context during a >>>>> CPU power transition cycle." >>>> >>>> How about the following: >>>> >>>> "Indicates that the hardware will loose register context on CPU power down (e.g. >>>> CPUIdle), despite the TRCPDCR.PU bit being set." >>>> >>>> I'm keen to avoid making suggestions about what the kernel will do when it sees >>>> this flag and thus prefer to focus on describing what the hardware does. So I >>>> dropped your last sentence. However the name of the flag still implies policy >>>> which I don't like. >>>> >>>> I also changed the 'may not honor' wording, I'm not sure if this is really the >>>> case or if the spec is open to interpretation. >>>> >>>> It would great for this wording to also apply to other CS components though I >>>> haven't investigated if these have a PU bit or something different. >>> >>> Exactly - the definition needs to be broad enough to apply to other CS >>> components. Mike what do you think would be appropriate for CTIs? >> > CTIs have no power control at all - i.e. no PU bit to request we stay > up - and reside in the debug power domain. So they are coupled to the > CS/CPU/ETM/ power domains and reliant on outside forces to request > power. > The expectation is that for a PE bound CTI, if debug is powered then > it will be fully powered - so an ETM with PU respected, or the > external debug logic with DBGNOPWRDWN respected should be sufficient > for CTI to stay alive. I am trying to understand why we need this property for CTI. Don't we always need to save-restore the CTI controls on a CPU_DOWN for the associated CTI ? Since it may not be really tied to an ETM (e.g, if the CTI is purely used to handle CPU triggers, PMU etc,). If that is the case, do we need this property for CTI at all ? > >> How about we keep this short and simple: >> >> * arm,coresight-loses-context-with-cpu : boolean. Indicates that the hardware nit: s/loses/looses ? >> will lose register context on CPU power down (e.g. CPUIdle). >> > > So the above name is generic enough to encompass the CTI as well. > >> I could have added something like "... despite TRCPDCR.PU being set", or to >> apply more generically: "... despite available register controls being set to >> prevent such context loss". However whilst these are more informative - they >> elude to some of reasons as to why context is lost and as we cannot be >> exhaustive I'd rather not give a limited example. >> >> However if a longer explaination is required: >> >> * arm,coresight-loses-context-with-cpu : boolean. Indicates that the hardware >> will lose register context on CPU power down (e.g. CPUIdle). An example of >> where this may be needed are systems which contain a coresight component and >> CPU in the same power domain. When the CPU powers down the coresight >> component also powers down and loses its context. This looks fine for me. But I am trying to understand the rationale behind using this for CTI Suzuki
On Wed, Aug 14, 2019 at 01:35:27PM +0100, Suzuki K Poulose wrote: > Hi Mike, > > On 14/08/2019 12:06, Mike Leach wrote: > > Hi, > > > > On Wed, 14 Aug 2019 at 11:01, Andrew Murray <andrew.murray@arm.com> wrote: > > > > > > On Sun, Aug 04, 2019 at 07:13:45AM -0600, Mathieu Poirier wrote: > > > > On Fri, 2 Aug 2019 at 08:37, Andrew Murray <andrew.murray@arm.com> wrote: > > > > > > > > > > On Fri, Aug 02, 2019 at 11:40:54AM +0100, Suzuki K Poulose wrote: > > > > > > Hi Andrew, > > > > > > > > > > > > On 30/07/2019 13:51, Andrew Murray wrote: > > > > > > > Some coresight components, because of choices made during hardware > > > > > > > integration, require their state to be saved and restored across CPU low > > > > > > > power states. > > ... > > > > > > > > --- a/Documentation/devicetree/bindings/arm/coresight.txt > > > > > > > +++ b/Documentation/devicetree/bindings/arm/coresight.txt > > > > > > > @@ -92,6 +92,9 @@ its hardware characteristcs. > > > > > > > * arm,cp14: must be present if the system accesses ETM/PTM management > > > > > > > registers via co-processor 14. > > > > > > > + * arm,coresight-needs-save-restore: boolean. Indicates that software > > > > > > > + should save/restore state across power down. > > > > > > > + > > > > > > > > > > > > Do you think we could be a bit more descriptive here about when people could add > > > > > > it to the DT ? Here we don't mention when someone should use this property and > > > > > > it may be added to platforms where it may be absolutely unnecessary. How about : > > > > > > > > > > > > "Indicates that the hardware implementation may not honor the Powerup request > > > > > > from the software and thus might loose the register context on CPU power > > > > > > down (e.g, during CPUIdle). Software must save/restore the context during a > > > > > > CPU power transition cycle." > > > > > > > > > > How about the following: > > > > > > > > > > "Indicates that the hardware will loose register context on CPU power down (e.g. > > > > > CPUIdle), despite the TRCPDCR.PU bit being set." > > > > > > > > > > I'm keen to avoid making suggestions about what the kernel will do when it sees > > > > > this flag and thus prefer to focus on describing what the hardware does. So I > > > > > dropped your last sentence. However the name of the flag still implies policy > > > > > which I don't like. > > > > > > > > > > I also changed the 'may not honor' wording, I'm not sure if this is really the > > > > > case or if the spec is open to interpretation. > > > > > > > > > > It would great for this wording to also apply to other CS components though I > > > > > haven't investigated if these have a PU bit or something different. > > > > > > > > Exactly - the definition needs to be broad enough to apply to other CS > > > > components. Mike what do you think would be appropriate for CTIs? > > > > > CTIs have no power control at all - i.e. no PU bit to request we stay > > up - and reside in the debug power domain. So they are coupled to the > > CS/CPU/ETM/ power domains and reliant on outside forces to request > > power. > > The expectation is that for a PE bound CTI, if debug is powered then > > it will be fully powered - so an ETM with PU respected, or the > > external debug logic with DBGNOPWRDWN respected should be sufficient > > for CTI to stay alive. > > I am trying to understand why we need this property for CTI. > Don't we always need to save-restore the CTI controls on a CPU_DOWN for the > associated CTI ? Since it may not be really tied to an ETM (e.g, if the CTI is > purely used to handle CPU triggers, PMU etc,). If that is the case, do we need > this property for CTI at all ? > > > > > > How about we keep this short and simple: > > > > > > * arm,coresight-loses-context-with-cpu : boolean. Indicates that the hardware > > nit: s/loses/looses ? Given that lose refers to missing something and loose refers to something not fitting well, I'd have thought the pural is loses. Though I've now looked at these words for too long and nothing makes sense any more... > > > > will lose register context on CPU power down (e.g. CPUIdle). > > > > > > > So the above name is generic enough to encompass the CTI as well. > > > > > I could have added something like "... despite TRCPDCR.PU being set", or to > > > apply more generically: "... despite available register controls being set to > > > prevent such context loss". However whilst these are more informative - they > > > elude to some of reasons as to why context is lost and as we cannot be > > > exhaustive I'd rather not give a limited example. > > > > > > However if a longer explaination is required: > > > > > > * arm,coresight-loses-context-with-cpu : boolean. Indicates that the hardware > > > will lose register context on CPU power down (e.g. CPUIdle). An example of > > > where this may be needed are systems which contain a coresight component and > > > CPU in the same power domain. When the CPU powers down the coresight > > > component also powers down and loses its context. > > This looks fine for me. But I am trying to understand the rationale behind > using this for CTI Thanks. Thanks, Andrew Murray > > Suzuki
Hi Suzuki, On Wed, 14 Aug 2019 at 13:35, Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > > Hi Mike, > > On 14/08/2019 12:06, Mike Leach wrote: > > Hi, > > > > On Wed, 14 Aug 2019 at 11:01, Andrew Murray <andrew.murray@arm.com> wrote: > >> > >> On Sun, Aug 04, 2019 at 07:13:45AM -0600, Mathieu Poirier wrote: > >>> On Fri, 2 Aug 2019 at 08:37, Andrew Murray <andrew.murray@arm.com> wrote: > >>>> > >>>> On Fri, Aug 02, 2019 at 11:40:54AM +0100, Suzuki K Poulose wrote: > >>>>> Hi Andrew, > >>>>> > >>>>> On 30/07/2019 13:51, Andrew Murray wrote: > >>>>>> Some coresight components, because of choices made during hardware > >>>>>> integration, require their state to be saved and restored across CPU low > >>>>>> power states. > > ... > > >>>>>> --- a/Documentation/devicetree/bindings/arm/coresight.txt > >>>>>> +++ b/Documentation/devicetree/bindings/arm/coresight.txt > >>>>>> @@ -92,6 +92,9 @@ its hardware characteristcs. > >>>>>> * arm,cp14: must be present if the system accesses ETM/PTM management > >>>>>> registers via co-processor 14. > >>>>>> + * arm,coresight-needs-save-restore: boolean. Indicates that software > >>>>>> + should save/restore state across power down. > >>>>>> + > >>>>> > >>>>> Do you think we could be a bit more descriptive here about when people could add > >>>>> it to the DT ? Here we don't mention when someone should use this property and > >>>>> it may be added to platforms where it may be absolutely unnecessary. How about : > >>>>> > >>>>> "Indicates that the hardware implementation may not honor the Powerup request > >>>>> from the software and thus might loose the register context on CPU power > >>>>> down (e.g, during CPUIdle). Software must save/restore the context during a > >>>>> CPU power transition cycle." > >>>> > >>>> How about the following: > >>>> > >>>> "Indicates that the hardware will loose register context on CPU power down (e.g. > >>>> CPUIdle), despite the TRCPDCR.PU bit being set." > >>>> > >>>> I'm keen to avoid making suggestions about what the kernel will do when it sees > >>>> this flag and thus prefer to focus on describing what the hardware does. So I > >>>> dropped your last sentence. However the name of the flag still implies policy > >>>> which I don't like. > >>>> > >>>> I also changed the 'may not honor' wording, I'm not sure if this is really the > >>>> case or if the spec is open to interpretation. > >>>> > >>>> It would great for this wording to also apply to other CS components though I > >>>> haven't investigated if these have a PU bit or something different. > >>> > >>> Exactly - the definition needs to be broad enough to apply to other CS > >>> components. Mike what do you think would be appropriate for CTIs? > >> > > CTIs have no power control at all - i.e. no PU bit to request we stay > > up - and reside in the debug power domain. So they are coupled to the > > CS/CPU/ETM/ power domains and reliant on outside forces to request > > power. > > The expectation is that for a PE bound CTI, if debug is powered then > > it will be fully powered - so an ETM with PU respected, or the > > external debug logic with DBGNOPWRDWN respected should be sufficient > > for CTI to stay alive. > > I am trying to understand why we need this property for CTI. > Don't we always need to save-restore the CTI controls on a CPU_DOWN for the > associated CTI ? Since it may not be really tied to an ETM (e.g, if the CTI is > purely used to handle CPU triggers, PMU etc,). If that is the case, do we need > this property for CTI at all ? > CTI will be in use for one of two reasons:- 1) External Debug - in which case the DBGNOPOWERDOWN bit should be set and the debug domain remains powered. 2) Trace (self hosted or external) - so we have an ETM and PU is set and the debug power domain remains powered. In these ideal cases we never need to save and restore as the debug power domain remains powered. Now in this phase of development we are disregarding external debug and trace. So we are only in self hosted trace mode - which is probably the most common use case for a linux system. Therefore the CTI will only be in use if there is an ETM tracing self hosted. If PU is not working and the parameter is set then we know we need to hook CPUIdle notifications and save and restore (thought in the case of CTI it is really restore only for self hosted as there are no dynamic registers.). If we are not saving and restoring then we do not need to register for CPUIdle notifiers (which like hotplug need to be centralised, not re-implemented in each and every driver), saving some latency. The architecture specifically precludes using the CTI PMU trigger to the generic CTI PE interrupt - so the PMU overflow trigger will only ever be used to activate some debug event (e.g. debug halt, trace halt etc). Regards Mike > > > >> How about we keep this short and simple: > >> > >> * arm,coresight-loses-context-with-cpu : boolean. Indicates that the hardware > > nit: s/loses/looses ? > > >> will lose register context on CPU power down (e.g. CPUIdle). > >> > > > > So the above name is generic enough to encompass the CTI as well. > > > >> I could have added something like "... despite TRCPDCR.PU being set", or to > >> apply more generically: "... despite available register controls being set to > >> prevent such context loss". However whilst these are more informative - they > >> elude to some of reasons as to why context is lost and as we cannot be > >> exhaustive I'd rather not give a limited example. > >> > >> However if a longer explaination is required: > >> > >> * arm,coresight-loses-context-with-cpu : boolean. Indicates that the hardware > >> will lose register context on CPU power down (e.g. CPUIdle). An example of > >> where this may be needed are systems which contain a coresight component and > >> CPU in the same power domain. When the CPU powers down the coresight > >> component also powers down and loses its context. > > This looks fine for me. But I am trying to understand the rationale behind > using this for CTI > > Suzuki
diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt index fcc3bacfd8bc..7cbdb7893af8 100644 --- a/Documentation/devicetree/bindings/arm/coresight.txt +++ b/Documentation/devicetree/bindings/arm/coresight.txt @@ -92,6 +92,9 @@ its hardware characteristcs. * arm,cp14: must be present if the system accesses ETM/PTM management registers via co-processor 14. + * arm,coresight-needs-save-restore: boolean. Indicates that software + should save/restore state across power down. + * Optional property for TMC: * arm,buffer-size: size of contiguous buffer space for TMC ETR
Some coresight components, because of choices made during hardware integration, require their state to be saved and restored across CPU low power states. The software has no reliable method of detecting when save/restore is required thus let's add a binding to inform the kernel. Signed-off-by: Andrew Murray <andrew.murray@arm.com> --- Documentation/devicetree/bindings/arm/coresight.txt | 3 +++ 1 file changed, 3 insertions(+)