[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJ9a7Vg+z50TOghys8mXpkLt1fO2YCr74PEk-8PMr61-s7R6xA@mail.gmail.com>
Date: Wed, 22 Mar 2023 09:29:36 +0000
From: Mike Leach <mike.leach@...aro.org>
To: Suzuki K Poulose <suzuki.poulose@....com>
Cc: James Clark <james.clark@....com>, coresight@...ts.linaro.org,
Mathieu Poirier <mathieu.poirier@...aro.org>,
Leo Yan <leo.yan@...aro.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 8/9] coresight: Enable and disable helper devices
adjacent to the path
On Fri, 17 Mar 2023 at 11:04, Suzuki K Poulose <suzuki.poulose@....com> wrote:
>
> On 10/03/2023 16:06, James Clark wrote:
> > Currently CATU is the only helper device, and its enable and disable
> > calls are hard coded. To allow more helper devices to be added in a
> > generic way, remove these hard coded calls and just enable and disable
> > all helper devices.
> >
> > This has to apply to helpers adjacent to the path, because they will
> > never be in the path. CATU was already discovered in this way, so
> > there is no change there.
> >
> > One change that is needed is for CATU to call back into ETR to allocate
> > the buffer. Because the enable call was previously hard coded, it was
> > done at a point where the buffer was already allocated, but this is no
> > longer the case.
> >
> > Signed-off-by: James Clark <james.clark@....com>
> > ---
> > drivers/hwtracing/coresight/coresight-catu.c | 34 ++++++++--
> > drivers/hwtracing/coresight/coresight-core.c | 68 ++++++++++++++++++-
> > .../hwtracing/coresight/coresight-tmc-etr.c | 28 --------
> > include/linux/coresight.h | 3 +-
> > 4 files changed, 99 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> > index bc90a03f478f..24a08a2b96b1 100644
> > --- a/drivers/hwtracing/coresight/coresight-catu.c
> > +++ b/drivers/hwtracing/coresight/coresight-catu.c
> > @@ -395,13 +395,32 @@ static inline int catu_wait_for_ready(struct catu_drvdata *drvdata)
> > return coresight_timeout(csa, CATU_STATUS, CATU_STATUS_READY, 1);
> > }
> >
> > -static int catu_enable_hw(struct catu_drvdata *drvdata, void *data)
> > +static struct coresight_device *
> > +catu_get_etr_device(struct coresight_device *csdev)
> > +{
> > + int i;
> > + struct coresight_device *tmp;
> > +
> > + for (i = 0; i < csdev->pdata->nr_inconns; i++) {
> > + tmp = csdev->pdata->in_conns[i].remote_dev;
> > + if (tmp && tmp->type == CORESIGHT_DEV_TYPE_SINK &&
> > + tmp->subtype.sink_subtype ==
> > + CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM)
> > + return tmp;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +static int catu_enable_hw(struct catu_drvdata *drvdata, enum cs_mode cs_mode,
> > + void *data)
> > {
> > int rc;
> > u32 control, mode;
> > - struct etr_buf *etr_buf = data;
> > + struct etr_buf *etr_buf = NULL;
> > struct device *dev = &drvdata->csdev->dev;
> > struct coresight_device *csdev = drvdata->csdev;
> > + struct coresight_device *etrdev;
> >
> > if (catu_wait_for_ready(drvdata))
> > dev_warn(dev, "Timeout while waiting for READY\n");
> > @@ -416,6 +435,12 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, void *data)
> > if (rc)
> > return rc;
> >
> > + etrdev = catu_get_etr_device(csdev);
> > + if (etrdev) {
> > + etr_buf = tmc_etr_get_buffer(etrdev, cs_mode, data);
> > + if (IS_ERR(etr_buf))
> > + return PTR_ERR(etr_buf);
> > + }
>
> WARN_ON(!etrdev) ? We are not supposed to reach in the first place and
> return.
>
>
> > control |= BIT(CATU_CONTROL_ENABLE);
> >
> > if (etr_buf && etr_buf->mode == ETR_MODE_CATU) {
> > @@ -441,13 +466,14 @@ static int catu_enable_hw(struct catu_drvdata *drvdata, void *data)
> > return 0;
> > }
> >
> > -static int catu_enable(struct coresight_device *csdev, void *data)
> > +static int catu_enable(struct coresight_device *csdev, enum cs_mode mode,
> > + void *data)
> > {
> > int rc;
> > struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
> >
> > CS_UNLOCK(catu_drvdata->base);
> > - rc = catu_enable_hw(catu_drvdata, data);
> > + rc = catu_enable_hw(catu_drvdata, mode, data);
> > CS_LOCK(catu_drvdata->base);
> > return rc;
> > }
> > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > index a8ba7493c09a..3e6ccd9e8d4e 100644
> > --- a/drivers/hwtracing/coresight/coresight-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > @@ -441,6 +441,34 @@ static int coresight_enable_source(struct coresight_device *csdev,
> > return 0;
> > }
> >
> > +static int coresight_enable_helper(struct coresight_device *csdev,
> > + enum cs_mode mode, void *sink_data)
>
> minor nit: s/sink_data/data/ ? Though it is always either sink_data
> (perf mode) or NULL (sysfs mode), for the core code it is simply an
> opaque data.
>
> Rest looks fine to me.
>
> Suzuki
>
>
Reviewed by: Mike Leach <mike.leach@...aro.org>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
Powered by blists - more mailing lists