Re: [PATCH 08/25] coresight: tmc: Clean up device specific data

Mathieu Poirier
2019-03-26T21:53:28Z

Hi Suzuki,

On Wed, Mar 20, 2019 at 06:49:25PM +0000, Suzuki K Poulose wrote:
> In preparation to use a consistent device naming scheme,
> clean up the device link tracking in replicator driver.
> Use the "coresight" device instead of the "real" parent device
> for all internal purposes. All other requests (e.g, power management,
> DMA operations) must use the "real" device which is the parent device.
>
> Since the CATU driver also uses the TMC-SG infrastructure, update
> the callers to ensure they pass the appropriate device argument
> for the tables.
>
> Cc: Mathieu Poirier <mathie.poirier@linaro.org>

I was wondering why patches 6, 7, and 8 didn't make it to my inbox... Now I
have the answer :o)

> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> drivers/hwtracing/coresight/coresight-catu.c | 5 ++---
> drivers/hwtracing/coresight/coresight-tmc-etr.c | 26 ++++++++++++++-----------
> drivers/hwtracing/coresight/coresight-tmc.c | 22 ++++++++++-----------
> 3 files changed, 27 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> index 170fbb6..11c6f2f 100644
> --- a/drivers/hwtracing/coresight/coresight-catu.c
> +++ b/drivers/hwtracing/coresight/coresight-catu.c
> @@ -328,19 +328,18 @@ static int catu_alloc_etr_buf(struct tmc_drvdata *tmc_drvdata,
> struct etr_buf *etr_buf, int node, void **pages)
> {
> struct coresight_device *csdev;
> - struct device *catu_dev;
> struct tmc_sg_table *catu_table;
> struct catu_etr_buf *catu_buf;
>
> csdev = tmc_etr_get_catu_device(tmc_drvdata);
> if (!csdev)
> return -ENODEV;
> - catu_dev = csdev->dev.parent;
> catu_buf = kzalloc(sizeof(*catu_buf), GFP_KERNEL);
> if (!catu_buf)
> return -ENOMEM;
>
> - catu_table = catu_init_sg_table(catu_dev, node, etr_buf->size, pages);
> + catu_table = catu_init_sg_table(&csdev->dev, node,
> + etr_buf->size, pages);
> if (IS_ERR(catu_table)) {
> kfree(catu_buf);
> return PTR_ERR(catu_table);
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index f684283..4152774 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -153,10 +153,11 @@ static void tmc_pages_free(struct tmc_pages *tmc_pages,
> struct device *dev, enum dma_data_direction dir)
> {
> int i;
> + struct device *real_dev = dev->parent;
>
> for (i = 0; i < tmc_pages->nr_pages; i++) {
> if (tmc_pages->daddrs && tmc_pages->daddrs[i])
> - dma_unmap_page(dev, tmc_pages->daddrs[i],
> + dma_unmap_page(real_dev, tmc_pages->daddrs[i],
> PAGE_SIZE, dir);
> if (tmc_pages->pages && tmc_pages->pages[i])
> __free_page(tmc_pages->pages[i]);
> @@ -184,6 +185,7 @@ static int tmc_pages_alloc(struct tmc_pages *tmc_pages,
> int i, nr_pages;
> dma_addr_t paddr;
> struct page *page;
> + struct device *real_dev = dev->parent;
>
> nr_pages = tmc_pages->nr_pages;
> tmc_pages->daddrs = kcalloc(nr_pages, sizeof(*tmc_pages->daddrs),
> @@ -207,8 +209,8 @@ static int tmc_pages_alloc(struct tmc_pages *tmc_pages,
> page = alloc_pages_node(node,
> GFP_KERNEL | __GFP_ZERO, 0);
> }
> - paddr = dma_map_page(dev, page, 0, PAGE_SIZE, dir);
> - if (dma_mapping_error(dev, paddr))
> + paddr = dma_map_page(real_dev, page, 0, PAGE_SIZE, dir);
> + if (dma_mapping_error(real_dev, paddr))
> goto err;
> tmc_pages->daddrs[i] = paddr;
> tmc_pages->pages[i] = page;
> @@ -295,7 +297,7 @@ static int tmc_alloc_data_pages(struct tmc_sg_table *sg_table, void **pages)
> * and data buffers. TMC writes to the data buffers and reads from the SG
> * Table pages.
> *
> - * @dev - Device to which page should be DMA mapped.
> + * @dev - Coresight device to which page should be DMA mapped.
> * @node - Numa node for mem allocations
> * @nr_tpages - Number of pages for the table entries.
> * @nr_dpages - Number of pages for Data buffer.
> @@ -339,13 +341,13 @@ void tmc_sg_table_sync_data_range(struct tmc_sg_table *table,
> {
> int i, index, start;
> int npages = DIV_ROUND_UP(size, PAGE_SIZE);
> - struct device *dev = table->dev;
> + struct device *real_dev = table->dev->parent;
> struct tmc_pages *data = &table->data_pages;
>
> start = offset >> PAGE_SHIFT;
> for (i = start; i < (start + npages); i++) {
> index = i % data->nr_pages;
> - dma_sync_single_for_cpu(dev, data->daddrs[index],
> + dma_sync_single_for_cpu(real_dev, data->daddrs[index],
> PAGE_SIZE, DMA_FROM_DEVICE);
> }
> }
> @@ -354,11 +356,11 @@ void tmc_sg_table_sync_data_range(struct tmc_sg_table *table,
> void tmc_sg_table_sync_table(struct tmc_sg_table *sg_table)
> {
> int i;
> - struct device *dev = sg_table->dev;
> + struct device *real_dev = sg_table->dev->parent;
> struct tmc_pages *table_pages = &sg_table->table_pages;
>
> for (i = 0; i < table_pages->nr_pages; i++)
> - dma_sync_single_for_device(dev, table_pages->daddrs[i],
> + dma_sync_single_for_device(real_dev, table_pages->daddrs[i],
> PAGE_SIZE, DMA_TO_DEVICE);
> }
>
> @@ -581,6 +583,7 @@ static int tmc_etr_alloc_flat_buf(struct tmc_drvdata *drvdata,
> void **pages)
> {
> struct etr_flat_buf *flat_buf;
> + struct device *real_dev = drvdata->dev->parent;
>
> /* We cannot reuse existing pages for flat buf */
> if (pages)
> @@ -590,7 +593,7 @@ static int tmc_etr_alloc_flat_buf(struct tmc_drvdata *drvdata,
> if (!flat_buf)
> return -ENOMEM;
>
> - flat_buf->vaddr = dma_alloc_coherent(drvdata->dev, etr_buf->size,
> + flat_buf->vaddr = dma_alloc_coherent(real_dev, etr_buf->size,
> &flat_buf->daddr, GFP_KERNEL);
> if (!flat_buf->vaddr) {
> kfree(flat_buf);
> @@ -608,9 +611,10 @@ static int tmc_etr_alloc_flat_buf(struct tmc_drvdata *drvdata,
> static void tmc_etr_free_flat_buf(struct etr_buf *etr_buf)
> {
> struct etr_flat_buf *flat_buf = etr_buf->private;
> + struct device *real_dev = flat_buf->dev->parent;
>
> if (flat_buf && flat_buf->daddr)
> - dma_free_coherent(flat_buf->dev, flat_buf->size,
> + dma_free_coherent(real_dev, flat_buf->size,
> flat_buf->vaddr, flat_buf->daddr);
> kfree(flat_buf);
> }
> @@ -815,7 +819,7 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata,
> struct etr_buf *etr_buf;
>
> has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG);
> - has_iommu = iommu_get_domain_for_dev(drvdata->dev);
> + has_iommu = iommu_get_domain_for_dev(drvdata->dev->parent);
> has_catu = !!tmc_etr_get_catu_device(drvdata);
>
> has_sg = has_catu || has_etr_sg;
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
> index 647b6aa..9e7ff83 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc.c
> @@ -330,24 +330,22 @@ const struct attribute_group *coresight_tmc_groups[] = {
> NULL,
> };
>
> -static inline bool tmc_etr_can_use_sg(struct tmc_drvdata *drvdata)
> +static inline bool tmc_etr_can_use_sg(struct device *dev)
> {
> - return fwnode_property_present(drvdata->dev->fwnode,
> - "arm,scatter-gather");
> + return fwnode_property_present(dev->fwnode, "arm,scatter-gather");
> }
>
> /* Detect and initialise the capabilities of a TMC ETR */
> -static int tmc_etr_setup_caps(struct tmc_drvdata *drvdata,
> - u32 devid, void *dev_caps)
> +static int tmc_etr_setup_caps(struct device *parent, u32 devid, void *dev_caps)
> {
> int rc;
> -
> u32 dma_mask = 0;
> + struct tmc_drvdata *drvdata = dev_get_drvdata(parent);
>
> /* Set the unadvertised capabilities */
> tmc_etr_init_caps(drvdata, (u32)(unsigned long)dev_caps);
>
> - if (!(devid & TMC_DEVID_NOSCAT) && tmc_etr_can_use_sg(drvdata))
> + if (!(devid & TMC_DEVID_NOSCAT) && tmc_etr_can_use_sg(parent))
> tmc_etr_set_cap(drvdata, TMC_ETR_SG);
>
> /* Check if the AXI address width is available */
> @@ -365,15 +363,15 @@ static int tmc_etr_setup_caps(struct tmc_drvdata *drvdata,
> case 44:
> case 48:
> case 52:
> - dev_info(drvdata->dev, "Detected dma mask %dbits\n", dma_mask);
> + dev_info(parent, "Detected dma mask %dbits\n", dma_mask);
> break;
> default:
> dma_mask = 40;
> }
>
> - rc = dma_set_mask_and_coherent(drvdata->dev, DMA_BIT_MASK(dma_mask));
> + rc = dma_set_mask_and_coherent(parent, DMA_BIT_MASK(dma_mask));
> if (rc)
> - dev_err(drvdata->dev, "Failed to setup DMA mask: %d\n", rc);
> + dev_err(parent, "Failed to setup DMA mask: %d\n", rc);
> return rc;
> }
>
> @@ -403,7 +401,6 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
> if (!drvdata)
> goto out;
>
> - drvdata->dev = &adev->dev;
> dev_set_drvdata(dev, drvdata);
>
> /* Validity for the resource is already checked by the AMBA core */
> @@ -448,7 +445,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
> desc.type = CORESIGHT_DEV_TYPE_SINK;
> desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
> desc.ops = &tmc_etr_cs_ops;
> - ret = tmc_etr_setup_caps(drvdata, devid,
> + ret = tmc_etr_setup_caps(dev, devid,
> coresight_get_uci_data(id));
> if (ret)
> goto out;
> @@ -470,6 +467,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
> goto out;
> }
>
> + drvdata->dev = &drvdata->csdev->dev;

Any reason for not getting rid of tmc_drvdata::dev like you did for the funnel,
replicator and catu?

> drvdata->miscdev.name = pdata->name;
> drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
> drvdata->miscdev.fops = &tmc_fops;
> --
> 2.7.4
>