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@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@xxxxxxxxxxxxxx>
> > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@xxxxxxxxxxxxxx>
>
> Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@xxxxxxxxxxxxxx>
> Signed-off-by: Tingwei Zhang <tingwei@xxxxxxxxxxxxxx>

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