by Mike Leach

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] coresight: Make new csdev_access offsets unsigned

On Tue, 30 Aug 2022 at 18:26, James Clark <[email protected]> wrote:
>
> New csdev_access functions were added as part of the previous
> refactor. In order to make them more consistent with the
> existing ones, change any signed offset types to be unsigned.
>
> Now that they are unsigned, stop using hi_off = -1 to signify
> a single 32bit access. Instead just call the existing 32bit
> accessors. This is also applied to other parts of the codebase,
> and the coresight_{read,write}_reg_pair() functions can be
> deleted.
>
> Signed-off-by: James Clark <[email protected]>
> ---
> drivers/hwtracing/coresight/coresight-catu.h | 8 ++---
> drivers/hwtracing/coresight/coresight-core.c | 18 ++++++++--
> drivers/hwtracing/coresight/coresight-priv.h | 35 +++++---------------
> drivers/hwtracing/coresight/coresight-tmc.h | 4 +--
> include/linux/coresight.h | 27 +++++++++------
> 5 files changed, 47 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h
> index 6160c2d75a56..442e034bbfba 100644
> --- a/drivers/hwtracing/coresight/coresight-catu.h
> +++ b/drivers/hwtracing/coresight/coresight-catu.h
> @@ -70,24 +70,24 @@ struct catu_drvdata {
> static inline u32 \
> catu_read_##name(struct catu_drvdata *drvdata) \
> { \
> - return coresight_read_reg_pair(drvdata->base, offset, -1); \
> + return csdev_access_relaxed_read32(&drvdata->csdev->access, offset); \
> } \
> static inline void \
> catu_write_##name(struct catu_drvdata *drvdata, u32 val) \
> { \
> - coresight_write_reg_pair(drvdata->base, val, offset, -1); \
> + csdev_access_relaxed_write32(&drvdata->csdev->access, val, offset); \
> }
>
> #define CATU_REG_PAIR(name, lo_off, hi_off) \
> static inline u64 \
> catu_read_##name(struct catu_drvdata *drvdata) \
> { \
> - return coresight_read_reg_pair(drvdata->base, lo_off, hi_off); \
> + return csdev_access_relaxed_read_pair(&drvdata->csdev->access, lo_off, hi_off); \
> } \
> static inline void \
> catu_write_##name(struct catu_drvdata *drvdata, u64 val) \
> { \
> - coresight_write_reg_pair(drvdata->base, val, lo_off, hi_off); \
> + csdev_access_relaxed_write_pair(&drvdata->csdev->access, val, lo_off, hi_off); \
> }
>
> CATU_REG32(control, CATU_CONTROL);
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index c63b2167a69f..d5dbc67bacb4 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -60,7 +60,7 @@ EXPORT_SYMBOL_GPL(coresight_barrier_pkt);
>
> static const struct cti_assoc_op *cti_assoc_ops;
>
> -ssize_t coresight_simple_show(struct device *_dev,
> +ssize_t coresight_simple_show_pair(struct device *_dev,
> struct device_attribute *attr, char *buf)
> {
> struct coresight_device *csdev = container_of(_dev, struct coresight_device, dev);
> @@ -72,7 +72,21 @@ ssize_t coresight_simple_show(struct device *_dev,
> pm_runtime_put_sync(_dev->parent);
> return sysfs_emit(buf, "0x%llx\n", val);
> }
> -EXPORT_SYMBOL_GPL(coresight_simple_show);
> +EXPORT_SYMBOL_GPL(coresight_simple_show_pair);
> +
> +ssize_t coresight_simple_show32(struct device *_dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct coresight_device *csdev = container_of(_dev, struct coresight_device, dev);
> + struct cs_off_attribute *cs_attr = container_of(attr, struct cs_off_attribute, attr);
> + u64 val;
> +
> + pm_runtime_get_sync(_dev->parent);
> + val = csdev_access_relaxed_read32(&csdev->access, cs_attr->off);
> + pm_runtime_put_sync(_dev->parent);
> + return sysfs_emit(buf, "0x%llx\n", val);
> +}
> +EXPORT_SYMBOL_GPL(coresight_simple_show32);
>
> void coresight_set_cti_ops(const struct cti_assoc_op *cti_op)
> {
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index c211979deca5..595ce5862056 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -41,8 +41,8 @@
> #define ETM_MODE_EXCL_USER BIT(31)
> struct cs_pair_attribute {
> struct device_attribute attr;
> - s32 lo_off;
> - s32 hi_off;
> + u32 lo_off;
> + u32 hi_off;
> };
>
> struct cs_off_attribute {
> @@ -50,21 +50,23 @@ struct cs_off_attribute {
> u32 off;
> };
>
> -extern ssize_t coresight_simple_show(struct device *_dev,
> +extern ssize_t coresight_simple_show32(struct device *_dev,
> + struct device_attribute *attr, char *buf);
> +extern ssize_t coresight_simple_show_pair(struct device *_dev,
> struct device_attribute *attr, char *buf);
>
> #define coresight_simple_reg32(name, offset) \
> - (&((struct cs_pair_attribute[]) { \
> + (&((struct cs_off_attribute[]) { \
> { \
> - __ATTR(name, 0444, coresight_simple_show, NULL), \
> - offset, -1 \
> + __ATTR(name, 0444, coresight_simple_show32, NULL), \
> + offset \
> } \
> })[0].attr.attr)
>
> #define coresight_simple_reg64(name, lo_off, hi_off) \
> (&((struct cs_pair_attribute[]) { \
> { \
> - __ATTR(name, 0444, coresight_simple_show, NULL), \
> + __ATTR(name, 0444, coresight_simple_show_pair, NULL), \
> lo_off, hi_off \
> } \
> })[0].attr.attr)
> @@ -130,25 +132,6 @@ static inline void CS_UNLOCK(void __iomem *addr)
> } while (0);
> }
>
> -static inline u64
> -coresight_read_reg_pair(void __iomem *addr, s32 lo_offset, s32 hi_offset)
> -{
> - u64 val;
> -
> - val = readl_relaxed(addr + lo_offset);
> - val |= (hi_offset < 0) ? 0 :
> - (u64)readl_relaxed(addr + hi_offset) << 32;
> - return val;
> -}
> -
> -static inline void coresight_write_reg_pair(void __iomem *addr, u64 val,
> - s32 lo_offset, s32 hi_offset)
> -{
> - writel_relaxed((u32)val, addr + lo_offset);
> - if (hi_offset >= 0)
> - writel_relaxed((u32)(val >> 32), addr + hi_offset);
> -}
> -
> void coresight_disable_path(struct list_head *path);
> int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data);
> struct coresight_device *coresight_get_sink(struct list_head *path);
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index 6bec20a392b3..66959557cf39 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -282,12 +282,12 @@ ssize_t tmc_etr_get_sysfs_trace(struct tmc_drvdata *drvdata,
> static inline u64 \
> tmc_read_##name(struct tmc_drvdata *drvdata) \
> { \
> - return coresight_read_reg_pair(drvdata->base, lo_off, hi_off); \
> + return csdev_access_relaxed_read_pair(&drvdata->csdev->access, lo_off, hi_off); \
> } \
> static inline void \
> tmc_write_##name(struct tmc_drvdata *drvdata, u64 val) \
> { \
> - coresight_write_reg_pair(drvdata->base, val, lo_off, hi_off); \
> + csdev_access_relaxed_write_pair(&drvdata->csdev->access, val, lo_off, hi_off); \
> }
>
> TMC_REG_PAIR(rrp, TMC_RRP, TMC_RRPHI)
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index a47dd1f62216..1554021231f9 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -373,21 +373,26 @@ static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,
> }
>
> static inline u64 csdev_access_relaxed_read_pair(struct csdev_access *csa,
> - s32 lo_offset, s32 hi_offset)
> + u32 lo_offset, u32 hi_offset)
> {
> - u64 val;
> -
> if (likely(csa->io_mem)) {
> - val = readl_relaxed(csa->base + lo_offset);
> - val |= (hi_offset < 0) ? 0 :
> - (u64)readl_relaxed(csa->base + hi_offset) << 32;
> - return val;
> + return readl_relaxed(csa->base + lo_offset) |
> + ((u64)readl_relaxed(csa->base + hi_offset) << 32);
> }
>
> - val = csa->read(lo_offset, true, false);
> - val |= (hi_offset < 0) ? 0 :
> - (u64)csa->read(hi_offset, true, false) << 32;
> - return val;
> + return csa->read(lo_offset, true, false) | (csa->read(hi_offset, true, false) << 32);
> +}
> +
> +static inline void csdev_access_relaxed_write_pair(struct csdev_access *csa, u64 val,
> + u32 lo_offset, u32 hi_offset)
> +{
> + if (likely(csa->io_mem)) {
> + writel_relaxed((u32)val, csa->base + lo_offset);
> + writel_relaxed((u32)(val >> 32), csa->base + hi_offset);
> + } else {
> + csa->write((u32)val, lo_offset, true, false);
> + csa->write((u32)(val >> 32), hi_offset, true, false);
> + }
> }
>
> static inline u32 csdev_access_read32(struct csdev_access *csa, u32 offset)
> --
> 2.28.0
>

Reviewed-by: Mike Leach <[email protected]>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

2022-08-31 17:21:40

by Mathieu Poirier

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] coresight: Reduce duplicated sysfs accessors

I have applied this set.

Thanks,
Mathieu

On Tue, Aug 30, 2022 at 06:26:08PM +0100, James Clark wrote:
> Changes since v1:
>
> * Keep existing signed offset value types until the very last commit
> and then remove them all at once
>
> * Also split out usages of read_pair() and read32() into separate
> functions in the last commit
>
> * Whitespace fixes
>
> * Replaced any touched scnprintf() with sysfs_emit()
>
> ======
>
> The intent of this change is to reduce the large number identical of
> functions created by macros for sysfs accessors. It's possible to re-use
> the same function but pass in the register to access as an argument.
> This reduces the size of the coresight modules folder by 244KB.
>
> The first two patches are refactors to simplify and remove some dead
> code, and the second two are the changes to use a shared function.
>
> Testing
> =======
>
> No changes in any of the outputs:
>
> cat /sys/bus/coresight/devices/*/mgmt/* > before.txt
> cat /sys/bus/coresight/devices/*/mgmt/* > after.txt
> diff before.txt after.txt
>
> With the following modules loaded:
>
> ls /sys/bus/coresight/devices/
> etm0 etm2 funnel0 funnel2 replicator0 tmc_etf0 tmc_etf2 tpiu0
> etm1 etm3 funnel1 funnel3 stm0 tmc_etf1 tmc_etr0
>
>
> James Clark (5):
> coresight: Remove unused function parameter
> coresight: Simplify sysfs accessors by using csdev_access abstraction
> coresight: Re-use same function for similar sysfs register accessors
> coresight: cti-sysfs: Re-use same functions for similar sysfs register
> accessors
> coresight: Make new csdev_access offsets unsigned
>
> drivers/hwtracing/coresight/coresight-catu.c | 27 +--
> drivers/hwtracing/coresight/coresight-catu.h | 8 +-
> drivers/hwtracing/coresight/coresight-core.c | 28 +++
> .../hwtracing/coresight/coresight-cti-sysfs.c | 213 +++++++-----------
> drivers/hwtracing/coresight/coresight-etb10.c | 28 +--
> .../coresight/coresight-etm3x-sysfs.c | 34 +--
> drivers/hwtracing/coresight/coresight-priv.h | 74 +++---
> .../coresight/coresight-replicator.c | 10 +-
> drivers/hwtracing/coresight/coresight-stm.c | 40 +---
> .../hwtracing/coresight/coresight-tmc-core.c | 48 ++--
> drivers/hwtracing/coresight/coresight-tmc.h | 4 +-
> include/linux/coresight.h | 23 ++
> 12 files changed, 227 insertions(+), 310 deletions(-)
>
> --
> 2.28.0
>