Re: [PATCH v4 5/9] drivers: base: cacheinfo: arm64: Add support for ACPI based firmware tables

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

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    \
(CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE) #define CACHE_ID
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.

http://linux-arm.org/git?p=linux-jlinton.git;a=log;h=refs/heads/pptt_v4

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

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux