Re: [PATCH] ARM: exynos: update l2c_aux_mask to fix alert message

[Russell King - ARM Linux admin]

On Thu, Apr 02, 2020 at 02:03:52PM +0100, Russell King - ARM Linux admin wrote:
> On Thu, Apr 02, 2020 at 01:13:24PM +0100, Guillaume Tucker wrote:
> > On 01/04/2020 17:31, Russell King - ARM Linux admin wrote:
> > > On Wed, Apr 01, 2020 at 05:08:03PM +0100, Guillaume Tucker wrote:
> > >> Allow setting the number of cycles for RAM reads in the pl310 cache
> > >> controller L2 auxiliary control register mask (bits 0-2) since it
> > >> needs to be changed in software.  This only affects exynos4210 and
> > >> exynos4412 as they use the pl310 cache controller.
> > >>
> > >> With the mask used until now, the following warnings were generated,
> > >> the 2nd one being a pr_alert():
> > >>
> > >>   L2C: platform modifies aux control register: 0x02070000 -> 0x3e470001
> > >>   L2C: platform provided aux values permit register corruption.
> > >>
> > >> This latency cycles value has always been set in software in spite of
> > >> the warnings.  Keep it this way but clear the alert message about
> > >> register corruption to acknowledge it is a valid thing to do.
> > > 
> > > This is telling you that you are doing something you should not be
> > > doing.  The L2C controller should be configured by board firmware
> > > first and foremost, because if, for example, u-boot makes use of the
> > > L2 cache, or any other pre-main kernel code (in other words,
> > > decompressor) the setup of the L2 controller will be wrong.
> > > 
> > > So, NAK.
> > 
> > OK thanks, I guess I misinterpreted the meaning of the error
> > message.  It's really saying that the register value was not the
> > right one before the kernel tried to change it.  Next step for me
> > is to look into U-Boot.
> 
> The message "L2C: platform provided aux values permit register
> corruption." means that bits are set in both the mask and the value
> fields.  Since the new value is calculated as:
> 
> 	old = register value;
> 	new = old & mask;
> 	new |= val;
> 
> If bits are set in both "mask" and "val" for a multi-bit field, the
> value ending up in the field may not be what is intended.  Consider
> a 5-bit field set initially to 10101, and the requested value is
> 01000 with a mask of 11111.  What you end up with is not 01000, but
> 11101.  Hence, register corruption.  It is not possible to easily
> tell whether the mask and values refer to a multi-bit field or not,
> so the mere fact that bits are set in both issues the alert.
> 
> The message "L2C: platform modifies aux control register ..." means
> that you're trying to modify the value of the auxiliary control
> register, which brings with it the problems I stated in my previous
> email; platform configuration of the L2C must be done by firmware and
> not the kernel for the reasons I've set out.

Actually, looking at the values there:

        .l2c_aux_val    = 0x3c400001,
-       .l2c_aux_mask   = 0xc20fffff,
+       .l2c_aux_mask   = 0xc20ffff8,

Bit 0 is L310_AUX_CTRL_FULL_LINE_ZERO feature bit, which platforms have
no business fiddling with - it is a Cortex-A9/L2C310 specific feature
that needs both ends to be configured correctly to work.  The L2C code
knows this and will deal with it.  So, .l2c_aux_val should drop setting
bit 0.

It's also setting L310_AUX_CTRL_NS_LOCKDOWN, which the kernel already
deals with - this bit should be dropped as well.

It's clearing L310_AUX_CTRL_CACHE_REPLACE_RR - this should be setup by
firmware.

For the prefetching, I thought there were DT properties for that.
Please look at that, and see whether you can eliminate most of the
.l2c_aux_val field set bits, and the .l2c_aux_mask clear bits.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up