RE: [PATCH RFC] soc: fujitsu: Add cache driver code

["tan.shaopeng@fujitsu.com" <tan.shaopeng@fujitsu.com>]

Hi, 
 
Thanks for your comments

> On Wed, Mar 3, 2021 at 10:38 AM tan.shaopeng
> <tan.shaopeng@jp.fujitsu.com> wrote:
> > +
> > +config FUJITSU_CACHE
> > +        tristate "FUJITSU Cache Driver"
> > +        depends on ARM64_VHE || COMPILE_TEST
> > +        help
> > +          FUJITSU Cache Driver
> > +
> > +          This driver offers cache functions for A64FX system.
> > +         Loading this cache driver, control registers will be set to enable
> > +         these functions, and advanced settings registers will be set by
> default
> > +         values. After loading this driver, you can use the default values of
> the
> > +         advanced settings registers or set the advanced settings registers
> > +         from EL0. Unloading this driver, control registers will be clear to
> > +         disable these functions.
> > +          When built as a module, this will be called as "fujitsu_cache".
> 
> My feeling is that this code should be in arch/arm64/, as the cache
> is generally considered part of the CPU, rather than part of the wider
> SoC design, or something that can be controlled separately from the
> core kernel and memory management code.

Thanks for your advice. I also would like to hear the opinions from 
other soc&arm maintainers, and then consider whether to add this to 
arch/arm64/. 

> > +module_param(tagaddr_ctrl_reg, ulong, 0444);
> > +MODULE_PARM_DESC(tagaddr_ctrl_reg, "HPC tag address override
> control register");
> > +module_param(hwpf_ctrl_reg, ulong, 0444);
> > +MODULE_PARM_DESC(hwpf_ctrl_reg, "hardware prefetch assist control
> register");
> > +module_param(sec_ctrl_reg, ulong, 0444);
> > +MODULE_PARM_DESC(sec_ctrl_reg, "sector cache control register");
> > +module_param(sec_assign_reg, ulong, 0444);
> > +MODULE_PARM_DESC(sec_assign_reg, "sector cache assign register");
> > +module_param(sec_set0_l2_reg, ulong, 0444);
> > +MODULE_PARM_DESC(sec_set0_l2_reg, "sector cache L2 way
> register(sector=0,1)");
> > +module_param(sec_set1_l2_reg, ulong, 0444);
> > +MODULE_PARM_DESC(sec_set1_l2_reg, "sector cache L2 way
> register(sector=2,3)");
> 
> My feeling is that the actual settings need to be on a higher level, not tied
> to the specific register-level implementation of this chip. Normally,  the L2
> cache is set up by the firmware according to local policy, and the settings
> can either be read by the kernel from registers or passed down through the
> device tree. It sounds like you want to control the policy at runtime in the
> operating system rather than at boot time, so for each setting you wish to
> override, there should be description of what the setting does and what
> the purpose of overriding the firmware setting is.

OK, I will change module parameter from specific register-level to 
a higher level. And, I will modify the description of module parameters. 
To be clear, we don't suppose these parameters (EL1 registers) are 
often changed at runtime.

> Looking at the first one in the list, I see the specification mentions
> multiple distinct features that can be enabled or disabled, so these
> should probably get controlled individually.

It is not necessary to enable/disable every feature individually. 
There are no plans to use these features individually.  

> I also see that it is possible
> to control this for TTBR1 and TTBR0 separately, and we probably
> cannot allow user space (through module parameters or any other
> interface) to control TTBR1, which is where the kernel resides.

This driver does not change the values of TTBR0 and TTBR1, 
and the values of TCR_EL1.TBI0 and TCR_EL1.TBI1.
The cache functions can be used when TBI is already enabled.

> The TTBR0 settings in turn would seem to interact with
> CONFIG_ARM64_MTE, and should not be controlled independently
> but through the same interfaces as that if we find that it does need
> to be controlled at all.

MTE is not supported on A64FX. 
So, this function does not conflict with MTE.

> I have not looked at any further details, but that should help get an
> idea of what I think would happen with the other registers.
> 
> > +static int __init fujitsu_drv_init(void)
> > +{
> > +       int ret;
> > +
> > +       if (read_cpuid_implementor() != ARM_CPU_IMP_FUJITSU)
> > +               return -ENODEV;
> > +       if (read_cpuid_part_number() != FUJITSU_CPU_PART_A64FX)
> > +               return -ENODEV;
> 
> The module_init function should not return an error when it is running on
> incompatible hardware, please just change this to silently return success
> to avoid warning about the failed initcall if the driver is built into a generic
> kernel.

OK, I will change these codes to return success. 

Best regards, 
Tan Shaopeng  

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel