[PATCH v2 1/3] coresight: Update timeout functions to allow return of test register value

[Mike Leach]

Hi Suzuki,

On Fri, 13 Dec 2024 at 16:38, Suzuki K Poulose <suzuki.poulose at arm.com> wrote:
>
> Hi Mike
>
> On 13/12/2024 14:49, Mike Leach wrote:
> > Current coresight_timeout function spins on a bit on a test register,
> > till bit value achieved or timeout hit.
> >
> > Add another function to return the full value of the register being
> > tested.
> >
>
> The patch looks good to me. However, this change doesn't bring much
> value, other than avoiding a read at the caller side. I guess it is
> simpler to leave things as is and let the caller read the register value
> while logging rather than providing another variant.
>

The reason for returning the register value is that you get the exact
read value used at the time of the test - not potentially a different
value read later.


> I am inclined to keep the interface simpler.
>
>
> > Signed-off-by: Mike Leach <mike.leach at linaro.org>
> > ---
> >   drivers/hwtracing/coresight/coresight-core.c | 50 +++++++++++++++-----
> >   include/linux/coresight.h                    |  2 +
> >   2 files changed, 41 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > index ea38ecf26fcb..feb1a1db355f 100644
> > --- a/drivers/hwtracing/coresight/coresight-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > @@ -1017,32 +1017,37 @@ static void coresight_remove_conns(struct coresight_device *csdev)
> >   }
> >
> >   /**
> > - * coresight_timeout - loop until a bit has changed to a specific register
> > - *                   state.
> > + * coresight_timeout_retval - loop until a bit has changed to a specific register
> > + *                         state. Return final register value
> >    * @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.
> > + * @rval:  the last read value of the register being tested.
> >    *
> >    * 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(struct csdev_access *csa, u32 offset,
> > -                   int position, int value)
> > +int coresight_timeout_retval(struct csdev_access *csa, u32 offset,
> > +                          int position, int value, u32 *rval)
> >   {
> > -     int i;
> > -     u32 val;
> > +     int i, rc = -EAGAIN;
> > +     u32 val = 0;
> >
> >       for (i = TIMEOUT_US; i > 0; i--) {
> >               val = csdev_access_read32(csa, offset);
> >               /* waiting on the bit to go from 0 to 1 */
> >               if (value) {
> > -                     if (val & BIT(position))
> > -                             return 0;
> > +                     if (val & BIT(position)) {
> > +                             rc = 0;
> > +                             goto return_rval;
> > +                     }
> >               /* waiting on the bit to go from 1 to 0 */
> >               } else {
> > -                     if (!(val & BIT(position)))
> > -                             return 0;
> > +                     if (!(val & BIT(position))) {
> > +                             rc = 0;
> > +                             goto return_rval;
> > +                     }
> >               }
> >
> >               /*
> > @@ -1054,7 +1059,30 @@ int coresight_timeout(struct csdev_access *csa, u32 offset,
> >                       udelay(1);
> >       }
> >
> > -     return -EAGAIN;
> > +return_rval:
> > +     *rval = val;
>
> super minor nit: we could be flexible in accepting a "NULL" rval and
> thus keep the normal variants pass the NULL straight in. i.e,
>
>         if (rval)
>                 *rval = val;
>

Sure.

Mike
> > +
> > +     return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(coresight_timeout_retval);
> > +
> > +/**
> > + * 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(struct csdev_access *csa, u32 offset,
> > +                   int position, int value)
> > +{
> > +     u32 rval = 0;
> > +
> > +     return coresight_timeout_retval(csa, offset, position, value, &rval);
>
>         return core_timeout_retval(csa, offset, position, value, NULL);
>
> Suzuki
>
> >   }
> >   EXPORT_SYMBOL_GPL(coresight_timeout);
> >
> > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > index 055ce5cd5c44..29cb71e82b0b 100644
> > --- a/include/linux/coresight.h
> > +++ b/include/linux/coresight.h
> > @@ -639,6 +639,8 @@ extern int coresight_enable_sysfs(struct coresight_device *csdev);
> >   extern void coresight_disable_sysfs(struct coresight_device *csdev);
> >   extern int coresight_timeout(struct csdev_access *csa, u32 offset,
> >                            int position, int value);
> > +extern int coresight_timeout_retval(struct csdev_access *csa, u32 offset,
> > +                                 int position, int value, u32 *rval);
> >
> >   extern int coresight_claim_device(struct coresight_device *csdev);
> >   extern int coresight_claim_device_unlocked(struct coresight_device *csdev);
>


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK