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
