lkml.org 
[lkml]   [2019]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 32/36] coresight: Support for ACPI bindings
From
Date


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

\
 
 \ /
  Last update: 2019-04-25 19:31    [W:1.583 / U:0.708 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site