[RFC PATCH 02/14] coresight: Introduce device access abstraction
We are about to introduce support for sysreg access to ETMv4.4+ component. Since there are generic routines that access the registers (e.g, CS_LOCK/UNLOCK , claim/disclaim operations, timeout) and in order to preserve the logic of these operations at a single place we introduce an abstraction layer for the accesses to a given device. This will also be helpful in consolidating the sysfs.attribute helpers, that we define per driver. Cc: Mathieu Poirier Cc: Mike Leach Signed-off-by: Suzuki K Poulose --- drivers/hwtracing/coresight/coresight-catu.c | 1 + drivers/hwtracing/coresight/coresight-cti.c | 1 + drivers/hwtracing/coresight/coresight-etb10.c | 1 + drivers/hwtracing/coresight/coresight-etm3x.c | 1 + drivers/hwtracing/coresight/coresight-etm4x.c | 1 + .../hwtracing/coresight/coresight-funnel.c| 1 + .../coresight/coresight-replicator.c | 1 + drivers/hwtracing/coresight/coresight-stm.c | 1 + drivers/hwtracing/coresight/coresight-tmc.c | 1 + drivers/hwtracing/coresight/coresight-tpiu.c | 1 + drivers/hwtracing/coresight/coresight.c | 49 ++ include/linux/coresight.h | 156 ++ 12 files changed, 215 insertions(+) diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c index 1801804a7762..6299ff7b8a14 100644 --- a/drivers/hwtracing/coresight/coresight-catu.c +++ b/drivers/hwtracing/coresight/coresight-catu.c @@ -551,6 +551,7 @@ static int catu_probe(struct amba_device *adev, const struct amba_id *id) dev->platform_data = pdata; drvdata->base = base; + catu_desc.access.base = base; catu_desc.pdata = pdata; catu_desc.dev = dev; catu_desc.groups = catu_groups; diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c index 3ccc703dc940..c810ea3ba155 100644 --- a/drivers/hwtracing/coresight/coresight-cti.c +++ b/drivers/hwtracing/coresight/coresight-cti.c @@ -865,6 +865,7 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id) return PTR_ERR(base); drvdata->base = base; + cti_desc.access.base = base; dev_set_drvdata(dev, drvdata); diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c index 03e3f2590191..0f2735e15119 100644 --- a/drivers/hwtracing/coresight/coresight-etb10.c +++ b/drivers/hwtracing/coresight/coresight-etb10.c @@ -755,6 +755,7 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id) return PTR_ERR(base); drvdata->base = base; + desc.access.base = base; spin_lock_init(&drvdata->spinlock); diff --git a/drivers/hwtracing/coresight/coresight-etm3x.c b/drivers/hwtracing/coresight/coresight-etm3x.c index bf22dcfd3327..7ddcb7fcb2d6 100644 --- a/drivers/hwtracing/coresight/coresight-etm3x.c +++ b/drivers/hwtracing/coresight/coresight-etm3x.c @@ -805,6 +805,7 @@ static int etm_probe(struct amba_device *adev, const struct amba_id *id) return PTR_ERR(base); drvdata->base = base; + desc.access.base = base; spin_lock_init(&drvdata->spinlock); diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index cb83fb77ded6..7bb74c659c4f 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -1501,6 +1501,7 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) return PTR_ERR(base); drvdata->base = base; + desc.access.base = base; spin_lock_init(&drvdata->spinlock); diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c index 900690a9f7f0..67fc3e3b77d8 100644 --- a/drivers/hwtracing/coresight/coresight-funnel.c +++ b/drivers/hwtracing/coresight/coresight-funnel.c @@ -242,6 +242,7 @@ static int funnel_probe(struct device *dev, struct resource *res) } drvdata->base = base; desc.groups = coresight_funnel_groups; + desc.access.base = base; } dev_set_drvdata(dev, drvdata); diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c index 78acf29c49ca..65704ada20a5 100644 --- a/drivers/hwtracing/coresight/coresight-replicator.c +++ b/drivers/hwtracing/coresight/coresight-replicator.c @@ -254,6 +254,7 @@ static int replicator_probe(struct device *dev, struct resource *res) } drvdata->base = base; desc.groups = replicator_groups; + desc.access.base = base; } if (fwnode_property_present(dev_fwnode(dev), diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c index 673d2f
Re: [RFC PATCH 02/14] coresight: Introduce device access abstraction
On 07/29/2020 08:56 PM, Mathieu Poirier wrote:
On Wed, Jul 22, 2020 at 06:20:28PM +0100, Suzuki K Poulose wrote:
We are about to introduce support for sysreg access to ETMv4.4+
component. Since there are generic routines that access the
registers (e.g, CS_LOCK/UNLOCK , claim/disclaim operations, timeout)
and in order to preserve the logic of these operations at a single place
we introduce an abstraction layer for the accesses to a given device.
This will also be helpful in consolidating the sysfs.attribute helpers,
that we define per driver.
Please drop the last sentence, it doesn't add to the current patch.
Sure.
diff --git a/drivers/hwtracing/coresight/coresight.c
b/drivers/hwtracing/coresight/coresight.c
index e9c90f2de34a..38e9c03ab754 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -1387,6 +1387,54 @@ static int __init coresight_init(void)
}
...
* coresight_release_platform_data: Release references to the devices
connected
* to the output port of this device.
@@ -1451,6 +1499,7 @@ struct coresight_device *coresight_register(struct
coresight_desc *desc)
csdev->type = desc->type;
csdev->subtype = desc->subtype;
csdev->ops = desc->ops;
+ csdev->access = desc->access;
csdev->orphan = false;
csdev->dev.type = &coresight_dev_type[desc->type];
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 58fffdecdbfd..81ac708689f8 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -7,6 +7,7 @@
#define _LINUX_CORESIGHT_H
#include
+#include
#include
#include
@@ -114,6 +115,32 @@ struct coresight_platform_data {
struct coresight_connection *conns;
};
+/**
+ * struct csdev_access - Abstraction of a CoreSight device access.
+ *
+ * @no_iomem : True if the device doesn't have iomem access.
+ * @base : When no_iomem == false, base address of the component
+ * @read : Read from the given "offset" of the given instance.
+ * @write : Write "val" to the given "offset".
+ */
+struct csdev_access {
+ bool no_iomem;
I find the no_iomen to be difficult to understand, especially when prefixed with
'!'. Using "has_iomem" would be a lot more intuitive and would avoid extra
mental gymnastics.
I agree. That was a bit of laziness in part, to limit the changes to the
existing drivers, where almost everyone, except the ETM would need to
simply use the MMIO approach. So, in order to keep those changes to
minimum, i.e, simply initialize the base, I used the inverted logic.
I will fix it.
+u32 coresight_relaxed_read32(struct coresight_device *csdev, u32 offset);
+u32 coresight_read32(struct coresight_device *csdev, u32 offset);
+void coresight_write32(struct coresight_device *csdev, u32 val, u32 offset);
+void coresight_relaxed_write32(struct coresight_device *csdev,
+ u32 val,
+ u32 offset);
+
Why are the 64 bit version outside of the #ifdef and the 32 bit within?
Mistake ;-). I will address all of the comments above in my next version.
...
+static inline u64 coresight_read64(struct coresight_device *csdev, u32 offset)
+{
+ WARN_ON_ONCE(1);
Not sure about the motivation behind using WARN_ON_ONCE(), and only in the read
functions. I would simply return 0 here. After all if CONFIG_CORESIGHT is not
defined they won't make it very far.
If someone is reading the values, they might do something with the
value, i.e, make a decision when they shouldn't. This is just to prevent
such cases.
+ return 0;
+}
+
+static inline void coresight_relaxed_write64(struct coresight_device *csdev,
+u64 val,
+u32 offset)
+{
+}
+
+static inline void coresight_write64(struct coresight_device *csdev, u64 val,
u32 offset)
+{
+}
+
#endif
I will likely come back to this patch once I have reviewed the rest of the set.
Sure. I am looking for thoughts on the proposed API (not ABI) changes,
as they are quite significant and invasive changes in the code, without
much functionality changes.
Thank you for the review !
Suzuki
Re: [RFC PATCH 02/14] coresight: Introduce device access abstraction
On Wed, Jul 22, 2020 at 06:20:28PM +0100, Suzuki K Poulose wrote: > We are about to introduce support for sysreg access to ETMv4.4+ > component. Since there are generic routines that access the > registers (e.g, CS_LOCK/UNLOCK , claim/disclaim operations, timeout) > and in order to preserve the logic of these operations at a single place > we introduce an abstraction layer for the accesses to a given device. > This will also be helpful in consolidating the sysfs.attribute helpers, > that we define per driver. Please drop the last sentence, it doesn't add to the current patch. > > Cc: Mathieu Poirier > Cc: Mike Leach > Signed-off-by: Suzuki K Poulose > --- > drivers/hwtracing/coresight/coresight-catu.c | 1 + > drivers/hwtracing/coresight/coresight-cti.c | 1 + > drivers/hwtracing/coresight/coresight-etb10.c | 1 + > drivers/hwtracing/coresight/coresight-etm3x.c | 1 + > drivers/hwtracing/coresight/coresight-etm4x.c | 1 + > .../hwtracing/coresight/coresight-funnel.c| 1 + > .../coresight/coresight-replicator.c | 1 + > drivers/hwtracing/coresight/coresight-stm.c | 1 + > drivers/hwtracing/coresight/coresight-tmc.c | 1 + > drivers/hwtracing/coresight/coresight-tpiu.c | 1 + > drivers/hwtracing/coresight/coresight.c | 49 ++ > include/linux/coresight.h | 156 ++ > 12 files changed, 215 insertions(+) > > diff --git a/drivers/hwtracing/coresight/coresight-catu.c > b/drivers/hwtracing/coresight/coresight-catu.c > index 1801804a7762..6299ff7b8a14 100644 > --- a/drivers/hwtracing/coresight/coresight-catu.c > +++ b/drivers/hwtracing/coresight/coresight-catu.c > @@ -551,6 +551,7 @@ static int catu_probe(struct amba_device *adev, const > struct amba_id *id) > dev->platform_data = pdata; > > drvdata->base = base; > + catu_desc.access.base = base; > catu_desc.pdata = pdata; > catu_desc.dev = dev; > catu_desc.groups = catu_groups; > diff --git a/drivers/hwtracing/coresight/coresight-cti.c > b/drivers/hwtracing/coresight/coresight-cti.c > index 3ccc703dc940..c810ea3ba155 100644 > --- a/drivers/hwtracing/coresight/coresight-cti.c > +++ b/drivers/hwtracing/coresight/coresight-cti.c > @@ -865,6 +865,7 @@ static int cti_probe(struct amba_device *adev, const > struct amba_id *id) > return PTR_ERR(base); > > drvdata->base = base; > + cti_desc.access.base = base; > > dev_set_drvdata(dev, drvdata); > > diff --git a/drivers/hwtracing/coresight/coresight-etb10.c > b/drivers/hwtracing/coresight/coresight-etb10.c > index 03e3f2590191..0f2735e15119 100644 > --- a/drivers/hwtracing/coresight/coresight-etb10.c > +++ b/drivers/hwtracing/coresight/coresight-etb10.c > @@ -755,6 +755,7 @@ static int etb_probe(struct amba_device *adev, const > struct amba_id *id) > return PTR_ERR(base); > > drvdata->base = base; > + desc.access.base = base; > > spin_lock_init(&drvdata->spinlock); > > diff --git a/drivers/hwtracing/coresight/coresight-etm3x.c > b/drivers/hwtracing/coresight/coresight-etm3x.c > index bf22dcfd3327..7ddcb7fcb2d6 100644 > --- a/drivers/hwtracing/coresight/coresight-etm3x.c > +++ b/drivers/hwtracing/coresight/coresight-etm3x.c > @@ -805,6 +805,7 @@ static int etm_probe(struct amba_device *adev, const > struct amba_id *id) > return PTR_ERR(base); > > drvdata->base = base; > + desc.access.base = base; > > spin_lock_init(&drvdata->spinlock); > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c > b/drivers/hwtracing/coresight/coresight-etm4x.c > index cb83fb77ded6..7bb74c659c4f 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x.c > @@ -1501,6 +1501,7 @@ static int etm4_probe(struct amba_device *adev, const > struct amba_id *id) > return PTR_ERR(base); > > drvdata->base = base; > + desc.access.base = base; > > spin_lock_init(&drvdata->spinlock); > > diff --git a/drivers/hwtracing/coresight/coresight-funnel.c > b/drivers/hwtracing/coresight/coresight-funnel.c > index 900690a9f7f0..67fc3e3b77d8 100644 > --- a/drivers/hwtracing/coresight/coresight-funnel.c > +++ b/drivers/hwtracing/coresight/coresight-funnel.c > @@ -242,6 +242,7 @@ static int funnel_probe(struct device *dev, struct > resource *res) > } > drvdata->base = base; > desc.groups = coresight_funnel_groups; > + desc.access.base = base; > } > > dev_set_drvdata(dev, drvdata); > diff --git a/drivers/hwtracing/coresight/coresight-replicator.c > b/drivers/hwtracing/coresight/coresight-replicator.c > index 78acf29c49ca..65704ada20a5 100644 > --- a/drivers/hwtracing/coresight/coresight-replicator.c > +++ b/drivers/hwtracing/coresight/coresight-replicator.c > @@ -254,6 +254,7 @@ static int replicator_probe(struct device *dev, struct > resource *res) >
