[lkml]   [2017]   [Nov]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v4 5/9] drivers: base: cacheinfo: arm64: Add support for ACPI based firmware tables

BTW: Thanks for looking at this!

On 11/20/2017 12:14 PM, Sudeep Holla wrote:
> On 20/11/17 18:02, Jeremy Linton wrote:
>> On 11/20/2017 10:56 AM, Sudeep Holla wrote:
>> (trimming)
>>>> * case there's no explicit cache node or the cache node
>>>> itself in the * device tree + * @firmware_node: Shared with
>>>> of_node. When not using DT, this may contain + * pointers to
>>>> other firmware based values. Particularly ACPI/PPTT + * unique
>>>> values. * @disable_sysfs: indicates whether this node is visible
>>>> to the user via * sysfs or not * @priv: pointer to any private
>>>> data structure specific to particular @@ -64,8 +67,10 @@ struct
>>>> cacheinfo { #define CACHE_ALLOCATE_POLICY_MASK \
>>>> BIT(4) - - struct device_node *of_node; + union { +
>>>> struct device_node *of_node; + void *firmware_node; +
>>>> };
>>> I would prefer struct device_node *of_node; changed to struct
>>> fwnode_handle *fwnode;
>>> You can then have struct pptt_fwnode { <.....> /*below fwnode
>>> allocated using acpi_alloc_fwnode_static */ struct fwnode_handle
>>> *fwnode; };
>>> This gives a good starting point to abstract DT and ACPI.
>>> If not now, we can later implement fwnode.ops=pptt_cache_ops and
>>> then use get property for both DT and ACPI.
>> I'm obviously confused why this keeps coming up. On the surface it
>> sounds like a good idea. But then, given that I've actually
>> implemented a portion of it, what becomes clear is that the PPTT
>> isn't a good match.
> Fair enough.
>> Converting the OF routines to use the fwnode is fairly
>> straightforward, but that doesn't help the ACPI situation other than
>> to create a lot of misleading code (and the possibility of creating
>> nonstandard DSDT entries). The fact that this hasn't been done for
>> other tables MADT/SLIT/SRAT/etc makes me wonder why we should do it
>> for the PPTT?
> IRQ/IORT does use it. If we don't want to use it fine. But the union
> doesn't make sense and breaks the flow many other subsystems follow.
> Hence I raised. Sorry, I hadn't followed the last revision/discussion on
> this, my bad. But I had this thought since the beginning, hence I
> brought this up.
>> Particularly, when one considers fwnode is more a DSDT<->DT
>> abstraction and thus has a lot of API surface that simply doesn't
>> make any sense given the PPTT binary tree structure. Given that most
>> of the fwnode routines are translating string properties (for
>> example fwnode_property_read_string()) it might be possible to build
>> a translator of some form which takes DT style properties and
>> attempts to map them to the ACPI PPTT tree. What this adds I can't
>> fathom, beyond the fact that suddenly the fwnode interface is a
>> partial/brittle implementation where a large subset of the
>> fwnode_operations will tend to be degenerate cases. The result likely
>> will be a poorly implemented translator which breaks or is
>> meaningless over a large part of the fwnode API surface.
> Sure, I just mentioned ops thing, but that's optional. I just didn't
> like the union which has of_node and void ptr instead of fwhandle. I am
> fine if many agree that it's bad idea to use fwhandle here.

So, if we say the union is bad, as is a common fwnode_handle, shall I
just make the "firmware_node" (pptt_node?) standalone? That adds a if
(acpi) check in cache_leaves_are_shared() which is the only place that
the cache topology code does anything with the ACPI field.

Also, if you missed it there is a further patch which overrides the
cache type field if everything else on the PPTT node is valid and the
cache type is NONE.;a=log;h=refs/heads/pptt_v4

finally, I will split out the of_node/fw_node, and move the #ifdef ACPI
somewhere else.

 \ /
  Last update: 2017-11-20 23:41    [W:0.067 / U:1.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site