Re: [PATCH v3 09/26] coresight: Convert coresight_timeout to use access abstraction

[Suzuki K Poulose]

On 11/3/20 6:03 PM, Mathieu Poirier wrote:
> On Wed, Oct 28, 2020 at 10:09:28PM +0000, Suzuki K Poulose wrote:
> > Convert the generic routines to use the new access abstraction layer
> > gradually, starting with coresigth_timeout.
> >
> > Cc: Mike Leach <mike.leach@xxxxxxxxxx>
> > Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > ---



> > diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> > index 5baf29510f1b..34c74b05c542 100644
> > --- a/drivers/hwtracing/coresight/coresight-catu.c
> > +++ b/drivers/hwtracing/coresight/coresight-catu.c
> > @@ -401,8 +401,9 @@ static const struct attribute_group *catu_groups[] = {
> >   
> >   static inline int catu_wait_for_ready(struct catu_drvdata *drvdata)
> >   {
> > -	return coresight_timeout(drvdata->base,
> > -				 CATU_STATUS, CATU_STATUS_READY, 1);
> > +	struct csdev_access *csa = &drvdata->csdev->access;
> > +
> > +	return coresight_timeout(csa, CATU_STATUS, CATU_STATUS_READY, 1);
> >   }
> >   
> >   static int catu_enable_hw(struct catu_drvdata *drvdata, void *data)
> > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > index e96deaca8cab..42ba989a6b5e 100644
> > --- a/drivers/hwtracing/coresight/coresight-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > @@ -1412,23 +1412,26 @@ static void coresight_remove_conns(struct coresight_device *csdev)
> >   }
> >   
> >   /**
> > - * coresight_timeout - loop until a bit has changed to a specific state.
> > - * @addr: base address of the area of interest.
> > - * @offset: address of a register, starting from @addr.
> > + * coresight_timeout - loop until a bit has changed to a specific register
> > + *			state.
> > + * @csa: coresight device access for the device
> > + * @offset: Offset of the register from the base of the device.
> >    * @position: the position of the bit of interest.
> >    * @value: the value the bit should have.
> >    *
> >    * Return: 0 as soon as the bit has taken the desired state or -EAGAIN if
> >    * TIMEOUT_US has elapsed, which ever happens first.
> >    */
> > -
> > -int coresight_timeout(void __iomem *addr, u32 offset, int position, int value)
> > +int coresight_timeout(struct csdev_access *csa,
> > +		      u32 offset,
> > +		      int position,
> > +		      int value)
>
> There is no need for stacking, please maximise the 80 characters.  The function
> stubs in coresight.h should also be revised.
>
> >   {
> >   	int i;
> >   	u32 val;
> >   
> >   	for (i = TIMEOUT_US; i > 0; i--) {
> > -		val = __raw_readl(addr + offset);
> > +		val = csdev_access_read32(csa, offset);
>
> I vaguely remember commenting on this, or perhaps it was on some othe patch you
> wrote...  Anyways, I think it is a good thing to go from an unordered access to
> an ordered access for the timeout function.

Yes, you did mention it in the v1.

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

Thanks, will address the comments.

Suzuki