From: Mike Leach <mike.leach@linaro.org> To: Suzuki Kuruppassery Poulose <suzuki.poulose@arm.com> Cc: Coresight ML <coresight@lists.linaro.org>, linux-arm-kernel <linux-arm-kernel@lists.infradead.org>, devicetree@vger.kernel.org, "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>, Mathieu Poirier <mathieu.poirier@linaro.org> Subject: Re: [PATCH v5 03/14] coresight: cti: Add sysfs access to program function regs Date: Fri, 29 Nov 2019 12:47:20 +0000 [thread overview] Message-ID: <CAJ9a7VhV=5okV1oCW8sAUZVsUzgDWnDNgP-eWejquGx+M9Xv2w@mail.gmail.com> (raw) In-Reply-To: <df1f3912-4096-bc96-e65a-5db1593ad8f4@arm.com> Hi Suzuki, On Wed, 27 Nov 2019 at 18:26, Suzuki Kuruppassery Poulose <suzuki.poulose@arm.com> wrote: > > On 19/11/2019 23:19, Mike Leach wrote: > > Adds in sysfs programming support for the CTI function register sets. > > Allows direct manipulation of channel / trigger association registers. > > > > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > Signed-off-by: Mike Leach <mike.leach@linaro.org> > > --- > > .../hwtracing/coresight/coresight-cti-sysfs.c | 362 ++++++++++++++++++ > > drivers/hwtracing/coresight/coresight-cti.c | 19 + > > drivers/hwtracing/coresight/coresight-cti.h | 5 + > > 3 files changed, 386 insertions(+) > > > > diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c > > index 507f8eb487fe..02d3ee0c1278 100644 > > --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c > > +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c > > @@ -109,6 +109,362 @@ static struct attribute *coresight_cti_mgmt_attrs[] = { > > NULL, > > }; > > > > +/* CTI low level programming registers */ > > + > > +/* > > + * Show a simple 32 bit value if enabled and powered. > > + * If inaccessible & pcached_val not NULL then show cached value. > > + */ > > Also I am not sure if it makes sense to mention that the value is cached. > > > +static ssize_t cti_reg32_show(struct device *dev, char *buf, > > + u32 *pcached_val, int reg_offset) > > +{ > > + u32 val = 0; > + char *state = ""; > > > + struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > > + struct cti_config *config = &drvdata->config; > > + > > + spin_lock(&drvdata->spinlock); > > + if ((reg_offset >= 0) && CTI_PWR_ENA(config)) { > > minor nit: Personally I don't like the naming here. This could simply > be: cti_accessible(config) , may be defined as a static inline function > instead of a macro: > > static inline bool cti_accessible(struct cti_drvdata *drvdata) > { > struct cti_config *cfg = &drvdata->config; > > return cfg->hw_powered && cfg->hw_enabled; > } > > Since this is a generic access function used throughout the file - the cached pointer is an indicator used by the callee that there is a value available if the CTI is unpowered / disabled - so the function can show an appropriate value which will be taken from the config structure. So I don't think it is relevant to show that a "cached" value is being used to show the user. If you look at similar functions in the ETM drivers for example, quite often a show function simple shows that stored value from a config structure without ever looking at the register in the device. As to naming - the name is chosen to represent a specific state - both powered and enabled. The sysfs interface is accessible in any state - powered / unpowered , enabled /disabled - so I am being specific. Unlike the ETM, this hardware can have registers programmed while enabled - and for some such as apppulse this is the only time it makes sense to use them. I don't mind either way between macro / inline function - though it still has to be declared in the header as it is used in multiple .c files. I'd be inclined to call it cti_active() if preferred to cti_pwr_ena - active implies that the CTI is in operation. Thanks Mike > > + CS_UNLOCK(drvdata->base); > > + val = readl_relaxed(drvdata->base + reg_offset); > > + if (pcached_val) > > + *pcached_val = val; > > + CS_LOCK(drvdata->base); > > + } else if (pcached_val) { > > + val = *pcached_val; > > + state = " (cached)"; > > + } > > + spin_unlock(&drvdata->spinlock); > > + return scnprintf(buf, PAGE_SIZE, "%#x\n", val); > + return scnprintf(buf, PAGE_SIZE, "%#x%s\n", val, state); > > > +} > > + > > +/* > > + * Store a simple 32 bit value. > > + * If pcached_val not NULL, then copy to here too, > > + * if reg_offset >= 0 then write through if enabled. > > + */ > > +static ssize_t cti_reg32_store(struct device *dev, const char *buf, > > + size_t size, u32 *pcached_val, int reg_offset) > > > > +static ssize_t appclear_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t size) > > +{ > > + unsigned long val; > > + struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > > + struct cti_config *config = &drvdata->config; > > + > > + if (kstrtoul(buf, 0, &val)) > > + return -EINVAL; > > + > > + spin_lock(&drvdata->spinlock); > > + > > + /* a 1'b1 in appclr clears down the same bit in appset*/ > > nit: a 0b1 ? > Syntax is <bitwidth>'<radix><value> - a habit picked up from verilog. > > + config->ctiappset &= ~val; > > + > > + /* write through if enabled */ > > + if (CTI_PWR_ENA(config)) > > + cti_write_single_reg(drvdata, CTIAPPCLEAR, val); > > + spin_unlock(&drvdata->spinlock); > > + return size; > > +} > > +static DEVICE_ATTR_WO(appclear); > > + > > Otherwise looks good to me. > > Suzuki -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
WARNING: multiple messages have this Message-ID (diff)
From: Mike Leach <mike.leach@linaro.org> To: Suzuki Kuruppassery Poulose <suzuki.poulose@arm.com> Cc: devicetree@vger.kernel.org, Coresight ML <coresight@lists.linaro.org>, Mathieu Poirier <mathieu.poirier@linaro.org>, linux-arm-kernel <linux-arm-kernel@lists.infradead.org>, "open list:DOCUMENTATION" <linux-doc@vger.kernel.org> Subject: Re: [PATCH v5 03/14] coresight: cti: Add sysfs access to program function regs Date: Fri, 29 Nov 2019 12:47:20 +0000 [thread overview] Message-ID: <CAJ9a7VhV=5okV1oCW8sAUZVsUzgDWnDNgP-eWejquGx+M9Xv2w@mail.gmail.com> (raw) In-Reply-To: <df1f3912-4096-bc96-e65a-5db1593ad8f4@arm.com> Hi Suzuki, On Wed, 27 Nov 2019 at 18:26, Suzuki Kuruppassery Poulose <suzuki.poulose@arm.com> wrote: > > On 19/11/2019 23:19, Mike Leach wrote: > > Adds in sysfs programming support for the CTI function register sets. > > Allows direct manipulation of channel / trigger association registers. > > > > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > Signed-off-by: Mike Leach <mike.leach@linaro.org> > > --- > > .../hwtracing/coresight/coresight-cti-sysfs.c | 362 ++++++++++++++++++ > > drivers/hwtracing/coresight/coresight-cti.c | 19 + > > drivers/hwtracing/coresight/coresight-cti.h | 5 + > > 3 files changed, 386 insertions(+) > > > > diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c > > index 507f8eb487fe..02d3ee0c1278 100644 > > --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c > > +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c > > @@ -109,6 +109,362 @@ static struct attribute *coresight_cti_mgmt_attrs[] = { > > NULL, > > }; > > > > +/* CTI low level programming registers */ > > + > > +/* > > + * Show a simple 32 bit value if enabled and powered. > > + * If inaccessible & pcached_val not NULL then show cached value. > > + */ > > Also I am not sure if it makes sense to mention that the value is cached. > > > +static ssize_t cti_reg32_show(struct device *dev, char *buf, > > + u32 *pcached_val, int reg_offset) > > +{ > > + u32 val = 0; > + char *state = ""; > > > + struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > > + struct cti_config *config = &drvdata->config; > > + > > + spin_lock(&drvdata->spinlock); > > + if ((reg_offset >= 0) && CTI_PWR_ENA(config)) { > > minor nit: Personally I don't like the naming here. This could simply > be: cti_accessible(config) , may be defined as a static inline function > instead of a macro: > > static inline bool cti_accessible(struct cti_drvdata *drvdata) > { > struct cti_config *cfg = &drvdata->config; > > return cfg->hw_powered && cfg->hw_enabled; > } > > Since this is a generic access function used throughout the file - the cached pointer is an indicator used by the callee that there is a value available if the CTI is unpowered / disabled - so the function can show an appropriate value which will be taken from the config structure. So I don't think it is relevant to show that a "cached" value is being used to show the user. If you look at similar functions in the ETM drivers for example, quite often a show function simple shows that stored value from a config structure without ever looking at the register in the device. As to naming - the name is chosen to represent a specific state - both powered and enabled. The sysfs interface is accessible in any state - powered / unpowered , enabled /disabled - so I am being specific. Unlike the ETM, this hardware can have registers programmed while enabled - and for some such as apppulse this is the only time it makes sense to use them. I don't mind either way between macro / inline function - though it still has to be declared in the header as it is used in multiple .c files. I'd be inclined to call it cti_active() if preferred to cti_pwr_ena - active implies that the CTI is in operation. Thanks Mike > > + CS_UNLOCK(drvdata->base); > > + val = readl_relaxed(drvdata->base + reg_offset); > > + if (pcached_val) > > + *pcached_val = val; > > + CS_LOCK(drvdata->base); > > + } else if (pcached_val) { > > + val = *pcached_val; > > + state = " (cached)"; > > + } > > + spin_unlock(&drvdata->spinlock); > > + return scnprintf(buf, PAGE_SIZE, "%#x\n", val); > + return scnprintf(buf, PAGE_SIZE, "%#x%s\n", val, state); > > > +} > > + > > +/* > > + * Store a simple 32 bit value. > > + * If pcached_val not NULL, then copy to here too, > > + * if reg_offset >= 0 then write through if enabled. > > + */ > > +static ssize_t cti_reg32_store(struct device *dev, const char *buf, > > + size_t size, u32 *pcached_val, int reg_offset) > > > > +static ssize_t appclear_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t size) > > +{ > > + unsigned long val; > > + struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > > + struct cti_config *config = &drvdata->config; > > + > > + if (kstrtoul(buf, 0, &val)) > > + return -EINVAL; > > + > > + spin_lock(&drvdata->spinlock); > > + > > + /* a 1'b1 in appclr clears down the same bit in appset*/ > > nit: a 0b1 ? > Syntax is <bitwidth>'<radix><value> - a habit picked up from verilog. > > + config->ctiappset &= ~val; > > + > > + /* write through if enabled */ > > + if (CTI_PWR_ENA(config)) > > + cti_write_single_reg(drvdata, CTIAPPCLEAR, val); > > + spin_unlock(&drvdata->spinlock); > > + return size; > > +} > > +static DEVICE_ATTR_WO(appclear); > > + > > Otherwise looks good to me. > > Suzuki -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-11-29 12:47 UTC|newest] Thread overview: 124+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-19 23:18 [PATCH v5 00/14] CoreSight CTI Driver Mike Leach 2019-11-19 23:18 ` Mike Leach 2019-11-19 23:18 ` [PATCH v5 01/14] coresight: cti: Initial " Mike Leach 2019-11-19 23:18 ` Mike Leach 2019-11-21 20:21 ` Mathieu Poirier 2019-11-21 20:21 ` Mathieu Poirier 2019-11-29 12:05 ` Mike Leach 2019-11-29 12:05 ` Mike Leach 2019-12-03 16:53 ` Mathieu Poirier 2019-12-03 16:53 ` Mathieu Poirier 2019-11-25 19:03 ` Suzuki Kuruppassery Poulose 2019-11-25 19:03 ` Suzuki Kuruppassery Poulose 2019-11-29 12:06 ` Mike Leach 2019-11-29 12:06 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 02/14] coresight: cti: Add sysfs coresight mgmt reg access Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-22 17:19 ` Mathieu Poirier 2019-11-22 17:19 ` Mathieu Poirier 2019-11-19 23:19 ` [PATCH v5 03/14] coresight: cti: Add sysfs access to program function regs Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-27 18:26 ` Suzuki Kuruppassery Poulose 2019-11-27 18:26 ` Suzuki Kuruppassery Poulose 2019-11-29 12:47 ` Mike Leach [this message] 2019-11-29 12:47 ` Mike Leach 2019-11-28 10:54 ` Suzuki Kuruppassery Poulose 2019-11-28 10:54 ` Suzuki Kuruppassery Poulose 2019-11-28 17:20 ` Mathieu Poirier 2019-11-28 17:20 ` Mathieu Poirier 2019-11-28 18:00 ` Suzuki Kuruppassery Poulose 2019-11-28 18:00 ` Suzuki Kuruppassery Poulose 2019-11-29 12:50 ` Mike Leach 2019-11-29 12:50 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 04/14] coresight: cti: Add sysfs trigger / channel programming API Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-22 18:40 ` Mathieu Poirier 2019-11-22 18:40 ` Mathieu Poirier 2019-11-27 18:40 ` Suzuki Kuruppassery Poulose 2019-11-27 18:40 ` Suzuki Kuruppassery Poulose 2019-11-29 13:01 ` Mike Leach 2019-11-29 13:01 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 05/14] dt-bindings: arm: Adds CoreSight CTI hardware definitions Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-20 19:06 ` Mathieu Poirier 2019-11-20 19:06 ` Mathieu Poirier 2019-11-20 22:39 ` Mike Leach 2019-11-20 22:39 ` Mike Leach 2019-11-22 23:33 ` Rob Herring 2019-11-22 23:33 ` Rob Herring 2019-11-29 13:50 ` Mike Leach 2019-11-29 13:50 ` Mike Leach 2019-11-29 14:12 ` Suzuki Kuruppassery Poulose 2019-11-29 14:12 ` Suzuki Kuruppassery Poulose 2019-11-28 18:38 ` Suzuki Kuruppassery Poulose 2019-11-28 18:38 ` Suzuki Kuruppassery Poulose 2019-11-29 13:57 ` Mike Leach 2019-11-29 13:57 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 06/14] coresight: cti: Add device tree support for v8 arch CTI Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-25 19:00 ` Mathieu Poirier 2019-11-25 19:00 ` Mathieu Poirier 2019-11-29 11:33 ` Suzuki Kuruppassery Poulose 2019-11-29 11:33 ` Suzuki Kuruppassery Poulose 2019-12-03 10:59 ` Mike Leach 2019-12-03 10:59 ` Mike Leach 2019-12-03 11:28 ` Suzuki Kuruppassery Poulose 2019-12-03 11:28 ` Suzuki Kuruppassery Poulose 2019-12-03 12:25 ` Mike Leach 2019-12-03 12:25 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 07/14] coresight: cti: Add device tree support for custom CTI Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-25 21:22 ` Mathieu Poirier 2019-11-25 21:22 ` Mathieu Poirier 2019-11-29 14:16 ` Suzuki Kuruppassery Poulose 2019-11-29 14:16 ` Suzuki Kuruppassery Poulose 2019-11-29 21:11 ` Mathieu Poirier 2019-11-29 21:11 ` Mathieu Poirier 2019-11-29 14:18 ` Suzuki Kuruppassery Poulose 2019-11-29 14:18 ` Suzuki Kuruppassery Poulose 2019-12-03 14:05 ` Mike Leach 2019-12-03 14:05 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 08/14] coresight: cti: Enable CTI associated with devices Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-25 22:45 ` Mathieu Poirier 2019-11-25 22:45 ` Mathieu Poirier 2019-12-05 16:33 ` Mike Leach 2019-12-05 16:33 ` Mike Leach 2019-11-29 18:28 ` Suzuki Kuruppassery Poulose 2019-11-29 18:28 ` Suzuki Kuruppassery Poulose 2019-11-29 21:25 ` Mathieu Poirier 2019-11-29 21:25 ` Mathieu Poirier 2019-12-05 16:33 ` Mike Leach 2019-12-05 16:33 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 09/14] coresight: cti: Add connection information to sysfs Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-27 18:09 ` Mathieu Poirier 2019-11-27 18:09 ` Mathieu Poirier 2019-12-06 16:24 ` Mike Leach 2019-12-06 16:24 ` Mike Leach 2019-12-02 9:47 ` Suzuki Kuruppassery Poulose 2019-12-02 9:47 ` Suzuki Kuruppassery Poulose 2019-12-06 16:24 ` Mike Leach 2019-12-06 16:24 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 10/14] dt-bindings: qcom: Add CTI options for qcom msm8916 Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-27 18:18 ` Mathieu Poirier 2019-11-27 18:18 ` Mathieu Poirier 2019-11-19 23:19 ` [PATCH v5 11/14] dt-bindings: arm: Juno platform - add CTI entries to device tree Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-27 18:25 ` Mathieu Poirier 2019-11-27 18:25 ` Mathieu Poirier 2019-11-19 23:19 ` [PATCH v5 12/14] dt-bindings: hisilicon: Add CTI bindings for hi-6220 Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 13/14] docs: coresight: Update documentation for CoreSight to cover CTI Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-27 19:00 ` Mathieu Poirier 2019-11-27 19:00 ` Mathieu Poirier 2019-12-02 10:43 ` Suzuki Kuruppassery Poulose 2019-12-02 10:43 ` Suzuki Kuruppassery Poulose 2019-12-06 17:39 ` Mike Leach 2019-12-06 17:39 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 14/14] docs: sysfs: coresight: Add sysfs ABI documentation for CTI Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-27 19:08 ` Mathieu Poirier 2019-11-27 19:08 ` Mathieu Poirier
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAJ9a7VhV=5okV1oCW8sAUZVsUzgDWnDNgP-eWejquGx+M9Xv2w@mail.gmail.com' \ --to=mike.leach@linaro.org \ --cc=coresight@lists.linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-doc@vger.kernel.org \ --cc=mathieu.poirier@linaro.org \ --cc=suzuki.poulose@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.