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

2016-02-16 Thread Chunyan Zhang 
Hi Michael,

One question below need to be clarified.

On Fri, Feb 12, 2016 at 10:55 PM, Michael Williams
 wrote:
> Mathieu Poirier [mailto:mathieu.poir...@linaro.org] wrote:
>> On 6 February 2016 at 04:04, Chunyan Zhang  wrote:
>>> From: Pratik Patel 
>>>
>>> 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 CoreSight 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 configfs.
>>>
>>> Signed-off-by: Pratik Patel 
>>> Signed-off-by: Mathieu Poirier 
>>> Signed-off-by: Chunyan Zhang 
>>> ---
>>>  .../ABI/testing/sysfs-bus-coresight-devices-stm|  53 ++
>>>  Documentation/trace/coresight.txt  |  37 +-
>>>  drivers/hwtracing/coresight/Kconfig|  11 +
>>>  drivers/hwtracing/coresight/Makefile   |   1 +
>>>  drivers/hwtracing/coresight/coresight-stm.c| 928 
>>> +
>>>  include/linux/coresight-stm.h  |   6 +
>>>  include/uapi/linux/coresight-stm.h |  12 +
>>>  7 files changed, 1046 insertions(+), 2 deletions(-)
>>>  create mode 100644 
>>> Documentation/ABI/testing/sysfs-bus-coresight-devices-stm
>>>  create mode 100644 drivers/hwtracing/coresight/coresight-stm.c
>>>  create mode 100644 include/linux/coresight-stm.h
>>>  create mode 100644 include/uapi/linux/coresight-stm.h
>>>

[...]

