[PATCH] coresight: etm4x: Add support to disable trace unit power up

[PATCH] coresight: etm4x: Add support to disable trace unit power up

[Sai Prakash Ranjan]

From: Tingwei Zhang <tingwei@codeaurora.org>

On some Qualcomm Technologies Inc. SoCs like SC7180, there
exists a hardware errata where the APSS (Application Processor
SubSystem)/CPU watchdog counter is stopped when ETM register
TRCPDCR.PU=1. Since the ETMs share the same power domain as
that of respective CPU cores, they are powered on when the
CPU core is powered on. So we can disable powering up of the
trace unit after checking for this errata via new property
called "qcom,tupwr-disable".

Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
---
 .../devicetree/bindings/arm/coresight.txt     |  6 ++++
 drivers/hwtracing/coresight/coresight-etm4x.c | 29 ++++++++++++-------
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
index 846f6daae71b..d2030128fe46 100644
--- a/Documentation/devicetree/bindings/arm/coresight.txt
+++ b/Documentation/devicetree/bindings/arm/coresight.txt
@@ -108,6 +108,12 @@ its hardware characteristcs.
 	* arm,cp14: must be present if the system accesses ETM/PTM management
 	  registers via co-processor 14.
 
+	* qcom,tupwr-disable: boolean. Indicates that trace unit power up can
+	  be disabled on Qualcomm Technologies Inc. systems where ETMs are in
+	  the same power domain as their CPU cores. This property is required
+	  to identify such systems with hardware errata where the CPU watchdog
+	  counter is stopped when TRCPDCR.PU=1.
+
 * Optional property for TMC:
 
 	* arm,buffer-size: size of contiguous buffer space for TMC ETR
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
index fb0f5f4f3a91..6886b44f6947 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -104,6 +104,11 @@ struct etm4_enable_arg {
 	int rc;
 };
 
+static inline bool etm4_can_disable_tupwr(struct device *dev)
+{
+	return fwnode_property_present(dev_fwnode(dev), "qcom,tupwr-disable");
+}
+
 static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
 {
 	int i, rc;
@@ -196,12 +201,14 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
 	writel_relaxed(config->vmid_mask0, drvdata->base + TRCVMIDCCTLR0);
 	writel_relaxed(config->vmid_mask1, drvdata->base + TRCVMIDCCTLR1);
 
-	/*
-	 * Request to keep the trace unit powered and also
-	 * emulation of powerdown
-	 */
-	writel_relaxed(readl_relaxed(drvdata->base + TRCPDCR) | TRCPDCR_PU,
-		       drvdata->base + TRCPDCR);
+	if (!etm4_can_disable_tupwr(etm_dev)) {
+		/*
+		 * Request to keep the trace unit powered and also
+		 * emulation of powerdown
+		 */
+		writel_relaxed(readl_relaxed(drvdata->base + TRCPDCR) | TRCPDCR_PU,
+			       drvdata->base + TRCPDCR);
+	}
 
 	/* Enable the trace unit */
 	writel_relaxed(1, drvdata->base + TRCPRGCTLR);
@@ -476,10 +483,12 @@ static void etm4_disable_hw(void *info)
 
 	CS_UNLOCK(drvdata->base);
 
-	/* power can be removed from the trace unit now */
-	control = readl_relaxed(drvdata->base + TRCPDCR);
-	control &= ~TRCPDCR_PU;
-	writel_relaxed(control, drvdata->base + TRCPDCR);
+	if (!etm4_can_disable_tupwr(etm_dev)) {
+		/* power can be removed from the trace unit now */
+		control = readl_relaxed(drvdata->base + TRCPDCR);
+		control &= ~TRCPDCR_PU;
+		writel_relaxed(control, drvdata->base + TRCPDCR);
+	}
 
 	control = readl_relaxed(drvdata->base + TRCPRGCTLR);
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] coresight: etm4x: Add support to disable trace unit power up

[Mathieu Poirier]

Good morning Sai,

On Thu, May 14, 2020 at 04:29:15PM +0530, Sai Prakash Ranjan wrote:
> From: Tingwei Zhang <tingwei@codeaurora.org>
> 
> On some Qualcomm Technologies Inc. SoCs like SC7180, there
> exists a hardware errata where the APSS (Application Processor
> SubSystem)/CPU watchdog counter is stopped when ETM register
> TRCPDCR.PU=1.

Fun stuff...

> Since the ETMs share the same power domain as
> that of respective CPU cores, they are powered on when the
> CPU core is powered on. So we can disable powering up of the
> trace unit after checking for this errata via new property
> called "qcom,tupwr-disable".
> 
> Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>

Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>

> ---
>  .../devicetree/bindings/arm/coresight.txt     |  6 ++++
>  drivers/hwtracing/coresight/coresight-etm4x.c | 29 ++++++++++++-------

Please split in two patches.

>  2 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
> index 846f6daae71b..d2030128fe46 100644
> --- a/Documentation/devicetree/bindings/arm/coresight.txt
> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> @@ -108,6 +108,12 @@ its hardware characteristcs.
>  	* arm,cp14: must be present if the system accesses ETM/PTM management
>  	  registers via co-processor 14.
>  
> +	* qcom,tupwr-disable: boolean. Indicates that trace unit power up can
> +	  be disabled on Qualcomm Technologies Inc. systems where ETMs are in
> +	  the same power domain as their CPU cores. This property is required
> +	  to identify such systems with hardware errata where the CPU watchdog
> +	  counter is stopped when TRCPDCR.PU=1.
> +

I think something like "qcom,skip-power-up" would be clearer. 

Also, a better choice of words is that TRCPDCR.PU does not have to be set on
Qualcomm...

>  * Optional property for TMC:
>  
>  	* arm,buffer-size: size of contiguous buffer space for TMC ETR
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> index fb0f5f4f3a91..6886b44f6947 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> @@ -104,6 +104,11 @@ struct etm4_enable_arg {
>  	int rc;
>  };
>  
> +static inline bool etm4_can_disable_tupwr(struct device *dev)
> +{
> +	return fwnode_property_present(dev_fwnode(dev), "qcom,tupwr-disable");
> +}
> +

Please call fwnode_property_present() at initialisation time to set a new
drvdata::skip_power_up variable.  From there just switch on that in
etm4_enable/disable_hw().  

Thanks,
Mathieu

>  static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>  {
>  	int i, rc;
> @@ -196,12 +201,14 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>  	writel_relaxed(config->vmid_mask0, drvdata->base + TRCVMIDCCTLR0);
>  	writel_relaxed(config->vmid_mask1, drvdata->base + TRCVMIDCCTLR1);
>  
> -	/*
> -	 * Request to keep the trace unit powered and also
> -	 * emulation of powerdown
> -	 */
> -	writel_relaxed(readl_relaxed(drvdata->base + TRCPDCR) | TRCPDCR_PU,
> -		       drvdata->base + TRCPDCR);
> +	if (!etm4_can_disable_tupwr(etm_dev)) {
> +		/*
> +		 * Request to keep the trace unit powered and also
> +		 * emulation of powerdown
> +		 */
> +		writel_relaxed(readl_relaxed(drvdata->base + TRCPDCR) | TRCPDCR_PU,
> +			       drvdata->base + TRCPDCR);
> +	}
>  
>  	/* Enable the trace unit */
>  	writel_relaxed(1, drvdata->base + TRCPRGCTLR);
> @@ -476,10 +483,12 @@ static void etm4_disable_hw(void *info)
>  
>  	CS_UNLOCK(drvdata->base);
>  
> -	/* power can be removed from the trace unit now */
> -	control = readl_relaxed(drvdata->base + TRCPDCR);
> -	control &= ~TRCPDCR_PU;
> -	writel_relaxed(control, drvdata->base + TRCPDCR);
> +	if (!etm4_can_disable_tupwr(etm_dev)) {
> +		/* power can be removed from the trace unit now */
> +		control = readl_relaxed(drvdata->base + TRCPDCR);
> +		control &= ~TRCPDCR_PU;
> +		writel_relaxed(control, drvdata->base + TRCPDCR);
> +	}
>  
>  	control = readl_relaxed(drvdata->base + TRCPRGCTLR);
>  
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] coresight: etm4x: Add support to disable trace unit power up

