Re: [PATCH 1/4] RISC-V: Create unique identification for SoC PMU

["João Mário Domingos" <joao.mario@tecnico.ulisboa.pt>]

On Mon, Nov 15, 2021 at 11:23:16AM +0300, Nikita Shubin wrote:
> Hello Mário.
> 
> On Tue,  9 Nov 2021 10:25:52 +0000
> João Mário Domingos <joao.mario@tecnico.ulisboa.pt> wrote:
> 
> > The SBI PMU platform driver did not provide any identification for
> > perf events matching. This patch introduces a new sysfs file inside
> > the platform device (soc:pmu/id) for pmu identification.
> > 
> > The identification is a 64-bit value generated as:
> > [63-32]: mvendorid;
> > [31]: marchid[MSB];
> > [30-16]: marchid[15-0];
> > [15-0]: mimpid[15MSBs];
> > 
> > The CSRs are detailed in the RISC-V privileged spec [1].
> > The marchid is split in MSB + 15LSBs, due to the MSB being used for
> > open-source architecture identification.
> > 
> 
> This patch doesn't compile and also has a warning:
Corrected in v2.
> ```
> drivers/perf/riscv_pmu_sbi.c:239:43: error: 'pmu_sbi_id_show'
> undeclared here (not in a function)
>   239 | static DEVICE_ATTR(id, S_IRUGO | S_IWUSR, pmu_sbi_id_show, 0);
>       |                                           ^~~~~~~~~~~~~~~
> include/linux/sysfs.h:104:19: note: in definition of macro '__ATTR'
>   104 |         .show   = _show,
>         \
>       |                   ^~~~~
> drivers/perf/riscv_pmu_sbi.c:239:8: note: in expansion of macro
> 'DEVICE_ATTR'
>   239 | static DEVICE_ATTR(id, S_IRUGO | S_IWUSR, pmu_sbi_id_show, 0);
>       |        ^~~~~~~~~~~
> drivers/perf/riscv_pmu_sbi.c: In function 'pmu_sbi_id_show':
> drivers/perf/riscv_pmu_sbi.c:675:29: warning: format '%lx' expects
> argument of type 'long unsigned int', 
> but argument 3 has type 'uint64_t' {aka 'long long unsigned int'}
> [-Wformat=]
>   675 |     len = sprintf(buf, "0x%lx\n", pmu_sbi_get_pmu_id());
>       |                           ~~^     ~~~~~~~~~~~~~~~~~~~~
>       |                             |     |
>       |                             |     uint64_t {aka long long
> unsigned int}
>       |                             long unsigned int
>       |                           %llx
> ```
> 
> May be you wanted to place DEVICE_ATTR after pmu_sbi_id_show function
> declaration ?
> 
> Please check with a clean build.
Recompiled everything with a clean build and the necessary changes are
reflected in version 2 of the series.
If you have the time, please check again.
> 
> Yours,
> Nikita Shubin
> 
> > [1] https://github.com/riscv/riscv-isa-manual
> > 
> > Signed-off-by: João Mário Domingos <joao.mario@tecnico.ulisboa.pt>
> > ---
> >  arch/riscv/kernel/sbi.c      |  3 +++
> >  drivers/perf/riscv_pmu_sbi.c | 46
> > ++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+)
> > 
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > index 7402a417f38e..4e4f5671b864 100644
> > --- a/arch/riscv/kernel/sbi.c
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -551,16 +551,19 @@ long sbi_get_mvendorid(void)
> >  {
> >  	return __sbi_base_ecall(SBI_EXT_BASE_GET_MVENDORID);
> >  }
> > +EXPORT_SYMBOL(sbi_get_mvendorid);
> >  
> >  long sbi_get_marchid(void)
> >  {
> >  	return __sbi_base_ecall(SBI_EXT_BASE_GET_MARCHID);
> >  }
> > +EXPORT_SYMBOL(sbi_get_marchid);
> >  
> >  long sbi_get_mimpid(void)
> >  {
> >  	return __sbi_base_ecall(SBI_EXT_BASE_GET_MIMPID);
> >  }
> > +EXPORT_SYMBOL(sbi_get_mimpid);
> >  
> >  static void sbi_send_cpumask_ipi(const struct cpumask *target)
> >  {
> > diff --git a/drivers/perf/riscv_pmu_sbi.c
> > b/drivers/perf/riscv_pmu_sbi.c index 7a68dfa89f6f..f913d8ddfe73 100644
> > --- a/drivers/perf/riscv_pmu_sbi.c
> > +++ b/drivers/perf/riscv_pmu_sbi.c
> > @@ -236,6 +236,15 @@ static const struct pmu_event_data
> > pmu_cache_event_map[PERF_COUNT_HW_CACHE_MAX] },
> >  };
> >  
> > +static DEVICE_ATTR(id, S_IRUGO | S_IWUSR, pmu_sbi_id_show, 0);
> > +
> > +static struct attribute *pmu_sbi_attrs[] = {
> > +    &dev_attr_id.attr,
> > +    NULL
> > +};
> > +
> > +ATTRIBUTE_GROUPS(pmu_sbi);
> > +
> >  static int pmu_sbi_ctr_get_width(int idx)
> >  {
> >  	return pmu_ctr_list[idx].width;
> > @@ -642,6 +651,36 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu
> > *pmu, struct platform_device *pde return 0;
> >  }
> >  
> > +static uint64_t pmu_sbi_get_pmu_id(void)
> > +{
> > +	union sbi_pmu_id {
> > +		uint64_t value;
> > +		struct {
> > +			uint16_t imp:16;
> > +			uint16_t arch:16;
> > +			uint32_t vendor:32;
> > +		};
> > +	}pmuid;
> > +
> > +	pmuid.value = 0;
> > +	pmuid.vendor = (uint32_t) sbi_get_mvendorid();
> > +	pmuid.arch = (sbi_get_marchid() >> (63 - 15) & (1 << 15)) |
> > ( sbi_get_marchid() & 0x7FFF );
> > +	pmuid.imp = (sbi_get_mimpid() >> 16);
> > +
> > +	return pmuid.value;
> > +}
> > +
> > +static ssize_t pmu_sbi_id_show(struct device *dev,
> > +        struct device_attribute *attr, char *buf)
> > +{
> > +    int len;
> > +    len = sprintf(buf, "0x%lx\n", pmu_sbi_get_pmu_id());
> > +    if (len <= 0)
> > +        dev_err(dev, "mydrv: Invalid sprintf len: %dn", len);
> > +
> > +    return len;
> > +}
> > +
> >  static int pmu_sbi_device_probe(struct platform_device *pdev)
> >  {
> >  	struct riscv_pmu *pmu = NULL;
> > @@ -680,6 +719,13 @@ static int pmu_sbi_device_probe(struct
> > platform_device *pdev) pmu->ctr_clear_idx = pmu_sbi_ctr_clear_idx;
> >  	pmu->ctr_read = pmu_sbi_ctr_read;
> >  
> > +	ret = sysfs_create_group(&pdev->dev.kobj, &pmu_sbi_group);
> > +	if (ret) {
> > +        dev_err(&pdev->dev, "sysfs creation failed\n");
> > +        return ret;
> > +    }
> > +	pdev->dev.groups = pmu_sbi_groups;
> > +
> >  	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_RISCV_STARTING,
> > &pmu->node); if (ret)
> >  		return ret;
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv