diff mbox series

[RFC,v1,09/21] RISC-V: cacheflush: Initialize CBO variables on ACPI systems

Message ID 20230803175916.3174453-10-sunilvl@ventanamicro.com
State New
Headers
Series RISC-V: ACPI: Add external interrupt controller support |

Commit Message

Sunil V L Aug. 3, 2023, 5:59 p.m. UTC 
  Using new interface to get the CBO block size information in
RHCT, initialize the variables on ACPI platforms.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/mm/cacheflush.c | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

Comments

Andy Shevchenko Aug. 4, 2023, 5:56 a.m. UTC | #1 
On Thu, Aug 03, 2023 at 11:29:04PM +0530, Sunil V L wrote:
> Using new interface to get the CBO block size information in
> RHCT, initialize the variables on ACPI platforms.

...

>  #include <linux/of.h>
> +#include <linux/acpi.h>

Can you keep it sorted (to some extent)?

> +#include <asm/acpi.h>

What do you need this for?

>  #include <asm/cacheflush.h>
Sunil V L Aug. 4, 2023, 9:20 a.m. UTC | #2 
On Fri, Aug 04, 2023 at 08:56:29AM +0300, Andy Shevchenko wrote:
> On Thu, Aug 03, 2023 at 11:29:04PM +0530, Sunil V L wrote:
> > Using new interface to get the CBO block size information in
> > RHCT, initialize the variables on ACPI platforms.
> 
> ...
> 
> >  #include <linux/of.h>
> > +#include <linux/acpi.h>
> 
> Can you keep it sorted (to some extent)?
> 
Sure.

> > +#include <asm/acpi.h>
> 
> What do you need this for?
> 
> >  #include <asm/cacheflush.h>
> 
When CONFIG_ACPI is disabled, this include is required to get
acpi_get_cbo_block_size().

Thanks,
Sunil
Andy Shevchenko Aug. 4, 2023, 2:59 p.m. UTC | #3 
On Fri, Aug 04, 2023 at 02:50:34PM +0530, Sunil V L wrote:
> On Fri, Aug 04, 2023 at 08:56:29AM +0300, Andy Shevchenko wrote:
> > On Thu, Aug 03, 2023 at 11:29:04PM +0530, Sunil V L wrote:
> > > Using new interface to get the CBO block size information in
> > > RHCT, initialize the variables on ACPI platforms.

...

> > > +#include <asm/acpi.h>
> > 
> > What do you need this for?
> > 
> > >  #include <asm/cacheflush.h>
> > 
> When CONFIG_ACPI is disabled, this include is required to get
> acpi_get_cbo_block_size().

How is it useful without ACPI being enabled?  If it's indeed
(in which I do not believe), better to make sure you have it
avaiable independently on CONFIG_ACPI. Otherwise, just put
#ifdef CONFIG_ACPI around the call.
Conor Dooley Aug. 4, 2023, 3:19 p.m. UTC | #4 
On Fri, Aug 04, 2023 at 05:59:51PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 04, 2023 at 02:50:34PM +0530, Sunil V L wrote:
> > On Fri, Aug 04, 2023 at 08:56:29AM +0300, Andy Shevchenko wrote:
> > > On Thu, Aug 03, 2023 at 11:29:04PM +0530, Sunil V L wrote:
> > > > Using new interface to get the CBO block size information in
> > > > RHCT, initialize the variables on ACPI platforms.
> 
> ...
> 
> > > > +#include <asm/acpi.h>
> > > 
> > > What do you need this for?
> > > 
> > > >  #include <asm/cacheflush.h>
> > > 
> > When CONFIG_ACPI is disabled, this include is required to get
> > acpi_get_cbo_block_size().
> 
> How is it useful without ACPI being enabled?

It is not, as evidenced by the `return -EINVAL;`.

> If it's indeed
> (in which I do not believe), better to make sure you have it
> avaiable independently on CONFIG_ACPI. Otherwise, just put
> #ifdef CONFIG_ACPI around the call.

Let's not litter the code with ifdeffery please where it can be easily
avoided.
Andy Shevchenko Aug. 4, 2023, 4:52 p.m. UTC | #5 
On Fri, Aug 04, 2023 at 04:19:27PM +0100, Conor Dooley wrote:
> On Fri, Aug 04, 2023 at 05:59:51PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 04, 2023 at 02:50:34PM +0530, Sunil V L wrote:
> > > On Fri, Aug 04, 2023 at 08:56:29AM +0300, Andy Shevchenko wrote:
> > > > On Thu, Aug 03, 2023 at 11:29:04PM +0530, Sunil V L wrote:

...

> > > > > +#include <asm/acpi.h>
> > > > 
> > > > What do you need this for?
> > > > 
> > > > >  #include <asm/cacheflush.h>
> > > > 
> > > When CONFIG_ACPI is disabled, this include is required to get
> > > acpi_get_cbo_block_size().
> > 
> > How is it useful without ACPI being enabled?
> 
> It is not, as evidenced by the `return -EINVAL;`.
> 
> > If it's indeed
> > (in which I do not believe), better to make sure you have it
> > avaiable independently on CONFIG_ACPI. Otherwise, just put
> > #ifdef CONFIG_ACPI around the call.
> 
> Let's not litter the code with ifdeffery please where it can be easily
> avoided.

Including asm/acpi.h looks to me as a "let's avoid it with a hack that it
is uglier than ifdeffery". Sorry, but ifdeffery for ACPI, with all my full
agreement with the statement that it's not good, is the correct way to fix
this.
Andy Shevchenko Aug. 4, 2023, 4:56 p.m. UTC | #6 
On Fri, Aug 04, 2023 at 07:52:56PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 04, 2023 at 04:19:27PM +0100, Conor Dooley wrote:
> > On Fri, Aug 04, 2023 at 05:59:51PM +0300, Andy Shevchenko wrote:
> > > On Fri, Aug 04, 2023 at 02:50:34PM +0530, Sunil V L wrote:
> > > > On Fri, Aug 04, 2023 at 08:56:29AM +0300, Andy Shevchenko wrote:
> > > > > On Thu, Aug 03, 2023 at 11:29:04PM +0530, Sunil V L wrote:

...

> > > > > > +#include <asm/acpi.h>
> > > > > 
> > > > > What do you need this for?
> > > > > 
> > > > > >  #include <asm/cacheflush.h>
> > > > > 
> > > > When CONFIG_ACPI is disabled, this include is required to get
> > > > acpi_get_cbo_block_size().
> > > 
> > > How is it useful without ACPI being enabled?
> > 
> > It is not, as evidenced by the `return -EINVAL;`.
> > 
> > > If it's indeed
> > > (in which I do not believe), better to make sure you have it
> > > avaiable independently on CONFIG_ACPI. Otherwise, just put
> > > #ifdef CONFIG_ACPI around the call.
> > 
> > Let's not litter the code with ifdeffery please where it can be easily
> > avoided.
> 
> Including asm/acpi.h looks to me as a "let's avoid it with a hack that it
> is uglier than ifdeffery". Sorry, but ifdeffery for ACPI, with all my full
> agreement with the statement that it's not good, is the correct way to fix
> this.

On the other hand this is an arch code and I see precedents of using the
headers together, alas, it seems not better to me that ugly ifdeffery.

So, I leave it to the respective maintainers to decide.
diff mbox series

Patch

diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
index fbc59b3f69f2..63bb56819b37 100644
--- a/arch/riscv/mm/cacheflush.c
+++ b/arch/riscv/mm/cacheflush.c
@@ -4,6 +4,8 @@ 
  */
 
 #include <linux/of.h>
+#include <linux/acpi.h>
+#include <asm/acpi.h>
 #include <asm/cacheflush.h>
 
 #ifdef CONFIG_SMP
@@ -131,15 +133,38 @@  void __init riscv_init_cbo_blocksizes(void)
 	unsigned long cbom_hartid, cboz_hartid;
 	u32 cbom_block_size = 0, cboz_block_size = 0;
 	struct device_node *node;
+	struct acpi_table_header *rhct;
+	acpi_status status;
+	unsigned int cpu;
+
+	if (!acpi_disabled) {
+		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
+		if (ACPI_FAILURE(status))
+			return;
+	}
 
-	for_each_of_cpu_node(node) {
-		/* set block-size for cbom and/or cboz extension if available */
-		cbo_get_block_size(node, "riscv,cbom-block-size",
-				   &cbom_block_size, &cbom_hartid);
-		cbo_get_block_size(node, "riscv,cboz-block-size",
-				   &cboz_block_size, &cboz_hartid);
+	for_each_possible_cpu(cpu) {
+		if (acpi_disabled) {
+			node = of_cpu_device_node_get(cpu);
+			if (!node) {
+				pr_warn("Unable to find cpu node\n");
+				continue;
+			}
+
+			/* set block-size for cbom and/or cboz extension if available */
+			cbo_get_block_size(node, "riscv,cbom-block-size",
+					   &cbom_block_size, &cbom_hartid);
+			cbo_get_block_size(node, "riscv,cboz-block-size",
+					   &cboz_block_size, &cboz_hartid);
+		} else {
+			acpi_get_cbo_block_size(rhct, cpu, &cbom_block_size,
+						&cboz_block_size, NULL);
+		}
 	}
 
+	if (!acpi_disabled && rhct)
+		acpi_put_table((struct acpi_table_header *)rhct);
+
 	if (cbom_block_size)
 		riscv_cbom_block_size = cbom_block_size;