Message-ID: <20190417212325.GC14163@xps15>
Date:   Wed, 17 Apr 2019 15:23:25 -0600
From:   Mathieu Poirier <mathieu.poirier@...aro.org>
To:     Suzuki K Poulose <suzuki.poulose@....com>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        coresight@...ts.linaro.org, mike.leach@...aro.org,
        rjw@...ysocki.net, robert.walker@....com
Subject: Re: [PATCH v2 08/36] coresight: tmc: Clean up device specific data

On Mon, Apr 15, 2019 at 05:03:51PM +0100, 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 <mathieu.poirier@...aro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
> ---
>  drivers/hwtracing/coresight/coresight-catu.c    |  5 ++--
>  drivers/hwtracing/coresight/coresight-tmc-etf.c |  9 +++---
>  drivers/hwtracing/coresight/coresight-tmc-etr.c | 40 ++++++++++++++-----------
>  drivers/hwtracing/coresight/coresight-tmc.c     | 38 +++++++++++------------
>  drivers/hwtracing/coresight/coresight-tmc.h     |  1 -
>  5 files changed, 47 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> index f2a6a8d..ba3c005 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-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index a5f053f..6f60606 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -251,7 +251,6 @@ static int tmc_enable_etf_sink(struct coresight_device *csdev,
>  			       u32 mode, void *data)
>  {
>  	int ret;
> -	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>  
>  	switch (mode) {
>  	case CS_MODE_SYSFS:
> @@ -269,7 +268,7 @@ static int tmc_enable_etf_sink(struct coresight_device *csdev,
>  	if (ret)
>  		return ret;
>  
> -	dev_dbg(drvdata->dev, "TMC-ETB/ETF enabled\n");
> +	dev_dbg(&csdev->dev, "TMC-ETB/ETF enabled\n");
>  	return 0;
>  }
>  
> @@ -292,7 +291,7 @@ static void tmc_disable_etf_sink(struct coresight_device *csdev)
>  
>  	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>  
> -	dev_dbg(drvdata->dev, "TMC-ETB/ETF disabled\n");
> +	dev_dbg(&csdev->dev, "TMC-ETB/ETF disabled\n");
>  }
>  
>  static int tmc_enable_etf_link(struct coresight_device *csdev,
> @@ -314,7 +313,7 @@ static int tmc_enable_etf_link(struct coresight_device *csdev,
>  	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>  
>  	if (!ret)
> -		dev_dbg(drvdata->dev, "TMC-ETF enabled\n");
> +		dev_dbg(&csdev->dev, "TMC-ETF enabled\n");
>  	return ret;
>  }
>  
> @@ -334,7 +333,7 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
>  	drvdata->mode = CS_MODE_DISABLED;
>  	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>  
> -	dev_dbg(drvdata->dev, "TMC-ETF disabled\n");
> +	dev_dbg(&csdev->dev, "TMC-ETF disabled\n");
>  }
>  
>  static void *tmc_alloc_etf_buffer(struct coresight_device *csdev, int cpu,
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index f684283..0911f9c 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;

I would have kept the 'dev' as it is quite obvious from the dev->parent that we
are getting a reference on the parent.  That is just my opinion and it is
entirely up to you.

>  
>  	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->csdev->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);
> @@ -598,7 +601,7 @@ static int tmc_etr_alloc_flat_buf(struct tmc_drvdata *drvdata,
>  	}
>  
>  	flat_buf->size = etr_buf->size;
> -	flat_buf->dev = drvdata->dev;
> +	flat_buf->dev = &drvdata->csdev->dev;
>  	etr_buf->hwaddr = flat_buf->daddr;
>  	etr_buf->mode = ETR_MODE_FLAT;
>  	etr_buf->private = 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);
>  }
> @@ -657,8 +661,9 @@ static int tmc_etr_alloc_sg_buf(struct tmc_drvdata *drvdata,
>  				void **pages)
>  {
>  	struct etr_sg_table *etr_table;
> +	struct device *dev = &drvdata->csdev->dev;
>  
> -	etr_table = tmc_init_etr_sg_table(drvdata->dev, node,
> +	etr_table = tmc_init_etr_sg_table(dev, node,
>  					  etr_buf->size, pages);
>  	if (IS_ERR(etr_table))
>  		return -ENOMEM;
> @@ -813,9 +818,10 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata,
>  	bool has_etr_sg, has_iommu;
>  	bool has_sg, has_catu;
>  	struct etr_buf *etr_buf;
> +	struct device *dev = &drvdata->csdev->dev;
>  
>  	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(dev->parent);
>  	has_catu = !!tmc_etr_get_catu_device(drvdata);
>  
>  	has_sg = has_catu || has_etr_sg;
> @@ -853,7 +859,7 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata,
>  		return ERR_PTR(rc);
>  	}
>  
> -	dev_dbg(drvdata->dev, "allocated buffer of size %ldKB in mode %d\n",
> +	dev_dbg(dev, "allocated buffer of size %ldKB in mode %d\n",
>  		(unsigned long)size >> 10, etr_buf->mode);
>  	return etr_buf;
>  }
> @@ -1148,7 +1154,7 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
>  		tmc_etr_free_sysfs_buf(free_buf);
>  
>  	if (!ret)
> -		dev_dbg(drvdata->dev, "TMC-ETR enabled\n");
> +		dev_dbg(&csdev->dev, "TMC-ETR enabled\n");
>  
>  	return ret;
>  }
> @@ -1217,7 +1223,7 @@ static void *tmc_alloc_etr_buffer(struct coresight_device *csdev,
>  	etr_perf = tmc_etr_setup_perf_buf(drvdata, cpu_to_node(cpu),
>  					  nr_pages, pages, snapshot);
>  	if (IS_ERR(etr_perf)) {
> -		dev_dbg(drvdata->dev, "Unable to allocate ETR buffer\n");
> +		dev_dbg(&csdev->dev, "Unable to allocate ETR buffer\n");
>  		return NULL;
>  	}
>  
> @@ -1411,7 +1417,7 @@ static void tmc_disable_etr_sink(struct coresight_device *csdev)
>  
>  	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>  
> -	dev_dbg(drvdata->dev, "TMC-ETR disabled\n");
> +	dev_dbg(&csdev->dev, "TMC-ETR disabled\n");
>  }
>  
>  static const struct coresight_ops_sink tmc_etr_sink_ops = {
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
> index c6a5462..819873a 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc.c
> @@ -30,7 +30,7 @@ void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
>  	/* Ensure formatter, unformatter and hardware fifo are empty */
>  	if (coresight_timeout(drvdata->base,
>  			      TMC_STS, TMC_STS_TMCREADY_BIT, 1)) {
> -		dev_err(drvdata->dev,
> +		dev_err(&drvdata->csdev->dev,
>  			"timeout while waiting for TMC to be Ready\n");
>  	}
>  }
> @@ -47,7 +47,7 @@ void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
>  	/* Ensure flush completes */
>  	if (coresight_timeout(drvdata->base,
>  			      TMC_FFCR, TMC_FFCR_FLUSHMAN_BIT, 0)) {
> -		dev_err(drvdata->dev,
> +		dev_err(&drvdata->csdev->dev,
>  		"timeout while waiting for completion of Manual Flush\n");
>  	}
>  
> @@ -81,7 +81,7 @@ static int tmc_read_prepare(struct tmc_drvdata *drvdata)
>  	}
>  
>  	if (!ret)
> -		dev_dbg(drvdata->dev, "TMC read start\n");
> +		dev_dbg(&drvdata->csdev->dev, "TMC read start\n");
>  
>  	return ret;
>  }
> @@ -103,7 +103,7 @@ static int tmc_read_unprepare(struct tmc_drvdata *drvdata)
>  	}
>  
>  	if (!ret)
> -		dev_dbg(drvdata->dev, "TMC read end\n");
> +		dev_dbg(&drvdata->csdev->dev, "TMC read end\n");
>  
>  	return ret;
>  }
> @@ -120,7 +120,7 @@ static int tmc_open(struct inode *inode, struct file *file)
>  
>  	nonseekable_open(inode, file);
>  
> -	dev_dbg(drvdata->dev, "%s: successfully opened\n", __func__);
> +	dev_dbg(&drvdata->csdev->dev, "%s: successfully opened\n", __func__);
>  	return 0;
>  }
>  
> @@ -150,12 +150,13 @@ static ssize_t tmc_read(struct file *file, char __user *data, size_t len,
>  		return 0;
>  
>  	if (copy_to_user(data, bufp, actual)) {
> -		dev_dbg(drvdata->dev, "%s: copy_to_user failed\n", __func__);
> +		dev_dbg(&drvdata->csdev->dev,
> +			"%s: copy_to_user failed\n", __func__);
>  		return -EFAULT;
>  	}
>  
>  	*ppos += actual;
> -	dev_dbg(drvdata->dev, "%zu bytes copied\n", actual);
> +	dev_dbg(&drvdata->csdev->dev, "%zu bytes copied\n", actual);
>  
>  	return actual;
>  }
> @@ -170,7 +171,7 @@ static int tmc_release(struct inode *inode, struct file *file)
>  	if (ret)
>  		return ret;
>  
> -	dev_dbg(drvdata->dev, "%s: released\n", __func__);
> +	dev_dbg(&drvdata->csdev->dev, "%s: released\n", __func__);
>  	return 0;
>  }
>  
> @@ -330,24 +331,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 +364,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 +402,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 */
> @@ -446,7 +444,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;
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index 487c537..3eeadcf 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -175,7 +175,6 @@ struct etr_buf {
>   */
>  struct tmc_drvdata {
>  	void __iomem		*base;
> -	struct device		*dev;

Please clean up the structure documentation.

With that and regardless of what you decide to do about the 'real_dev':

Reviewed-by: Mathieu Poirier <mathieu.poirier@...aro.org>

>  	struct coresight_device	*csdev;
>  	struct miscdevice	miscdev;
>  	spinlock_t		spinlock;
> -- 
> 2.7.4
> 

Powered by blists - more mailing lists