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

[York Sun]

On 08/20/2013 01:03 PM, Scott Wood wrote:
> On Tue, 2013-08-20 at 12:57 -0700, York Sun wrote:
>> 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.
> 
> That's how I'd interpret it as well, but then the comment that "this
> entry replaces..." doesn't make sense.

The default TLB is TLB1 entry 0. This is the same TLB. Along the booting
process, we switch to AS=1 and replace the default TLB with this one,
then switch back. That's why I said "replace". I am sure you are very
familiar with this process.
> 
> This entry is for L3SRAM which is 1 MiB at 0xf00000000 which is nowhere
> near the boot page.

It maps from CONFIG_SYS_INIT_L3_ADDR ( == CONFIG_RAMBOOT_TEXT_BASE ==
CONFIG_SYS_TEXT_BASE == 0xfff80000) to CONFIG_SYS_INIT_L3_ADDR_PHYS ( ==
0xf00000000ull | CONFIG_RAMBOOT_TEXT_BASE).


> 
>>>>>>
>>>>>> 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.
> 
> That entry would have already been replaced by asm code.

No argument here. I was pointing out one case "I" and "G" don't go together.

York