Re: [PATCHv2 3/4] coresight: replicator: Reset replicator if context is lost

[Sai Prakash Ranjan]

Hi Mathieu,

On 2020-05-22 23:10, Mathieu Poirier wrote:
> Hi Sai,
>
> On Tue, May 19, 2020 at 07:06:02PM +0530, Sai Prakash Ranjan wrote:
> > On some QCOM SoCs, replicators in Always-On domain loses its
> > context as soon as the clock is disabled. Currently as a part
> > of pm_runtime workqueue, clock is disabled after the replicator
> > is initialized by amba_pm_runtime_suspend assuming that context
> > is not lost which is not true for replicators with such
> > limitations. So add a new property "qcom,replicator-loses-context"
> > to identify such replicators and reset them.
> >
> > Suggested-by: Mike Leach <mike.leach@xxxxxxxxxx>
> > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@xxxxxxxxxxxxxx>
> > ---
> >
> > Added Mike's suggested by for parts other than the DT property.
> > Perhaps I should add Co-developed-by Mike since the full skeletal
> > was given by Mike. I can add that if required on the next version.
>
>  I will let Mike decide what he wants to do - I'm fine either way.

Mike was ok with suggested-by, so I will go with that.

> > ---
> >  .../coresight/coresight-replicator.c          | 53 
> > +++++++++++++------
> >  1 file changed, 36 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-replicator.c 
> > b/drivers/hwtracing/coresight/coresight-replicator.c
> > index c619b456f55a..ba66160c8140 100644
> > --- a/drivers/hwtracing/coresight/coresight-replicator.c
> > +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> > @@ -38,6 +38,7 @@ struct replicator_drvdata {
> >  	struct clk		*atclk;
> >  	struct coresight_device	*csdev;
> >  	spinlock_t		spinlock;
> > +	bool			check_idfilter_val;
>
> Please add documentation for the new field, the same way other fields 
> are
> documented.

Sure will add that.

> >  };
> >
> >  static void dynamic_replicator_reset(struct replicator_drvdata 
> > *drvdata)
> > @@ -66,29 +67,43 @@ static int dynamic_replicator_enable(struct 
> > replicator_drvdata *drvdata,
> >  				     int inport, int outport)
> >  {
> >  	int rc = 0;
> > -	u32 reg;
> > -
> > -	switch (outport) {
> > -	case 0:
> > -		reg = REPLICATOR_IDFILTER0;
> > -		break;
> > -	case 1:
> > -		reg = REPLICATOR_IDFILTER1;
> > -		break;
> > -	default:
> > -		WARN_ON(1);
> > -		return -EINVAL;
> > -	}
> > +	u32 id0val, id1val;
> >
> >  	CS_UNLOCK(drvdata->base);
> >
> > -	if ((readl_relaxed(drvdata->base + REPLICATOR_IDFILTER0) == 0xff) &&
> > -	    (readl_relaxed(drvdata->base + REPLICATOR_IDFILTER1) == 0xff))
> > +	id0val = readl_relaxed(drvdata->base + REPLICATOR_IDFILTER0);
> > +	id1val = readl_relaxed(drvdata->base + REPLICATOR_IDFILTER1);
> > +
> > +	/*
> > +	 * Some replicator designs lose context when AMBA clocks are 
> > removed,
> > +	 * so have a check for this.
> > +	 */
> > +	if (drvdata->check_idfilter_val && id0val == 0x0 && id1val == 0x0)
> > +		id0val = id1val = 0xff;
> > +
> > +	if (id0val == 0xff && id1val == 0xff)
> >  		rc = coresight_claim_device_unlocked(drvdata->base);
> >
> > +	if (!rc) {
> > +		switch (outport) {
> > +		case 0:
> > +			id0val = 0x0;
> > +			break;
> > +		case 1:
> > +			id1val = 0x0;
> > +			break;
> > +		default:
> > +			WARN_ON(1);
> > +			rc = -EINVAL;
> > +		}
> > +	}
> > +
> >  	/* Ensure that the outport is enabled. */
> > -	if (!rc)
> > -		writel_relaxed(0x00, drvdata->base + reg);
> > +	if (!rc) {
> > +		writel_relaxed(id0val, drvdata->base + REPLICATOR_IDFILTER0);
> > +		writel_relaxed(id1val, drvdata->base + REPLICATOR_IDFILTER1);
> > +	}
> > +
> >  	CS_LOCK(drvdata->base);
> >
> >  	return rc;
> > @@ -239,6 +254,10 @@ static int replicator_probe(struct device *dev, 
> > struct resource *res)
> >  		desc.groups = replicator_groups;
> >  	}
> >
> > +	if (fwnode_property_present(dev_fwnode(dev),
> > +				    "qcom,replicator-loses-context"))
> > +		drvdata->check_idfilter_val = true;
> > +
>
> The header <linux/property.h> needs to be added for function
> fwnode_property_present().

Sure will add in next version.

> What is the clock situation with other QC components like funnels?  
> Have they
> also been designed the same way?  If so the binding should probably be
> "qcom,component-loses-context", otherwise what you have suggested will 
> work just
> fine.  My goal here is to avoid having "qcom,replicator-loses-context" 
> and
> "qcom,funnel-loses-context".

Yes I understand it is quite ugly, but AFAIK we do not have any SoCs 
already released
and coming in near future with such limitations in funnels or other 
components. So I will
stick to the replicator specific property.

> Lastly, I have applied patch 1 and 2 of this set to my tree so no need 
> to resend
> them again with the next revision.

Thanks for reviewing these patches.

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