Re: [PATCH 2/2] coresight: tmc: Add shutdown callback for TMC ETR/ETF

[Sai Prakash Ranjan]

Hi Mike,

Thanks for the review.

On 2020-06-01 19:05, Mike Leach wrote:
> Hi,
>
> On Mon, 1 Jun 2020 at 09:02, Sai Prakash Ranjan
> <saiprakash.ranjan@xxxxxxxxxxxxxx> wrote:
> > Implement a shutdown callback to ensure ETR/ETF hardware is
> > properly shutdown in reboot/shutdown path. This is required
> > for ETR/ETF which has SMMU address translation enabled like
> > on SC7180 SoC and few others. If the hardware is still accessing
> > memory after SMMU translation is disabled as part of SMMU
> > shutdown callback in system reboot or shutdown path, then
> > IOVAs(I/O virtual address) which it was using will go on the bus
> > as the physical addresses which might result in unknown crashes
> > (NoC/interconnect errors). So we make sure from this shutdown
> > callback that the ETR/ETF is shutdown before SMMU translation is
> > disabled and device_link in SMMU driver will take care of ordering
> > of shutdown callbacks such that SMMU shutdown callback is not
> > called before any of its consumer shutdown callbacks.
> >
> > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@xxxxxxxxxxxxxx>
> > ---
> >  .../hwtracing/coresight/coresight-tmc-etf.c   |  4 +--
> >  .../hwtracing/coresight/coresight-tmc-etr.c   |  2 +-
> >  drivers/hwtracing/coresight/coresight-tmc.c   | 29 
> > +++++++++++++++++++
> >  drivers/hwtracing/coresight/coresight-tmc.h   |  3 ++
> >  4 files changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c 
> > b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > index 36cce2bfb744..cba3e7592820 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> > @@ -85,7 +85,7 @@ static void __tmc_etb_disable_hw(struct tmc_drvdata 
> > *drvdata)
> >         CS_LOCK(drvdata->base);
> >  }
> >
> > -static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
> > +void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
> >  {
> >         __tmc_etb_disable_hw(drvdata);
> >         coresight_disclaim_device(drvdata->base);
> > @@ -118,7 +118,7 @@ static int tmc_etf_enable_hw(struct tmc_drvdata 
> > *drvdata)
> >         return 0;
> >  }
> >
> > -static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
> > +void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
> >  {
> >         CS_UNLOCK(drvdata->base);
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c 
> > b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > index 625882bc8b08..b29c2db94d96 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > @@ -1110,7 +1110,7 @@ static void __tmc_etr_disable_hw(struct 
> > tmc_drvdata *drvdata)
> >
> >  }
> >
> > -static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
> > +void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
> >  {
> >         __tmc_etr_disable_hw(drvdata);
> >         /* Disable CATU device if this ETR is connected to one */
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc.c 
> > b/drivers/hwtracing/coresight/coresight-tmc.c
> > index 5a271ebc4585..7e687a356fe0 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc.c
> > @@ -540,6 +540,34 @@ static int tmc_probe(struct amba_device *adev, 
> > const struct amba_id *id)
> >         return ret;
> >  }
> >
> > +static void tmc_shutdown(struct amba_device *adev)
> > +{
> > +       struct tmc_drvdata *drvdata = amba_get_drvdata(adev);
> > +
>
> Take drvdata->spinlock here? The tmc_xxx_disable_hw functions are
> normally called with the spinlock claimed.

Sure will take spinlock here.

> > +       if (!drvdata->enable)
>
> As per previous patch drvdata->mode can be used here.

Yes, will use mode here and drop enable flag in the next version.

Thanks,
Sai

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation