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

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

> On Thu, Mar 4, 2021 at 11:34 AM tan.shaopeng@fujitsu.com
> <tan.shaopeng@fujitsu.com> wrote:
> > > On Wed, Mar 3, 2021 at 10:38 AM tan.shaopeng
> > > <tan.shaopeng@jp.fujitsu.com> wrote:
> 
> > > > +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.
> 
> Ok, in that case you probably want a smaller number of global
> settings can describe all the combinations one may need in
> practice.

Thanks for your advice. 
I will consider the optimal module parameters if we end up make a driver.

> > > 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.
> 
> But could you guarantee that no future generation might support
> both features? In either case, it sounds like the kernel would need
> to know when this is enabled in the same way: as far as I understand,
> both of these put extra information into the upper eight bits of a
> pointer, and any code that interacts with user addresses would need
> to know about this.

I think these functions and MTE cannot be enabled 
at the same time on hardware level.

Best regards,
Tan Shaopeng