mbox series

[00/25] coresight: Support for ACPI bindings

Message ID 1553107783-3340-1-git-send-email-suzuki.poulose@arm.com
Headers show
Series coresight: Support for ACPI bindings | expand

Message

Suzuki K Poulose March 20, 2019, 6:49 p.m. UTC
This series adds the support for CoreSight devices on ACPI based
platforms. The device connections are encoded as _DSD graph property[0],
with CoreSight specific extensions to indicate the direction of data
flow as described in [1]. Components attached to CPUs are listed
as child devices of the corresponding CPU, removing explicit links
to the CPU like we do in the DT.

As a prepartion for the ACPI support, we merge the driver for dynamic
and non-programmable replicators. We introduce platform independent
helpers to parse the platform supplied information. Thus we rename
the platform handling code from:
	of_coresight.c  => coresight-platform.c

The CoreSight driver creates shadow devices that appear on the Coresight
bus, in addition to the real devices (e.g, AMBA bus devices). The name
of these devices match the real device. This makes the device name
a bit cryptic for ACPI platform. So this series also introduces a generic
platform agnostic device naming scheme for the shadow Coresight devices.
Towards this we also make changes to the way we lookup devices to resolve
the connections, as we can't use the names to identify the devices. So,
we use the "fwnode_handle" of the real device for the device lookups.
Towards that we clean up the drivers to keep track of the "CoreSight"
device rather than the "real" device. However, all real operations,
like DMA allocation, Power management etc. must be performed on
the real device which is the parent of the shadow device.

Finally we add the support for parsing the ACPI platform data. The power
management support is missing in the ACPI (and this is not specific to
CoreSight). The firmware must ensure that the respective power domains
are turned on.

Applies on v5.1-rc1

Tested on a Juno-r0 board with ACPI bindings patch (Patch 26/25) added on
top of [2]. You would need to make sure that the debug power domain is
turned on before the Linux kernel boots. (e.g, connect the DS-5 to the
Juno board while at UEFI). arm32 code is only compile tested.

[0] ACPI Device Graphs using _DSD (Not available online yet, approved but
    awaiting publish and eventually should be linked at).
    https://uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel-1_1.htm
[1] https://developer.arm.com/docs/den0067/latest/acpi-for-coresighttm-10-platform-design-document
[2] https://github.com/tianocore/edk2-platforms.git

Suzuki K Poulose (25):
  coresight: tmc: Report DMA setup failures
  coresight: dynamic-replicator: Clean up error handling
  coresight: replicator: Prepare for merging with dynamic-replicator
  coresight: dynamic-replicator: Prepare for merging with static
    replicator
  coresight: Merge the static and dynamic replicator drivers
  coresight: funnel: Clean up device book keeping
  coresight: replicator: Cleanup device tracking
  coresight: tmc: Clean up device specific data
  coresight: catu: Cleanup device specific data
  coresight: tpiu: Clean up device specific data
  coresight: stm: Cleanup device specific data
  coresight: etm: Clean up device specific data
  coresight: etb10: Clean up device specific data
  coresight: Rename of_coresight to coresight-platform
  coresight: etm3x: Rearrange cp14 access detection
  coresight: stm: Rearrange probing the stimulus area
  coresight: tmc-etr: Rearrange probing default buffer size
  coresight: Introduce generic platform data helper
  coresight: Make device to CPU mapping generic
  coresight: platform: Use fwnode handle for device search
  coresight: Use fwnode handle instead of device names
  coresight: Use platform agnostic names
  coresight: stm: ACPI support for parsing stimulus base
  coresight: Support for ACPI bindings
  coresight: acpi: Support for components

 drivers/acpi/acpi_amba.c                           |   9 +
 drivers/hwtracing/coresight/Kconfig                |   8 -
 drivers/hwtracing/coresight/Makefile               |   4 +-
 drivers/hwtracing/coresight/coresight-catu.c       |  38 +-
 drivers/hwtracing/coresight/coresight-catu.h       |   1 -
 drivers/hwtracing/coresight/coresight-cpu-debug.c  |   3 +-
 .../coresight/coresight-dynamic-replicator.c       | 255 --------
 drivers/hwtracing/coresight/coresight-etb10.c      |  23 +-
 drivers/hwtracing/coresight/coresight-etm3x.c      |  23 +-
 drivers/hwtracing/coresight/coresight-etm4x.c      |  19 +-
 drivers/hwtracing/coresight/coresight-funnel.c     |  27 +-
 drivers/hwtracing/coresight/coresight-platform.c   | 723 +++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-priv.h       |   2 +
 drivers/hwtracing/coresight/coresight-replicator.c | 255 ++++++--
 drivers/hwtracing/coresight/coresight-stm.c        | 119 +++-
 drivers/hwtracing/coresight/coresight-tmc-etr.c    |  26 +-
 drivers/hwtracing/coresight/coresight-tmc.c        |  71 +-
 drivers/hwtracing/coresight/coresight-tpiu.c       |  29 +-
 drivers/hwtracing/coresight/coresight.c            |  77 ++-
 drivers/hwtracing/coresight/of_coresight.c         | 297 ---------
 include/linux/coresight.h                          |  43 +-
 21 files changed, 1287 insertions(+), 765 deletions(-)
 delete mode 100644 drivers/hwtracing/coresight/coresight-dynamic-replicator.c
 create mode 100644 drivers/hwtracing/coresight/coresight-platform.c
 delete mode 100644 drivers/hwtracing/coresight/of_coresight.c

