[U-Boot] [PATCH v1 8/8] mpc85xx: introduce the kmp204x reference design support

[York Sun]

On 08/20/2013 12:47 PM, Scott Wood wrote:
> On Tue, 2013-08-20 at 12:40 -0700, York Sun wrote:
>> On 08/20/2013 11:21 AM, Scott Wood wrote:
>>> On Tue, 2013-08-20 at 13:20 -0500, Scott Wood wrote:
>>>> On Mon, 2013-08-19 at 18:02 -0700, York Sun wrote:
>>>>> On 08/19/2013 05:48 PM, Scott Wood wrote:
>>>>>> On Mon, 2013-08-19 at 17:50 +0200, Valentin Longchamp wrote:
>>>>>>> On 08/13/2013 11:38 PM, Scott Wood wrote:
>>>>>>>> On Fri, 2013-07-26 at 12:02 +0200, Valentin Longchamp wrote:
>>>>>
>>>>> <snip>
>>>>>
>>>>>>>>> +	/* TLB 1 */
>>>>>>>>> +	/* *I*** - Covers boot page */
>>>>>>>>> +	/* *I*G - L3SRAM. When L3 is used as 1M SRAM, the address of the
>>>>>>>>> +	 * SRAM is at 0xfff00000, it covered the 0xfffff000.
>>>>>>>>> +	 */
>>>>>>>>> +	SET_TLB_ENTRY(1, CONFIG_SYS_INIT_L3_ADDR, CONFIG_SYS_INIT_L3_ADDR,
>>>>>>>>> +		      MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
>>>>>>>>> +		      0, 0, BOOKE_PAGESZ_1M, 1),
>>>>>>>>
>>>>>>>> What does that "covers boot page" comment refer to?
>>>>>>>>
>>>>>>>> Why is L3SRAM I+G?
>>>>>>>>
>>>>>>>
>>>>>>> I have taken this from the corenet SYS_RAMBOOT boot scenario since it's also the
>>>>>>> way our board boots.
>>>>>>
>>>>>> York, can you answer this?
>>>>>>
>>>>>> I suspect the "covers boot page" comment is left over from before the
>>>>>> recent spin table changes.
>>>>>
>>>>> Look at the context, this is used as SRAM with PBL boot method. Notice
>>>>> these macros in header file
>>>>
>>>> I'm not talking about the SRAM comment, but the "covers boot page"
>>>> comment before it.
>>
>> I think this entry replaces the default TLB out of reset and it does
>> cover the boot page 0xfffff000~0xffffffff.
> 
> That's not what the comment appears to say (unless you read the word
> "cover" in a non-intuitive and ambiguous way).  These comments generally
> talk about what the new TLB is, not what is being replaced.
> 
>>  It is not unique to this platform. You can find many similar existing code.
> 
> I know that.  That's why I'm asking you to explain it rather than
> Valentin. :-)

We have many developers around the globe so people understand "cover"
differently. I interpret the "cover" here as this TLB translates the
address space which includes the boot page.

> 
>>>>
>>>> At the very least this mapping can't be *I*G and *I** at the same time.
>>
>> I agree the G bit shouldn't be set here.
> 
> Usually I and G go together...

The default TLB out of reset has I bit but not G bit. I have to admit
that I don't remember when I used G bit intentionally.

> 
>>>>
>>>>> +#define CONFIG_SYS_RAMBOOT
>>>>> +#define CONFIG_RAMBOOT_PBL
>>>>> +#define CONFIG_RAMBOOT_TEXT_BASE	CONFIG_SYS_TEXT_BASE
>>>>>
>>>>> and
>>>>>
>>>>> +/*
>>>>> + *  Config the L3 Cache as L3 SRAM
>>>>> + */
>>>>> +#define CONFIG_SYS_INIT_L3_ADDR		CONFIG_RAMBOOT_TEXT_BASE
>>>>> +#define CONFIG_SYS_INIT_L3_ADDR_PHYS	(0xf00000000ull | \
>>>>> +		CONFIG_RAMBOOT_TEXT_BASE)
>>>>> +#define CONFIG_SYS_L3_SIZE		(1024 << 10)
>>>>> +#define CONFIG_SYS_INIT_L3_END (CONFIG_SYS_INIT_L3_ADDR +
>>>>> CONFIG_SYS_L3_SIZE)
>>>>
>>>> ...and this doesn't cover the boot page.
>>>
>>> Also, can you answer the question about why the L3 SRAM mapping is
>>> cache-inhibited?
>>
>> I suspect this is the idea carried from early NAND boot implementation.
>> You are mostly familiar with NAND and SPL boot, can you examine if we
>> can turn on the cache for these cases?
> 
> NAND SPL on some targets is so space constrained that adding a few
> instructions to turn cache on might go over the limit. :-)
> 
> Are you talking about mapping the NAND buffer that we boot directly out
> of, or the L2SRAM that we sometimes load the SPL payload into?  If the
> former, that is I+G because we proceed to use it for I/O after
> relocating out of it.

I am talking aout the latter one. For SPL cases, the code is copied to
some type of volatile memory and the core boots from there. I am not
sure if we can turn on cache for all cases. Probably yes.

York