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@xxxxxxxxxxxxxx> 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@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?

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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel