[U-Boot] Problems with D-cache invalidation in Freescale eSDHC driver

[Mario Six]

Hi York,

Quoting york sun <york.sun at nxp.com>:

> On 03/21/2016 03:11 AM, Peng Fan wrote:
>> Hi Maro,
>>
>> +More people. There maybe more ideas about this.
>>
>> On Mon, Mar 21, 2016 at 10:32:46AM +0100, mario.six at gdsys.cc wrote:
>>> Hi Peng,
>>>
>>> Quoting Peng Fan <van.freenix at gmail.com>:
>>>
>>>> Hi Mario,
>>>>
>>>> On Fri, Mar 18, 2016 at 09:16:48AM +0100, mario.six at gdsys.cc wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> I've been working on a QorIQ P1022 board (controlcenterd) to run  
>>>>> the newest
>>>>> U-Boot on it, and I encountered some strange behavior.
>>>>>
>>>>> During boot, we get these error messages
>>>>>
>>>>> "
>>>>> ERROR: Cannot import environment: errno = 12
>>>>>
>>>>> at common/env_common.c:221/env_import()
>>>>> *** Warning - import failed, using default environment
>>>>>
>>>>> ERROR: Environment import failed: errno = 12
>>>>>
>>>>> at common/env_common.c:123/set_default_env()
>>>>> ## Can't malloc 9 bytes
>>>>> "
>>>>>
>>>>> After bisecting, I found out that the commit 4683b22 (mmc:fsl_esdhc
>>>>> invalidate dcache before read) apparently causes this. With the  
>>>>> additional
>>>>> d-cache invalidations in place, malloc and free calls fail.
>>>>>
>>>>> Now, after some experimenting, I found out that deactivating the d-cache
>>>>> invalidation when reading the first sector is enough; that is, if one
>>>>> replaces the first invalidation
>>>>>
>>>>> "
>>>>> if (data->flags & MMC_DATA_READ)
>>>>> 	check_and_invalidate_dcache_range(cmd, data);
>>>>> "
>>>>>
>>>>> with
>>>>>
>>>>> "
>>>>> if (data->flags & MMC_DATA_READ && (cmd->cmdidx !=  
>>>>> MMC_CMD_READ_SINGLE_BLOCK
>>>>> || cmd->cmdarg != 0))
>>>>> 	check_and_invalidate_dcache_range(cmd, data);
>>>>> "
>>>>>
>>>>> in fsl_esdhc.c, it seems to work, but I have no idea why that is  
>>>>> the case.
>>>>>
>>>>> Any ideas would be greatly appreciated.
>>>>
>>>> Before you chaning:
>>>> "
>>>> if (data->flags & MMC_DATA_READ)
>>>> 	check_and_invalidate_dcache_range(cmd, data);
>>>> "
>>>>
>>>> can you please try this, https://patchwork.ozlabs.org/patch/511889/ and
>>>> kindly give some feedback whether the patch can fix you issue or not?
>>>>
>>>> I strongly agree that U-Boot should flow the Linux DMA flow to avoid
>>>> potential write back and speculative read which may cause DMA data
>>>> be polluted.
>>>>
>>>> Anyway please first try first invalidate L2, then invalidate L1.
>>>>
>>>> I plan to pick the upper patch or rework for DMA usage, but have not
>>>> began the work (:
>>>
>>> Thanks for the reply!
>>>
>>> I tried to add the L2 invalidation from release.S into the
>>> invalidate_dcache_range function from ppccache.S:
>>
>> Oh. I missed you are using ppc.
>> Sorry. Actually the flow I suggested is for ARM.
>>
>>>
>>> "
>>>
>>> Subject: [PATCH] Invalidate L2 cache before L1 cache
>>>
>>> ---
>>> arch/powerpc/lib/ppccache.S | 9 +++++++++
>>> 1 file changed, 9 insertions(+)
>>>
>>> diff --git a/arch/powerpc/lib/ppccache.S b/arch/powerpc/lib/ppccache.S
>>> index b96dbc6..3f68be2 100644
>>> --- a/arch/powerpc/lib/ppccache.S
>>> +++ b/arch/powerpc/lib/ppccache.S
>>> @@ -87,6 +87,15 @@ _GLOBAL(flush_dcache_range)
>>>  * invalidate_dcache_range(unsigned long start, unsigned long stop)
>>>  */
>>> _GLOBAL(invalidate_dcache_range)
>>> +	msync
>>> +	lis	r2,(L2CSR0_L2FI|L2CSR0_L2LFC)@h
>>> +	ori	r2,r2,(L2CSR0_L2FI|L2CSR0_L2LFC)@l
>>> +	mtspr	SPRN_L2CSR0,r2
>>> +2:
>>> +	mfspr	r5,SPRN_L2CSR0
>>> +	and.	r1,r5,r2
>>> +	bne	2b
>>> +
>>> 	li	r5,L1_CACHE_BYTES-1
>>> 	andc	r3,r3,r5
>>> 	subf	r4,r3,r4
>>> --
>>> 1.8.3
>>>
>>> "
>>>
>>> That had the effect of crashing the board on the first MMC access, sadly.
>>>
>>> But, this had me thinking. Correct me if I'm wrong, since I'm in *no way* a
>>> DMA
>>> expert, but the logic currently works as follows:
>>>
>>> 1. Before the DMA transfer, invalidate the d-cache, so no stale data in the
>>> cache can pollute the result of the DMA transfer.
>>> 2. Do the DMA transfer.
>>> 3. Invalidate the d-cache again, so the CPU doesn't read stale  
>>> data from the
>>> cache.
>>>
>>> Shouldn't the cache be *flushed* in the first step? The DMA transfer didn't
>>
>> To ARM, invalidate is enough, no need to flush and the flow is
>> 1. Invalidate L1, then invalidate L2 to fix potential write back.
>> 2. DMA read
>> 3. Invalidate L2, then invalidate L1 to fix potential speculative read.
>>
>> But in uboot, we do not have dma api, only use invalidate_cache_range to do
>> the invalidation and the flow does not follow ARM recommendations  
>> after DMA read.
>>
>> I have no knowledge of PPC (:
>
> Guys,
>
> This may be related to a similar issue I saw earlier
> http://lists.denx.de/pipermail/u-boot/2015-December/236272.html
>
> A invalidate_cache_range function was added by
> commitac337168ad81a18e768e5e3cfff8d229adeb2b25 (patch
> http://patchwork.ozlabs.org/patch/455439). It looks good, but caused problem
> when used on e500v2. This function was no-op for 512x, 83xx and 85xx  
> before. I
> was thinking to revert partially to keep the function only for 4xx and 86xx.
>
> York

Yes, might be for the best to partially revert the commit. Though, I do agree
with Scott's opinion from the thread you mentioned that it would be good to
find out what the actual problem is.

Best regards,

Mario