[Sai Prakash Ranjan]

Hi Mathieu,

On 2020-05-14 23:30, Mathieu Poirier wrote:
> Good morning Sai,
> 
> On Thu, May 14, 2020 at 04:29:15PM +0530, Sai Prakash Ranjan wrote:
>> From: Tingwei Zhang <tingwei@codeaurora.org>
>> 
>> On some Qualcomm Technologies Inc. SoCs like SC7180, there
>> exists a hardware errata where the APSS (Application Processor
>> SubSystem)/CPU watchdog counter is stopped when ETM register
>> TRCPDCR.PU=1.
> 
> Fun stuff...
> 

Yes :)

>> Since the ETMs share the same power domain as
>> that of respective CPU cores, they are powered on when the
>> CPU core is powered on. So we can disable powering up of the
>> trace unit after checking for this errata via new property
>> called "qcom,tupwr-disable".
>> 
>> Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
>> Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> 
> Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> 

Tingwei is the author, so if I understand correctly, his signed-off-by
should appear first, am I wrong?

>> ---
>>  .../devicetree/bindings/arm/coresight.txt     |  6 ++++
>>  drivers/hwtracing/coresight/coresight-etm4x.c | 29 
>> ++++++++++++-------
> 
> Please split in two patches.
> 

Sure, I will split the dt-binding into separate patch, checkpatch did 
warn.

>>  2 files changed, 25 insertions(+), 10 deletions(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt 
>> b/Documentation/devicetree/bindings/arm/coresight.txt
>> index 846f6daae71b..d2030128fe46 100644
>> --- a/Documentation/devicetree/bindings/arm/coresight.txt
>> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
>> @@ -108,6 +108,12 @@ its hardware characteristcs.
>>  	* arm,cp14: must be present if the system accesses ETM/PTM 
>> management
>>  	  registers via co-processor 14.
>> 
>> +	* qcom,tupwr-disable: boolean. Indicates that trace unit power up 
>> can
>> +	  be disabled on Qualcomm Technologies Inc. systems where ETMs are 
>> in
>> +	  the same power domain as their CPU cores. This property is 
>> required
>> +	  to identify such systems with hardware errata where the CPU 
>> watchdog
>> +	  counter is stopped when TRCPDCR.PU=1.
>> +
> 
> I think something like "qcom,skip-power-up" would be clearer.
> 
> Also, a better choice of words is that TRCPDCR.PU does not have to be 
> set on
> Qualcomm...
> 

Yes "qcom,skip-power-up" is a lot better, thanks. Also will use 
something as
you suggested for description.

>>  * Optional property for TMC:
>> 
>>  	* arm,buffer-size: size of contiguous buffer space for TMC ETR
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c 
>> b/drivers/hwtracing/coresight/coresight-etm4x.c
>> index fb0f5f4f3a91..6886b44f6947 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
>> @@ -104,6 +104,11 @@ struct etm4_enable_arg {
>>  	int rc;
>>  };
>> 
>> +static inline bool etm4_can_disable_tupwr(struct device *dev)
>> +{
>> +	return fwnode_property_present(dev_fwnode(dev), 
>> "qcom,tupwr-disable");
>> +}
>> +
> 
> Please call fwnode_property_present() at initialisation time to set a 
> new
> drvdata::skip_power_up variable.  From there just switch on that in
> etm4_enable/disable_hw().
> 

Will do, thanks.

Thanks,
Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] coresight: etm4x: Add support to disable trace unit power up

[Mathieu Poirier]

On Thu, 14 May 2020 at 12:39, Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> Hi Mathieu,
>
> On 2020-05-14 23:30, Mathieu Poirier wrote:
> > Good morning Sai,
> >
> > On Thu, May 14, 2020 at 04:29:15PM +0530, Sai Prakash Ranjan wrote:
> >> From: Tingwei Zhang <tingwei@codeaurora.org>
> >>
> >> On some Qualcomm Technologies Inc. SoCs like SC7180, there
> >> exists a hardware errata where the APSS (Application Processor
> >> SubSystem)/CPU watchdog counter is stopped when ETM register
> >> TRCPDCR.PU=1.
> >
> > Fun stuff...
> >
>
> Yes :)
>
> >> Since the ETMs share the same power domain as
> >> that of respective CPU cores, they are powered on when the
> >> CPU core is powered on. So we can disable powering up of the
> >> trace unit after checking for this errata via new property
> >> called "qcom,tupwr-disable".
> >>
> >> Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> >> Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> >> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> >
> > Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> >
>
> Tingwei is the author, so if I understand correctly, his signed-off-by
> should appear first, am I wrong?

It's a gray area and depends on who's code is more prevalent in the
patch.  If Tingwei wrote the most of the code then his name is in the
"from:" section, yours as co-developer and he signs off on it (as I
suggested).  If you did most of the work then it is the opposite.
Adding a Co-developed and a signed-off with the same name doesn't make
sense.

>
> >> ---
> >>  .../devicetree/bindings/arm/coresight.txt     |  6 ++++
> >>  drivers/hwtracing/coresight/coresight-etm4x.c | 29
> >> ++++++++++++-------
> >
> > Please split in two patches.
> >
>
> Sure, I will split the dt-binding into separate patch, checkpatch did
> warn.

And you still sent me the patch...  I usually run checkpatch before
all the submissions I review and flatly ignore patches that return
errors.  You got lucky...

>
> >>  2 files changed, 25 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt
> >> b/Documentation/devicetree/bindings/arm/coresight.txt
> >> index 846f6daae71b..d2030128fe46 100644
> >> --- a/Documentation/devicetree/bindings/arm/coresight.txt
> >> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> >> @@ -108,6 +108,12 @@ its hardware characteristcs.
> >>      * arm,cp14: must be present if the system accesses ETM/PTM
> >> management
> >>        registers via co-processor 14.
> >>
> >> +    * qcom,tupwr-disable: boolean. Indicates that trace unit power up
> >> can
> >> +      be disabled on Qualcomm Technologies Inc. systems where ETMs are
> >> in
> >> +      the same power domain as their CPU cores. This property is
> >> required
> >> +      to identify such systems with hardware errata where the CPU
> >> watchdog
> >> +      counter is stopped when TRCPDCR.PU=1.
> >> +
> >
> > I think something like "qcom,skip-power-up" would be clearer.
> >
> > Also, a better choice of words is that TRCPDCR.PU does not have to be
> > set on
> > Qualcomm...
> >
>
> Yes "qcom,skip-power-up" is a lot better, thanks. Also will use
> something as
> you suggested for description.
>
> >>  * Optional property for TMC:
> >>
> >>      * arm,buffer-size: size of contiguous buffer space for TMC ETR
> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c
> >> b/drivers/hwtracing/coresight/coresight-etm4x.c
> >> index fb0f5f4f3a91..6886b44f6947 100644
> >> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> >> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> >> @@ -104,6 +104,11 @@ struct etm4_enable_arg {
> >>      int rc;
> >>  };
> >>
> >> +static inline bool etm4_can_disable_tupwr(struct device *dev)
> >> +{
> >> +    return fwnode_property_present(dev_fwnode(dev),
> >> "qcom,tupwr-disable");
> >> +}
> >> +
> >
> > Please call fwnode_property_present() at initialisation time to set a
> > new
> > drvdata::skip_power_up variable.  From there just switch on that in
> > etm4_enable/disable_hw().
> >
>
> Will do, thanks.
>
> Thanks,
> Sai
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member
> of Code Aurora Forum, hosted by The Linux Foundation
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] coresight: etm4x: Add support to disable trace unit power up

[Sai Prakash Ranjan]

Hi Mathieu,

