Re: [PATCH v3 -next 2/3] RISC-V: ACPI: RHCT: Add function to get CBO block sizes

[Andrew Jones]

On Mon, Oct 16, 2023 at 10:19:57PM +0530, Sunil V L wrote:
> Cache Block Operation (CBO) related block size in ACPI is provided by RHCT.
> Add support to read the CMO node in RHCT to get this information.
> 
> Signed-off-by: Sunil V L <sunilvl@xxxxxxxxxxxxxxxx>
> ---
>  arch/riscv/include/asm/acpi.h |  6 +++
>  drivers/acpi/riscv/rhct.c     | 93 +++++++++++++++++++++++++++++++++++
>  2 files changed, 99 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> index d5604d2073bc..7dad0cf9d701 100644
> --- a/arch/riscv/include/asm/acpi.h
> +++ b/arch/riscv/include/asm/acpi.h
> @@ -66,6 +66,8 @@ int acpi_get_riscv_isa(struct acpi_table_header *table,
>  		       unsigned int cpu, const char **isa);
>  
>  static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
> +void acpi_get_cbo_block_size(struct acpi_table_header *table, u32 *cbom_size,
> +			     u32 *cboz_size, u32 *cbop_size);
>  #else
>  static inline void acpi_init_rintc_map(void) { }
>  static inline struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> @@ -79,6 +81,10 @@ static inline int acpi_get_riscv_isa(struct acpi_table_header *table,
>  	return -EINVAL;
>  }
>  
> +static inline void acpi_get_cbo_block_size(struct acpi_table_header *table,
> +					   u32 *cbom_size, u32 *cboz_size,
> +					   u32 *cbop_size) { }
> +
>  #endif /* CONFIG_ACPI */
>  
>  #endif /*_ASM_ACPI_H*/
> diff --git a/drivers/acpi/riscv/rhct.c b/drivers/acpi/riscv/rhct.c
> index b280b3e9c7d9..105f1aaa3fac 100644
> --- a/drivers/acpi/riscv/rhct.c
> +++ b/drivers/acpi/riscv/rhct.c
> @@ -8,6 +8,7 @@
>  #define pr_fmt(fmt)     "ACPI: RHCT: " fmt
>  
>  #include <linux/acpi.h>
> +#include <linux/bits.h>
>  
>  static struct acpi_table_header *acpi_get_rhct(void)
>  {
> @@ -81,3 +82,95 @@ int acpi_get_riscv_isa(struct acpi_table_header *table, unsigned int cpu, const
>  
>  	return -1;
>  }
> +
> +static void acpi_parse_hart_info_cmo_node(struct acpi_table_rhct *rhct,
> +					  struct acpi_rhct_hart_info *hart_info,
> +					  u32 *cbom_size, u32 *cboz_size, u32 *cbop_size)
> +{
> +	u32 size_hartinfo = sizeof(struct acpi_rhct_hart_info);
> +	u32 size_hdr = sizeof(struct acpi_rhct_node_header);
> +	struct acpi_rhct_node_header *ref_node;
> +	struct acpi_rhct_cmo_node *cmo_node;
> +	u32 *hart_info_node_offset;
> +
> +	hart_info_node_offset = ACPI_ADD_PTR(u32, hart_info, size_hartinfo);
> +	for (int i = 0; i < hart_info->num_offsets; i++) {
> +		ref_node = ACPI_ADD_PTR(struct acpi_rhct_node_header,
> +					rhct, hart_info_node_offset[i]);
> +		if (ref_node->type == ACPI_RHCT_NODE_TYPE_CMO) {
> +			cmo_node = ACPI_ADD_PTR(struct acpi_rhct_cmo_node,
> +						ref_node, size_hdr);
> +			if (cbom_size && cmo_node->cbom_size <= 30) {
> +				if (!*cbom_size) {
> +					*cbom_size = BIT(cmo_node->cbom_size);
> +				} else if (*cbom_size !=
> +						BIT(cmo_node->cbom_size)) {

No need to break the if line, we can go to 100 chars. And then, since both
the if and else if arms only have single statements, all the {} can be
dropped too. Same comment for cboz and cbop.

> +					pr_warn("CBOM size is not the same across harts\n");
> +				}
> +			}
> +
> +			if (cboz_size && cmo_node->cboz_size <= 30) {
> +				if (!*cboz_size) {
> +					*cboz_size = BIT(cmo_node->cboz_size);
> +				} else if (*cboz_size !=
> +						BIT(cmo_node->cboz_size)) {
> +					pr_warn("CBOZ size is not the same across harts\n");
> +				}
> +			}
> +
> +			if (cbop_size && cmo_node->cbop_size <= 30) {
> +				if (!*cbop_size) {
> +					*cbop_size = BIT(cmo_node->cbop_size);
> +				} else if (*cbop_size !=
> +						BIT(cmo_node->cbop_size)) {
> +					pr_warn("CBOP size is not the same across harts\n");
> +				}
> +			}
> +		}
> +	}
> +}
> +
> +/*
> + * During early boot, the caller should call acpi_get_table() and pass its pointer to
> + * these functions(and free up later). At run time, since this table can be used
                     ^ add a space here

> + * multiple times, pass NULL so that the table remains in memory
> + */
> +void acpi_get_cbo_block_size(struct acpi_table_header *table, u32 *cbom_size,
> +			     u32 *cboz_size, u32 *cbop_size)
> +{
> +	u32 size_hdr = sizeof(struct acpi_rhct_node_header);
> +	struct acpi_rhct_node_header *node, *end;
> +	struct acpi_rhct_hart_info *hart_info;
> +	struct acpi_table_rhct *rhct;
> +
> +	if (acpi_disabled)
> +		return;
> +
> +	if (table) {
> +		rhct = (struct acpi_table_rhct *)table;
> +	} else {
> +		rhct = (struct acpi_table_rhct *)acpi_get_rhct();

Not an issue of this patch, but it seems like acpi_get_rhct() should
return a struct acpi_table_rhct pointer instead of a struct
acpi_table_header pointer since it's specifically returning an RHCT.

> +		if (!rhct)
> +			return;
> +	}
> +
> +	if (cbom_size)
> +		*cbom_size = 0;
> +
> +	if (cboz_size)
> +		*cboz_size = 0;
> +
> +	if (cbop_size)
> +		*cbop_size = 0;
> +
> +	end = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->header.length);
> +	for (node = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->node_offset);
> +	     node < end;
> +	     node = ACPI_ADD_PTR(struct acpi_rhct_node_header, node, node->length)) {
> +		if (node->type == ACPI_RHCT_NODE_TYPE_HART_INFO) {
> +			hart_info = ACPI_ADD_PTR(struct acpi_rhct_hart_info, node, size_hdr);
> +			acpi_parse_hart_info_cmo_node(rhct, hart_info, cbom_size,
> +						      cboz_size, cbop_size);
> +		}
> +	}
> +}
> -- 
> 2.39.2
>

Other than the nits

Reviewed-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>

Thanks,
drew