lkml.org 
[lkml]   [2022]   [Aug]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v1 2/4] coresight: Simplify sysfs accessors by using csdev_access abstraction
[...]

> >>
> >> static struct attribute *coresight_tmc_mgmt_attrs[] = {
> >> &dev_attr_rsz.attr,
> >> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> >> index 9f445f09fcfe..c1bb93c7c1de 100644
> >> --- a/include/linux/coresight.h
> >> +++ b/include/linux/coresight.h
> >> @@ -372,6 +372,24 @@ static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,
> >> return csa->read(offset, true, false);
> >> }
> >>
> >> +static inline u64 csdev_access_relaxed_read_pair(struct csdev_access *csa,
> >> + u32 lo_offset, u32 hi_offset)
> >
> > Parameters lo_offset and hi_offset are s32 in coresight_read_reg_pair()...
>
> Hi Mathieu,
>
> I probably should have mentioned this in the commit message. You're
> right that the previous version used signed values, but the csdev
> accessors in include/linux/coresight.h all use u32 and I had to add a
> new one in there for 'csdev_access_relaxed_read_pair()' which would have
> looked very out of place if it was the only one to used signed values.
>
> Because of this I also changed the 'not set' test for hi_offset from '<
> -1' to '== -1' which would also work with unsigned values. So although
> it looks different, it is still working the same way as before.
>
> I can think of some possible options to make it better:
>
> * Have csdev_access_relaxed_read_pair() be the only csdev_access_
> function to take signed values

That part is not a big deal for me.

>
> * Keep the unsigned type but change the unset value of -1 to be
> UINT_MAX

I find this counterintuitive and error prone. And sparse will likely yell
at you profusely.

>
> * Split the accessors into ones that are 64 bit pairs, and ones that
> are a single read. It's always known when it's defined whether it's
> a 'pair' or not, so technically this if statement with the 'not set'
> value isn't actually needed, you just use a different accessor type
>

That would work.

> I was tempted to do the 3rd one during the refactor, but I wanted to
> keep it more like the original than not. I'm not a fan of the first
> option, I think that would be confusing to read the code and would look
> like a mistake. So I'm more in favour of 2 or 3. What are your thoughts?

Let's meet in the middle and go with option 3.

Thanks,
Mathieu

>
> >
> >> +{
> >> + u64 val;
> >> +
> >> + if (likely(csa->io_mem)) {
> >> + val = readl_relaxed(csa->base + lo_offset);
> >> + val |= (hi_offset == -1) ? 0 :
> >> + (u64)readl_relaxed(csa->base + hi_offset) << 32;
> >> + return val;
> >> + }
> >> +
> >> + val = csa->read(lo_offset, true, false);
> >> + val |= (hi_offset == -1) ? 0 :
> >
> > ... and hi_offset can't take on a negative value.
>
> This is just shorthand for UINT_MAX. I could change it to be more
> explicit (option 2 above)?
>
> >
> >> + (u64)csa->read(hi_offset, true, false) << 32;
> >> + return val;
> >> +}
> >> +
> >> static inline u32 csdev_access_read32(struct csdev_access *csa, u32 offset)
> >> {
> >> if (likely(csa->io_mem))
> >> --
> >> 2.28.0
> >>

\
 
 \ /
  Last update: 2022-08-25 18:55    [W:0.062 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site