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

[Scott Wood]

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.

This entry is for L3SRAM which is 1 MiB at 0xf00000000 which is nowhere
near 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.

That entry would have already been replaced by asm code.

-Scott