Re: [PATCH V3 05/14] coresight: ete: Add support for ETE tracing

[Suzuki K Poulose]

On 2/2/21 6:56 PM, Mathieu Poirier wrote:
> On Wed, Jan 27, 2021 at 02:25:29PM +0530, Anshuman Khandual wrote:
> > From: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> >
> > Add ETE as one of the supported device types we support
> > with ETM4x driver. The devices are named following the
> > existing convention as ete<N>.
> >
> > ETE mandates that the trace resource status register is programmed
> > before the tracing is turned on. For the moment simply write to
> > it indicating TraceActive.
> >
> > Cc: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> > Cc: Mike Leach <mike.leach@xxxxxxxxxx>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> > ---

...

> > @@ -1834,10 +1854,6 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
> >   	if (drvdata->cpu < 0)
> >   		return drvdata->cpu;
> >   
> > -	desc.name = devm_kasprintf(dev, GFP_KERNEL, "etm%d", drvdata->cpu);
> > -	if (!desc.name)
> > -		return -ENOMEM;
> > -
> >   	init_arg.drvdata = drvdata;
> >   	init_arg.csa = &desc.access;
> >   	init_arg.pid = etm_pid;
> > @@ -1853,6 +1869,20 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
> >   	if (!desc.access.io_mem ||
> >   	    fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up"))
> >   		drvdata->skip_power_up = true;
>
> Add a space here...
>
> > +	major = ETM_ARCH_MAJOR_VERSION(drvdata->arch);
> > +	minor = ETM_ARCH_MINOR_VERSION(drvdata->arch);
>
> And here too.  Othersiwe it makes a big blob in the middle of the function.
>
> > +	if (etm4x_is_ete(drvdata)) {
> > +		type_name = "ete";
> > +		/* ETE v1 has major version == 5. Adjust this for logging.*/
> > +		major -= 4;
>
> I don't have the documentation for the ETE but I would not adjust @major.  I
> would simply leave it to what the HW gives us since regardless of the name, the
> major revision of the IP block is 5.

At the moment only register definitions are public and can be found here :

https://developer.arm.com/docs/ddi0601/g/aarch64-system-registers/trcdevarch

The ETE is natural extension of the ETM architecture to support future
architecture changes and is designed in a way that the same software
can driver both ETM and ETE without much changes.

> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> > index ca24ac5..8b90de5 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> > @@ -128,6 +128,8 @@
> >   #define TRCCIDR2			0xFF8
> >   #define TRCCIDR3			0xFFC
> >   
> > +#define TRCRSR_TA			BIT(12)
> > +
> >   /*
> >    * System instructions to access ETM registers.
> >    * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
> > @@ -390,6 +392,9 @@
> >   #define ETM_COMMON_SYSREG_LIST_CASES		\
> >   	ETM_COMMON_SYSREG_LIST(NOP, __unused)
> >   
> > +#define ETM4x_ONLY_SYSREG_LIST_CASES		\
> > +	ETM4x_ONLY_SYSREG_LIST(NOP, __unused)
> > +
> >   #define ETM4x_SYSREG_LIST_CASES			\
> >   	ETM_COMMON_SYSREG_LIST_CASES		\
> >   	ETM4x_ONLY_SYSREG_LIST(NOP, __unused)
> > @@ -406,7 +411,6 @@
> >   	ETE_ONLY_SYSREG_LIST(WRITE, (val))
> >   
> >   #define ETE_ONLY_SYSREG_LIST_CASES		\
> > -	ETM_COMMON_SYSREG_LIST_CASES		\
>
> This goes in patch 04.

Sure, will move it.

> With the above:
>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>

Thanks for the review

Suzuki