On 2020-05-15 20:22, Mathieu Poirier wrote:
> On Thu, 14 May 2020 at 12:39, Sai Prakash Ranjan
> <saiprakash.ranjan@codeaurora.org> wrote:
>> 
>> Hi Mathieu,
>> 
>> On 2020-05-14 23:30, Mathieu Poirier wrote:
>> > Good morning Sai,
>> >
>> > On Thu, May 14, 2020 at 04:29:15PM +0530, Sai Prakash Ranjan wrote:
>> >> From: Tingwei Zhang <tingwei@codeaurora.org>
>> >>
>> >> On some Qualcomm Technologies Inc. SoCs like SC7180, there
>> >> exists a hardware errata where the APSS (Application Processor
>> >> SubSystem)/CPU watchdog counter is stopped when ETM register
>> >> TRCPDCR.PU=1.
>> >
>> > Fun stuff...
>> >
>> 
>> Yes :)
>> 
>> >> Since the ETMs share the same power domain as
>> >> that of respective CPU cores, they are powered on when the
>> >> CPU core is powered on. So we can disable powering up of the
>> >> trace unit after checking for this errata via new property
>> >> called "qcom,tupwr-disable".
>> >>
>> >> Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
>> >> Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>> >> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>> >
>> > Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>> > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
>> >
>> 
>> Tingwei is the author, so if I understand correctly, his signed-off-by
>> should appear first, am I wrong?
> 
> It's a gray area and depends on who's code is more prevalent in the
> patch.  If Tingwei wrote the most of the code then his name is in the
> "from:" section, yours as co-developer and he signs off on it (as I
> suggested).  If you did most of the work then it is the opposite.
> Adding a Co-developed and a signed-off with the same name doesn't make
> sense.
> 

I did check the documentation for submitting patches:
Documentation/process/submitting-patches.rst. And it clearly states
that "Co-developed-by must be followed by Signed-off by the co-author
and the last Signed-off-by: must always be that of the developer
submitting the patch".

Quoting below from the doc:

Co-developed-by: <snip> ...Since
Co-developed-by: denotes authorship, every Co-developed-by: must be 
immediately
followed by a Signed-off-by: of the associated co-author.  Standard 
sign-off
procedure applies, i.e. the ordering of Signed-off-by: tags should 
reflect the
chronological history of the patch insofar as possible, regardless of 
whether
the author is attributed via From: or Co-developed-by:.  Notably, the 
last
Signed-off-by: must always be that of the developer submitting the 
patch.

>> 
>> >> ---
>> >>  .../devicetree/bindings/arm/coresight.txt     |  6 ++++
>> >>  drivers/hwtracing/coresight/coresight-etm4x.c | 29
>> >> ++++++++++++-------
>> >
>> > Please split in two patches.
>> >
>> 
>> Sure, I will split the dt-binding into separate patch, checkpatch did
>> warn.
> 
> And you still sent me the patch...  I usually run checkpatch before
> all the submissions I review and flatly ignore patches that return
> errors.  You got lucky...
> 

I did not mean to ignore it or else I wouldn't have run checkpatch 
itself.
I checked other cases like "arm,scatter-gather" where the binding and 
the
driver change was in a single patch, hence I thought it's not a very 
strict rule.

Thanks,
Sai
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] coresight: etm4x: Add support to disable trace unit power up

[Mathieu Poirier]

On Fri, May 15, 2020 at 08:37:13PM +0530, Sai Prakash Ranjan wrote:
> Hi Mathieu,
> 
> On 2020-05-15 20:22, Mathieu Poirier wrote:
> > On Thu, 14 May 2020 at 12:39, Sai Prakash Ranjan
> > <saiprakash.ranjan@codeaurora.org> wrote:
> > > 
> > > Hi Mathieu,
> > > 
> > > On 2020-05-14 23:30, Mathieu Poirier wrote:
> > > > Good morning Sai,
> > > >
> > > > On Thu, May 14, 2020 at 04:29:15PM +0530, Sai Prakash Ranjan wrote:
> > > >> From: Tingwei Zhang <tingwei@codeaurora.org>
> > > >>
> > > >> On some Qualcomm Technologies Inc. SoCs like SC7180, there
> > > >> exists a hardware errata where the APSS (Application Processor
> > > >> SubSystem)/CPU watchdog counter is stopped when ETM register
> > > >> TRCPDCR.PU=1.
> > > >
> > > > Fun stuff...
> > > >
> > > 
> > > Yes :)
> > > 
> > > >> Since the ETMs share the same power domain as
> > > >> that of respective CPU cores, they are powered on when the
> > > >> CPU core is powered on. So we can disable powering up of the
> > > >> trace unit after checking for this errata via new property
> > > >> called "qcom,tupwr-disable".
> > > >>
> > > >> Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > > >> Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> > > >> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> > > >
> > > > Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> > > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > > >
> > > 
> > > Tingwei is the author, so if I understand correctly, his signed-off-by
> > > should appear first, am I wrong?
> > 
> > It's a gray area and depends on who's code is more prevalent in the
> > patch.  If Tingwei wrote the most of the code then his name is in the
> > "from:" section, yours as co-developer and he signs off on it (as I
> > suggested).  If you did most of the work then it is the opposite.
> > Adding a Co-developed and a signed-off with the same name doesn't make
> > sense.
> > 
> 
> I did check the documentation for submitting patches:
> Documentation/process/submitting-patches.rst. And it clearly states
> that "Co-developed-by must be followed by Signed-off by the co-author
> and the last Signed-off-by: must always be that of the developer
> submitting the patch".
> 
> Quoting below from the doc:
> 
> Co-developed-by: <snip> ...Since
> Co-developed-by: denotes authorship, every Co-developed-by: must be
> immediately
> followed by a Signed-off-by: of the associated co-author.  Standard sign-off
> procedure applies, i.e. the ordering of Signed-off-by: tags should reflect
> the
> chronological history of the patch insofar as possible, regardless of
> whether
> the author is attributed via From: or Co-developed-by:.  Notably, the last
> Signed-off-by: must always be that of the developer submitting the patch.

Ah yes, glad to see that got clarified.  You can ignore my recommendation on
that snippet.

> 
> > > 
> > > >> ---
> > > >>  .../devicetree/bindings/arm/coresight.txt     |  6 ++++
> > > >>  drivers/hwtracing/coresight/coresight-etm4x.c | 29
> > > >> ++++++++++++-------
> > > >
> > > > Please split in two patches.
> > > >
> > > 
> > > Sure, I will split the dt-binding into separate patch, checkpatch did
> > > warn.
> > 
> > And you still sent me the patch...  I usually run checkpatch before
> > all the submissions I review and flatly ignore patches that return
> > errors.  You got lucky...
> > 
> 
> I did not mean to ignore it or else I wouldn't have run checkpatch itself.
> I checked other cases like "arm,scatter-gather" where the binding and the
> driver change was in a single patch, hence I thought it's not a very strict
> rule.

The patch has another warning for a line over 80 characters, that should have
been fixed before sending.  Putting DT changes in a separate patch is always
better for the DT people.  They review tons of patches and making their life
easier is always a good thing.

Regards,
Mathieu

> 
> Thanks,
> Sai
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] coresight: etm4x: Add support to disable trace unit power up

[Sai Prakash Ranjan]

Hi Mathieu,

On 2020-05-15 21:21, Mathieu Poirier wrote:
> On Fri, May 15, 2020 at 08:37:13PM +0530, Sai Prakash Ranjan wrote:
>> Hi Mathieu,
>> 
>> On 2020-05-15 20:22, Mathieu Poirier wrote:
>> > On Thu, 14 May 2020 at 12:39, Sai Prakash Ranjan
>> > <saiprakash.ranjan@codeaurora.org> wrote:
>> > >
>> > > Hi Mathieu,
>> > >
>> > > On 2020-05-14 23:30, Mathieu Poirier wrote:
>> > > > Good morning Sai,
>> > > >
>> > > > On Thu, May 14, 2020 at 04:29:15PM +0530, Sai Prakash Ranjan wrote:
>> > > >> From: Tingwei Zhang <tingwei@codeaurora.org>
>> > > >>
>> > > >> On some Qualcomm Technologies Inc. SoCs like SC7180, there
>> > > >> exists a hardware errata where the APSS (Application Processor
>> > > >> SubSystem)/CPU watchdog counter is stopped when ETM register
>> > > >> TRCPDCR.PU=1.
>> > > >
>> > > > Fun stuff...
>> > > >
>> > >
>> > > Yes :)
>> > >
>> > > >> Since the ETMs share the same power domain as
>> > > >> that of respective CPU cores, they are powered on when the
>> > > >> CPU core is powered on. So we can disable powering up of the
>> > > >> trace unit after checking for this errata via new property
>> > > >> called "qcom,tupwr-disable".
>> > > >>
>> > > >> Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
>> > > >> Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>> > > >> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>> > > >
>> > > > Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>> > > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
>> > > >
>> > >
>> > > Tingwei is the author, so if I understand correctly, his signed-off-by
>> > > should appear first, am I wrong?
>> >
>> > It's a gray area and depends on who's code is more prevalent in the
>> > patch.  If Tingwei wrote the most of the code then his name is in the
>> > "from:" section, yours as co-developer and he signs off on it (as I
>> > suggested).  If you did most of the work then it is the opposite.
>> > Adding a Co-developed and a signed-off with the same name doesn't make
>> > sense.
>> >
>> 
>> I did check the documentation for submitting patches:
>> Documentation/process/submitting-patches.rst. And it clearly states
>> that "Co-developed-by must be followed by Signed-off by the co-author
>> and the last Signed-off-by: must always be that of the developer
>> submitting the patch".
>> 
>> Quoting below from the doc:
>> 
>> Co-developed-by: <snip> ...Since
>> Co-developed-by: denotes authorship, every Co-developed-by: must be
>> immediately
>> followed by a Signed-off-by: of the associated co-author.  Standard 
>> sign-off
>> procedure applies, i.e. the ordering of Signed-off-by: tags should 
>> reflect
>> the
>> chronological history of the patch insofar as possible, regardless of
>> whether
>> the author is attributed via From: or Co-developed-by:.  Notably, the 
>> last
>> Signed-off-by: must always be that of the developer submitting the 
>> patch.
> 
> Ah yes, glad to see that got clarified.  You can ignore my 
> recommendation on
> that snippet.
> 
>> 
>> > >
>> > > >> ---
>> > > >>  .../devicetree/bindings/arm/coresight.txt     |  6 ++++
>> > > >>  drivers/hwtracing/coresight/coresight-etm4x.c | 29
>> > > >> ++++++++++++-------
>> > > >
>> > > > Please split in two patches.
>> > > >
>> > >
>> > > Sure, I will split the dt-binding into separate patch, checkpatch did
>> > > warn.
>> >
>> > And you still sent me the patch...  I usually run checkpatch before
>> > all the submissions I review and flatly ignore patches that return
>> > errors.  You got lucky...
>> >
>> 
>> I did not mean to ignore it or else I wouldn't have run checkpatch 
>> itself.
>> I checked other cases like "arm,scatter-gather" where the binding and 
>> the
>> driver change was in a single patch, hence I thought it's not a very 
>> strict
>> rule.
> 
> The patch has another warning for a line over 80 characters, that 
> should have
> been fixed before sending.  Putting DT changes in a separate patch is 
> always
> better for the DT people.  They review tons of patches and making their 
> life
> easier is always a good thing.
> 

Ok, I will fix this and resend. I did not want to change it in case if 
it affects
readability since most maintainers prefer to ignore this 80 characters 
warning if
it affects readability. I will keep this in mind for future patches as 
well.

Thanks,
Sai
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] coresight: etm4x: Add support to disable trace unit power up

[Sai Prakash Ranjan]

