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

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Guillaume Tucker <guillaume.tucker@collabora.com>
Cc: linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Kukjin Kim <kgene@kernel.org>,
	kernel@collabora.com, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] ARM: exynos: update l2c_aux_mask to fix alert message
Date: Thu, 2 Apr 2020 14:03:52 +0100	[thread overview]
Message-ID: <20200402130352.GY25745@shell.armlinux.org.uk> (raw)
In-Reply-To: <35c7cf4b-e6b8-43aa-d934-4a1c2e738372@collabora.com>

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.

-- 
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

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