ACPI bindings for Juno-r0 (applies on [2] above)

Suzuki K Poulose (1):
  edk2-platform: juno: Update ACPI CoreSight Bindings

 Platform/ARM/JunoPkg/AcpiTables/Dsdt.asl | 241 +++++++++++++++++++++++++++++++
 1 file changed, 241 insertions(+)

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>

-- 
2.7.4

Comments

Suzuki K Poulose March 22, 2019, 10:13 a.m. UTC | #1
On 03/20/2019 06:49 PM, Suzuki K Poulose wrote:
> This series adds the support for CoreSight devices on ACPI based

> platforms. The device connections are encoded as _DSD graph property[0],

> with CoreSight specific extensions to indicate the direction of data

> flow as described in [1]. Components attached to CPUs are listed

> as child devices of the corresponding CPU, removing explicit links

> to the CPU like we do in the DT.

> 

> As a prepartion for the ACPI support, we merge the driver for dynamic

> and non-programmable replicators. We introduce platform independent

> helpers to parse the platform supplied information. Thus we rename

> the platform handling code from:

> 	of_coresight.c  => coresight-platform.c

> 

> The CoreSight driver creates shadow devices that appear on the Coresight

> bus, in addition to the real devices (e.g, AMBA bus devices). The name

> of these devices match the real device. This makes the device name

> a bit cryptic for ACPI platform. So this series also introduces a generic

> platform agnostic device naming scheme for the shadow Coresight devices.

> Towards this we also make changes to the way we lookup devices to resolve

> the connections, as we can't use the names to identify the devices. So,

> we use the "fwnode_handle" of the real device for the device lookups.

> Towards that we clean up the drivers to keep track of the "CoreSight"

> device rather than the "real" device. However, all real operations,

> like DMA allocation, Power management etc. must be performed on

> the real device which is the parent of the shadow device.

> 

> Finally we add the support for parsing the ACPI platform data. The power

> management support is missing in the ACPI (and this is not specific to

> CoreSight). The firmware must ensure that the respective power domains

> are turned on.

> 

> Applies on v5.1-rc1

> 

> Tested on a Juno-r0 board with ACPI bindings patch (Patch 26/25) added on

> top of [2]. You would need to make sure that the debug power domain is

> turned on before the Linux kernel boots. (e.g, connect the DS-5 to the

> Juno board while at UEFI). arm32 code is only compile tested.

> 

> [0] ACPI Device Graphs using _DSD (Not available online yet, approved but

>      awaiting publish and eventually should be linked at).

>      https://uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel-1_1.htm

> [1] https://developer.arm.com/docs/den0067/latest/acpi-for-coresighttm-10-platform-design-document

> [2] https://github.com/tianocore/edk2-platforms.git



The kernel patches are also available here :

git://linux-arm.org/linux-skp.git coresight-acpi/v1

Kind regards
Suzuki
Mathieu Poirier March 26, 2019, 9:54 p.m. UTC | #2
On Wed, Mar 20, 2019 at 06:49:27PM +0000, Suzuki K Poulose wrote:
> Switch to using the coresight device instead of the parent

> amba device.

> 

> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>

> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

> ---

>  drivers/hwtracing/coresight/coresight-tpiu.c | 12 ++++++------

>  1 file changed, 6 insertions(+), 6 deletions(-)

> 

> diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c

> index b2f72a1..9763721 100644

> --- a/drivers/hwtracing/coresight/coresight-tpiu.c

> +++ b/drivers/hwtracing/coresight/coresight-tpiu.c

> @@ -48,15 +48,13 @@

>  

>  /**

>   * @base:	memory mapped base address for this component.

> - * @dev:	the device entity associated to this component.

> + * @dev:	the coresight device entity associated to this component.

>   * @atclk:	optional clock for the core parts of the TPIU.

> - * @csdev:	component vitals needed by the framework.

>   */

>  struct tpiu_drvdata {

>  	void __iomem		*base;

>  	struct device		*dev;

>  	struct clk		*atclk;

> -	struct coresight_device	*csdev;


Here again I would get rid of @dev rather than @csdev to be consistent with what
was done previously.

Mathieu

>  };

>  

>  static void tpiu_enable_hw(struct tpiu_drvdata *drvdata)

> @@ -121,6 +119,7 @@ static int tpiu_probe(struct amba_device *adev, const struct amba_id *id)

>  	struct tpiu_drvdata *drvdata;

>  	struct resource *res = &adev->res;

>  	struct coresight_desc desc = { 0 };

> +	struct coresight_device *csdev;

>  	struct device_node *np = adev->dev.of_node;

>  

>  	if (np) {

> @@ -134,7 +133,6 @@ static int tpiu_probe(struct amba_device *adev, const struct amba_id *id)

>  	if (!drvdata)

>  		return -ENOMEM;

>  

> -	drvdata->dev = &adev->dev;

>  	drvdata->atclk = devm_clk_get(&adev->dev, "atclk"); /* optional */

>  	if (!IS_ERR(drvdata->atclk)) {

>  		ret = clk_prepare_enable(drvdata->atclk);

> @@ -160,9 +158,11 @@ static int tpiu_probe(struct amba_device *adev, const struct amba_id *id)

>  	desc.ops = &tpiu_cs_ops;

>  	desc.pdata = pdata;

>  	desc.dev = dev;

> -	drvdata->csdev = coresight_register(&desc);

> +	csdev = coresight_register(&desc);

> +	if (!IS_ERR(csdev))

> +		drvdata->dev = &csdev->dev;

>  

> -	return PTR_ERR_OR_ZERO(drvdata->csdev);

> +	return PTR_ERR_OR_ZERO(csdev);

>  }

>  

>  #ifdef CONFIG_PM

> -- 

> 2.7.4

>
Suzuki K Poulose March 27, 2019, 11:45 a.m. UTC | #3
On 03/26/2019 09:53 PM, Mathieu Poirier wrote:
> 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)


Ah! Sorry about that. Will fix it

Suzuki
Suzuki K Poulose March 27, 2019, 11:52 a.m. UTC | #4
On 03/26/2019 09:54 PM, Mathieu Poirier wrote:
> On Wed, Mar 20, 2019 at 06:49:27PM +0000, Suzuki K Poulose wrote:

>> Switch to using the coresight device instead of the parent

>> amba device.

>>

>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>

>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

>> ---

>>   drivers/hwtracing/coresight/coresight-tpiu.c | 12 ++++++------

>>   1 file changed, 6 insertions(+), 6 deletions(-)

>>

>> diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c

>> index b2f72a1..9763721 100644

>> --- a/drivers/hwtracing/coresight/coresight-tpiu.c

>> +++ b/drivers/hwtracing/coresight/coresight-tpiu.c

>> @@ -48,15 +48,13 @@

>>   

>>   /**

>>    * @base:	memory mapped base address for this component.

>> - * @dev:	the device entity associated to this component.

>> + * @dev:	the coresight device entity associated to this component.

>>    * @atclk:	optional clock for the core parts of the TPIU.

>> - * @csdev:	component vitals needed by the framework.

>>    */

>>   struct tpiu_drvdata {

>>   	void __iomem		*base;

>>   	struct device		*dev;

>>   	struct clk		*atclk;

>> -	struct coresight_device	*csdev;

> 

> Here again I would get rid of @dev rather than @csdev to be consistent with what

> was done previously.


Sure, will do.

Cheers
Suzuki
Mathieu Poirier March 27, 2019, 9:39 p.m. UTC | #5
On Wed, Mar 20, 2019 at 06:49:30PM +0000, Suzuki K Poulose wrote:
> Track the coresight device instead of the real device.

> 

> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>

> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

> ---

>  drivers/hwtracing/coresight/coresight-etb10.c | 6 +++---

>  1 file changed, 3 insertions(+), 3 deletions(-)

> 

> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c

> index 105782e..a471cbd 100644

> --- a/drivers/hwtracing/coresight/coresight-etb10.c

> +++ b/drivers/hwtracing/coresight/coresight-etb10.c

> @@ -97,12 +97,12 @@ static unsigned int etb_get_buffer_depth(struct etb_drvdata *drvdata)

>  {

>  	u32 depth = 0;

>  

> -	pm_runtime_get_sync(drvdata->dev);

> +	pm_runtime_get_sync(drvdata->dev->parent);

>  

>  	/* RO registers don't need locking */

>  	depth = readl_relaxed(drvdata->base + ETB_RAM_DEPTH_REG);

>  

> -	pm_runtime_put(drvdata->dev);

> +	pm_runtime_put(drvdata->dev->parent);

>  	return depth;

>  }

>  

> @@ -701,7 +701,6 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id)

>  	if (!drvdata)

>  		return -ENOMEM;

>  

> -	drvdata->dev = &adev->dev;

>  	drvdata->atclk = devm_clk_get(&adev->dev, "atclk"); /* optional */

>  	if (!IS_ERR(drvdata->atclk)) {

>  		ret = clk_prepare_enable(drvdata->atclk);

> @@ -740,6 +739,7 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id)

>  	if (IS_ERR(drvdata->csdev))

>  		return PTR_ERR(drvdata->csdev);

>  

> +	drvdata->dev = &drvdata->csdev->dev;


For patch 11, 12, 13 - same comment as earlier.  Let's use drvdata::csdev
and get rid of drvdata::dev.

>  	drvdata->miscdev.name = pdata->name;

>  	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;

>  	drvdata->miscdev.fops = &etb_fops;

> -- 

> 2.7.4

>
Mathieu Poirier March 27, 2019, 10:05 p.m. UTC | #6
On Wed, Mar 20, 2019 at 06:49:32PM +0000, Suzuki K Poulose wrote:
> As we are about to about refactor the platform specific handling,


s/about//

> move the DT property handling to generic helpers.

> 

> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>

> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

> ---

>  drivers/hwtracing/coresight/coresight-etm3x.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/drivers/hwtracing/coresight/coresight-etm3x.c b/drivers/hwtracing/coresight/coresight-etm3x.c

> index 1b9ae3a..7137f06 100644

> --- a/drivers/hwtracing/coresight/coresight-etm3x.c

> +++ b/drivers/hwtracing/coresight/coresight-etm3x.c

> @@ -800,9 +800,9 @@ static int etm_probe(struct amba_device *adev, const struct amba_id *id)

>  			return PTR_ERR(pdata);

>  

>  		adev->dev.platform_data = pdata;

> -		drvdata->use_cp14 = of_property_read_bool(np, "arm,cp14");

>  	}

>  

> +	drvdata->use_cp14 = fwnode_property_read_bool(dev->fwnode, "arm,cp14");

>  	dev_set_drvdata(dev, drvdata);

>  

>  	/* Validity for the resource is already checked by the AMBA core */

> -- 

> 2.7.4

>
Suzuki K Poulose March 28, 2019, 10:59 a.m. UTC | #7
On 03/27/2019 10:57 PM, Mathieu Poirier wrote:
> On Wed, Mar 20, 2019 at 06:49:35PM +0000, Suzuki K Poulose wrote:

>> So far we have hard coded the DT platform parsing code in

>> every driver. Introduce generic helper to parse the information

>> provided by the firmware in a platform agnostic manner, in preparation

>> for the ACPI support.

>>

>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>

>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>



>>   

>> +static int coresight_alloc_conns(struct device *dev,

>> +				 struct coresight_platform_data *pdata)

>> +{

>> +	if (pdata->nr_outport) {

>> +		pdata->conns = devm_kzalloc(dev, pdata->nr_outport *

>> +					    sizeof(*pdata->conns),

>> +					    GFP_KERNEL);

>> +		if (!pdata->conns)

>> +			return -ENOMEM;

>> +	}

>> +

>> +	return 0;

>> +}

>> +


>> -	ret = of_coresight_alloc_memory(dev, pdata);

>> +	ret = coresight_alloc_conns(dev, pdata);

> 

> I'm pretty sure you're doing this because you want to use

> coresight_alloc_conns() for ACPI as well, and I'm fine with that.  But it is

> quite orthogonal to the rest of the work done in this patch and as such I think

> it needs a patch of its own.

> 


Sure, will split this into a separate patch.

Suzuki
Suzuki K Poulose March 28, 2019, 11:03 a.m. UTC | #8
On 03/27/2019 10:05 PM, Mathieu Poirier wrote:
> On Wed, Mar 20, 2019 at 06:49:32PM +0000, Suzuki K Poulose wrote:

>> As we are about to about refactor the platform specific handling,

> 

> s/about//

> 

>> move the DT property handling to generic helpers.

>>


Thanks for spotting. Will fix it.

Suzuki
Suzuki K Poulose March 28, 2019, 11:09 a.m. UTC | #9
On 03/27/2019 09:39 PM, Mathieu Poirier wrote:
> On Wed, Mar 20, 2019 at 06:49:30PM +0000, Suzuki K Poulose wrote:

>> Track the coresight device instead of the real device.

>>

>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>

>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>


>>   

>> @@ -701,7 +701,6 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id)

>>   	if (!drvdata)

>>   		return -ENOMEM;

>>   

>> -	drvdata->dev = &adev->dev;

>>   	drvdata->atclk = devm_clk_get(&adev->dev, "atclk"); /* optional */

>>   	if (!IS_ERR(drvdata->atclk)) {

>>   		ret = clk_prepare_enable(drvdata->atclk);

>> @@ -740,6 +739,7 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id)

>>   	if (IS_ERR(drvdata->csdev))

>>   		return PTR_ERR(drvdata->csdev);

>>   

>> +	drvdata->dev = &drvdata->csdev->dev;

> 

> For patch 11, 12, 13 - same comment as earlier.  Let's use drvdata::csdev

> and get rid of drvdata::dev.

> 


Sure, will do.

Thanks for the detailed review !

Suzuki
Suzuki K Poulose March 28, 2019, 6:42 p.m. UTC | #10
On 03/28/2019 05:42 PM, Mathieu Poirier wrote:
> On Wed, Mar 20, 2019 at 06:49:38PM +0000, Suzuki K Poulose wrote:

>> We rely on the device names to find a CoreSight device on the

>> coresight bus. The device name however is obtained from the platform,

>> which is bound to the real platform/amba device. As we are about

>> to use different naming scheme for the coresight devices, we can't

>> rely on the platform device name to find the corresponding

>> coresight device. Instead we use the platform agnostic

>> "fwnode handle" of the parent device to find the devices.

>> We also reuse the same fwnode as the parent for the Coresight

>> device we create.

>>

>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>

>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>


Agreed to all comments. Will address them in the next revision.

Cheers
Suzuki
Mathieu Poirier March 28, 2019, 7:43 p.m. UTC | #11
On Wed, Mar 20, 2019 at 06:49:39PM +0000, 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

> 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@linaro.org>

> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

> ---

>  drivers/hwtracing/coresight/coresight-catu.c       |  7 +++

>  drivers/hwtracing/coresight/coresight-etb10.c      |  6 +++

>  drivers/hwtracing/coresight/coresight-etm3x.c      |  5 ++

>  drivers/hwtracing/coresight/coresight-etm4x.c      |  4 ++

>  drivers/hwtracing/coresight/coresight-funnel.c     |  6 +++

>  drivers/hwtracing/coresight/coresight-replicator.c |  7 +++

>  drivers/hwtracing/coresight/coresight-stm.c        |  6 +++

>  drivers/hwtracing/coresight/coresight-tmc.c        | 14 ++++++

>  drivers/hwtracing/coresight/coresight-tpiu.c       |  6 +++

>  drivers/hwtracing/coresight/coresight.c            | 58 ++++++++++++++++++++++

>  include/linux/coresight.h                          | 25 +++++++++-

>  11 files changed, 143 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c

> index 4595c67..6492bce 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;

> @@ -510,6 +512,11 @@ static int catu_probe(struct amba_device *adev, const struct amba_id *id)

>  		ret = PTR_ERR(pdata);

>  		goto out;

>  	}

> +

> +	pdata->name = coresight_alloc_device_name(&catu_devs, dev);

> +	if (!pdata->name)

> +		return -ENOMEM;

> +

>  	dev->platform_data = pdata;

>  

>  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);

> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c

> index e4175849..42b525c 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(etb10_devs, "etb10_");


I would drop the "10_" and just call it "etb".  Below we have "tmc_etb" so there
is no chance of mistake.

> +

>  /**

>   * struct etb_drvdata - specifics associated to an ETB component

>   * @base:	memory mapped base address for this component.

> @@ -692,6 +694,10 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id)

>  	pdata = coresight_get_platform_data(dev);

>  	if (IS_ERR(pdata))

>  		return PTR_ERR(pdata);

> +	pdata->name = coresight_alloc_device_name(&etb10_devs, dev);

> +	if (!pdata->name)

> +		return -ENOMEM;

> +

>  	adev->dev.platform_data = pdata;

>  

>  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);

> diff --git a/drivers/hwtracing/coresight/coresight-etm3x.c b/drivers/hwtracing/coresight/coresight-etm3x.c

> index b101464..35ed953 100644

> --- a/drivers/hwtracing/coresight/coresight-etm3x.c

> +++ b/drivers/hwtracing/coresight/coresight-etm3x.c

> @@ -797,6 +797,11 @@ static int etm_probe(struct amba_device *adev, const struct amba_id *id)

>  	if (IS_ERR(pdata))

>  		return PTR_ERR(pdata);

>  

> +	pdata->name  = devm_kasprintf(dev, GFP_KERNEL,

> +				      "etm%d", pdata->cpu);

> +	if (!pdata->name)

> +		return -ENOMEM;

> +

>  	adev->dev.platform_data = pdata;

>  	drvdata->use_cp14 = fwnode_property_read_bool(dev->fwnode, "arm,cp14");

>  	dev_set_drvdata(dev, drvdata);

> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c

> index bfc23ab..55fcb78 100644

> --- a/drivers/hwtracing/coresight/coresight-etm4x.c

> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c

> @@ -982,6 +982,10 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)

>  	pdata = coresight_get_platform_data(dev);

>  	if (IS_ERR(pdata))

>  		return PTR_ERR(pdata);

> +	pdata->name = devm_kasprintf(dev, GFP_KERNEL, "etm%d", pdata->cpu);

> +	if (!pdata->name)

> +		return -ENOMEM;

> +

>  	adev->dev.platform_data = pdata;

>  

>  	dev_set_drvdata(dev, drvdata);

> diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c

> index 2590744..96b6773 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.

> @@ -189,6 +191,10 @@ static int funnel_probe(struct amba_device *adev, const struct amba_id *id)

>  	pdata = coresight_get_platform_data(dev);

>  	if (IS_ERR(pdata))

>  		return PTR_ERR(pdata);

> +	pdata->name = coresight_alloc_device_name(&funnel_devs, dev);

> +	if (!pdata->name)

> +		return -ENOMEM;

> +

>  	adev->dev.platform_data = pdata;

>  

>  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);

> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c

> index 7eb3bf7..7b937c0 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

> @@ -184,6 +186,11 @@ static int replicator_probe(struct device *dev, struct resource *res)

>  		return PTR_ERR(pdata);

>  	dev->platform_data = pdata;

>  

> +	pdata->name = coresight_alloc_device_name(&replicator_devs, dev);

> +


Please remove the extra new line here.

> +	if (!pdata->name)

> +		return -ENOMEM;

> +

>  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);

>  	if (!drvdata)

>  		return -ENOMEM;

> diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c

> index 6514586..d94ae22 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.

> @@ -813,6 +815,10 @@ static int stm_probe(struct amba_device *adev, const struct amba_id *id)

>  	pdata = coresight_get_platform_data(dev);

>  	if (IS_ERR(pdata))

>  		return PTR_ERR(pdata);

> +	pdata->name = coresight_alloc_device_name(&stm_devs, dev);

> +	if (!pdata->name)

> +		return -ENOMEM;

> +

>  	adev->dev.platform_data = pdata;

>  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);

>  	if (!drvdata)

> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c

> index 147ab17..030303d 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 */

> @@ -394,6 +398,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;

>  

>  	pdata = coresight_get_platform_data(dev);

>  	if (IS_ERR(pdata)) {

> @@ -440,6 +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_etb_cs_ops;

> +		dev_list = &etb_devs;

>  		break;

>  	case TMC_CONFIG_TYPE_ETR:

>  		desc.type = CORESIGHT_DEV_TYPE_SINK;

> @@ -449,11 +455,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", pdata->name);

> @@ -461,6 +469,12 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)

>  		goto out;

>  	}

>  

> +	pdata->name = coresight_alloc_device_name(dev_list, dev);

> +	if (!pdata->name) {

> +		ret = -ENOMEM;

> +		goto out;

> +	}

> +

>  	drvdata->csdev = coresight_register(&desc);

>  	if (IS_ERR(drvdata->csdev)) {

>  		ret = PTR_ERR(drvdata->csdev);

> diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c

> index 18a749a..f5cc457 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 coresight device entity associated to this component.

> @@ -124,6 +126,10 @@ static int tpiu_probe(struct amba_device *adev, const struct amba_id *id)

>  	pdata = coresight_get_platform_data(dev);

>  	if (IS_ERR(pdata))

>  		return PTR_ERR(pdata);

> +	pdata->name = coresight_alloc_device_name(&tpiu_devs, dev);

> +	if (!pdata->name)

> +		return -ENOMEM;

> +

>  	adev->dev.platform_data = pdata;

>  

>  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);

> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c

> index 9cdedab..ca40c55 100644

> --- a/drivers/hwtracing/coresight/coresight.c

> +++ b/drivers/hwtracing/coresight/coresight.c

> @@ -1253,3 +1253,61 @@ void coresight_unregister(struct coresight_device *csdev)

>  	device_unregister(&csdev->dev);

>  }

>  EXPORT_SYMBOL_GPL(coresight_unregister);

> +

> +


Extra line

> +/*

> + * 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;


This is not needed.

> +			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 76c31b2..9d933bb 100644

> --- a/include/linux/coresight.h

> +++ b/include/linux/coresight.h

> @@ -174,6 +174,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,	\

> +}

> +


 I like how you did this.

>  #define to_coresight_device(d) container_of(d, struct coresight_device, dev)

>  

>  #define source_ops(csdev)	csdev->ops->source_ops

> @@ -266,7 +288,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

>
Suzuki K Poulose April 4, 2019, 11:27 a.m. UTC | #12
Hi Mathieu,

On 28/03/2019 20:41, Mathieu Poirier wrote:
> On Wed, Mar 20, 2019 at 06:49:40PM +0000, Suzuki K Poulose wrote:

>> The stimulus base for STM device must be listed as the second memory

>> resource, followed by the programming base address. Add support for

>> parsing the information for ACPI.

>>

>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>

>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>

>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

>> ---


>> +#ifdef CONFIG_ACPI

>> +static int acpi_stm_get_stimulus_area(struct device *dev, struct resource *res)

>> +{

>> +	int rc;

>> +	bool found_base = false;

>> +	struct resource_entry *rent;

>> +	LIST_HEAD(res_list);

>> +

>> +	struct acpi_device *adev = ACPI_COMPANION(dev);

>> +

>> +	if (!adev)

>> +		return -ENODEV;

>> +	rc = acpi_dev_get_resources(adev, &res_list, NULL, NULL);

>> +	if (rc < 0)

>> +		return rc;

>> +

>> +	rc = -ENOENT;

>> +	list_for_each_entry(rent, &res_list, node) {

>> +		if (resource_type(rent->res) != IORESOURCE_MEM)

>> +			continue;

>> +		if (found_base) {

>> +			*res = *rent->res;

>> +			rc = 0;

>> +			break;

>> +		}

>> +

>> +		found_base = true;

> 

> Is the ACPI binding crystal clear on the fact that the second resource region

> has to be for stimulus ports?


Yes. Section 2.3 Resources in ACPI for CoreSightTM 1.0 (DEN0067) :

"Each CoresSight component needs to declare the resources it owns using the _CRS
method. This must include base address and span covering the MMIO interface of
the device. In addition those that can raise interrupts must describe the
interrupts they consume.

For STM two base addresses must be presented, these must be provided in order.
First the configuration base address, and then external stimuli memory region
base address"

>>   static int stm_get_stimulus_area(struct device *dev, struct resource *res)

>>   {

>>   	if (dev->of_node)

> 

> Wouldn't it be better to use is_of_node()?



> 

>>   		return of_stm_get_stimulus_area(dev, res);

>> +	else if (is_acpi_node(dev->fwnode)

> 

> is_acpi_device_node()?

> 


Yes, to both the above.

Cheers
Suzuki