| Message ID | 20171201222330.18863-5-jeremy.linton@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday, December 1, 2017 11:23:25 PM CET Jeremy Linton wrote: > Add a entry to to struct cacheinfo to maintain a reference to the PPTT > node which can be used to match identical caches across cores. Also > stub out cache_setup_acpi() so that individual architectures can > enable ACPI topology parsing. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > drivers/acpi/pptt.c | 1 + > drivers/base/cacheinfo.c | 20 ++++++++++++++------ > include/linux/cacheinfo.h | 13 ++++++++++++- > 3 files changed, 27 insertions(+), 7 deletions(-) > > diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c > index 0f8a1631af33..a35e457cefb7 100644 > --- a/drivers/acpi/pptt.c > +++ b/drivers/acpi/pptt.c > @@ -329,6 +329,7 @@ static void update_cache_properties(struct cacheinfo *this_leaf, > { > int valid_flags = 0; > > + this_leaf->firmware_node = cpu_node; > if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) { > this_leaf->size = found_cache->size; > valid_flags++; > diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c > index eb3af2739537..ba89f9310e6f 100644 > --- a/drivers/base/cacheinfo.c > +++ b/drivers/base/cacheinfo.c > @@ -86,7 +86,10 @@ static int cache_setup_of_node(unsigned int cpu) > static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf, > struct cacheinfo *sib_leaf) > { > - return sib_leaf->of_node == this_leaf->of_node; > + if (acpi_disabled) > + return sib_leaf->of_node == this_leaf->of_node; > + else > + return sib_leaf->firmware_node == this_leaf->firmware_node; > } > > /* OF properties to query for a given cache type */ > @@ -215,6 +218,11 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf, > } > #endif > > +int __weak cache_setup_acpi(unsigned int cpu) > +{ > + return -ENOTSUPP; > +} > + > static int cache_shared_cpu_map_setup(unsigned int cpu) > { > struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu); > @@ -225,11 +233,11 @@ static int cache_shared_cpu_map_setup(unsigned int cpu) > if (this_cpu_ci->cpu_map_populated) > return 0; > > - if (of_have_populated_dt()) > + if (!acpi_disabled) > + ret = cache_setup_acpi(cpu); > + else if (of_have_populated_dt()) > ret = cache_setup_of_node(cpu); > - else if (!acpi_disabled) > - /* No cache property/hierarchy support yet in ACPI */ > - ret = -ENOTSUPP; > + > if (ret) > return ret; > > @@ -286,7 +294,7 @@ static void cache_shared_cpu_map_remove(unsigned int cpu) > > static void cache_override_properties(unsigned int cpu) > { > - if (of_have_populated_dt()) > + if (acpi_disabled && of_have_populated_dt()) > return cache_of_override_properties(cpu); > } > > diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h > index 3d9805297cda..7ebff157ae6c 100644 > --- a/include/linux/cacheinfo.h > +++ b/include/linux/cacheinfo.h > @@ -37,6 +37,8 @@ enum cache_type { > * @of_node: if devicetree is used, this represents either the cpu node in > * case there's no explicit cache node or the cache node itself in the > * device tree > + * @firmware_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 > @@ -65,8 +67,8 @@ struct cacheinfo { > #define CACHE_ALLOCATE_POLICY_MASK \ > (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE) > #define CACHE_ID BIT(4) > - > struct device_node *of_node; > + void *firmware_node; What about converting this to using struct fwnode instead of adding fields to it? > bool disable_sysfs; > void *priv; > }; > @@ -99,6 +101,15 @@ int func(unsigned int cpu) \ > struct cpu_cacheinfo *get_cpu_cacheinfo(unsigned int cpu); > int init_cache_level(unsigned int cpu); > int populate_cache_leaves(unsigned int cpu); > +int cache_setup_acpi(unsigned int cpu); > +int acpi_find_last_cache_level(unsigned int cpu); > +#ifndef CONFIG_ACPI > +int acpi_find_last_cache_level(unsigned int cpu) > +{ > + /*ACPI kernels should be built with PPTT support*/ > + return 0; > +} > +#endif > > const struct attribute_group *cache_get_priv_group(struct cacheinfo *this_leaf); > > Thanks, Rafael
Hi, On 12/11/2017 07:11 PM, Rafael J. Wysocki wrote: > On Friday, December 1, 2017 11:23:25 PM CET Jeremy Linton wrote: >> Add a entry to to struct cacheinfo to maintain a reference to the PPTT >> node which can be used to match identical caches across cores. Also >> stub out cache_setup_acpi() so that individual architectures can >> enable ACPI topology parsing. >> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> --- >> drivers/acpi/pptt.c | 1 + >> drivers/base/cacheinfo.c | 20 ++++++++++++++------ >> include/linux/cacheinfo.h | 13 ++++++++++++- >> 3 files changed, 27 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c >> index 0f8a1631af33..a35e457cefb7 100644 >> --- a/drivers/acpi/pptt.c >> +++ b/drivers/acpi/pptt.c >> @@ -329,6 +329,7 @@ static void update_cache_properties(struct cacheinfo *this_leaf, >> { >> int valid_flags = 0; >> >> + this_leaf->firmware_node = cpu_node; >> if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) { >> this_leaf->size = found_cache->size; >> valid_flags++; >> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c >> index eb3af2739537..ba89f9310e6f 100644 >> --- a/drivers/base/cacheinfo.c >> +++ b/drivers/base/cacheinfo.c >> @@ -86,7 +86,10 @@ static int cache_setup_of_node(unsigned int cpu) >> static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf, >> struct cacheinfo *sib_leaf) >> { >> - return sib_leaf->of_node == this_leaf->of_node; >> + if (acpi_disabled) >> + return sib_leaf->of_node == this_leaf->of_node; >> + else >> + return sib_leaf->firmware_node == this_leaf->firmware_node; >> } >> >> /* OF properties to query for a given cache type */ >> @@ -215,6 +218,11 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf, >> } >> #endif >> >> +int __weak cache_setup_acpi(unsigned int cpu) >> +{ >> + return -ENOTSUPP; >> +} >> + >> static int cache_shared_cpu_map_setup(unsigned int cpu) >> { >> struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu); >> @@ -225,11 +233,11 @@ static int cache_shared_cpu_map_setup(unsigned int cpu) >> if (this_cpu_ci->cpu_map_populated) >> return 0; >> >> - if (of_have_populated_dt()) >> + if (!acpi_disabled) >> + ret = cache_setup_acpi(cpu); >> + else if (of_have_populated_dt()) >> ret = cache_setup_of_node(cpu); >> - else if (!acpi_disabled) >> - /* No cache property/hierarchy support yet in ACPI */ >> - ret = -ENOTSUPP; >> + >> if (ret) >> return ret; >> >> @@ -286,7 +294,7 @@ static void cache_shared_cpu_map_remove(unsigned int cpu) >> >> static void cache_override_properties(unsigned int cpu) >> { >> - if (of_have_populated_dt()) >> + if (acpi_disabled && of_have_populated_dt()) >> return cache_of_override_properties(cpu); >> } >> >> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h >> index 3d9805297cda..7ebff157ae6c 100644 >> --- a/include/linux/cacheinfo.h >> +++ b/include/linux/cacheinfo.h >> @@ -37,6 +37,8 @@ enum cache_type { >> * @of_node: if devicetree is used, this represents either the cpu node in >> * case there's no explicit cache node or the cache node itself in the >> * device tree >> + * @firmware_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 >> @@ -65,8 +67,8 @@ struct cacheinfo { >> #define CACHE_ALLOCATE_POLICY_MASK \ >> (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE) >> #define CACHE_ID BIT(4) >> - >> struct device_node *of_node; >> + void *firmware_node; > > What about converting this to using struct fwnode instead of adding > fields to it? I didn't really want to add another field here, but I've also pointed out how I thought converting it to a fwnode wasn't a good choice. https://lkml.org/lkml/2017/11/20/502 Mostly because IMHO its even more misleading (lacking any fwnode_operations) than misusing the of_node as a void *. Given that I'm in the minority thinking this, how far down the fwnode path on the ACPI side do we want to go? Is simply treating it as a void pointer sufficient for the ACPI side, considering all the PPTT code needs is a unique token? > >> bool disable_sysfs; >> void *priv; >> }; >> @@ -99,6 +101,15 @@ int func(unsigned int cpu) \ >> struct cpu_cacheinfo *get_cpu_cacheinfo(unsigned int cpu); >> int init_cache_level(unsigned int cpu); >> int populate_cache_leaves(unsigned int cpu); >> +int cache_setup_acpi(unsigned int cpu); >> +int acpi_find_last_cache_level(unsigned int cpu); >> +#ifndef CONFIG_ACPI >> +int acpi_find_last_cache_level(unsigned int cpu) >> +{ >> + /*ACPI kernels should be built with PPTT support*/ >> + return 0; >> +} >> +#endif >> >> const struct attribute_group *cache_get_priv_group(struct cacheinfo *this_leaf); >> >> > > Thanks, > Rafael >
On Tue, Dec 12, 2017 at 6:03 PM, Jeremy Linton <jeremy.linton@arm.com> wrote: > Hi, > > > On 12/11/2017 07:11 PM, Rafael J. Wysocki wrote: >> >> On Friday, December 1, 2017 11:23:25 PM CET Jeremy Linton wrote: >>> >>> Add a entry to to struct cacheinfo to maintain a reference to the PPTT >>> node which can be used to match identical caches across cores. Also >>> stub out cache_setup_acpi() so that individual architectures can >>> enable ACPI topology parsing. >>> >>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >>> --- >>> drivers/acpi/pptt.c | 1 + >>> drivers/base/cacheinfo.c | 20 ++++++++++++++------ >>> include/linux/cacheinfo.h | 13 ++++++++++++- >>> 3 files changed, 27 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c >>> index 0f8a1631af33..a35e457cefb7 100644 >>> --- a/drivers/acpi/pptt.c >>> +++ b/drivers/acpi/pptt.c >>> @@ -329,6 +329,7 @@ static void update_cache_properties(struct cacheinfo >>> *this_leaf, >>> { >>> int valid_flags = 0; >>> + this_leaf->firmware_node = cpu_node; >>> if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) { >>> this_leaf->size = found_cache->size; >>> valid_flags++; >>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c >>> index eb3af2739537..ba89f9310e6f 100644 >>> --- a/drivers/base/cacheinfo.c >>> +++ b/drivers/base/cacheinfo.c >>> @@ -86,7 +86,10 @@ static int cache_setup_of_node(unsigned int cpu) >>> static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf, >>> struct cacheinfo *sib_leaf) >>> { >>> - return sib_leaf->of_node == this_leaf->of_node; >>> + if (acpi_disabled) >>> + return sib_leaf->of_node == this_leaf->of_node; >>> + else >>> + return sib_leaf->firmware_node == >>> this_leaf->firmware_node; >>> } >>> /* OF properties to query for a given cache type */ >>> @@ -215,6 +218,11 @@ static inline bool cache_leaves_are_shared(struct >>> cacheinfo *this_leaf, >>> } >>> #endif >>> +int __weak cache_setup_acpi(unsigned int cpu) >>> +{ >>> + return -ENOTSUPP; >>> +} >>> + >>> static int cache_shared_cpu_map_setup(unsigned int cpu) >>> { >>> struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu); >>> @@ -225,11 +233,11 @@ static int cache_shared_cpu_map_setup(unsigned int >>> cpu) >>> if (this_cpu_ci->cpu_map_populated) >>> return 0; >>> - if (of_have_populated_dt()) >>> + if (!acpi_disabled) >>> + ret = cache_setup_acpi(cpu); >>> + else if (of_have_populated_dt()) >>> ret = cache_setup_of_node(cpu); >>> - else if (!acpi_disabled) >>> - /* No cache property/hierarchy support yet in ACPI */ >>> - ret = -ENOTSUPP; >>> + >>> if (ret) >>> return ret; >>> @@ -286,7 +294,7 @@ static void cache_shared_cpu_map_remove(unsigned >>> int cpu) >>> static void cache_override_properties(unsigned int cpu) >>> { >>> - if (of_have_populated_dt()) >>> + if (acpi_disabled && of_have_populated_dt()) >>> return cache_of_override_properties(cpu); >>> } >>> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h >>> index 3d9805297cda..7ebff157ae6c 100644 >>> --- a/include/linux/cacheinfo.h >>> +++ b/include/linux/cacheinfo.h >>> @@ -37,6 +37,8 @@ enum cache_type { >>> * @of_node: if devicetree is used, this represents either the cpu node >>> in >>> * case there's no explicit cache node or the cache node itself in >>> the >>> * device tree >>> + * @firmware_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 >>> @@ -65,8 +67,8 @@ struct cacheinfo { >>> #define CACHE_ALLOCATE_POLICY_MASK \ >>> (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE) >>> #define CACHE_ID BIT(4) >>> - >>> struct device_node *of_node; >>> + void *firmware_node; >> >> >> What about converting this to using struct fwnode instead of adding >> fields to it? > > > I didn't really want to add another field here, but I've also pointed out > how I thought converting it to a fwnode wasn't a good choice. > > https://lkml.org/lkml/2017/11/20/502 > > Mostly because IMHO its even more misleading (lacking any fwnode_operations) > than misusing the of_node as a void *. I'm not sure what you mean. Anyway, the idea is to have one pointer in there instead of two that cannot be used at the same time and there's no reason why of_node should be special. of_node should just be one of multiple choices. > Given that I'm in the minority thinking this, how far down the fwnode path > on the ACPI side do we want to go? I have no idea. :-) > Is simply treating it as a void pointer > sufficient for the ACPI side, considering all the PPTT code needs is a > unique token? I guess you can think about it as of_node under a different name, but whether or not this is sufficient depends on what you need it for. Thanks, Rafael
Hi, On 12/12/2017 11:25 AM, Rafael J. Wysocki wrote: > On Tue, Dec 12, 2017 at 6:03 PM, Jeremy Linton <jeremy.linton@arm.com> wrote: >> Hi, >> >> >> On 12/11/2017 07:11 PM, Rafael J. Wysocki wrote: >>> >>> On Friday, December 1, 2017 11:23:25 PM CET Jeremy Linton wrote: >>>> >>>> Add a entry to to struct cacheinfo to maintain a reference to the PPTT >>>> node which can be used to match identical caches across cores. Also >>>> stub out cache_setup_acpi() so that individual architectures can >>>> enable ACPI topology parsing. >>>> >>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >>>> --- >>>> drivers/acpi/pptt.c | 1 + >>>> drivers/base/cacheinfo.c | 20 ++++++++++++++------ >>>> include/linux/cacheinfo.h | 13 ++++++++++++- >>>> 3 files changed, 27 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c >>>> index 0f8a1631af33..a35e457cefb7 100644 >>>> --- a/drivers/acpi/pptt.c >>>> +++ b/drivers/acpi/pptt.c >>>> @@ -329,6 +329,7 @@ static void update_cache_properties(struct cacheinfo >>>> *this_leaf, >>>> { >>>> int valid_flags = 0; >>>> + this_leaf->firmware_node = cpu_node; >>>> if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) { >>>> this_leaf->size = found_cache->size; >>>> valid_flags++; >>>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c >>>> index eb3af2739537..ba89f9310e6f 100644 >>>> --- a/drivers/base/cacheinfo.c >>>> +++ b/drivers/base/cacheinfo.c >>>> @@ -86,7 +86,10 @@ static int cache_setup_of_node(unsigned int cpu) >>>> static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf, >>>> struct cacheinfo *sib_leaf) >>>> { >>>> - return sib_leaf->of_node == this_leaf->of_node; >>>> + if (acpi_disabled) >>>> + return sib_leaf->of_node == this_leaf->of_node; >>>> + else >>>> + return sib_leaf->firmware_node == >>>> this_leaf->firmware_node; >>>> } >>>> /* OF properties to query for a given cache type */ >>>> @@ -215,6 +218,11 @@ static inline bool cache_leaves_are_shared(struct >>>> cacheinfo *this_leaf, >>>> } >>>> #endif >>>> +int __weak cache_setup_acpi(unsigned int cpu) >>>> +{ >>>> + return -ENOTSUPP; >>>> +} >>>> + >>>> static int cache_shared_cpu_map_setup(unsigned int cpu) >>>> { >>>> struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu); >>>> @@ -225,11 +233,11 @@ static int cache_shared_cpu_map_setup(unsigned int >>>> cpu) >>>> if (this_cpu_ci->cpu_map_populated) >>>> return 0; >>>> - if (of_have_populated_dt()) >>>> + if (!acpi_disabled) >>>> + ret = cache_setup_acpi(cpu); >>>> + else if (of_have_populated_dt()) >>>> ret = cache_setup_of_node(cpu); >>>> - else if (!acpi_disabled) >>>> - /* No cache property/hierarchy support yet in ACPI */ >>>> - ret = -ENOTSUPP; >>>> + >>>> if (ret) >>>> return ret; >>>> @@ -286,7 +294,7 @@ static void cache_shared_cpu_map_remove(unsigned >>>> int cpu) >>>> static void cache_override_properties(unsigned int cpu) >>>> { >>>> - if (of_have_populated_dt()) >>>> + if (acpi_disabled && of_have_populated_dt()) >>>> return cache_of_override_properties(cpu); >>>> } >>>> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h >>>> index 3d9805297cda..7ebff157ae6c 100644 >>>> --- a/include/linux/cacheinfo.h >>>> +++ b/include/linux/cacheinfo.h >>>> @@ -37,6 +37,8 @@ enum cache_type { >>>> * @of_node: if devicetree is used, this represents either the cpu node >>>> in >>>> * case there's no explicit cache node or the cache node itself in >>>> the >>>> * device tree >>>> + * @firmware_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 >>>> @@ -65,8 +67,8 @@ struct cacheinfo { >>>> #define CACHE_ALLOCATE_POLICY_MASK \ >>>> (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE) >>>> #define CACHE_ID BIT(4) >>>> - >>>> struct device_node *of_node; >>>> + void *firmware_node; >>> >>> >>> What about converting this to using struct fwnode instead of adding >>> fields to it? >> >> >> I didn't really want to add another field here, but I've also pointed out >> how I thought converting it to a fwnode wasn't a good choice. >> >> https://lkml.org/lkml/2017/11/20/502 >> >> Mostly because IMHO its even more misleading (lacking any fwnode_operations) >> than misusing the of_node as a void *. > > I'm not sure what you mean. Converting the DT drivers/cacheinfo.c code to use a fwnode_handle is straightforward. But IMHO it doesn't solve the readability problem of either casting the ACPI/PPTT token directly to the resulting fwnode_handle *, or alternatively an actual fwnode_handle with bogus fwnode_operations to wrap that token. > > Anyway, the idea is to have one pointer in there instead of two that > cannot be used at the same time and there's no reason why of_node > should be special. Avoid two pointers for size, or readability? Because the last version had a union with of_node, which isn't strictly necessary as I can just cast the pptt token to of_node. There is exactly one line of code after that which uses the token and it doesn't care about type. (in cache_leaves_are_shared()) > > of_node should just be one of multiple choices. > >> Given that I'm in the minority thinking this, how far down the fwnode path >> on the ACPI side do we want to go? > > I have no idea. :-) > >> Is simply treating it as a void pointer >> sufficient for the ACPI side, considering all the PPTT code needs is a >> unique token? > > I guess you can think about it as of_node under a different name, but > whether or not this is sufficient depends on what you need it for. > > Thanks, > Rafael >
On Tue, Dec 12, 2017 at 11:55 PM, Jeremy Linton <jeremy.linton@arm.com> wrote: > Hi, > > > On 12/12/2017 11:25 AM, Rafael J. Wysocki wrote: >> [cut] >>>> >>>> >>>> >>>> What about converting this to using struct fwnode instead of adding >>>> fields to it? >>> >>> >>> >>> I didn't really want to add another field here, but I've also pointed out >>> how I thought converting it to a fwnode wasn't a good choice. >>> >>> https://lkml.org/lkml/2017/11/20/502 >>> >>> Mostly because IMHO its even more misleading (lacking any >>> fwnode_operations) >>> than misusing the of_node as a void *. >> >> >> I'm not sure what you mean. > > > Converting the DT drivers/cacheinfo.c code to use a fwnode_handle is > straightforward. But IMHO it doesn't solve the readability problem of either > casting the ACPI/PPTT token directly to the resulting fwnode_handle *, or > alternatively an actual fwnode_handle with bogus fwnode_operations to wrap > that token. I'm not talking about that at all. >> >> Anyway, the idea is to have one pointer in there instead of two that >> cannot be used at the same time and there's no reason why of_node >> should be special. > > > Avoid two pointers for size, or readability? Because the last > version had a union with of_node, which isn't strictly necessary as I can > just cast the pptt token to of_node. There is exactly one line of code after > that which uses the token and it doesn't care about type. So call this field "token" or similar. Don't call it "of_node" and don't introduce another "firmware_node" thing in addition to that. That just is a mess, sorry. Thanks, Rafael
On 12/12/2017 05:02 PM, Rafael J. Wysocki wrote: > On Tue, Dec 12, 2017 at 11:55 PM, Jeremy Linton <jeremy.linton@arm.com> wrote: >> Hi, >> >> >> On 12/12/2017 11:25 AM, Rafael J. Wysocki wrote: >>> > > [cut] (trimming list) > >>>>> >>>>> >>>>> >>>>> What about converting this to using struct fwnode instead of adding >>>>> fields to it? >>>> >>>> >>>> >>>> I didn't really want to add another field here, but I've also pointed out >>>> how I thought converting it to a fwnode wasn't a good choice. >>>> >>>> https://lkml.org/lkml/2017/11/20/502 >>>> >>>> Mostly because IMHO its even more misleading (lacking any >>>> fwnode_operations) >>>> than misusing the of_node as a void *. >>> >>> >>> I'm not sure what you mean. >> >> >> Converting the DT drivers/cacheinfo.c code to use a fwnode_handle is >> straightforward. But IMHO it doesn't solve the readability problem of either >> casting the ACPI/PPTT token directly to the resulting fwnode_handle *, or >> alternatively an actual fwnode_handle with bogus fwnode_operations to wrap >> that token. > > I'm not talking about that at all. > >>> >>> Anyway, the idea is to have one pointer in there instead of two that >>> cannot be used at the same time and there's no reason why of_node >>> should be special. >> >> >> Avoid two pointers for size, or readability? Because the last >> version had a union with of_node, which isn't strictly necessary as I can >> just cast the pptt token to of_node. There is exactly one line of code after >> that which uses the token and it doesn't care about type. > > So call this field "token" or similar. Don't call it "of_node" and > don't introduce another "firmware_node" thing in addition to that. > That just is a mess, sorry. I sort of agree, I think I can just change the whole of_node to a generic 'void *firmware_unique' which works fine for the PPTT code, it should also work for the DT code in cache_leaves_are_shared(). The slight gocha is there is a bit of DT code which initially runs earlier that uses of_node as an indirect parameter to a couple functions (by just passing the cacheinfo). Let me see if I can tweak that a bit. Frankly, If I understood completely all the *priv cases I suspect it might be possible to collapse *of_node into that as well. That is as long as no one decides to flush out DT on x86, or PPTT on x86.
On Wed, Dec 13, 2017 at 12:37 AM, Jeremy Linton <jeremy.linton@arm.com> wrote: > On 12/12/2017 05:02 PM, Rafael J. Wysocki wrote: >> >> On Tue, Dec 12, 2017 at 11:55 PM, Jeremy Linton <jeremy.linton@arm.com> >> wrote: >>> >>> Hi, >>> >>> >>> On 12/12/2017 11:25 AM, Rafael J. Wysocki wrote: >>>> >>>> >> >> [cut] > > > (trimming list) > > >> >>>>>> >>>>>> >>>>>> >>>>>> What about converting this to using struct fwnode instead of adding >>>>>> fields to it? >>>>> >>>>> >>>>> >>>>> >>>>> I didn't really want to add another field here, but I've also pointed >>>>> out >>>>> how I thought converting it to a fwnode wasn't a good choice. >>>>> >>>>> https://lkml.org/lkml/2017/11/20/502 >>>>> >>>>> Mostly because IMHO its even more misleading (lacking any >>>>> fwnode_operations) >>>>> than misusing the of_node as a void *. >>>> >>>> >>>> >>>> I'm not sure what you mean. >>> >>> >>> >>> Converting the DT drivers/cacheinfo.c code to use a fwnode_handle is >>> straightforward. But IMHO it doesn't solve the readability problem of >>> either >>> casting the ACPI/PPTT token directly to the resulting fwnode_handle *, or >>> alternatively an actual fwnode_handle with bogus fwnode_operations to >>> wrap >>> that token. >> >> >> I'm not talking about that at all. >> >>>> >>>> Anyway, the idea is to have one pointer in there instead of two that >>>> cannot be used at the same time and there's no reason why of_node >>>> should be special. >>> >>> >>> >>> Avoid two pointers for size, or readability? Because the last >>> version had a union with of_node, which isn't strictly necessary as I can >>> just cast the pptt token to of_node. There is exactly one line of code >>> after >>> that which uses the token and it doesn't care about type. >> >> >> So call this field "token" or similar. Don't call it "of_node" and >> don't introduce another "firmware_node" thing in addition to that. >> That just is a mess, sorry. > > > I sort of agree, I think I can just change the whole of_node to a generic > 'void *firmware_unique' which works fine for the PPTT code, it should also > work for the DT code in cache_leaves_are_shared(). > > The slight gocha is there is a bit of DT code which initially runs earlier > that uses of_node as an indirect parameter to a couple functions (by just > passing the cacheinfo). Let me see if I can tweak that a bit. > > Frankly, If I understood completely all the *priv cases I suspect it might > be possible to collapse *of_node into that as well. That is as long as no > one decides to flush out DT on x86, or PPTT on x86. I'm not aware of any plans to go in that direction. Anyway, that would be a worry of whoever wanted to do that. No need to worry about it upfront.
(Sorry for the delay, just returning from vacation) On 12/12/17 23:37, Jeremy Linton wrote: > On 12/12/2017 05:02 PM, Rafael J. Wysocki wrote: [...] >> >> So call this field "token" or similar. Don't call it "of_node" and >> don't introduce another "firmware_node" thing in addition to that. >> That just is a mess, sorry. I completely agree. Both me and Lorenzo pointed that out in previous revisions and fair enough you have a valid concern it's use with PPTT. > > I sort of agree, I think I can just change the whole of_node to a > generic 'void *firmware_unique' which works fine for the PPTT code, it > should also work for the DT code in cache_leaves_are_shared(). > Should be fine. > The slight gocha is there is a bit of DT code which initially runs > earlier that uses of_node as an indirect parameter to a couple functions > (by just passing the cacheinfo). Let me see if I can tweak that a bit. > May be use a simple inline wrapper functions to convert, might help if we diverge too. > Frankly, If I understood completely all the *priv cases I suspect it > might be possible to collapse *of_node into that as well. That is as > long as no one decides to flush out DT on x86, or PPTT on x86. > priv is used to save architecture/cache specific details that can't be generalized. I doubt if this of_node or PPTT pointer/offset falls in that category.
On 03/01/18 14:21, Sudeep Holla wrote: > On 12/12/17 23:37, Jeremy Linton wrote: >> On 12/12/2017 05:02 PM, Rafael J. Wysocki wrote: > > [...] > >>> >>> So call this field "token" or similar. Don't call it "of_node" and >>> don't introduce another "firmware_node" thing in addition to that. >>> That just is a mess, sorry. > > I completely agree. Both me and Lorenzo pointed that out in previous > revisions and fair enough you have a valid concern it's use with PPTT. > >> >> I sort of agree, I think I can just change the whole of_node to a >> generic 'void *firmware_unique' which works fine for the PPTT code, it >> should also work for the DT code in cache_leaves_are_shared(). >> > > Should be fine. > Just to let you know, I don't have much to add. I will wait for the patches that replace of_node with firmware cookie or something similar.
diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c index 0f8a1631af33..a35e457cefb7 100644 --- a/drivers/acpi/pptt.c +++ b/drivers/acpi/pptt.c @@ -329,6 +329,7 @@ static void update_cache_properties(struct cacheinfo *this_leaf, { int valid_flags = 0; + this_leaf->firmware_node = cpu_node; if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) { this_leaf->size = found_cache->size; valid_flags++; diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c index eb3af2739537..ba89f9310e6f 100644 --- a/drivers/base/cacheinfo.c +++ b/drivers/base/cacheinfo.c @@ -86,7 +86,10 @@ static int cache_setup_of_node(unsigned int cpu) static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf, struct cacheinfo *sib_leaf) { - return sib_leaf->of_node == this_leaf->of_node; + if (acpi_disabled) + return sib_leaf->of_node == this_leaf->of_node; + else + return sib_leaf->firmware_node == this_leaf->firmware_node; } /* OF properties to query for a given cache type */ @@ -215,6 +218,11 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf, } #endif +int __weak cache_setup_acpi(unsigned int cpu) +{ + return -ENOTSUPP; +} + static int cache_shared_cpu_map_setup(unsigned int cpu) { struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu); @@ -225,11 +233,11 @@ static int cache_shared_cpu_map_setup(unsigned int cpu) if (this_cpu_ci->cpu_map_populated) return 0; - if (of_have_populated_dt()) + if (!acpi_disabled) + ret = cache_setup_acpi(cpu); + else if (of_have_populated_dt()) ret = cache_setup_of_node(cpu); - else if (!acpi_disabled) - /* No cache property/hierarchy support yet in ACPI */ - ret = -ENOTSUPP; + if (ret) return ret; @@ -286,7 +294,7 @@ static void cache_shared_cpu_map_remove(unsigned int cpu) static void cache_override_properties(unsigned int cpu) { - if (of_have_populated_dt()) + if (acpi_disabled && of_have_populated_dt()) return cache_of_override_properties(cpu); } diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h index 3d9805297cda..7ebff157ae6c 100644 --- a/include/linux/cacheinfo.h +++ b/include/linux/cacheinfo.h @@ -37,6 +37,8 @@ enum cache_type { * @of_node: if devicetree is used, this represents either the cpu node in * case there's no explicit cache node or the cache node itself in the * device tree + * @firmware_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 @@ -65,8 +67,8 @@ struct cacheinfo { #define CACHE_ALLOCATE_POLICY_MASK \ (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE) #define CACHE_ID BIT(4) - struct device_node *of_node; + void *firmware_node; bool disable_sysfs; void *priv; }; @@ -99,6 +101,15 @@ int func(unsigned int cpu) \ struct cpu_cacheinfo *get_cpu_cacheinfo(unsigned int cpu); int init_cache_level(unsigned int cpu); int populate_cache_leaves(unsigned int cpu); +int cache_setup_acpi(unsigned int cpu); +int acpi_find_last_cache_level(unsigned int cpu); +#ifndef CONFIG_ACPI +int acpi_find_last_cache_level(unsigned int cpu) +{ + /*ACPI kernels should be built with PPTT support*/ + return 0; +} +#endif const struct attribute_group *cache_get_priv_group(struct cacheinfo *this_leaf);
Add a entry to to struct cacheinfo to maintain a reference to the PPTT node which can be used to match identical caches across cores. Also stub out cache_setup_acpi() so that individual architectures can enable ACPI topology parsing. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- drivers/acpi/pptt.c | 1 + drivers/base/cacheinfo.c | 20 ++++++++++++++------ include/linux/cacheinfo.h | 13 ++++++++++++- 3 files changed, 27 insertions(+), 7 deletions(-)