Re: [PATCH v17 2/7] firmware: arm_rmm: Check for RMI support at init

[Suzuki K Poulose]

On 08/09/2026 07:46, Gavin Shan wrote:
> Hi Suzuki,
>
> On 9/7/26 7:59 PM, Suzuki K Poulose wrote:
> > From: Steven Price <steven.price@xxxxxxx>
> >
> > Query the RMI version number and check if it is a compatible version.
> > The first two feature registers are read and exposed for future code to
> > use.
> >
> > Signed-off-by: Steven Price <steven.price@xxxxxxx>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>

...

> > diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/ 
> > arm_rmm/rmi.c
> > new file mode 100644
> > index 0000000000000..008a783407b4e
> > --- /dev/null
> > +++ b/drivers/firmware/arm_rmm/rmi.c
> > @@ -0,0 +1,101 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2023-2026 ARM Ltd.
> > + */
> > +
> > +#include <linux/cpufeature.h>
> > +#include <linux/memblock.h>
> > +#include <linux/arm-rmi-cmds.h>
> > +#include <linux/slab.h>
> > +
> > +#include <asm/memory.h>
> > +#include <asm/pgtable-hwdef.h>
> > +
> > +/* Currently only the first 2 registers are used by Linux */
> > +#define RMI_FEAT_REG_COUNT    2
> > +static __ro_after_init unsigned long 
> > rmi_feat_reg_cache[RMI_FEAT_REG_COUNT];
> > +
>
> I would drop RMI_FEAT_REG_COUNT and use ARRAY_SIZE(rmi_feat_reg_cache) 
> in the code.
> Besides, __ro_after_init is usually put at the last the declaration.
>
> static unsigned long rmi_feat_reg_cache[RMI_FEAT_REG_COUNT] 
> __ro_after_init;
>
> > +unsigned long rmi_feat_reg(unsigned long id)
> > +{
> > +    if (WARN_ON(id >= RMI_FEAT_REG_COUNT))
> > +        return 0;
> > +
> > +    return rmi_feat_reg_cache[id];
> > +}
> > +EXPORT_SYMBOL_GPL(rmi_feat_reg);
> > +
>
> I would suggest to rename 'id' to 'index' since it represents the feature
> register index instead of feature register ID.
>
> unsigned long rmi_feat_reg(unsigned long index)
> {
>     :
> }
> EXPORT_SYMBOL_GPL(rmi_feat_reg);


> > +/*
> > + * Note arm64_init_rmi() must be called before kvm_init_rmi() 
> > otherwise KVM
> > + * will not support realm guests. subsys_initcall() is called before
> > + * module_init() (used for KVM) so this is OK.
> > + */
> > +subsys_initcall(arm64_init_rmi);
> > diff --git a/include/linux/arm-rmi-cmds.h b/include/linux/arm-rmi-cmds.h
> > new file mode 100644
> > index 0000000000000..2fb1f7f86d71e

> > +static inline int rmi_features(unsigned long index, unsigned long *out)
> > +{
> > +    struct arm_smccc_res res;
> > +
> > +    arm_smccc_1_1_invoke(SMC_RMI_FEATURES, index, &res);
> > +
> > +    if (res.a0 == RMI_SUCCESS && out)
> > +        *out = res.a1;
> > +
> > +    return res.a0;
> > +}
> > +
>
> We needn't expose rmi_features() through include/linux/arm-rmi-cmds.h 
> since it's
> used for once in rmi.c::rmi_read_features(). I would suggest to drop it 
> by combining
> the code with that function.

Thanks for the review, I will address them in the next version.

Cheers
Suzuki