Re: [PATCH 02/13] coresight: cti: Add sysfs coresight mgmt register access
From: Greg KH
Date: Thu Mar 19 2020 - 03:54:20 EST
On Wed, Mar 18, 2020 at 01:28:05PM -0600, Mathieu Poirier wrote:
> On Wed, 18 Mar 2020 at 12:22, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Wed, Mar 18, 2020 at 12:16:04PM -0600, Mathieu Poirier wrote:
> > > On Wed, Mar 18, 2020 at 02:18:21PM +0100, Greg KH wrote:
> > > > On Mon, Mar 09, 2020 at 10:17:37AM -0600, Mathieu Poirier wrote:
> > > > > From: Mike Leach <mike.leach@xxxxxxxxxx>
> > > > >
> > > > > Adds sysfs access to the coresight management registers.
> > > > >
> > > > > Signed-off-by: Mike Leach <mike.leach@xxxxxxxxxx>
> > > > > Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> > > > > Reviewed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > > > > [Fixed abbreviation in title]
> > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> > > > > ---
> > > > > .../hwtracing/coresight/coresight-cti-sysfs.c | 53 +++++++++++++++++++
> > > > > drivers/hwtracing/coresight/coresight-priv.h | 1 +
> > > > > 2 files changed, 54 insertions(+)
> > > > >
> > > > > diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> > > > > index a832b8c6b866..507f8eb487fe 100644
> > > > > --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> > > > > +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> > > > > @@ -62,11 +62,64 @@ static struct attribute *coresight_cti_attrs[] = {
> > > > > NULL,
> > > > > };
> > > > >
> > > > > +/* register based attributes */
> > > > > +
> > > > > +/* macro to access RO registers with power check only (no enable check). */
> > > > > +#define coresight_cti_reg(name, offset) \
> > > > > +static ssize_t name##_show(struct device *dev, \
> > > > > + struct device_attribute *attr, char *buf) \
> > > > > +{ \
> > > > > + struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); \
> > > > > + u32 val = 0; \
> > > > > + pm_runtime_get_sync(dev->parent); \
> > > > > + spin_lock(&drvdata->spinlock); \
> > > > > + if (drvdata->config.hw_powered) \
> > > > > + val = readl_relaxed(drvdata->base + offset); \
> > > > > + spin_unlock(&drvdata->spinlock); \
> > > > > + pm_runtime_put_sync(dev->parent); \
> > > > > + return scnprintf(buf, PAGE_SIZE, "0x%x\n", val); \
> > > > > +} \
> > > > > +static DEVICE_ATTR_RO(name)
> > > > > +
> > > > > +/* coresight management registers */
> > > > > +coresight_cti_reg(devaff0, CTIDEVAFF0);
> > > > > +coresight_cti_reg(devaff1, CTIDEVAFF1);
> > > > > +coresight_cti_reg(authstatus, CORESIGHT_AUTHSTATUS);
> > > > > +coresight_cti_reg(devarch, CORESIGHT_DEVARCH);
> > > > > +coresight_cti_reg(devid, CORESIGHT_DEVID);
> > > > > +coresight_cti_reg(devtype, CORESIGHT_DEVTYPE);
> > > > > +coresight_cti_reg(pidr0, CORESIGHT_PERIPHIDR0);
> > > > > +coresight_cti_reg(pidr1, CORESIGHT_PERIPHIDR1);
> > > > > +coresight_cti_reg(pidr2, CORESIGHT_PERIPHIDR2);
> > > > > +coresight_cti_reg(pidr3, CORESIGHT_PERIPHIDR3);
> > > > > +coresight_cti_reg(pidr4, CORESIGHT_PERIPHIDR4);
> > > > > +
> > > > > +static struct attribute *coresight_cti_mgmt_attrs[] = {
> > > > > + &dev_attr_devaff0.attr,
> > > > > + &dev_attr_devaff1.attr,
> > > > > + &dev_attr_authstatus.attr,
> > > > > + &dev_attr_devarch.attr,
> > > > > + &dev_attr_devid.attr,
> > > > > + &dev_attr_devtype.attr,
> > > > > + &dev_attr_pidr0.attr,
> > > > > + &dev_attr_pidr1.attr,
> > > > > + &dev_attr_pidr2.attr,
> > > > > + &dev_attr_pidr3.attr,
> > > > > + &dev_attr_pidr4.attr,
> > > > > + NULL,
> > > > > +};
> > > > > +
> > > > > static const struct attribute_group coresight_cti_group = {
> > > > > .attrs = coresight_cti_attrs,
> > > > > };
> > > > >
> > > > > +static const struct attribute_group coresight_cti_mgmt_group = {
> > > > > + .attrs = coresight_cti_mgmt_attrs,
> > > > > + .name = "mgmt",
> > > > > +};
> > > > > +
> > > > > const struct attribute_group *coresight_cti_groups[] = {
> > > > > &coresight_cti_group,
> > > > > + &coresight_cti_mgmt_group,
> > > > > NULL,
> > > > > };
> > > > > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> > > > > index 82e563cdc879..aba6b789c969 100644
> > > > > --- a/drivers/hwtracing/coresight/coresight-priv.h
> > > > > +++ b/drivers/hwtracing/coresight/coresight-priv.h
> > > > > @@ -22,6 +22,7 @@
> > > > > #define CORESIGHT_CLAIMCLR 0xfa4
> > > > > #define CORESIGHT_LAR 0xfb0
> > > > > #define CORESIGHT_LSR 0xfb4
> > > > > +#define CORESIGHT_DEVARCH 0xfbc
> > > > > #define CORESIGHT_AUTHSTATUS 0xfb8
> > > > > #define CORESIGHT_DEVID 0xfc8
> > > > > #define CORESIGHT_DEVTYPE 0xfcc
> > > > > --
> > > > > 2.20.1
> > > > >
> > > >
> > > > I do not see any Documentation/ABI/ entries for these new sysfs files,
> > > > did I miss it somehow? I can't take new sysfs code without
> > > > documentation.
> > >
> > > All the ABI is documented in this patch, which is part of this set.
> > >
> > > [1]. https://lkml.org/lkml/2020/3/9/642
> >
> > That is not in the required Documentation/ABI/ form that all sysfs files
> > should have. If they don't, it's a bug.
>
> Now I'm very confused... As far as I can tell Mike has followed the
> (very loose) guidelines set out in the ABI documentation [1]. I have
> also taken a look at the patches that were merged in the v5.5 cycle -
> nothing is very different than what Mike has put together. It is also
> based on the work I did a while back [2] that you merged.
{sigh}
I churned through 1200+ patches yesterday to get caught up and I think I
totally missed this, sorry. Yes, that is the correct format, I was
looking at the .rst files you added to this series and missed this API
file instead.
Ugh, my fault.
Can you just resend this whole series and I'll go through it again?
thanks,
greg k-h