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@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...

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

> ---
>  .../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