Messages in this thread | | | Date | Mon, 24 Oct 2022 15:49:45 +0100 | Subject | Re: [PATCH 6/9] coresight-tpdm: Add node to set dsb programming mode | From | Suzuki K Poulose <> |
| |
On 08/09/2022 09:45, Tao Zhang wrote: > Add node to set and show programming mode for TPDM DSB subunit. > Once the DSB programming mode is set, it will be written to the > register DSB_CR. Bit[10:9] of the DSB_CR register is used to set > the DSB test mode. > > Signed-off-by: Tao Zhang <quic_taozha@quicinc.com> > --- > drivers/hwtracing/coresight/coresight-tpdm.c | 49 +++++++++++++++++++++++++++- > drivers/hwtracing/coresight/coresight-tpdm.h | 10 ++++++ > 2 files changed, 58 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index fae9963..7265793 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -22,7 +22,7 @@ DEFINE_CORESIGHT_DEVLIST(tpdm_devs, "tpdm"); > > static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) > { > - u32 val; > + u32 val, mode; > > val = readl_relaxed(drvdata->base + TPDM_DSB_TIER); > /* Set trigger timestamp */ > @@ -42,6 +42,19 @@ static void tpdm_enable_dsb(struct tpdm_drvdata *drvdata) > > /* Set the enable bit of DSB control register to 1 */ > val = readl_relaxed(drvdata->base + TPDM_DSB_CR); > + /* Set the cycle accurate mode */ > + mode = TPDM_DSB_MODE_CYCACC(drvdata->dsb->mode); > + val = val & ~(0x7 << 9); > + val = val | (mode << 9);
Please do not hard code numbers like that above. Please could you define proper masks for the fields in DSB_CR and use FIELD_GET, FIELD_PREP for setting the values.
> + /* Set the byte lane for high-performance mode */ > + mode = TPDM_DSB_MODE_HPBYTESEL(drvdata->dsb->mode); > + val = val & ~(0x1F << 2); > + val = val | (mode << 2); > + /* Set the performance mode */
Same as above.
> + if (drvdata->dsb->mode & TPDM_DSB_MODE_PERF) > + val |= TPDM_DSB_MODE; > + else > + val &= ~TPDM_DSB_MODE; > val |= TPDM_DSB_CR_ENA; > writel_relaxed(val, drvdata->base + TPDM_DSB_CR); > } > @@ -232,6 +245,39 @@ static struct attribute_group tpdm_attr_grp = { > .attrs = tpdm_attrs, > }; > > +static ssize_t dsb_mode_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) > + return -EPERM;
As mentioned earlier, use is_visble() instead of hard coding this in every function.
> + > + return scnprintf(buf, PAGE_SIZE, "%lx\n", > + (unsigned long)drvdata->dsb->mode); > +} > + > +static ssize_t dsb_mode_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if ((kstrtoul(buf, 16, &val)) || val < 0 || val > 3)
Why not leave kstrtoul to detect the base instead of always forcing 16 ? Some other code had forced to base of 10. Please be flexible and avoid the inconsistencies.
> + return -EINVAL; > + if (!(drvdata->datasets & TPDM_PIDR0_DS_DSB)) > + return -EPERM; > + > + spin_lock(&drvdata->spinlock); > + drvdata->dsb->mode = val & TPDM_MODE_ALL; > + spin_unlock(&drvdata->spinlock); > + return size; > +} > +static DEVICE_ATTR_RW(dsb_mode); > + > static ssize_t dsb_trig_type_show(struct device *dev, > struct device_attribute *attr, > char *buf) > @@ -312,6 +358,7 @@ static ssize_t dsb_trig_ts_store(struct device *dev, > } > static DEVICE_ATTR_RW(dsb_trig_ts); > static struct attribute *tpdm_dsb_attrs[] = { > + &dev_attr_dsb_mode.attr, > &dev_attr_dsb_trig_ts.attr, > &dev_attr_dsb_trig_type.attr, > NULL, > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/hwtracing/coresight/coresight-tpdm.h > index dd4a013..4d57488 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.h > +++ b/drivers/hwtracing/coresight/coresight-tpdm.h > @@ -19,6 +19,14 @@ > #define TPDM_DSB_XTRIG_TSENAB BIT(1) > /* Enable bit for DSB subunit trigger type */ > #define TPDM_DSB_TRIG_TYPE BIT(12) > +/* Enable bit for DSB subunit perfmance mode */ > +#define TPDM_DSB_MODE BIT(1) > + > +/* DSB programming modes */ > +#define TPDM_DSB_MODE_CYCACC(val) BMVAL(val, 0, 2) > +#define TPDM_DSB_MODE_PERF BIT(3) > +#define TPDM_DSB_MODE_HPBYTESEL(val) BMVAL(val, 4, 8)
Please could we use GENMASK to define the masks and use FIELD_GET/FILED_PREP macros for the dealing with the fields ?
Similarly for the DSB_CR register please.
Suzuki
| |