On 2020-05-15 21:28, Sai Prakash Ranjan wrote:
> Hi Mathieu,
> 
> On 2020-05-15 21:21, Mathieu Poirier wrote:
>> On Fri, May 15, 2020 at 08:37:13PM +0530, Sai Prakash Ranjan wrote:
>>> Hi Mathieu,
>>> 
>>> On 2020-05-15 20:22, Mathieu Poirier wrote:
>>> > On Thu, 14 May 2020 at 12:39, Sai Prakash Ranjan
>>> > <saiprakash.ranjan@codeaurora.org> wrote:
>>> > >
>>> > > Hi Mathieu,
>>> > >
>>> > > On 2020-05-14 23:30, Mathieu Poirier wrote:
>>> > > > Good morning Sai,
>>> > > >
>>> > > > On Thu, May 14, 2020 at 04:29:15PM +0530, Sai Prakash Ranjan wrote:
>>> > > >> From: Tingwei Zhang <tingwei@codeaurora.org>
>>> > > >>
>>> > > >> On some Qualcomm Technologies Inc. SoCs like SC7180, there
>>> > > >> exists a hardware errata where the APSS (Application Processor
>>> > > >> SubSystem)/CPU watchdog counter is stopped when ETM register
>>> > > >> TRCPDCR.PU=1.
>>> > > >
>>> > > > Fun stuff...
>>> > > >
>>> > >
>>> > > Yes :)
>>> > >
>>> > > >> Since the ETMs share the same power domain as
>>> > > >> that of respective CPU cores, they are powered on when the
>>> > > >> CPU core is powered on. So we can disable powering up of the
>>> > > >> trace unit after checking for this errata via new property
>>> > > >> called "qcom,tupwr-disable".
>>> > > >>
>>> > > >> Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
>>> > > >> Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>>> > > >> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>>> > > >
>>> > > > Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>>> > > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
>>> > > >
>>> > >
>>> > > Tingwei is the author, so if I understand correctly, his signed-off-by
>>> > > should appear first, am I wrong?
>>> >
>>> > It's a gray area and depends on who's code is more prevalent in the
>>> > patch.  If Tingwei wrote the most of the code then his name is in the
>>> > "from:" section, yours as co-developer and he signs off on it (as I
>>> > suggested).  If you did most of the work then it is the opposite.
>>> > Adding a Co-developed and a signed-off with the same name doesn't make
>>> > sense.
>>> >
>>> 
>>> I did check the documentation for submitting patches:
>>> Documentation/process/submitting-patches.rst. And it clearly states
>>> that "Co-developed-by must be followed by Signed-off by the co-author
>>> and the last Signed-off-by: must always be that of the developer
>>> submitting the patch".
>>> 
>>> Quoting below from the doc:
>>> 
>>> Co-developed-by: <snip> ...Since
>>> Co-developed-by: denotes authorship, every Co-developed-by: must be
>>> immediately
>>> followed by a Signed-off-by: of the associated co-author.  Standard 
>>> sign-off
>>> procedure applies, i.e. the ordering of Signed-off-by: tags should 
>>> reflect
>>> the
>>> chronological history of the patch insofar as possible, regardless of
>>> whether
>>> the author is attributed via From: or Co-developed-by:.  Notably, the 
>>> last
>>> Signed-off-by: must always be that of the developer submitting the 
>>> patch.
>> 
>> Ah yes, glad to see that got clarified.  You can ignore my 
>> recommendation on
>> that snippet.
>> 
>>> 
>>> > >
>>> > > >> ---
>>> > > >>  .../devicetree/bindings/arm/coresight.txt     |  6 ++++
>>> > > >>  drivers/hwtracing/coresight/coresight-etm4x.c | 29
>>> > > >> ++++++++++++-------
>>> > > >
>>> > > > Please split in two patches.
>>> > > >
>>> > >
>>> > > Sure, I will split the dt-binding into separate patch, checkpatch did
>>> > > warn.
>>> >
>>> > And you still sent me the patch...  I usually run checkpatch before
>>> > all the submissions I review and flatly ignore patches that return
>>> > errors.  You got lucky...
>>> >
>>> 
>>> I did not mean to ignore it or else I wouldn't have run checkpatch 
>>> itself.
>>> I checked other cases like "arm,scatter-gather" where the binding and 
>>> the
>>> driver change was in a single patch, hence I thought it's not a very 
>>> strict
>>> rule.
>> 
>> The patch has another warning for a line over 80 characters, that 
>> should have
>> been fixed before sending.  Putting DT changes in a separate patch is 
>> always
>> better for the DT people.  They review tons of patches and making 
>> their life
>> easier is always a good thing.
>> 
> 
> Ok, I will fix this and resend. I did not want to change it in case if
> it affects
> readability since most maintainers prefer to ignore this 80 characters
> warning if
> it affects readability. I will keep this in mind for future patches as 
> well.
> 

Now fixed all checkpatch warnings and addressed other review comments.
Posted v3 - https://lore.kernel.org/patchwork/cover/1242572/

Thanks,
Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation