| Message ID | 20180113005920.28658-3-jeremy.linton@arm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Jan 12, 2018 at 06:59:10PM -0600, Jeremy Linton wrote: > The original intent in cacheinfo was that an architecture > specific populate_cache_leaves() would probe the hardware > and then cache_shared_cpu_map_setup() and > cache_override_properties() would provide firmware help to > extend/expand upon what was probed. Arm64 was really > the only architecture that was working this way, and > with the removal of most of the hardware probing logic it > became clear that it was possible to simplify the logic a bit. > > This patch combines the walk of the DT nodes with the > code updating the cache size/line_size and nr_sets. > cache_override_properties() (which was DT specific) is > then removed. The result is that cacheinfo.of_node is > no longer used as a temporary place to hold DT references > for future calls that update cache properties. That change > helps to clarify its one remaining use (matching > cacheinfo nodes that represent shared caches) which > will be used by the ACPI/PPTT code in the following patches. > > Cc: Palmer Dabbelt <palmer@sifive.com> > Cc: Albert Ou <albert@sifive.com> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > arch/riscv/kernel/cacheinfo.c | 1 + > drivers/base/cacheinfo.c | 65 +++++++++++++++++++------------------------ > include/linux/cacheinfo.h | 1 + > 3 files changed, 31 insertions(+), 36 deletions(-) > > diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c > index 10ed2749e246..6f4500233cf8 100644 > --- a/arch/riscv/kernel/cacheinfo.c > +++ b/arch/riscv/kernel/cacheinfo.c > @@ -30,6 +30,7 @@ static void ci_leaf_init(struct cacheinfo *this_leaf, > CACHE_WRITE_BACK > | CACHE_READ_ALLOCATE > | CACHE_WRITE_ALLOCATE; > + cache_of_set_props(this_leaf, node); This may be necessary but can it be done as later patch ? So far nothing is added that may break riscv IIUC. Palmer, Albert, Can you confirm ? Also, as I see we can thin down arch specific implementation on riscv if it's just using DT like ARM64. Sorry if I am missing to see something, so thought of checking. [...] > diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h > index 3d9805297cda..d35299a590a4 100644 > --- a/include/linux/cacheinfo.h > +++ b/include/linux/cacheinfo.h > @@ -99,6 +99,7 @@ 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); > +void cache_of_set_props(struct cacheinfo *this_leaf, struct device_node *np); > IIUC riscv is the only user for this outside of cacheinfo.c, right ? Hopefully we can get rid of it. Other than that, it looks OK. I will wait for response from riscv team do that these riscv related changes can be dropped or move to later patch if really needed. -- Regards, Sudeep
On Mon, 15 Jan 2018 04:33:38 PST (-0800), sudeep.holla@arm.com wrote: > On Fri, Jan 12, 2018 at 06:59:10PM -0600, Jeremy Linton wrote: >> The original intent in cacheinfo was that an architecture >> specific populate_cache_leaves() would probe the hardware >> and then cache_shared_cpu_map_setup() and >> cache_override_properties() would provide firmware help to >> extend/expand upon what was probed. Arm64 was really >> the only architecture that was working this way, and >> with the removal of most of the hardware probing logic it >> became clear that it was possible to simplify the logic a bit. >> >> This patch combines the walk of the DT nodes with the >> code updating the cache size/line_size and nr_sets. >> cache_override_properties() (which was DT specific) is >> then removed. The result is that cacheinfo.of_node is >> no longer used as a temporary place to hold DT references >> for future calls that update cache properties. That change >> helps to clarify its one remaining use (matching >> cacheinfo nodes that represent shared caches) which >> will be used by the ACPI/PPTT code in the following patches. >> >> Cc: Palmer Dabbelt <palmer@sifive.com> >> Cc: Albert Ou <albert@sifive.com> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> --- >> arch/riscv/kernel/cacheinfo.c | 1 + >> drivers/base/cacheinfo.c | 65 +++++++++++++++++++------------------------ >> include/linux/cacheinfo.h | 1 + >> 3 files changed, 31 insertions(+), 36 deletions(-) >> >> diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c >> index 10ed2749e246..6f4500233cf8 100644 >> --- a/arch/riscv/kernel/cacheinfo.c >> +++ b/arch/riscv/kernel/cacheinfo.c >> @@ -30,6 +30,7 @@ static void ci_leaf_init(struct cacheinfo *this_leaf, >> CACHE_WRITE_BACK >> | CACHE_READ_ALLOCATE >> | CACHE_WRITE_ALLOCATE; >> + cache_of_set_props(this_leaf, node); > > This may be necessary but can it be done as later patch ? So far nothing > is added that may break riscv IIUC. > > Palmer, Albert, > > Can you confirm ? Also, as I see we can thin down arch specific > implementation on riscv if it's just using DT like ARM64. Sorry if > I am missing to see something, so thought of checking. > > [...] Sorry, I guess I'm a bit confused as to what's going on here. RISC-V uses device tree on all Linux systems. >> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h >> index 3d9805297cda..d35299a590a4 100644 >> --- a/include/linux/cacheinfo.h >> +++ b/include/linux/cacheinfo.h >> @@ -99,6 +99,7 @@ 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); >> +void cache_of_set_props(struct cacheinfo *this_leaf, struct device_node *np); >> > > IIUC riscv is the only user for this outside of cacheinfo.c, right ? > Hopefully we can get rid of it. > > Other than that, it looks OK. I will wait for response from riscv team > do that these riscv related changes can be dropped or move to later > patch if really needed.
Hi, On 01/15/2018 06:33 AM, Sudeep Holla wrote: > On Fri, Jan 12, 2018 at 06:59:10PM -0600, Jeremy Linton wrote: >> The original intent in cacheinfo was that an architecture >> specific populate_cache_leaves() would probe the hardware >> and then cache_shared_cpu_map_setup() and >> cache_override_properties() would provide firmware help to >> extend/expand upon what was probed. Arm64 was really >> the only architecture that was working this way, and >> with the removal of most of the hardware probing logic it >> became clear that it was possible to simplify the logic a bit. >> >> This patch combines the walk of the DT nodes with the >> code updating the cache size/line_size and nr_sets. >> cache_override_properties() (which was DT specific) is >> then removed. The result is that cacheinfo.of_node is >> no longer used as a temporary place to hold DT references >> for future calls that update cache properties. That change >> helps to clarify its one remaining use (matching >> cacheinfo nodes that represent shared caches) which >> will be used by the ACPI/PPTT code in the following patches. >> >> Cc: Palmer Dabbelt <palmer@sifive.com> >> Cc: Albert Ou <albert@sifive.com> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> --- >> arch/riscv/kernel/cacheinfo.c | 1 + >> drivers/base/cacheinfo.c | 65 +++++++++++++++++++------------------------ >> include/linux/cacheinfo.h | 1 + >> 3 files changed, 31 insertions(+), 36 deletions(-) >> >> diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c >> index 10ed2749e246..6f4500233cf8 100644 >> --- a/arch/riscv/kernel/cacheinfo.c >> +++ b/arch/riscv/kernel/cacheinfo.c >> @@ -30,6 +30,7 @@ static void ci_leaf_init(struct cacheinfo *this_leaf, >> CACHE_WRITE_BACK >> | CACHE_READ_ALLOCATE >> | CACHE_WRITE_ALLOCATE; >> + cache_of_set_props(this_leaf, node); > > This may be necessary but can it be done as later patch ? So far nothing > is added that may break riscv IIUC. Well I think you have a bisection issue where the additional information will disappear between this patch and wherever we put this code back in. > > Palmer, Albert, > > Can you confirm ? Also, as I see we can thin down arch specific > implementation on riscv if it's just using DT like ARM64. Sorry if > I am missing to see something, so thought of checking. > > [...] > >> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h >> index 3d9805297cda..d35299a590a4 100644 >> --- a/include/linux/cacheinfo.h >> +++ b/include/linux/cacheinfo.h >> @@ -99,6 +99,7 @@ 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); >> +void cache_of_set_props(struct cacheinfo *this_leaf, struct device_node *np); >> > > IIUC riscv is the only user for this outside of cacheinfo.c, right ? > Hopefully we can get rid of it. Yes, it should be the only user. I spent some time looking at the other users of this code to assure that they weren't doing partial DT setups and then having the common code use the resulting DT nodes. Riscv was the only case I found like that and that behavior is fairly new. I think that they are doing it that way in order to get the cache type setup earlier. (to work around problems like the one recently fixed for the NONE->UNIFIED node conversion). > > Other than that, it looks OK. I will wait for response from riscv team > do that these riscv related changes can be dropped or move to later > patch if really needed. > > -- > Regards, > Sudeep >
Hi, On 01/15/2018 10:07 AM, Palmer Dabbelt wrote: > On Mon, 15 Jan 2018 04:33:38 PST (-0800), sudeep.holla@arm.com wrote: >> On Fri, Jan 12, 2018 at 06:59:10PM -0600, Jeremy Linton wrote: >>> The original intent in cacheinfo was that an architecture >>> specific populate_cache_leaves() would probe the hardware >>> and then cache_shared_cpu_map_setup() and >>> cache_override_properties() would provide firmware help to >>> extend/expand upon what was probed. Arm64 was really >>> the only architecture that was working this way, and >>> with the removal of most of the hardware probing logic it >>> became clear that it was possible to simplify the logic a bit. >>> >>> This patch combines the walk of the DT nodes with the >>> code updating the cache size/line_size and nr_sets. >>> cache_override_properties() (which was DT specific) is >>> then removed. The result is that cacheinfo.of_node is >>> no longer used as a temporary place to hold DT references >>> for future calls that update cache properties. That change >>> helps to clarify its one remaining use (matching >>> cacheinfo nodes that represent shared caches) which >>> will be used by the ACPI/PPTT code in the following patches. >>> >>> Cc: Palmer Dabbelt <palmer@sifive.com> >>> Cc: Albert Ou <albert@sifive.com> >>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >>> --- >>> arch/riscv/kernel/cacheinfo.c | 1 + >>> drivers/base/cacheinfo.c | 65 >>> +++++++++++++++++++------------------------ >>> include/linux/cacheinfo.h | 1 + >>> 3 files changed, 31 insertions(+), 36 deletions(-) >>> >>> diff --git a/arch/riscv/kernel/cacheinfo.c >>> b/arch/riscv/kernel/cacheinfo.c >>> index 10ed2749e246..6f4500233cf8 100644 >>> --- a/arch/riscv/kernel/cacheinfo.c >>> +++ b/arch/riscv/kernel/cacheinfo.c >>> @@ -30,6 +30,7 @@ static void ci_leaf_init(struct cacheinfo *this_leaf, >>> CACHE_WRITE_BACK >>> | CACHE_READ_ALLOCATE >>> | CACHE_WRITE_ALLOCATE; >>> + cache_of_set_props(this_leaf, node); >> >> This may be necessary but can it be done as later patch ? So far nothing >> is added that may break riscv IIUC. >> >> Palmer, Albert, >> >> Can you confirm ? Also, as I see we can thin down arch specific >> implementation on riscv if it's just using DT like ARM64. Sorry if >> I am missing to see something, so thought of checking. >> >> [...] > > Sorry, I guess I'm a bit confused as to what's going on here. RISC-V > uses device tree on all Linux systems. If I'm understanding the context correctly: The first part of this patch set just straightens out the DT setup order so it happens in a single pass (rather that doing one pass to find the DT nodes, then another later on to update the cacheinfo from DT). This clarifies/simplifies how the firmware_unique (firmware_token?) field in cacheinfo is actually being used. riscv is a bit odd because its actually doing some DT manipulation in its arch setup (populate_cache_leaves()). I think the thought process is that it might be nicer if the common DT code handled whatever required that bit of logic to be added to riscv. If that were changed, then it might be possible to drop most of the DT cache setup code from the riscv/arch tree. > >>> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h >>> index 3d9805297cda..d35299a590a4 100644 >>> --- a/include/linux/cacheinfo.h >>> +++ b/include/linux/cacheinfo.h >>> @@ -99,6 +99,7 @@ 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); >>> +void cache_of_set_props(struct cacheinfo *this_leaf, struct >>> device_node *np); >>> >> >> IIUC riscv is the only user for this outside of cacheinfo.c, right ? >> Hopefully we can get rid of it. >> >> Other than that, it looks OK. I will wait for response from riscv team >> do that these riscv related changes can be dropped or move to later >> patch if really needed.
(Sorry, somehow I missed this email until I saw Jeremy's reply today) On 15/01/18 16:07, Palmer Dabbelt wrote: > On Mon, 15 Jan 2018 04:33:38 PST (-0800), sudeep.holla@arm.com wrote: >> On Fri, Jan 12, 2018 at 06:59:10PM -0600, Jeremy Linton wrote: >>> The original intent in cacheinfo was that an architecture >>> specific populate_cache_leaves() would probe the hardware >>> and then cache_shared_cpu_map_setup() and >>> cache_override_properties() would provide firmware help to >>> extend/expand upon what was probed. Arm64 was really >>> the only architecture that was working this way, and >>> with the removal of most of the hardware probing logic it >>> became clear that it was possible to simplify the logic a bit. >>> >>> This patch combines the walk of the DT nodes with the >>> code updating the cache size/line_size and nr_sets. >>> cache_override_properties() (which was DT specific) is >>> then removed. The result is that cacheinfo.of_node is >>> no longer used as a temporary place to hold DT references >>> for future calls that update cache properties. That change >>> helps to clarify its one remaining use (matching >>> cacheinfo nodes that represent shared caches) which >>> will be used by the ACPI/PPTT code in the following patches. >>> >>> Cc: Palmer Dabbelt <palmer@sifive.com> >>> Cc: Albert Ou <albert@sifive.com> >>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >>> --- >>> arch/riscv/kernel/cacheinfo.c | 1 + >>> drivers/base/cacheinfo.c | 65 >>> +++++++++++++++++++------------------------ >>> include/linux/cacheinfo.h | 1 + >>> 3 files changed, 31 insertions(+), 36 deletions(-) >>> >>> diff --git a/arch/riscv/kernel/cacheinfo.c >>> b/arch/riscv/kernel/cacheinfo.c >>> index 10ed2749e246..6f4500233cf8 100644 >>> --- a/arch/riscv/kernel/cacheinfo.c >>> +++ b/arch/riscv/kernel/cacheinfo.c >>> @@ -30,6 +30,7 @@ static void ci_leaf_init(struct cacheinfo *this_leaf, >>> CACHE_WRITE_BACK >>> | CACHE_READ_ALLOCATE >>> | CACHE_WRITE_ALLOCATE; >>> + cache_of_set_props(this_leaf, node); >> >> This may be necessary but can it be done as later patch ? So far nothing >> is added that may break riscv IIUC. >> >> Palmer, Albert, >> >> Can you confirm ? Also, as I see we can thin down arch specific >> implementation on riscv if it's just using DT like ARM64. Sorry if >> I am missing to see something, so thought of checking. >> >> [...] > > Sorry, I guess I'm a bit confused as to what's going on here. RISC-V > uses device tree on all Linux systems. > Good. By thin down, I was thinking of moving the init_cache_level and populate_cache_leaves implementation of riscv to generic weak function under CONFIG_OF. You may even endup deleting riscv cacheinfo.c Just a thought, sorry for not being clear earlier.
On 16/01/18 21:07, Jeremy Linton wrote: > Hi, > > On 01/15/2018 06:33 AM, Sudeep Holla wrote: >> On Fri, Jan 12, 2018 at 06:59:10PM -0600, Jeremy Linton wrote: >>> The original intent in cacheinfo was that an architecture >>> specific populate_cache_leaves() would probe the hardware >>> and then cache_shared_cpu_map_setup() and >>> cache_override_properties() would provide firmware help to >>> extend/expand upon what was probed. Arm64 was really >>> the only architecture that was working this way, and >>> with the removal of most of the hardware probing logic it >>> became clear that it was possible to simplify the logic a bit. >>> >>> This patch combines the walk of the DT nodes with the >>> code updating the cache size/line_size and nr_sets. >>> cache_override_properties() (which was DT specific) is >>> then removed. The result is that cacheinfo.of_node is >>> no longer used as a temporary place to hold DT references >>> for future calls that update cache properties. That change >>> helps to clarify its one remaining use (matching >>> cacheinfo nodes that represent shared caches) which >>> will be used by the ACPI/PPTT code in the following patches. >>> >>> Cc: Palmer Dabbelt <palmer@sifive.com> >>> Cc: Albert Ou <albert@sifive.com> >>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >>> --- >>> arch/riscv/kernel/cacheinfo.c | 1 + >>> drivers/base/cacheinfo.c | 65 >>> +++++++++++++++++++------------------------ >>> include/linux/cacheinfo.h | 1 + >>> 3 files changed, 31 insertions(+), 36 deletions(-) >>> >>> diff --git a/arch/riscv/kernel/cacheinfo.c >>> b/arch/riscv/kernel/cacheinfo.c >>> index 10ed2749e246..6f4500233cf8 100644 >>> --- a/arch/riscv/kernel/cacheinfo.c >>> +++ b/arch/riscv/kernel/cacheinfo.c >>> @@ -30,6 +30,7 @@ static void ci_leaf_init(struct cacheinfo *this_leaf, >>> CACHE_WRITE_BACK >>> | CACHE_READ_ALLOCATE >>> | CACHE_WRITE_ALLOCATE; >>> + cache_of_set_props(this_leaf, node); >> >> This may be necessary but can it be done as later patch ? So far nothing >> is added that may break riscv IIUC. > > Well I think you have a bisection issue where the additional information > will disappear between this patch and wherever we put this code back in. > Hmm, I am sorry but I fail to see the issue. Before this change, populate_cache_leaves just populated the info as per ci_leaf_init in arch/riscv/kernel/cacheinfo.c and cache_override_properties used to fill the remaining. After this patch, the same is achieved in cache_shared_cpu_map_setup. In both case, it was by the end of detect_cache_attributes, so I see no issue.
Hi, On 01/17/2018 12:20 PM, Sudeep Holla wrote: > > > On 16/01/18 21:07, Jeremy Linton wrote: >> Hi, >> >> On 01/15/2018 06:33 AM, Sudeep Holla wrote: >>> On Fri, Jan 12, 2018 at 06:59:10PM -0600, Jeremy Linton wrote: >>>> The original intent in cacheinfo was that an architecture >>>> specific populate_cache_leaves() would probe the hardware >>>> and then cache_shared_cpu_map_setup() and >>>> cache_override_properties() would provide firmware help to >>>> extend/expand upon what was probed. Arm64 was really >>>> the only architecture that was working this way, and >>>> with the removal of most of the hardware probing logic it >>>> became clear that it was possible to simplify the logic a bit. >>>> >>>> This patch combines the walk of the DT nodes with the >>>> code updating the cache size/line_size and nr_sets. >>>> cache_override_properties() (which was DT specific) is >>>> then removed. The result is that cacheinfo.of_node is >>>> no longer used as a temporary place to hold DT references >>>> for future calls that update cache properties. That change >>>> helps to clarify its one remaining use (matching >>>> cacheinfo nodes that represent shared caches) which >>>> will be used by the ACPI/PPTT code in the following patches. >>>> >>>> Cc: Palmer Dabbelt <palmer@sifive.com> >>>> Cc: Albert Ou <albert@sifive.com> >>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >>>> --- >>>> arch/riscv/kernel/cacheinfo.c | 1 + >>>> drivers/base/cacheinfo.c | 65 >>>> +++++++++++++++++++------------------------ >>>> include/linux/cacheinfo.h | 1 + >>>> 3 files changed, 31 insertions(+), 36 deletions(-) >>>> >>>> diff --git a/arch/riscv/kernel/cacheinfo.c >>>> b/arch/riscv/kernel/cacheinfo.c >>>> index 10ed2749e246..6f4500233cf8 100644 >>>> --- a/arch/riscv/kernel/cacheinfo.c >>>> +++ b/arch/riscv/kernel/cacheinfo.c >>>> @@ -30,6 +30,7 @@ static void ci_leaf_init(struct cacheinfo *this_leaf, >>>> CACHE_WRITE_BACK >>>> | CACHE_READ_ALLOCATE >>>> | CACHE_WRITE_ALLOCATE; >>>> + cache_of_set_props(this_leaf, node); >>> >>> This may be necessary but can it be done as later patch ? So far nothing >>> is added that may break riscv IIUC. >> >> Well I think you have a bisection issue where the additional information >> will disappear between this patch and wherever we put this code back in. >> > > Hmm, I am sorry but I fail to see the issue. Before this change, > populate_cache_leaves just populated the info as per ci_leaf_init in > arch/riscv/kernel/cacheinfo.c and cache_override_properties used to fill > the remaining. > > After this patch, the same is achieved in cache_shared_cpu_map_setup. > > In both case, it was by the end of detect_cache_attributes, so I see no > issue. > Hi, I must be misunderstanding something. AFAIK, The code in cache_setup_of_node() won't call cache_of_set_props() because it returns when there is an existing of_node (fw_unique) created by the riscv populate_cache_leaves(). That's why I'm making the direct call here. If we fail to get that change in place before cache_override_properties() is removed then the fields not set by the riscv code (size/etc) will be missing.
On 17/01/18 18:51, Jeremy Linton wrote: > Hi, > > On 01/17/2018 12:20 PM, Sudeep Holla wrote: >> >> >> On 16/01/18 21:07, Jeremy Linton wrote: >>> Hi, >>> >>> On 01/15/2018 06:33 AM, Sudeep Holla wrote: >>>> On Fri, Jan 12, 2018 at 06:59:10PM -0600, Jeremy Linton wrote: >>>>> The original intent in cacheinfo was that an architecture >>>>> specific populate_cache_leaves() would probe the hardware >>>>> and then cache_shared_cpu_map_setup() and >>>>> cache_override_properties() would provide firmware help to >>>>> extend/expand upon what was probed. Arm64 was really >>>>> the only architecture that was working this way, and >>>>> with the removal of most of the hardware probing logic it >>>>> became clear that it was possible to simplify the logic a bit. >>>>> >>>>> This patch combines the walk of the DT nodes with the >>>>> code updating the cache size/line_size and nr_sets. >>>>> cache_override_properties() (which was DT specific) is >>>>> then removed. The result is that cacheinfo.of_node is >>>>> no longer used as a temporary place to hold DT references >>>>> for future calls that update cache properties. That change >>>>> helps to clarify its one remaining use (matching >>>>> cacheinfo nodes that represent shared caches) which >>>>> will be used by the ACPI/PPTT code in the following patches. >>>>> >>>>> Cc: Palmer Dabbelt <palmer@sifive.com> >>>>> Cc: Albert Ou <albert@sifive.com> >>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >>>>> --- >>>>> arch/riscv/kernel/cacheinfo.c | 1 + >>>>> drivers/base/cacheinfo.c | 65 >>>>> +++++++++++++++++++------------------------ >>>>> include/linux/cacheinfo.h | 1 + >>>>> 3 files changed, 31 insertions(+), 36 deletions(-) >>>>> >>>>> diff --git a/arch/riscv/kernel/cacheinfo.c >>>>> b/arch/riscv/kernel/cacheinfo.c >>>>> index 10ed2749e246..6f4500233cf8 100644 >>>>> --- a/arch/riscv/kernel/cacheinfo.c >>>>> +++ b/arch/riscv/kernel/cacheinfo.c >>>>> @@ -30,6 +30,7 @@ static void ci_leaf_init(struct cacheinfo >>>>> *this_leaf, >>>>> CACHE_WRITE_BACK >>>>> | CACHE_READ_ALLOCATE >>>>> | CACHE_WRITE_ALLOCATE; >>>>> + cache_of_set_props(this_leaf, node); >>>> >>>> This may be necessary but can it be done as later patch ? So far >>>> nothing >>>> is added that may break riscv IIUC. >>> >>> Well I think you have a bisection issue where the additional information >>> will disappear between this patch and wherever we put this code back in. >>> >> >> Hmm, I am sorry but I fail to see the issue. Before this change, >> populate_cache_leaves just populated the info as per ci_leaf_init in >> arch/riscv/kernel/cacheinfo.c and cache_override_properties used to fill >> the remaining. >> >> After this patch, the same is achieved in cache_shared_cpu_map_setup. >> >> In both case, it was by the end of detect_cache_attributes, so I see no >> issue. >> > > > Hi, > > I must be misunderstanding something. > Looks like I was missing to understand something :) > AFAIK, The code in cache_setup_of_node() won't call cache_of_set_props() > because it returns when there is an existing of_node (fw_unique) created > by the riscv populate_cache_leaves(). That's why I'm making the direct > call here. If we fail to get that change in place before > cache_override_properties() is removed then the fields not set by the > riscv code (size/etc) will be missing. Indeed. I am trying to avoid use of cache_of_set_props outside. How about skipping setting up of fw_unique in ci_leaf_init instead ?
On Wed, 17 Jan 2018 10:08:27 PST (-0800), sudeep.holla@arm.com wrote: > (Sorry, somehow I missed this email until I saw Jeremy's reply today) > > On 15/01/18 16:07, Palmer Dabbelt wrote: >> On Mon, 15 Jan 2018 04:33:38 PST (-0800), sudeep.holla@arm.com wrote: >>> On Fri, Jan 12, 2018 at 06:59:10PM -0600, Jeremy Linton wrote: >>>> The original intent in cacheinfo was that an architecture >>>> specific populate_cache_leaves() would probe the hardware >>>> and then cache_shared_cpu_map_setup() and >>>> cache_override_properties() would provide firmware help to >>>> extend/expand upon what was probed. Arm64 was really >>>> the only architecture that was working this way, and >>>> with the removal of most of the hardware probing logic it >>>> became clear that it was possible to simplify the logic a bit. >>>> >>>> This patch combines the walk of the DT nodes with the >>>> code updating the cache size/line_size and nr_sets. >>>> cache_override_properties() (which was DT specific) is >>>> then removed. The result is that cacheinfo.of_node is >>>> no longer used as a temporary place to hold DT references >>>> for future calls that update cache properties. That change >>>> helps to clarify its one remaining use (matching >>>> cacheinfo nodes that represent shared caches) which >>>> will be used by the ACPI/PPTT code in the following patches. >>>> >>>> Cc: Palmer Dabbelt <palmer@sifive.com> >>>> Cc: Albert Ou <albert@sifive.com> >>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >>>> --- >>>> arch/riscv/kernel/cacheinfo.c | 1 + >>>> drivers/base/cacheinfo.c | 65 >>>> +++++++++++++++++++------------------------ >>>> include/linux/cacheinfo.h | 1 + >>>> 3 files changed, 31 insertions(+), 36 deletions(-) >>>> >>>> diff --git a/arch/riscv/kernel/cacheinfo.c >>>> b/arch/riscv/kernel/cacheinfo.c >>>> index 10ed2749e246..6f4500233cf8 100644 >>>> --- a/arch/riscv/kernel/cacheinfo.c >>>> +++ b/arch/riscv/kernel/cacheinfo.c >>>> @@ -30,6 +30,7 @@ static void ci_leaf_init(struct cacheinfo *this_leaf, >>>> CACHE_WRITE_BACK >>>> | CACHE_READ_ALLOCATE >>>> | CACHE_WRITE_ALLOCATE; >>>> + cache_of_set_props(this_leaf, node); >>> >>> This may be necessary but can it be done as later patch ? So far nothing >>> is added that may break riscv IIUC. >>> >>> Palmer, Albert, >>> >>> Can you confirm ? Also, as I see we can thin down arch specific >>> implementation on riscv if it's just using DT like ARM64. Sorry if >>> I am missing to see something, so thought of checking. >>> >>> [...] >> >> Sorry, I guess I'm a bit confused as to what's going on here. RISC-V >> uses device tree on all Linux systems. >> > > Good. By thin down, I was thinking of moving the init_cache_level and > populate_cache_leaves implementation of riscv to generic weak function > under CONFIG_OF. You may even endup deleting riscv cacheinfo.c > > Just a thought, sorry for not being clear earlier. OK, well, I'm always happy to convert things to generic implementations.
Hi, On 01/18/2018 04:14 AM, Sudeep Holla wrote: > > > On 17/01/18 18:51, Jeremy Linton wrote: >> Hi, >> >> On 01/17/2018 12:20 PM, Sudeep Holla wrote: >>> >>> >>> On 16/01/18 21:07, Jeremy Linton wrote: >>>> Hi, >>>> >>>> On 01/15/2018 06:33 AM, Sudeep Holla wrote: >>>>> On Fri, Jan 12, 2018 at 06:59:10PM -0600, Jeremy Linton wrote: >>>>>> The original intent in cacheinfo was that an architecture >>>>>> specific populate_cache_leaves() would probe the hardware >>>>>> and then cache_shared_cpu_map_setup() and >>>>>> cache_override_properties() would provide firmware help to >>>>>> extend/expand upon what was probed. Arm64 was really >>>>>> the only architecture that was working this way, and >>>>>> with the removal of most of the hardware probing logic it >>>>>> became clear that it was possible to simplify the logic a bit. >>>>>> >>>>>> This patch combines the walk of the DT nodes with the >>>>>> code updating the cache size/line_size and nr_sets. >>>>>> cache_override_properties() (which was DT specific) is >>>>>> then removed. The result is that cacheinfo.of_node is >>>>>> no longer used as a temporary place to hold DT references >>>>>> for future calls that update cache properties. That change >>>>>> helps to clarify its one remaining use (matching >>>>>> cacheinfo nodes that represent shared caches) which >>>>>> will be used by the ACPI/PPTT code in the following patches. >>>>>> >>>>>> Cc: Palmer Dabbelt <palmer@sifive.com> >>>>>> Cc: Albert Ou <albert@sifive.com> >>>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >>>>>> --- >>>>>> arch/riscv/kernel/cacheinfo.c | 1 + >>>>>> drivers/base/cacheinfo.c | 65 >>>>>> +++++++++++++++++++------------------------ >>>>>> include/linux/cacheinfo.h | 1 + >>>>>> 3 files changed, 31 insertions(+), 36 deletions(-) >>>>>> >>>>>> diff --git a/arch/riscv/kernel/cacheinfo.c >>>>>> b/arch/riscv/kernel/cacheinfo.c >>>>>> index 10ed2749e246..6f4500233cf8 100644 >>>>>> --- a/arch/riscv/kernel/cacheinfo.c >>>>>> +++ b/arch/riscv/kernel/cacheinfo.c >>>>>> @@ -30,6 +30,7 @@ static void ci_leaf_init(struct cacheinfo >>>>>> *this_leaf, >>>>>> CACHE_WRITE_BACK >>>>>> | CACHE_READ_ALLOCATE >>>>>> | CACHE_WRITE_ALLOCATE; >>>>>> + cache_of_set_props(this_leaf, node); >>>>> >>>>> This may be necessary but can it be done as later patch ? So far >>>>> nothing >>>>> is added that may break riscv IIUC. >>>> >>>> Well I think you have a bisection issue where the additional information >>>> will disappear between this patch and wherever we put this code back in. >>>> >>> >>> Hmm, I am sorry but I fail to see the issue. Before this change, >>> populate_cache_leaves just populated the info as per ci_leaf_init in >>> arch/riscv/kernel/cacheinfo.c and cache_override_properties used to fill >>> the remaining. >>> >>> After this patch, the same is achieved in cache_shared_cpu_map_setup. >>> >>> In both case, it was by the end of detect_cache_attributes, so I see no >>> issue. >>> >> >> >> Hi, >> >> I must be misunderstanding something. >> > > Looks like I was missing to understand something :) > >> AFAIK, The code in cache_setup_of_node() won't call cache_of_set_props() >> because it returns when there is an existing of_node (fw_unique) created >> by the riscv populate_cache_leaves(). That's why I'm making the direct >> call here. If we fail to get that change in place before >> cache_override_properties() is removed then the fields not set by the >> riscv code (size/etc) will be missing. > > Indeed. I am trying to avoid use of cache_of_set_props outside. > How about skipping setting up of fw_unique in ci_leaf_init instead ? > I've been thinking about this, but I'm hesitant because I don't have a good test platform for this code. Plus, I'm not 100% sure that the results are the same (is it possible that the platform setup node isn't the same as the one the the common code would find?). I also think I'm getting a little off topic with these patches in relation to what the core goal is (adding PPTT parsing).
diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c index 10ed2749e246..6f4500233cf8 100644 --- a/arch/riscv/kernel/cacheinfo.c +++ b/arch/riscv/kernel/cacheinfo.c @@ -30,6 +30,7 @@ static void ci_leaf_init(struct cacheinfo *this_leaf, CACHE_WRITE_BACK | CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE; + cache_of_set_props(this_leaf, node); } static int __init_cache_level(unsigned int cpu) diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c index a883a213fcd5..fc0d42bbd9eb 100644 --- a/drivers/base/cacheinfo.c +++ b/drivers/base/cacheinfo.c @@ -43,6 +43,7 @@ struct cpu_cacheinfo *get_cpu_cacheinfo(unsigned int cpu) } #ifdef CONFIG_OF + static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf, struct cacheinfo *sib_leaf) { @@ -82,7 +83,7 @@ static inline int get_cacheinfo_idx(enum cache_type type) return type; } -static void cache_size(struct cacheinfo *this_leaf) +static void cache_size(struct cacheinfo *this_leaf, struct device_node *np) { const char *propname; const __be32 *cache_size; @@ -91,13 +92,14 @@ static void cache_size(struct cacheinfo *this_leaf) ct_idx = get_cacheinfo_idx(this_leaf->type); propname = cache_type_info[ct_idx].size_prop; - cache_size = of_get_property(this_leaf->of_node, propname, NULL); + cache_size = of_get_property(np, propname, NULL); if (cache_size) this_leaf->size = of_read_number(cache_size, 1); } /* not cache_line_size() because that's a macro in include/linux/cache.h */ -static void cache_get_line_size(struct cacheinfo *this_leaf) +static void cache_get_line_size(struct cacheinfo *this_leaf, + struct device_node *np) { const __be32 *line_size; int i, lim, ct_idx; @@ -109,7 +111,7 @@ static void cache_get_line_size(struct cacheinfo *this_leaf) const char *propname; propname = cache_type_info[ct_idx].line_size_props[i]; - line_size = of_get_property(this_leaf->of_node, propname, NULL); + line_size = of_get_property(np, propname, NULL); if (line_size) break; } @@ -118,7 +120,7 @@ static void cache_get_line_size(struct cacheinfo *this_leaf) this_leaf->coherency_line_size = of_read_number(line_size, 1); } -static void cache_nr_sets(struct cacheinfo *this_leaf) +static void cache_nr_sets(struct cacheinfo *this_leaf, struct device_node *np) { const char *propname; const __be32 *nr_sets; @@ -127,7 +129,7 @@ static void cache_nr_sets(struct cacheinfo *this_leaf) ct_idx = get_cacheinfo_idx(this_leaf->type); propname = cache_type_info[ct_idx].nr_sets_prop; - nr_sets = of_get_property(this_leaf->of_node, propname, NULL); + nr_sets = of_get_property(np, propname, NULL); if (nr_sets) this_leaf->number_of_sets = of_read_number(nr_sets, 1); } @@ -146,32 +148,26 @@ static void cache_associativity(struct cacheinfo *this_leaf) this_leaf->ways_of_associativity = (size / nr_sets) / line_size; } -static bool cache_node_is_unified(struct cacheinfo *this_leaf) +static bool cache_node_is_unified(struct cacheinfo *this_leaf, + struct device_node *np) { - return of_property_read_bool(this_leaf->of_node, "cache-unified"); + return of_property_read_bool(np, "cache-unified"); } -static void cache_of_override_properties(unsigned int cpu) +void cache_of_set_props(struct cacheinfo *this_leaf, struct device_node *np) { - int index; - struct cacheinfo *this_leaf; - struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu); - - for (index = 0; index < cache_leaves(cpu); index++) { - this_leaf = this_cpu_ci->info_list + index; - /* - * init_cache_level must setup the cache level correctly - * overriding the architecturally specified levels, so - * if type is NONE at this stage, it should be unified - */ - if (this_leaf->type == CACHE_TYPE_NOCACHE && - cache_node_is_unified(this_leaf)) - this_leaf->type = CACHE_TYPE_UNIFIED; - cache_size(this_leaf); - cache_get_line_size(this_leaf); - cache_nr_sets(this_leaf); - cache_associativity(this_leaf); - } + /* + * init_cache_level must setup the cache level correctly + * overriding the architecturally specified levels, so + * if type is NONE at this stage, it should be unified + */ + if (this_leaf->type == CACHE_TYPE_NOCACHE && + cache_node_is_unified(this_leaf, np)) + this_leaf->type = CACHE_TYPE_UNIFIED; + cache_size(this_leaf, np); + cache_get_line_size(this_leaf, np); + cache_nr_sets(this_leaf, np); + cache_associativity(this_leaf); } static int cache_setup_of_node(unsigned int cpu) @@ -204,6 +200,7 @@ static int cache_setup_of_node(unsigned int cpu) np = of_node_get(np);/* cpu node itself */ if (!np) break; + cache_of_set_props(this_leaf, np); this_leaf->of_node = np; index++; } @@ -214,7 +211,6 @@ static int cache_setup_of_node(unsigned int cpu) return 0; } #else -static void cache_of_override_properties(unsigned int cpu) { } static inline int cache_setup_of_node(unsigned int cpu) { return 0; } static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf, struct cacheinfo *sib_leaf) @@ -297,12 +293,6 @@ static void cache_shared_cpu_map_remove(unsigned int cpu) } } -static void cache_override_properties(unsigned int cpu) -{ - if (of_have_populated_dt()) - return cache_of_override_properties(cpu); -} - static void free_cache_attributes(unsigned int cpu) { if (!per_cpu_cacheinfo(cpu)) @@ -336,6 +326,10 @@ static int detect_cache_attributes(unsigned int cpu) if (per_cpu_cacheinfo(cpu) == NULL) return -ENOMEM; + /* + * populate_cache_leaves() may completely setup the cache leaves and + * shared_cpu_map or it may leave it partially setup. + */ ret = populate_cache_leaves(cpu); if (ret) goto free_ci; @@ -349,7 +343,6 @@ static int detect_cache_attributes(unsigned int cpu) goto free_ci; } - cache_override_properties(cpu); return 0; free_ci: diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h index 3d9805297cda..d35299a590a4 100644 --- a/include/linux/cacheinfo.h +++ b/include/linux/cacheinfo.h @@ -99,6 +99,7 @@ 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); +void cache_of_set_props(struct cacheinfo *this_leaf, struct device_node *np); const struct attribute_group *cache_get_priv_group(struct cacheinfo *this_leaf);
The original intent in cacheinfo was that an architecture specific populate_cache_leaves() would probe the hardware and then cache_shared_cpu_map_setup() and cache_override_properties() would provide firmware help to extend/expand upon what was probed. Arm64 was really the only architecture that was working this way, and with the removal of most of the hardware probing logic it became clear that it was possible to simplify the logic a bit. This patch combines the walk of the DT nodes with the code updating the cache size/line_size and nr_sets. cache_override_properties() (which was DT specific) is then removed. The result is that cacheinfo.of_node is no longer used as a temporary place to hold DT references for future calls that update cache properties. That change helps to clarify its one remaining use (matching cacheinfo nodes that represent shared caches) which will be used by the ACPI/PPTT code in the following patches. Cc: Palmer Dabbelt <palmer@sifive.com> Cc: Albert Ou <albert@sifive.com> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- arch/riscv/kernel/cacheinfo.c | 1 + drivers/base/cacheinfo.c | 65 +++++++++++++++++++------------------------ include/linux/cacheinfo.h | 1 + 3 files changed, 31 insertions(+), 36 deletions(-)