>>> +static int stm_send(void *addr, const void *data, u32 size, u8 max)
>>> +{
>>> +   u32 len = size;
>>> +   u8 paload[8];
>>> +
>>> +   if (stm_addr_unaligned(data, max)) {
>>> +   memcpy(paload, data, size);
>>> +   data = paload;
>>> +   }
>>> +
>>> +   /* now we are 64bit/32bit aligned */
>>> +#ifdef CONFIG_64BIT
>>> +   if (size == 8)
>>> +   writeq_relaxed(*(u64 *)data, addr);
>>> +#endif
>>
>> We probably don't need an #ifdef here.  Checking the size of the
>> transfer against the fundamental data size will result in the same
>> outcome, i.e preventing 64 bit transfers on a 32 bit architecture.  An
>> error (understandable by the caller) should probably be returned in
>> this case.
>
> In theory this is correct, but if the code is unreachable for non-32-bit 
> architectures, why include it at all?

^^^
I guess you mean the code is unreachable for "non-64-bit" architectures?

>
>>> +   if (size >= 4) {
>>> +   writel_relaxed(*(u32 *)data, addr);
>>> +   data += 4;
>>> +   size -= 4;
>>> +   }
>>> +
>>> +   if (size >= 2) {
>>> +   writew_relaxed(*(u16 *)data, addr);
>>> +   data += 2;
>>> +   size -= 2;
>>> +   }
>>> +
>>> +   if (size == 1)
>>> +   writeb_relaxed(*(u8 *)data, addr);
>>> +
>>> +   return len;
>>> +}
>
> The API for stm_data::packet (include/linux/stm.h) is not wholly clearly 
> defined, but it does state "sends an STP packet," which I interpret as 
> meaning exactly one packet and no more. This function (which is called from 
> this module's implementation of stm_data::packet) will send multiple STP 
> packets if called with a value that is not exactly 8, 4, 2, or 1.

Yes, understand. Since the only first packet in one STM trace should
be marked timestamp, others should not be(their timestamps are same),
the function "stm_send" should indeed send one STP packet at a time.

I will address this in the next version.

Thanks for your time,
Chunyan

>
> To send only one packet, either:
>
> * These "if" statements are replaced with "else if" (as appropriate), and the 
> function returns the size of data consumed; or
> * The stm_generic_packet() function (below) forces "size" to a power-of-two 
> (<= write_max) before calling stm_send().
>
>>> +static int stm_generic_link(struct stm_data *stm_data,
>>> +   unsigned int master,  unsigned int channel)
>>> +{
>>> +   struct stm_drvdata *drvdata = container_of(stm_data,
>>> +  struct stm_drvdata, stm);
>>> +   if (!drvdata || !drvdata->csdev)
>>> +   return -EINVAL;
>>> +
>>> +   return coresight_enable(drvdata->csdev);
>>> +}
>>> +
>>> +static void stm_generic_unlink(struct stm_data *stm_data,
>>> +  unsigned int master,  unsigned int channel)
>>> +{
>>> +   struct stm_drvdata *drvdata = container_of(stm_data,
>>> +  struct stm_drvdata, stm);
>>> +   if (!drvdata || !drvdata->csdev)
>>> +   return;
>>> +
>>> +   stm_disable(drvdata->csdev);
>>> +}
>>> +
>>> +static long

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

2016-02-11 Thread Mathieu Poirier 
On 6 February 2016 at 04:04, Chunyan Zhang  wrote:
> From: Pratik Patel 
>
> 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 CoreSight 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 configfs.
>
> Signed-off-by: Pratik Patel 
> Signed-off-by: Mathieu Poirier 
> Signed-off-by: Chunyan Zhang 
> ---
>  .../ABI/testing/sysfs-bus-coresight-devices-stm|  53 ++
>  Documentation/trace/coresight.txt  |  37 +-
>  drivers/hwtracing/coresight/Kconfig|  11 +
>  drivers/hwtracing/coresight/Makefile   |   1 +
>  drivers/hwtracing/coresight/coresight-stm.c| 928 
> +
>  include/linux/coresight-stm.h  |   6 +
>  include/uapi/linux/coresight-stm.h |  12 +
>  7 files changed, 1046 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-stm
>  create mode 100644 drivers/hwtracing/coresight/coresight-stm.c
>  create mode 100644 include/linux/coresight-stm.h
>  create mode 100644 include/uapi/linux/coresight-stm.h
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-stm 
> b/Documentation/ABI/testing/sysfs-bus-coresight-devices-stm
> new file mode 100644
> index 000..a1d7022
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-stm
> @@ -0,0 +1,53 @@
> +What:  /sys/bus/coresight/devices/.stm/enable_source
> +Date:  February 2016
> +KernelVersion: 4.5
> +Contact:   Mathieu Poirier 
> +Description:   (RW) Enable/disable tracing on this specific trace macrocell.
> +   Enabling the trace macrocell implies it has been configured
> +   properly and a sink has been identidifed for it.  The path
> +   of coresight components linking the source to the sink is
> +   configured and managed automatically by the coresight 
> framework.
> +
> +What:  /sys/bus/coresight/devices/.stm/hwevent_enable
> +Date:  February 2016
> +KernelVersion: 4.5
> +Contact:   Mathieu Poirier 
> +Description:   (RW) Provides access to the HW event enable register, used in
> +   conjunction with HW event bank select register.
> +
> +What:  /sys/bus/coresight/devices/.stm/hwevent_select
> +Date:  February 2016
> +KernelVersion: 4.5
> +Contact:   Mathieu Poirier 
> +Description:   (RW) Gives access to the HW event block select register
> +   (STMHEBSR) in order to configure up to 256 channels.  Used in
> +   conjunction with "hwevent_enable" register as described above.
> +
> +What:  /sys/bus/coresight/devices/.stm/port_enable
> +Date:  February 2016
> +KernelVersion: 4.5
> +Contact:   Mathieu Poirier 
> +Description:   (RW) Provides access to the stimlus port enable register
> +   (STMSPER).  Used in conjunction with "port_select" described
> +   below.
> +
> +What:  /sys/bus/coresight/devices/.stm/port_select
> +Date:  February 2016
> +KernelVersion: 4.5
> +Contact:   Mathieu Poirier 
> +Description:   (RW) Used to determine which bank of stimulus port bit in
> +   register STMSPER (see above) apply to.
> +
> +What:  /sys/bus/coresight/devices/.stm/status
> +Date:  February 2016
> +KernelVersion: 4.5
> +Contact:   Mathieu Poirier 
> +Description:   (R) List various control and status registers.  The specific
> +   layout and content is driver specific.
> +
> +What:  /sys/bus/coresight/devices/.stm/traceid
> +Date:  February 2016
> +KernelVersion: 4.5
> +Contact:   Mathieu Poirier 
> +Description:   (RW) Holds the trace ID that will appear in the trace stream
> +   coming from this trace entity.
> diff --git a/Documentation/trace/coresight.txt 
> b/Documentation/trace/coresight.txt
> index 0a5c329..a33c88c 100644
> --- a/Documentation/trace/coresight.txt
> +++ b/Documentation/trace/coresight.txt
> @@ -190,8 +190,8 @@ expected to be accessed and controlled using those 
> entries.
>  Last but not least, "struct module *owner" is expected to be set to reflect
>  the information carried in "THIS_MODULE".
>
> -How to use
> ---
> +How to use the tracer modules
> +-
>
>  Before trace collection can start, a coresight sink needs to be identify.
>  There is no limit on the amount of sinks (nor sources) that