Date:   Thu, 25 Apr 2019 18:30:04 +0100
From:   Suzuki K Poulose <suzuki.poulose@....com>
To:     mathieu.poirier@...aro.org
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 32/36] coresight: Support for ACPI bindings



On 25/04/2019 17:50, Mathieu Poirier wrote:
> On Mon, Apr 15, 2019 at 05:04:15PM +0100, Suzuki K Poulose wrote:
>> Add support for parsing the ACPI platform description
>> for CoreSight. The connections are encoded in a DSD graph
>> property with CoreSight specific variation of the property.
>>
>> The ETMs are listed as the children device of the respective
>> CPU.
>>
>> Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>
>> Cc: Mathieu Poirier <mathieu.poirier@...aro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>

...

>> +/*
>> + * acpi_validate_dsd_graph	- Make sure the given _DSD graph conforms
>> + * to the ACPI _DSD Graph specification.
>> + *
>> + * ACPI Devices Graph property has the following format:
>> + *	Revision	- Integer, must be 0
>> + *	NumberOfGraphs	- Integer, N indicating the following list.
>> + *	Graph[1],
>> + *	 ...
>> + *	Graph[N]
>> + *
>> + * And each Graph entry has the following format:
>> + *	GraphID		- Integer, identifying a graph the device belongs to.
>> + *	UUID		- UUID identifying the specification that governs
>> + *			  this graph. (e.g, see is_acpi_coresight_graph())
>> + *	NumberOfLinks	- Number "N" of connections on this node of the graph.
>> + *	Links[1]
>> + *	...
>> + *	Links[N]
>> + */
>> +static inline bool acpi_validate_dsd_graph(const union acpi_object *graph)
>> +{
>> +	int i, n;
>> +	const union acpi_object *rev, *nr_graphs;
>> +
>> +	/* The graph must contain at least the Revision and Number of Graphs */
>> +	if (graph->package.count < 2)
>> +		return false;
>> +
>> +	rev = &graph->package.elements[0];
>> +	nr_graphs = &graph->package.elements[1];
>> +
>> +	if (rev->type != ACPI_TYPE_INTEGER ||
>> +	    nr_graphs->type != ACPI_TYPE_INTEGER)
>> +		return false;
>> +
>> +	/* We only support revision 0 */
>> +	if (rev->integer.value != 0)
>> +		return false;
>> +
>> +	n = nr_graphs->integer.value;
>> +	/* Make sure the package has right number of elements */
>> +	if (graph->package.count != (n + 2))
>> +		return false;
>> +
>> +	/* Each entry must be a graph package with at least 3 members */
> 
> Please mention what the 3 members must be to avoid having to go back to the
> specs all the time.

Actually this is explained in the comment above. Graph entry contains :
GraphID, UUID and number of links. I could add make it explicit here,
just like I did it for the "Revision and Number of Graphs" above.

> 
>> +	for (i = 2; i < n + 2; i++) {
>> +		const union acpi_object *obj = &graph->package.elements[i];
>> +
>> +		if (obj->type != ACPI_TYPE_PACKAGE ||
>> +		    obj->package.count < 3)
>> +			return false;
>> +	}
>> +
>> +	return true;
>> +}


>> +
>> +/* acpi_get_dsd_graph	- Find the _DSD Graph property for the given device. */
>> +const union acpi_object *
>> +acpi_get_dsd_graph(struct acpi_device *adev)
>> +{
>> +	int i;
>> +	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
>> +	acpi_status status;
>> +	const union acpi_object *dsd;
>> +
>> +	status = acpi_evaluate_object_typed(adev->handle, "_DSD", NULL,
>> +					    &buf, ACPI_TYPE_PACKAGE);
>> +	if (ACPI_FAILURE(status))
>> +		return NULL;
>> +
>> +	dsd = buf.pointer;
> 
> Please add a comment to mention that a _DSD package consists of tuples, i.e a
> UUID and a package to explain the += 2.
> 

Sure, it took me a while to figure that out myself.

>> +static inline bool
>> +acpi_validate_coresight_graph(const union acpi_object *cs_graph)
>> +{
>> +	int nlinks;
>> +
>> +	nlinks = cs_graph->package.elements[2].integer.value;
> 
> As you did in acpi_validate_dsd_graph(), please add the rational for the + 3.

Sure.

>> +	if (cs_graph->package.count != (nlinks + 3))
>> +		return false;
>> +	/* The links are validated in acpi_coresight_parse_link() */
>> +	return true;
>> +}
>> +
>> +/*
>> + * acpi_get_coresight_graph	- Parse the device _DSD tables and find
>> + * the Graph property matching the CoreSight Graphs.
>> + *
>> + * Returns the pointer to the CoreSight Graph Package when found. Otherwise
>> + * returns NULL.
>> + */
>> +const union acpi_object *
>> +acpi_get_coresight_graph(struct acpi_device *adev)
>> +{
>> +	const union acpi_object *graph_list, *graph;
>> +	int i, nr_graphs;
>> +
>> +	graph_list = acpi_get_dsd_graph(adev);
>> +	if (!graph_list)
>> +		return graph_list;
>> +
>> +	nr_graphs = graph_list->package.elements[1].integer.value;
> 
> In what kind of scenario nr_graphs would be more than 1?  From what I've seen in
> DEN0067 and the implementation in patch 37 it shouldn't be the case.  As such I
> would treat nr_graphs != 1 as an error.

The device could be part of multiple graphs theoretically. However, for
CoreSight we have only one graph, which shows the ATB connections. I could
enforce that here.


> 
> I must admit I had a really hard time following the hard coded .element[]
> throughout the code.  With time I was able to verify them all but it would
> really help if the example of an ACPI device description was given at the
> top of the file, just before the ACPI Graph UUID.  The following taken from
> DEN0067 did the trick for me:

I understand the pain. It is indeed a bit tricky. I will add an example graph
property.

> 
> Device ( FUN1 ) { // Funnel 1 described in \SB scope
>    Name (_HID , " ARMHC9FF ")
>    Name (_CID , " ARMHC500 ")
>    Name (_CRS , ResourceTemplate () {
>    Memory32Fixed ( ReadWrite , 0 x208c0000 , 0 x1000 )
>    })
> 
>    Name (_DSD , Package () {
>      ToUUID (" ab02a46b -74 c7 -45a2 -bd68 - f7d344ef2153 ") ,
>      Package () {
>        0 , // Revision
>        1 , // Number of graphs
>        Package () {
>          1 , // GraphID // CoreSight graphs UUID
>          ToUUID ("3 ecbc8b6 -1 d0e -4 fb3 -8107 - e627f805c6cd ") ,
>          3 , // Number of links
>          Package () {0 , 0 , // output port 0 connected
>               \_SB.RPL0 ,1} , // to input port 0 on RPL0 .
>          Package () {0 , 0 , // input port 0 connected
>               \_SB.ETF0 ,0} , // to output port 0 on ETF0 .
>          Package () {1 , 0 , // input port 1 connected
>               \_SB.ETF1 ,0} // to output port 0 on ETF1 .
>          }
>      }
>    })
> 

...

>> +/*
>> + * acpi_coresigh_get_cpu - Find the logical CPU id of the CPU associated
>> + * with this coresight device. With ACPI bindings, the CoreSight components
>> + * are listed as child device of the associated CPU.
>> + *
>> + * Returns the logical CPU id when found. Otherwise returns < 0.
> 
> As far as I can tell this function can't return < 0.
> 

You're right. I will fix the comment.

>> +static struct coresight_platform_data *
>> +acpi_get_coresight_platform_data(struct device *dev,
>> +				 struct coresight_platform_data *pdata)
>> +{
>> +	int rc = -EINVAL;
>> +	struct acpi_device *adev;
>> +
>> +	adev = ACPI_COMPANION(dev);
>> +	if (!adev)
>> +		goto out;
>> +
>> +	rc = acpi_coresight_parse_graph(adev, pdata);
>> +	if (rc)
>> +		goto out;
> 
> The goto statement is not needed here.
> 

OK

>> +out:
>> +	if (rc)
>> +		return ERR_PTR(rc);
>> +	return pdata;
>> +}
>> +
>> +#else
>> +
>> +static inline struct coresight_platform_data *
>> +acpi_get_coresight_platform_data(struct device *dev,
>> +				 struct coresight_platform_data *pdata)
>> +{
>> +	return NULL;
>> +}
>> +
>> +static inline int acpi_coresight_get_cpu(struct device *dev)
>> +{
>> +	return 0;
>> +}
>> +#endif
>> +
> 
> Function of_coresight_get_cpu() and of_get_coresight_platform_data() will also
> need a stub if CONFIG_OF=n.
>

Yep, agreed.

Thank you so much for the review !
Suzuki

Powered by blists - more mailing lists