On Mon, 20 May 2024, Shravan Kumar Ramani wrote: > Add support for programming any counter to monitor the cycle count. > Since counting of cycles using 32-bit ocunters would result in frequent counters > wraparounds, add the ability to combine 2 adjacent 32-bit counters to > form 1 64-bit counter. > Both these features are supported by BlueField-3 PMC hardware, hence > the required bit-fields are exposed by the driver via sysfs to allow > the user to configure as needed. I'm trying to understand what happens for the other counter, when the use_odd_counter is enabled? This change also doesn't add code that would make the other counter -EBUSY, should that be done? For 64-bit counter, I suppose the userspace is expected to read the full counter from two sysfs files and combine the value (your documentation doesn't explain this)? That seems non-optimal, why cannot kernel just return the full combined 64-value directly in kernel? Similarly, are these cycle counters occupying the same space as non-cycle counters (so both can/cannot be used that the same time)? I'm asking this because you're adding a parallel interface to read the value and if it's either-or, I don't understand why the value needs to be read from different file depending on the counter counting in cycles or not. -- i. > Signed-off-by: Shravan Kumar Ramani <shravankr@xxxxxxxxxx> > Reviewed-by: David Thompson <davthompson@xxxxxxxxxx> > Reviewed-by: Vadim Pasternak <vadimp@xxxxxxxxxx> > --- > drivers/platform/mellanox/mlxbf-pmc.c | 134 ++++++++++++++++++++++++++ > 1 file changed, 134 insertions(+) > > diff --git a/drivers/platform/mellanox/mlxbf-pmc.c b/drivers/platform/mellanox/mlxbf-pmc.c > index 4ed9c7fd2b62..635ecc3b3845 100644 > --- a/drivers/platform/mellanox/mlxbf-pmc.c > +++ b/drivers/platform/mellanox/mlxbf-pmc.c > @@ -88,6 +88,8 @@ > #define MLXBF_PMC_CRSPACE_PERFMON_CTL(n) (n * MLXBF_PMC_CRSPACE_PERFMON_REG0_SZ) > #define MLXBF_PMC_CRSPACE_PERFMON_EN BIT(30) > #define MLXBF_PMC_CRSPACE_PERFMON_CLR BIT(28) > +#define MLXBF_PMC_CRSPACE_PERFMON_UOC GENMASK(15, 0) > +#define MLXBF_PMC_CRSPACE_PERFMON_COUNT_CLOCK(n) (MLXBF_PMC_CRSPACE_PERFMON_CTL(n) + 0x4) > #define MLXBF_PMC_CRSPACE_PERFMON_VAL0(n) (MLXBF_PMC_CRSPACE_PERFMON_CTL(n) + 0xc) > > /** > @@ -114,6 +116,8 @@ struct mlxbf_pmc_attribute { > * @attr_event: Attributes for "event" sysfs files > * @attr_event_list: Attributes for "event_list" sysfs files > * @attr_enable: Attributes for "enable" sysfs files > + * @attr_use_odd_counter: Attributes for "use_odd_counter" sysfs files > + * @attr_count_clock: Attributes for "count_clock" sysfs files > * @block_attr: All attributes needed for the block > * @block_attr_grp: Attribute group for the block > */ > @@ -126,6 +130,8 @@ struct mlxbf_pmc_block_info { > struct mlxbf_pmc_attribute *attr_event; > struct mlxbf_pmc_attribute attr_event_list; > struct mlxbf_pmc_attribute attr_enable; > + struct mlxbf_pmc_attribute attr_use_odd_counter; > + struct mlxbf_pmc_attribute attr_count_clock; > struct attribute *block_attr[MLXBF_PMC_MAX_ATTRS]; > struct attribute_group block_attr_grp; > }; > @@ -1763,6 +1769,103 @@ static ssize_t mlxbf_pmc_enable_store(struct device *dev, > return count; > } > > +/* Show function for "use_odd_counter" sysfs files - only for crspace */ > +static ssize_t mlxbf_pmc_use_odd_counter_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct mlxbf_pmc_attribute *attr_use_odd_counter = container_of( > + attr, struct mlxbf_pmc_attribute, dev_attr); > + unsigned int blk_num; > + u32 value, reg; > + > + blk_num = attr_use_odd_counter->nr; > + > + if (mlxbf_pmc_readl(pmc->block[blk_num].mmio_base + > + MLXBF_PMC_CRSPACE_PERFMON_CTL(pmc->block[blk_num].counters), > + ®)) > + return -EINVAL; > + > + value = FIELD_GET(MLXBF_PMC_CRSPACE_PERFMON_UOC, reg); > + > + return sysfs_emit(buf, "%u\n", value); > +} > + > +/* Store function for "use_odd_counter" sysfs files - only for crspace */ > +static ssize_t mlxbf_pmc_use_odd_counter_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct mlxbf_pmc_attribute *attr_use_odd_counter = container_of( > + attr, struct mlxbf_pmc_attribute, dev_attr); > + unsigned int blk_num; > + u32 uoc, reg; > + int err; > + > + blk_num = attr_use_odd_counter->nr; > + > + err = kstrtouint(buf, 0, &uoc); > + if (err < 0) > + return err; > + > + err = mlxbf_pmc_readl(pmc->block[blk_num].mmio_base + > + MLXBF_PMC_CRSPACE_PERFMON_CTL(pmc->block[blk_num].counters), > + ®); > + if (err) > + return -EINVAL; > + > + reg &= ~MLXBF_PMC_CRSPACE_PERFMON_UOC; > + reg |= FIELD_PREP(MLXBF_PMC_CRSPACE_PERFMON_UOC, uoc); > + > + mlxbf_pmc_write(pmc->block[blk_num].mmio_base + > + MLXBF_PMC_CRSPACE_PERFMON_CTL(pmc->block[blk_num].counters), > + MLXBF_PMC_WRITE_REG_32, reg); > + > + return count; > +} > + > +/* Show function for "count_clock" sysfs files - only for crspace */ > +static ssize_t mlxbf_pmc_count_clock_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct mlxbf_pmc_attribute *attr_count_clock = container_of( > + attr, struct mlxbf_pmc_attribute, dev_attr); > + unsigned int blk_num; > + u32 reg; > + > + blk_num = attr_count_clock->nr; > + > + if (mlxbf_pmc_readl(pmc->block[blk_num].mmio_base + > + MLXBF_PMC_CRSPACE_PERFMON_COUNT_CLOCK(pmc->block[blk_num].counters), > + ®)) > + return -EINVAL; > + > + return sysfs_emit(buf, "%u\n", reg); > +} > + > +/* Store function for "count_clock" sysfs files - only for crspace */ > +static ssize_t mlxbf_pmc_count_clock_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct mlxbf_pmc_attribute *attr_count_clock = container_of( > + attr, struct mlxbf_pmc_attribute, dev_attr); > + unsigned int blk_num; > + u32 reg; > + int err; > + > + blk_num = attr_count_clock->nr; > + > + err = kstrtouint(buf, 0, ®); > + if (err < 0) > + return err; > + > + mlxbf_pmc_write(pmc->block[blk_num].mmio_base + > + MLXBF_PMC_CRSPACE_PERFMON_COUNT_CLOCK(pmc->block[blk_num].counters), > + MLXBF_PMC_WRITE_REG_32, reg); > + > + return count; > +} > + > /* Populate attributes for blocks with counters to monitor performance */ > static int mlxbf_pmc_init_perftype_counter(struct device *dev, unsigned int blk_num) > { > @@ -1799,6 +1902,37 @@ static int mlxbf_pmc_init_perftype_counter(struct device *dev, unsigned int blk_ > attr = NULL; > } > > + if (pmc->block[blk_num].type == MLXBF_PMC_TYPE_CRSPACE) { > + /* > + * Couple adjacent odd and even 32-bit counters to form 64-bit counters > + * using "use_odd_counter" sysfs which has one bit per even counter. > + */ > + attr = &pmc->block[blk_num].attr_use_odd_counter; > + attr->dev_attr.attr.mode = 0644; > + attr->dev_attr.show = mlxbf_pmc_use_odd_counter_show; > + attr->dev_attr.store = mlxbf_pmc_use_odd_counter_store; > + attr->nr = blk_num; > + attr->dev_attr.attr.name = devm_kasprintf(dev, GFP_KERNEL, > + "use_odd_counter"); > + if (!attr->dev_attr.attr.name) > + return -ENOMEM; > + pmc->block[blk_num].block_attr[++i] = &attr->dev_attr.attr; > + attr = NULL; > + > + /* Program crspace counters to count clock cycles using "count_clock" sysfs */ > + attr = &pmc->block[blk_num].attr_count_clock; > + attr->dev_attr.attr.mode = 0644; > + attr->dev_attr.show = mlxbf_pmc_count_clock_show; > + attr->dev_attr.store = mlxbf_pmc_count_clock_store; > + attr->nr = blk_num; > + attr->dev_attr.attr.name = devm_kasprintf(dev, GFP_KERNEL, > + "count_clock"); > + if (!attr->dev_attr.attr.name) > + return -ENOMEM; > + pmc->block[blk_num].block_attr[++i] = &attr->dev_attr.attr; > + attr = NULL; > + }