Re: [PATCH v2 19/27] coresight: catu: Plug in CATU as a backend for ETR buffer

[Suzuki K Poulose]

On 07/05/18 23:02, Mathieu Poirier wrote:
> On Tue, May 01, 2018 at 10:10:49AM +0100, Suzuki K Poulose wrote:
> > Now that we can use a CATU with a scatter gather table, add support
> > for the TMC ETR to make use of the connected CATU in translate mode.
> > This is done by adding CATU as new buffer mode. CATU's SLADDR must
> > always be 4K aligned. Thus the INADDR (base VA) is always 1M aligned
> > and we adjust the DBA for the ETR to align to the "offset" within
> > the 1MB page.


> > diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h
> > index cd58d6f..b673a73 100644
> > --- a/drivers/hwtracing/coresight/coresight-catu.h
> > +++ b/drivers/hwtracing/coresight/coresight-catu.h
> > @@ -29,6 +29,32 @@

> >   
> > +extern const struct etr_buf_operations etr_catu_buf_ops;
> > +

> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > index 25e7feb..41dde0a 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > @@ -941,6 +941,9 @@ static const struct etr_buf_operations etr_sg_buf_ops = {
> >   static const struct etr_buf_operations *etr_buf_ops[] = {
> >   	[ETR_MODE_FLAT] = &etr_flat_buf_ops,
> >   	[ETR_MODE_ETR_SG] = &etr_sg_buf_ops,
> > +#ifdef CONFIG_CORESIGHT_CATU
> > +	[ETR_MODE_CATU] = &etr_catu_buf_ops,
> > +#endif

...

> >   static inline int tmc_etr_mode_alloc_buf(int mode,
> > @@ -953,6 +956,9 @@ static inline int tmc_etr_mode_alloc_buf(int mode,
> >   	switch (mode) {
> >   	case ETR_MODE_FLAT:
> >   	case ETR_MODE_ETR_SG:
> > +#ifdef CONFIG_CORESIGHT_CATU
> > +	case ETR_MODE_CATU:
> > +#endif
>
> I really wish we could avoid doing something like this (and the above) but every
> alternate solution I come up with is either uglier or on par with it...
> Unless someone comes up with a bright idea we'll simply have to let it be.

We could do a little trick in the coresight-catu.h :

#ifdef CONFIG_CORESIGHT_CATU
extern struct etr_buf_operations etr_catu_buf_ops;
#else
static struct etr_buf_opertaions etr_catu_buf_ops;
#endif

And then add the following check to get rid of the #ifdef above in tmc_etr_mode_alloc_buf().

		if (etr_buf_ops[mode]->alloc)
			rc = etr_buf_ops[mode]->alloc(drvdata, etr_buf, node, pages);

> >   		rc = etr_buf_ops[mode]->alloc(drvdata, etr_buf, node, pages);

> While looking for a solution I noticed that tmc_etr_get_catu_device()
> could be moved to coresight-catu.h.  That way we wouldn't have to include
> coresight-catu.h every time coresight-tmc.h is present in a file.

Yes, we could do that. I don't remember if there was a specific reason behind it.
May be it is a left over from the rebases and how the CATU link was evolved. I
will clean it up.


Cheers
Suzuki