Re: [PATCH 08/11] coresight: Add generic TMC sg table framework

From: Suzuki K Poulose
Date: Fri May 25 2018 - 12:19:37 EST


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@xxxxxxxxxx>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
---
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