Re: [PATCH] coresight-stm: adding driver for CoreSight STM component

[Mathieu Poirier]

On 5 February 2015 at 04:27, Paul Bolle <pebolle@xxxxxxxxxx> wrote:
> On Wed, 2015-02-04 at 15:22 -0700, mathieu.poirier@xxxxxxxxxx wrote:
>> From: Pratik Patel <pratikp@xxxxxxxxxxxxxx>
>>
>> This driver adds support for the STM CoreSight IP block,
>> allowing any system compoment (HW or SW) to log and
>> aggregate messages via a single entity.
>>
>> The STM exposes an application defined number of channels
>> called stimulus port.  Configuration is done using entries
>> in sysfs and channels made available to userspace via devfs.
>>
>> Signed-off-by: Pratik Patel <pratikp@xxxxxxxxxxxxxx>
>> Signed-off-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
>
> This needs "coresight: Adding coresight support for arm64
> architecture" (https://lkml.org/lkml/2015/2/3/677 ) in order to get
> applied. Perhaps that's obvious to the people working on this.

Right, in my tree the code is tailored to use the other patch you're
referencing but having received no 'ack' for it yet I made this set
for 32 bit only (aside from the 64bit flags in Kconfig).  As such you
don't need https://lkml.org/lkml/2015/2/3/677 for this to compile and
run.

>
> A few comments follow.
>
>> ---
>>  .../ABI/testing/sysfs-bus-coresight-devices-stm    |   62 ++
>>  Documentation/trace/coresight.txt                  |   88 +-
>>  drivers/coresight/Kconfig                          |   10 +
>>  drivers/coresight/Makefile                         |    1 +
>>  drivers/coresight/coresight-stm.c                  | 1090 ++++++++++++++++++++
>>  include/linux/coresight-stm.h                      |   35 +
>>  include/uapi/linux/coresight-stm.h                 |   23 +
>>  7 files changed, 1307 insertions(+), 2 deletions(-)
>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-stm
>>  create mode 100644 drivers/coresight/coresight-stm.c
>>  create mode 100644 include/linux/coresight-stm.h
>>  create mode 100644 include/uapi/linux/coresight-stm.h
>>
>>[...]
>> diff --git a/drivers/coresight/Kconfig b/drivers/coresight/Kconfig
>> index fc1f1ae7a49d..08806cc7d737 100644
>> --- a/drivers/coresight/Kconfig
>> +++ b/drivers/coresight/Kconfig
>> @@ -58,4 +58,14 @@ config CORESIGHT_SOURCE_ETM3X
>>         which allows tracing the instructions that a processor is executing
>>         This is primarily useful for instruction level tracing.  Depending
>>         the ETM version data tracing may also be available.
>> +
>> +config CORESIGHT_STM
>> +     bool "CoreSight System Trace Macrocell driver"
>> +     depends on (ARM && !(CPU_32v4 || CPU_32v4T)) || ARM64 || (64BIT && COMPILE_TEST)
>
> I'm _guessing_ that CPU_32v4 and CPU_32v4T are needed for the ldrd and
> strd assembler instructions. If that's right a next _guess_ would be
> that you also need to mention CPU_32v3 here.
>
> Furthermore, this file is only sourced by arch/arm/Kconfig.debug and
> arch/arm64/Kconfig.debug. So 64BIT should always be equal to ARM64 and
> the
>      || (64BIT && COMPILE_TEST)
>
> part shouldn't be needed, isn't it?

Things have changed in the driver (and accessor functions) since I
wrote this Kconfig condition - Let me think on the above 2 comments.

>
>> +     select CORESIGHT_LINKS_AND_SINKS
>> +     help
>> +       This driver provides support for hardware assisted software
>> +       instrumentation based tracing. This is primarily used for
>> +       logging useful software events or data coming from various entities
>> +       in the system, possibly running different OSs
>>  endif
>>[...]
>> diff --git a/drivers/coresight/coresight-stm.c b/drivers/coresight/coresight-stm.c
>> new file mode 100644
>> index 000000000000..e59b0fe01d87
>> --- /dev/null
>> +++ b/drivers/coresight/coresight-stm.c
>> @@ -0,0 +1,1090 @@
>>[...]
>> +#ifndef CONFIG_64BIT
>> +static inline void __raw_writeq(u64 val, volatile void __iomem *addr)
>> +{
>> +     asm volatile("strd %1, %0"
>> +                  : "+Qo" (*(volatile u64 __force *)addr)
>> +                  : "r" (val));
>> +}
>> +
>> +static inline u64 __raw_readq(const volatile void __iomem *addr)
>> +{
>> +     u64 val;
>> +
>> +     asm volatile("ldrd %1, %0"
>> +                  : "+Qo" (*(volatile u64 __force *)addr),
>> +                    "=r" (val));
>> +     return val;
>> +}
>> +
>> +#undef readq_relaxed
>> +#define readq_relaxed(c) ({ u64 __r = le64_to_cpu((__force __le64) \
>> +                                     __raw_readq(c)); __r; })
>
> I spotted no users of readq_relaxed. Is it needed?
>
>> +#undef writeq_relaxed
>> +#define writeq_relaxed(v, c) __raw_writeq((__force u64) cpu_to_le64(v), c)
>> +#endif
>> +
>> [...]
>
>> +static ssize_t entities_show(struct device *dev,
>> +                          struct device_attribute *attr, char *buf)
>> +{
>> +     struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +     ssize_t len;
>> +
>> +     len = bitmap_scnprintf(buf, PAGE_SIZE, drvdata->entities,
>> +                            STM_ENTITY_MAX);
>> +
>
> bitmap_scnprintf is gone in current linux-next. I changed it to
>         len = scnprintf(buf, PAGE_SIZE, "%*pb", STM_ENTITY_MAX,
>                         drvdata->entities);
>
> to get this file to compile. (On x86_64, that is, but please don't tell
> anybody!)

I was not aware that "bitmap_scnprintf()" had disappeared - thanks for
pointing this out.  Did you mean cross-compile from an x86_64 host or
compile for a x86_64 target?  I don't get how you could have in the
latter case.

Thanks for the review,
Mathieu

>
>
> Paul Bolle
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/