lkml.org 
[lkml]   [2018]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 08/11] coresight: Add generic TMC sg table framework
From
Date
On 23/05/18 21:25, Mathieu Poirier wrote:
> On Fri, May 18, 2018 at 05:39:24PM +0100, Suzuki K Poulose wrote:
>> This patch introduces a generic sg table data structure and
>> associated operations. An SG table can be used to map a set
>> of Data pages where the trace data could be stored by the TMC
>> ETR. The information about the data pages could be stored in
>> different formats, depending on the type of the underlying
>> SG mechanism (e.g, TMC ETR SG vs Coresight CATU). The generic
>> structure provides book keeping of the pages used for the data
>> as well as the table contents. The table should be filled by
>> the user of the infrastructure.
>>
>> A table can be created by specifying the number of data pages
>> as well as the number of table pages required to hold the
>> pointers, where the latter could be different for different
>> types of tables. The pages are mapped in the appropriate dma
>> data direction mode (i.e, DMA_TO_DEVICE for table pages
>> and DMA_FROM_DEVICE for data pages). The framework can optionally
>> accept a set of allocated data pages (e.g, perf ring buffer) and
>> map them accordingly. The table and data pages are vmap'ed to allow
>> easier access by the drivers. The framework also provides helpers to
>> sync the data written to the pages with appropriate directions.
>>
>> This will be later used by the TMC ETR SG unit and CATU.
>>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>> Changes since v1:
>> - Address code style issues, more comments
>> ---
>> drivers/hwtracing/coresight/coresight-tmc-etr.c | 290 ++++++++++++++++++++++++
>> drivers/hwtracing/coresight/coresight-tmc.h | 50 ++++
>> 2 files changed, 340 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> index 9780798..1e844f8 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> @@ -17,9 +17,299 @@


>> +static inline dma_addr_t tmc_sg_table_base_paddr(struct tmc_sg_table *sg_table)
>> +{
>> + if (WARN_ON(!sg_table->data_pages.pages[0]))
>> + return 0;
>> + return sg_table->table_daddr;
>> +}
>> +
>> +static inline void *tmc_sg_table_base_vaddr(struct tmc_sg_table *sg_table)
>> +{
>> + if (WARN_ON(!sg_table->data_pages.pages[0]))
>> + return NULL;
>> + return sg_table->table_vaddr;
>> +}
>
> The above two functions deal with DMA'able and virtual addresses for the table
> page buffer. Yet the test in the WARN_ON is done on the data page array.
> Shouldn't this be sg_table->table_pages.pages[0] instead?

The table is as good as empty if there are no data pages associated with
the table. Hence the data_pages check.

>
> If not please add a comment justifying your position so that someone else
> looking at the code does't end up thinking the same in a year from now.

I will add a comment to reflect the above.

>
>> +
>> +static inline void *
>> +tmc_sg_table_data_vaddr(struct tmc_sg_table *sg_table)
>> +{
>> + if (WARN_ON(!sg_table->data_pages.nr_pages))
>> + return 0;
>> + return sg_table->data_vaddr;
>> +}
>
> I see that tmc_sg_table_base_vaddr() and tmc_sg_table_data_vaddr() are both
> returning the virtual address of the contiguous buffer for table and data
> respectively. Yet there is a discrepency in the naming convention. I would
> have expected tmc_sg_table_base_vaddr() and tmc_sg_data_base_vaddr() so that
> there is a little symmetry between them.

Agree. I will fix it.

Suzuki

\
 
 \ /
  Last update: 2018-05-25 18:07    [W:2.389 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site