Date:   Tue, 23 Apr 2019 11:38:37 -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 30/36] coresight: Use platform agnostic names

On Mon, Apr 15, 2019 at 05:04:13PM +0100, Suzuki K Poulose wrote:
> So far we have reused the name of the "platform" device for
> the CoreSight device. But this is not very intuitive when
> we move to ACPI. Also, the ACPI device names have ":" in them
> (e.g, ARMHC97C:01), which the perf tool doesn't like very much.
> This patch introduces a generic naming scheme, givin more intuitive

s/givin/"and gives"

> names for the devices that appear on the CoreSight bus.
> The names follow the pattern "prefix" followed by "index" (e.g, etm5).
> We maintain a list of allocated devices per "prefix" to make sure
> we don't allocate a new name when it is reprobed (e.g, due to
> unsatisifed device dependencies). So, we maintain the list
> of "fwnodes" of the parent devices to allocate a consistent name.
> All devices except the ETMs get an index allocated in the order
> of probing. ETMs get an index based on the CPU they are attached to.
> 
> TMC devices are named using "tmc_etf", "tmc_etb", and "tmc_etr"
> prefixes depending on the configuration of the device.
> 
> The replicators are not classified as dynamic/static anymore.
> One could easily figure that out by checking the presence of
> "mgmt" registers under sysfs.
> 
> Cc: Mathieu Poirier <mathieu.poirier@...aro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
> ---
>  drivers/hwtracing/coresight/coresight-catu.c       |  7 ++-
>  drivers/hwtracing/coresight/coresight-etb10.c      |  7 ++-
>  drivers/hwtracing/coresight/coresight-etm3x.c      |  4 +-
>  drivers/hwtracing/coresight/coresight-etm4x.c      |  5 +-
>  drivers/hwtracing/coresight/coresight-funnel.c     |  7 ++-
>  drivers/hwtracing/coresight/coresight-replicator.c |  7 ++-
>  drivers/hwtracing/coresight/coresight-stm.c        | 12 +++--
>  drivers/hwtracing/coresight/coresight-tmc.c        | 15 +++++-
>  drivers/hwtracing/coresight/coresight-tpiu.c       |  7 ++-
>  drivers/hwtracing/coresight/coresight.c            | 58 ++++++++++++++++++++++
>  include/linux/coresight.h                          | 25 +++++++++-
>  11 files changed, 142 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> index f249be6..e0637fc 100644
> --- a/drivers/hwtracing/coresight/coresight-catu.c
> +++ b/drivers/hwtracing/coresight/coresight-catu.c
> @@ -28,6 +28,8 @@
>  #define catu_dbg(x, ...) do {} while (0)
>  #endif
>  
> +DEFINE_CORESIGHT_DEVLIST(catu_devs, "catu");
> +
>  struct catu_etr_buf {
>  	struct tmc_sg_table *catu_table;
>  	dma_addr_t sladdr;
> @@ -505,6 +507,10 @@ static int catu_probe(struct amba_device *adev, const struct amba_id *id)
>  	struct device *dev = &adev->dev;
>  	void __iomem *base;
>  
> +	catu_desc.name = coresight_alloc_device_name(&catu_devs, dev);
> +	if (!catu_desc.name)
> +		return -ENOMEM;
> +
>  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>  	if (!drvdata) {
>  		ret = -ENOMEM;
> @@ -551,7 +557,6 @@ static int catu_probe(struct amba_device *adev, const struct amba_id *id)
>  	catu_desc.type = CORESIGHT_DEV_TYPE_HELPER;
>  	catu_desc.subtype.helper_subtype = CORESIGHT_DEV_SUBTYPE_HELPER_CATU;
>  	catu_desc.ops = &catu_ops;
> -	catu_desc.name = dev_name(dev);
>  
>  	drvdata->csdev = coresight_register(&catu_desc);
>  	if (IS_ERR(drvdata->csdev))
> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
> index e5b46c7..8361b91 100644
> --- a/drivers/hwtracing/coresight/coresight-etb10.c
> +++ b/drivers/hwtracing/coresight/coresight-etb10.c
> @@ -62,6 +62,8 @@
>  #define ETB_FFSR_BIT		1
>  #define ETB_FRAME_SIZE_WORDS	4
>  
> +DEFINE_CORESIGHT_DEVLIST(etb_devs, "etb");
> +
>  /**
>   * struct etb_drvdata - specifics associated to an ETB component
>   * @base:	memory mapped base address for this component.
> @@ -682,6 +684,10 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id)
>  	struct resource *res = &adev->res;
>  	struct coresight_desc desc = { 0 };
>  
> +	desc.name = coresight_alloc_device_name(&etb_devs, dev);
> +	if (!desc.name)
> +		return -ENOMEM;
> +
>  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>  	if (!drvdata)
>  		return -ENOMEM;
> @@ -723,7 +729,6 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id)
>  	desc.ops = &etb_cs_ops;
>  	desc.pdata = pdata;
>  	desc.dev = dev;
> -	desc.name = dev_name(dev);
>  	desc.groups = coresight_etb_groups;
>  	drvdata->csdev = coresight_register(&desc);
>  	if (IS_ERR(drvdata->csdev))
> diff --git a/drivers/hwtracing/coresight/coresight-etm3x.c b/drivers/hwtracing/coresight/coresight-etm3x.c
> index f2d4616..bed7291 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x.c
> @@ -815,6 +815,9 @@ static int etm_probe(struct amba_device *adev, const struct amba_id *id)
>  	}
>  
>  	drvdata->cpu = coresight_get_cpu(dev);
> +	desc.name  = devm_kasprintf(dev, GFP_KERNEL, "etm%d", drvdata->cpu);
> +	if (!desc.name)
> +		return -ENOMEM;
>  
>  	cpus_read_lock();
>  	etmdrvdata[drvdata->cpu] = drvdata;
> @@ -856,7 +859,6 @@ static int etm_probe(struct amba_device *adev, const struct amba_id *id)
>  	desc.ops = &etm_cs_ops;
>  	desc.pdata = pdata;
>  	desc.dev = dev;
> -	desc.name = dev_name(dev);
>  	desc.groups = coresight_etm_groups;
>  	drvdata->csdev = coresight_register(&desc);
>  	if (IS_ERR(drvdata->csdev)) {
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> index 7c53fb2..5c13a04 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> @@ -993,6 +993,10 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
>  	spin_lock_init(&drvdata->spinlock);
>  
>  	drvdata->cpu = coresight_get_cpu(dev);
> +	desc.name = devm_kasprintf(dev, GFP_KERNEL, "etm%d", drvdata->cpu);
> +	if (!desc.name)
> +		return -ENOMEM;
> +

One newline too many.

With the above:

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

>  
>  	cpus_read_lock();
>  	etmdrvdata[drvdata->cpu] = drvdata;
> @@ -1034,7 +1038,6 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
>  	desc.pdata = pdata;
>  	desc.dev = dev;
>  	desc.groups = coresight_etmv4_groups;
> -	desc.name = dev_name(dev);
>  	drvdata->csdev = coresight_register(&desc);
>  	if (IS_ERR(drvdata->csdev)) {
>  		ret = PTR_ERR(drvdata->csdev);
> diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c
> index 38af045..9b820b1 100644
> --- a/drivers/hwtracing/coresight/coresight-funnel.c
> +++ b/drivers/hwtracing/coresight/coresight-funnel.c
> @@ -27,6 +27,8 @@
>  #define FUNNEL_HOLDTIME		(0x7 << FUNNEL_HOLDTIME_SHFT)
>  #define FUNNEL_ENSx_MASK	0xff
>  
> +DEFINE_CORESIGHT_DEVLIST(funnel_devs, "funnel");
> +
>  /**
>   * struct funnel_drvdata - specifics associated to a funnel component
>   * @base:	memory mapped base address for this component.
> @@ -186,6 +188,10 @@ static int funnel_probe(struct amba_device *adev, const struct amba_id *id)
>  	struct resource *res = &adev->res;
>  	struct coresight_desc desc = { 0 };
>  
> +	desc.name = coresight_alloc_device_name(&funnel_devs, dev);
> +	if (!desc.name)
> +		return -ENOMEM;
> +
>  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>  	if (!drvdata)
>  		return -ENOMEM;
> @@ -216,7 +222,6 @@ static int funnel_probe(struct amba_device *adev, const struct amba_id *id)
>  	desc.ops = &funnel_cs_ops;
>  	desc.pdata = pdata;
>  	desc.dev = dev;
> -	desc.name = dev_name(dev);
>  	desc.groups = coresight_funnel_groups;
>  	drvdata->csdev = coresight_register(&desc);
>  
> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
> index 8fbc028..2eb489c 100644
> --- a/drivers/hwtracing/coresight/coresight-replicator.c
> +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> @@ -22,6 +22,8 @@
>  #define REPLICATOR_IDFILTER0		0x000
>  #define REPLICATOR_IDFILTER1		0x004
>  
> +DEFINE_CORESIGHT_DEVLIST(replicator_devs, "replicator");
> +
>  /**
>   * struct replicator_drvdata - specifics associated to a replicator component
>   * @base:	memory mapped base address for this component. Also indicates
> @@ -179,6 +181,10 @@ static int replicator_probe(struct device *dev, struct resource *res)
>  	struct coresight_desc desc = { 0 };
>  	void __iomem *base;
>  
> +	desc.name = coresight_alloc_device_name(&replicator_devs, dev);
> +	if (!desc.name)
> +		return -ENOMEM;
> +
>  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>  	if (!drvdata)
>  		return -ENOMEM;
> @@ -217,7 +223,6 @@ static int replicator_probe(struct device *dev, struct resource *res)
>  	desc.ops = &replicator_cs_ops;
>  	desc.pdata = dev->platform_data;
>  	desc.dev = dev;
> -	desc.name = dev_name(dev);
>  
>  	drvdata->csdev = coresight_register(&desc);
>  	if (IS_ERR(drvdata->csdev)) {
> diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
> index 5c7fc4a..2d6ae00 100644
> --- a/drivers/hwtracing/coresight/coresight-stm.c
> +++ b/drivers/hwtracing/coresight/coresight-stm.c
> @@ -107,6 +107,8 @@ struct channel_space {
>  	unsigned long		*guaranteed;
>  };
>  
> +DEFINE_CORESIGHT_DEVLIST(stm_devs, "stm");
> +
>  /**
>   * struct stm_drvdata - specifics associated to an STM component
>   * @base:		memory mapped base address for this component.
> @@ -810,6 +812,10 @@ static int stm_probe(struct amba_device *adev, const struct amba_id *id)
>  	size_t bitmap_size;
>  	struct coresight_desc desc = { 0 };
>  
> +	desc.name = coresight_alloc_device_name(&stm_devs, dev);
> +	if (!desc.name)
> +		return -ENOMEM;
> +
>  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>  	if (!drvdata)
>  		return -ENOMEM;
> @@ -854,11 +860,12 @@ static int stm_probe(struct amba_device *adev, const struct amba_id *id)
>  	spin_lock_init(&drvdata->spinlock);
>  
>  	stm_init_default_data(drvdata);
> -	stm_init_generic_data(drvdata, dev_name(dev));
> +	stm_init_generic_data(drvdata, desc.name);
>  
>  	if (stm_register_device(dev, &drvdata->stm, THIS_MODULE)) {
>  		dev_info(dev,
> -			 "stm_register_device failed, probing deferred\n");
> +			 "%s : stm_register_device failed, probing deferred\n",
> +			 desc.name);
>  		return -EPROBE_DEFER;
>  	}
>  
> @@ -874,7 +881,6 @@ static int stm_probe(struct amba_device *adev, const struct amba_id *id)
>  	desc.ops = &stm_cs_ops;
>  	desc.pdata = pdata;
>  	desc.dev = dev;
> -	desc.name = dev_name(dev);
>  	desc.groups = coresight_stm_groups;
>  	drvdata->csdev = coresight_register(&desc);
>  	if (IS_ERR(drvdata->csdev)) {
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
> index 94d3ebf..8b7b5d0 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc.c
> @@ -25,6 +25,10 @@
>  #include "coresight-priv.h"
>  #include "coresight-tmc.h"
>  
> +DEFINE_CORESIGHT_DEVLIST(etb_devs, "tmc_etb");
> +DEFINE_CORESIGHT_DEVLIST(etf_devs, "tmc_etf");
> +DEFINE_CORESIGHT_DEVLIST(etr_devs, "tmc_etr");
> +
>  void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
>  {
>  	/* Ensure formatter, unformatter and hardware fifo are empty */
> @@ -395,6 +399,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
>  	struct tmc_drvdata *drvdata;
>  	struct resource *res = &adev->res;
>  	struct coresight_desc desc = { 0 };
> +	struct coresight_dev_list *dev_list = NULL;
>  
>  	ret = -ENOMEM;
>  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> @@ -425,13 +430,13 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
>  
>  	desc.dev = dev;
>  	desc.groups = coresight_tmc_groups;
> -	desc.name = dev_name(dev);
>  
>  	switch (drvdata->config_type) {
>  	case TMC_CONFIG_TYPE_ETB:
>  		desc.type = CORESIGHT_DEV_TYPE_SINK;
>  		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
>  		desc.ops = &tmc_etb_cs_ops;
> +		dev_list = &etb_devs;
>  		break;
>  	case TMC_CONFIG_TYPE_ETR:
>  		desc.type = CORESIGHT_DEV_TYPE_SINK;
> @@ -441,11 +446,13 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
>  					 coresight_get_uci_data(id));
>  		if (ret)
>  			goto out;
> +		dev_list = &etr_devs;
>  		break;
>  	case TMC_CONFIG_TYPE_ETF:
>  		desc.type = CORESIGHT_DEV_TYPE_LINKSINK;
>  		desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO;
>  		desc.ops = &tmc_etf_cs_ops;
> +		dev_list = &etf_devs;
>  		break;
>  	default:
>  		pr_err("%s: Unsupported TMC config\n", desc.name);
> @@ -453,6 +460,12 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
>  		goto out;
>  	}
>  
> +	desc.name = coresight_alloc_device_name(dev_list, dev);
> +	if (!desc.name) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
>  	pdata = coresight_get_platform_data(dev);
>  	if (IS_ERR(pdata)) {
>  		ret = PTR_ERR(pdata);
> diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c
> index 39a509f..0404434 100644
> --- a/drivers/hwtracing/coresight/coresight-tpiu.c
> +++ b/drivers/hwtracing/coresight/coresight-tpiu.c
> @@ -46,6 +46,8 @@
>  #define FFCR_FON_MAN		BIT(6)
>  #define FFCR_STOP_FI		BIT(12)
>  
> +DEFINE_CORESIGHT_DEVLIST(tpiu_devs, "tpiu");
> +
>  /**
>   * @base:	memory mapped base address for this component.
>   * @dev:	the device entity associated to this component.
> @@ -121,6 +123,10 @@ static int tpiu_probe(struct amba_device *adev, const struct amba_id *id)
>  	struct resource *res = &adev->res;
>  	struct coresight_desc desc = { 0 };
>  
> +	desc.name = coresight_alloc_device_name(&tpiu_devs, dev);
> +	if (!desc.name)
> +		return -ENOMEM;
> +
>  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>  	if (!drvdata)
>  		return -ENOMEM;
> @@ -153,7 +159,6 @@ static int tpiu_probe(struct amba_device *adev, const struct amba_id *id)
>  	desc.ops = &tpiu_cs_ops;
>  	desc.pdata = pdata;
>  	desc.dev = dev;
> -	desc.name = dev_name(dev);
>  	drvdata->csdev = coresight_register(&desc);
>  
>  	if (!IS_ERR(drvdata->csdev)) {
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 5169ce1..e3b9321 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -1289,3 +1289,61 @@ void coresight_unregister(struct coresight_device *csdev)
>  	device_unregister(&csdev->dev);
>  }
>  EXPORT_SYMBOL_GPL(coresight_unregister);
> +
> +
> +/*
> + * coresight_search_device_idx - Search the fwnode handle of a device
> + * in the given dev_idx list. Must be called with the coresight_mutex held.
> + *
> + * Returns the index of the entry, when found. Otherwise, -ENOENT.
> + */
> +static inline int coresight_search_device_idx(struct coresight_dev_list *dict,
> +					      struct fwnode_handle *fwnode)
> +{
> +	int i;
> +
> +	for (i = 0; i < dict->nr_idx; i++)
> +		if (dict->fwnode_list[i] == fwnode)
> +			return i;
> +	return -ENOENT;
> +}
> +
> +/*
> + * coresight_alloc_device_name - Get an index for a given device in the
> + * device index list specific to a driver. An index is allocated for a
> + * device and is tracked with the fwnode_handle to prevent allocating
> + * duplicate indices for the same device (e.g, if we defer probing of
> + * a device due to dependencies), in case the index is requested again.
> + */
> +char *coresight_alloc_device_name(struct coresight_dev_list *dict,
> +				  struct device *dev)
> +{
> +	int idx;
> +	char *name = NULL;
> +	struct fwnode_handle **list;
> +
> +	mutex_lock(&coresight_mutex);
> +
> +	idx = coresight_search_device_idx(dict, dev_fwnode(dev));
> +	if (idx < 0) {
> +		/* Make space for the new entry */
> +		idx = dict->nr_idx;
> +		list = krealloc(dict->fwnode_list,
> +				(idx + 1) * sizeof(*dict->fwnode_list),
> +				GFP_KERNEL);
> +		if (ZERO_OR_NULL_PTR(list)) {
> +			idx = -ENOMEM;
> +			goto done;
> +		}
> +
> +		list[idx] = dev_fwnode(dev);
> +		dict->fwnode_list = list;
> +		dict->nr_idx = idx + 1;
> +	}
> +
> +	name = devm_kasprintf(dev, GFP_KERNEL, "%s%d", dict->pfx, idx);
> +done:
> +	mutex_unlock(&coresight_mutex);
> +	return name;
> +}
> +EXPORT_SYMBOL_GPL(coresight_alloc_device_name);
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 188e759..d6367a0 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -168,6 +168,28 @@ struct coresight_device {
>  	struct dev_ext_attribute *ea;
>  };
>  
> +/*
> + * coresight_dev_list - Mapping for devices to "name" index for device
> + * names.
> + *
> + * @nr_idx:		Number of entries already allocated.
> + * @pfx:		Prefix pattern for device name.
> + * @fwnode_list:	Array of fwnode_handles associated with each allocated
> + *			index, upto nr_idx entries.
> + */
> +struct coresight_dev_list {
> +	int			nr_idx;
> +	const char		*pfx;
> +	struct fwnode_handle	**fwnode_list;
> +};
> +
> +#define DEFINE_CORESIGHT_DEVLIST(var, dev_pfx)				\
> +static struct coresight_dev_list (var) = {				\
> +						.pfx = dev_pfx,		\
> +						.nr_idx = 0,		\
> +						.fwnode_list = NULL,	\
> +}
> +
>  #define to_coresight_device(d) container_of(d, struct coresight_device, dev)
>  
>  #define source_ops(csdev)	csdev->ops->source_ops
> @@ -260,7 +282,8 @@ extern int coresight_claim_device_unlocked(void __iomem *base);
>  
>  extern void coresight_disclaim_device(void __iomem *base);
>  extern void coresight_disclaim_device_unlocked(void __iomem *base);
> -
> +extern char *coresight_alloc_device_name(struct coresight_dev_list *devs,
> +					 struct device *dev);
>  #else
>  static inline struct coresight_device *
>  coresight_register(struct coresight_desc *desc) { return NULL; }
> -- 
> 2.7.4
> 

Powered by blists - more mailing lists