Re: [PATCH 02/13] coresight: cti: Add sysfs coresight mgmt register access

From: Greg KH <gregkh@linuxfoundation.org>
To: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 02/13] coresight: cti: Add sysfs coresight mgmt register access
Date: Thu, 19 Mar 2020 08:54:14 +0100	[thread overview]
Message-ID: <20200319075414.GA3445010@kroah.com> (raw)
In-Reply-To: <CANLsYkxkg1bCCN=iuSQBF_oG-wWwrSvEC2pLNVkP64EgTfVAvg@mail.gmail.com>

On Wed, Mar 18, 2020 at 01:28:05PM -0600, Mathieu Poirier wrote:
> On Wed, 18 Mar 2020 at 12:22, Greg KH <gregkh@linuxfoundation.org> 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@linaro.org>
> > > > >
> > > > > Adds sysfs access to the coresight management registers.
> > > > >
> > > > > Signed-off-by: Mike Leach <mike.leach@linaro.org>
> > > > > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > > > > Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > > > [Fixed abbreviation in title]
> > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > > > > ---
> > > > >  .../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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel