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@rjwysocki.net Cc: Mathieu Poirier mathieu.poirier@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.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