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

From: Mathieu Poirier
Date: Thu May 14 2020 - 14:01:05 EST


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