| Message ID | 20240418034330.84721-2-cuiyunhui@bytedance.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/3] riscv: cacheinfo: remove the useless input parameter (node) of ci_leaf_init() | expand |
On Thu, Apr 18, 2024 at 11:43:29AM +0800, Yunhui Cui wrote: > Before cacheinfo can be built correctly, we need to initialize level > and type. Since RSIC-V currently does not have a register group that > describes cache-related attributes like ARM64, we cannot obtain them > directly, so now we obtain cache leaves from the ACPI PPTT table > (acpi_get_cache_info()) and set the cache type through split_levels. > > Suggested-by: Jeremy Linton <jeremy.linton@arm.com> > Suggested-by: Sudeep Holla <sudeep.holla@arm.com> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > --- > arch/riscv/kernel/cacheinfo.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c > index 30a6878287ad..e47a1e6bd3fe 100644 > --- a/arch/riscv/kernel/cacheinfo.c > +++ b/arch/riscv/kernel/cacheinfo.c > @@ -6,6 +6,7 @@ > #include <linux/cpu.h> > #include <linux/of.h> > #include <asm/cacheinfo.h> > +#include <linux/acpi.h> > > static struct riscv_cacheinfo_ops *rv_cache_ops; > > @@ -78,6 +79,27 @@ int populate_cache_leaves(unsigned int cpu) > struct device_node *prev = NULL; > int levels = 1, level = 1; > > + if (!acpi_disabled) { > + int ret, fw_levels, split_levels; > + > + ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels); > + if (ret) > + return ret; > + > + BUG_ON((split_levels > fw_levels) || > + (split_levels + fw_levels > this_cpu_ci->num_leaves)); > + > + for (; level <= this_cpu_ci->num_levels; level++) { > + if (level <= split_levels) { > + ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level); > + ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level); > + } else { > + ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level); > + } > + } > + return 0; > + } > + Much better, so my review still stands 😄 Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > if (of_property_read_bool(np, "cache-size")) > ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level); > if (of_property_read_bool(np, "i-cache-size")) > -- > 2.20.1 >
Hi palmer, On Thu, Apr 18, 2024 at 4:42 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Thu, Apr 18, 2024 at 11:43:29AM +0800, Yunhui Cui wrote: > > Before cacheinfo can be built correctly, we need to initialize level > > and type. Since RSIC-V currently does not have a register group that > > describes cache-related attributes like ARM64, we cannot obtain them > > directly, so now we obtain cache leaves from the ACPI PPTT table > > (acpi_get_cache_info()) and set the cache type through split_levels. > > > > Suggested-by: Jeremy Linton <jeremy.linton@arm.com> > > Suggested-by: Sudeep Holla <sudeep.holla@arm.com> > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > > --- > > arch/riscv/kernel/cacheinfo.c | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c > > index 30a6878287ad..e47a1e6bd3fe 100644 > > --- a/arch/riscv/kernel/cacheinfo.c > > +++ b/arch/riscv/kernel/cacheinfo.c > > @@ -6,6 +6,7 @@ > > #include <linux/cpu.h> > > #include <linux/of.h> > > #include <asm/cacheinfo.h> > > +#include <linux/acpi.h> > > > > static struct riscv_cacheinfo_ops *rv_cache_ops; > > > > @@ -78,6 +79,27 @@ int populate_cache_leaves(unsigned int cpu) > > struct device_node *prev = NULL; > > int levels = 1, level = 1; > > > > + if (!acpi_disabled) { > > + int ret, fw_levels, split_levels; > > + > > + ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels); > > + if (ret) > > + return ret; > > + > > + BUG_ON((split_levels > fw_levels) || > > + (split_levels + fw_levels > this_cpu_ci->num_leaves)); > > + > > + for (; level <= this_cpu_ci->num_levels; level++) { > > + if (level <= split_levels) { > > + ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level); > > + ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level); > > + } else { > > + ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level); > > + } > > + } > > + return 0; > > + } > > + > > Much better, so my review still stands 😄 > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > > > if (of_property_read_bool(np, "cache-size")) > > ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level); > > if (of_property_read_bool(np, "i-cache-size")) > > -- > > 2.20.1 > > > > -- > Regards, > Sudeep Do you have any comments about this patch series? Thanks, Yunhui
Hi, On 4/17/24 22:43, Yunhui Cui wrote: > Before cacheinfo can be built correctly, we need to initialize level > and type. Since RSIC-V currently does not have a register group that > describes cache-related attributes like ARM64, we cannot obtain them > directly, so now we obtain cache leaves from the ACPI PPTT table > (acpi_get_cache_info()) and set the cache type through split_levels. > > Suggested-by: Jeremy Linton <jeremy.linton@arm.com> > Suggested-by: Sudeep Holla <sudeep.holla@arm.com> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > --- > arch/riscv/kernel/cacheinfo.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c > index 30a6878287ad..e47a1e6bd3fe 100644 > --- a/arch/riscv/kernel/cacheinfo.c > +++ b/arch/riscv/kernel/cacheinfo.c > @@ -6,6 +6,7 @@ > #include <linux/cpu.h> > #include <linux/of.h> > #include <asm/cacheinfo.h> > +#include <linux/acpi.h> > > static struct riscv_cacheinfo_ops *rv_cache_ops; > > @@ -78,6 +79,27 @@ int populate_cache_leaves(unsigned int cpu) > struct device_node *prev = NULL; > int levels = 1, level = 1; > > + if (!acpi_disabled) { > + int ret, fw_levels, split_levels; > + > + ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels); > + if (ret) > + return ret; > + > + BUG_ON((split_levels > fw_levels) || > + (split_levels + fw_levels > this_cpu_ci->num_leaves)); > + > + for (; level <= this_cpu_ci->num_levels; level++) { > + if (level <= split_levels) { > + ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level); > + ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level); > + } else { > + ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level); > + } > + } > + return 0; > + } > + > if (of_property_read_bool(np, "cache-size")) > ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level); > if (of_property_read_bool(np, "i-cache-size")) Yes, looks good. Reviewed-by: Jeremy Linton <jeremy.linton@arm.com> Thanks,
Hi Palmer, Gentle ping... On Tue, Apr 23, 2024 at 7:03 PM yunhui cui <cuiyunhui@bytedance.com> wrote: > > Hi Palmer, > > On Fri, Apr 19, 2024 at 11:29 PM Jeremy Linton <jeremy.linton@arm.com> wrote: > > > > Hi, > > > > On 4/17/24 22:43, Yunhui Cui wrote: > > > Before cacheinfo can be built correctly, we need to initialize level > > > and type. Since RSIC-V currently does not have a register group that > > > describes cache-related attributes like ARM64, we cannot obtain them > > > directly, so now we obtain cache leaves from the ACPI PPTT table > > > (acpi_get_cache_info()) and set the cache type through split_levels. > > > > > > Suggested-by: Jeremy Linton <jeremy.linton@arm.com> > > > Suggested-by: Sudeep Holla <sudeep.holla@arm.com> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > > > --- > > > arch/riscv/kernel/cacheinfo.c | 22 ++++++++++++++++++++++ > > > 1 file changed, 22 insertions(+) > > > > > > diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c > > > index 30a6878287ad..e47a1e6bd3fe 100644 > > > --- a/arch/riscv/kernel/cacheinfo.c > > > +++ b/arch/riscv/kernel/cacheinfo.c > > > @@ -6,6 +6,7 @@ > > > #include <linux/cpu.h> > > > #include <linux/of.h> > > > #include <asm/cacheinfo.h> > > > +#include <linux/acpi.h> > > > > > > static struct riscv_cacheinfo_ops *rv_cache_ops; > > > > > > @@ -78,6 +79,27 @@ int populate_cache_leaves(unsigned int cpu) > > > struct device_node *prev = NULL; > > > int levels = 1, level = 1; > > > > > > + if (!acpi_disabled) { > > > + int ret, fw_levels, split_levels; > > > + > > > + ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels); > > > + if (ret) > > > + return ret; > > > + > > > + BUG_ON((split_levels > fw_levels) || > > > + (split_levels + fw_levels > this_cpu_ci->num_leaves)); > > > + > > > + for (; level <= this_cpu_ci->num_levels; level++) { > > > + if (level <= split_levels) { > > > + ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level); > > > + ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level); > > > + } else { > > > + ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level); > > > + } > > > + } > > > + return 0; > > > + } > > > + > > > if (of_property_read_bool(np, "cache-size")) > > > ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level); > > > if (of_property_read_bool(np, "i-cache-size")) > > > > Yes, looks good. > > > > Reviewed-by: Jeremy Linton <jeremy.linton@arm.com> > > > > > > > > Thanks, > > Could you help review this patchset? Thanks. > > Thanks, > Yunhui Thanks, Yunhui
On Thu, Apr 18, 2024 at 11:43:29AM +0800, Yunhui Cui wrote: > Before cacheinfo can be built correctly, we need to initialize level > and type. Since RSIC-V currently does not have a register group that > describes cache-related attributes like ARM64, we cannot obtain them > directly, so now we obtain cache leaves from the ACPI PPTT table > (acpi_get_cache_info()) and set the cache type through split_levels. > > Suggested-by: Jeremy Linton <jeremy.linton@arm.com> > Suggested-by: Sudeep Holla <sudeep.holla@arm.com> : Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> I'm not an ACPI head, so whether or not the table is valid on RISC-V or w/e I do not know, but the code here looks sane to me, so Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor.
Hi Palmer, Gentle ping... On Fri, May 3, 2024 at 12:37 AM Conor Dooley <conor@kernel.org> wrote: > > On Thu, Apr 18, 2024 at 11:43:29AM +0800, Yunhui Cui wrote: > > Before cacheinfo can be built correctly, we need to initialize level > > and type. Since RSIC-V currently does not have a register group that > > describes cache-related attributes like ARM64, we cannot obtain them > > directly, so now we obtain cache leaves from the ACPI PPTT table > > (acpi_get_cache_info()) and set the cache type through split_levels. > > > > Suggested-by: Jeremy Linton <jeremy.linton@arm.com> > > Suggested-by: Sudeep Holla <sudeep.holla@arm.com> > : Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > > I'm not an ACPI head, so whether or not the table is valid on RISC-V or > w/e I do not know, but the code here looks sane to me, so > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > > Cheers, > Conor. Thanks, Yunhui
On Wed, 08 May 2024 04:19:01 PDT (-0700), cuiyunhui@bytedance.com wrote: > Hi Palmer, > > Gentle ping... +Sunil, as he's the ACPI/RISC-V maintainer and I generally wait for his review on this stuff. > > On Fri, May 3, 2024 at 12:37 AM Conor Dooley <conor@kernel.org> wrote: >> >> On Thu, Apr 18, 2024 at 11:43:29AM +0800, Yunhui Cui wrote: >> > Before cacheinfo can be built correctly, we need to initialize level >> > and type. Since RSIC-V currently does not have a register group that >> > describes cache-related attributes like ARM64, we cannot obtain them >> > directly, so now we obtain cache leaves from the ACPI PPTT table >> > (acpi_get_cache_info()) and set the cache type through split_levels. >> > >> > Suggested-by: Jeremy Linton <jeremy.linton@arm.com> >> > Suggested-by: Sudeep Holla <sudeep.holla@arm.com> >> : Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> >> >> I'm not an ACPI head, so whether or not the table is valid on RISC-V or >> w/e I do not know, but the code here looks sane to me, so > >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com> >> >> Cheers, >> Conor. > > Thanks, > Yunhui
On Thu, Apr 18, 2024 at 11:43:29AM +0800, Yunhui Cui wrote: > Before cacheinfo can be built correctly, we need to initialize level > and type. Since RSIC-V currently does not have a register group that NIT: Typo RISC-V > describes cache-related attributes like ARM64, we cannot obtain them > directly, so now we obtain cache leaves from the ACPI PPTT table > (acpi_get_cache_info()) and set the cache type through split_levels. > > Suggested-by: Jeremy Linton <jeremy.linton@arm.com> > Suggested-by: Sudeep Holla <sudeep.holla@arm.com> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > --- > arch/riscv/kernel/cacheinfo.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c > index 30a6878287ad..e47a1e6bd3fe 100644 > --- a/arch/riscv/kernel/cacheinfo.c > +++ b/arch/riscv/kernel/cacheinfo.c > @@ -6,6 +6,7 @@ > #include <linux/cpu.h> > #include <linux/of.h> > #include <asm/cacheinfo.h> > +#include <linux/acpi.h> > Can this be added in the order? Like, include acpi.h prior to cpu.h? > static struct riscv_cacheinfo_ops *rv_cache_ops; > > @@ -78,6 +79,27 @@ int populate_cache_leaves(unsigned int cpu) > struct device_node *prev = NULL; > int levels = 1, level = 1; > > + if (!acpi_disabled) { > + int ret, fw_levels, split_levels; > + > + ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels); > + if (ret) > + return ret; > + > + BUG_ON((split_levels > fw_levels) || > + (split_levels + fw_levels > this_cpu_ci->num_leaves)); > + > + for (; level <= this_cpu_ci->num_levels; level++) { > + if (level <= split_levels) { > + ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level); > + ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level); > + } else { > + ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level); > + } > + } > + return 0; > + } > + Other than above nits, it looks good to me. Thanks for the patch! Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>
Hi Sunil, On Thu, May 9, 2024 at 12:09 PM Sunil V L <sunilvl@ventanamicro.com> wrote: > > On Thu, Apr 18, 2024 at 11:43:29AM +0800, Yunhui Cui wrote: > > Before cacheinfo can be built correctly, we need to initialize level > > and type. Since RSIC-V currently does not have a register group that > > NIT: Typo RISC-V Okay, I'll update it in v5. > > > describes cache-related attributes like ARM64, we cannot obtain them > > directly, so now we obtain cache leaves from the ACPI PPTT table > > (acpi_get_cache_info()) and set the cache type through split_levels. > > > > Suggested-by: Jeremy Linton <jeremy.linton@arm.com> > > Suggested-by: Sudeep Holla <sudeep.holla@arm.com> > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > > --- > > arch/riscv/kernel/cacheinfo.c | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c > > index 30a6878287ad..e47a1e6bd3fe 100644 > > --- a/arch/riscv/kernel/cacheinfo.c > > +++ b/arch/riscv/kernel/cacheinfo.c > > @@ -6,6 +6,7 @@ > > #include <linux/cpu.h> > > #include <linux/of.h> > > #include <asm/cacheinfo.h> > > +#include <linux/acpi.h> > > > Can this be added in the order? Like, include acpi.h prior to cpu.h? Okay, I'll update it in v5. > > > static struct riscv_cacheinfo_ops *rv_cache_ops; > > > > @@ -78,6 +79,27 @@ int populate_cache_leaves(unsigned int cpu) > > struct device_node *prev = NULL; > > int levels = 1, level = 1; > > > > + if (!acpi_disabled) { > > + int ret, fw_levels, split_levels; > > + > > + ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels); > > + if (ret) > > + return ret; > > + > > + BUG_ON((split_levels > fw_levels) || > > + (split_levels + fw_levels > this_cpu_ci->num_leaves)); > > + > > + for (; level <= this_cpu_ci->num_levels; level++) { > > + if (level <= split_levels) { > > + ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level); > > + ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level); > > + } else { > > + ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level); > > + } > > + } > > + return 0; > > + } > > + > Other than above nits, it looks good to me. Thanks for the patch! > > Reviewed-by: Sunil V L <sunilvl@ventanamicro.com> Thanks, Yunhui
diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c index 30a6878287ad..e47a1e6bd3fe 100644 --- a/arch/riscv/kernel/cacheinfo.c +++ b/arch/riscv/kernel/cacheinfo.c @@ -6,6 +6,7 @@ #include <linux/cpu.h> #include <linux/of.h> #include <asm/cacheinfo.h> +#include <linux/acpi.h> static struct riscv_cacheinfo_ops *rv_cache_ops; @@ -78,6 +79,27 @@ int populate_cache_leaves(unsigned int cpu) struct device_node *prev = NULL; int levels = 1, level = 1; + if (!acpi_disabled) { + int ret, fw_levels, split_levels; + + ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels); + if (ret) + return ret; + + BUG_ON((split_levels > fw_levels) || + (split_levels + fw_levels > this_cpu_ci->num_leaves)); + + for (; level <= this_cpu_ci->num_levels; level++) { + if (level <= split_levels) { + ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level); + ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level); + } else { + ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level); + } + } + return 0; + } + if (of_property_read_bool(np, "cache-size")) ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level); if (of_property_read_bool(np, "i-cache-size"))
Before cacheinfo can be built correctly, we need to initialize level and type. Since RSIC-V currently does not have a register group that describes cache-related attributes like ARM64, we cannot obtain them directly, so now we obtain cache leaves from the ACPI PPTT table (acpi_get_cache_info()) and set the cache type through split_levels. Suggested-by: Jeremy Linton <jeremy.linton@arm.com> Suggested-by: Sudeep Holla <sudeep.holla@arm.com> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> --- arch/riscv/kernel/cacheinfo.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)