Re: [PATCH v3 -next 2/3] RISC-V: ACPI: RHCT: Add function to get CBO block sizes
From: Sunil V L
Date: Tue Oct 17 2023 - 11:39:36 EST
On Tue, Oct 17, 2023 at 10:37:41AM +0200, Andrew Jones wrote:
> 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.
>
Yeah, it is a side effect of working on repos with different coding
standards. It is interesting that checkpatch didn't recommend to remove
the braces. Let me fix it in next revision. Thanks!.
> > + 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.
>
Makes sense. Let me add a patch to improve this.
> > + 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 a lot for the review!
Sunil