[U-Boot] [PATCH v2 03/22] powerpc/mpc85xx: move debug tlb entry after TLB is in known state

[Prabhakar Kushwaha]

On 10/31/2012 02:37 AM, Scott Wood wrote:
> On 10/30/2012 04:26:16 AM, Prabhakar Kushwaha wrote:
>> On 10/30/2012 07:34 AM, Scott Wood wrote:
>>> Previously, in many if not all configs we were creating overlapping 
>>> TLB entries
>>> which is illegal.  This caused a crash during boot when moving 
>>> p2020rdb NAND SPL
>>> into L2 SRAM.
>>>
>>> Signed-off-by: Scott Wood <scottwood at freescale.com>
>>> Cc: Prabhakar Kushwaha <prabhakar at freescale.com>
>>> Cc: Andy Fleming <afleming at freescale.com>
>>> -- 
>>> Prabhakar, please test that debug still works.
>>>
>>
>> During RAMBOOT, both "temporary debug TLB entry" and "execution TLB 
>> entry" is same. So moving "temporary debug TLB  entry" creation after 
>> "execution TLB entry resizing" will make sure of debugging during 
>> NAND ramboot, SPI and SD boot.
>>
>> But for NOR  & NAND SPL there is a problem because boot-up TLB as 
>> 0xfffff000 and temporary debug TLB as 0xEFF80000. So we require to 
>> create temporary TLB entry to support early debugging.
>>
>> I will suggest to split the CONFIG_SYS_PPC_E500_DEBUG_TLB define into 
>> 2 parts.
>>  1)  For NOR , NAND spl debugging
>>  2)  For RAMBoot:  After resizing of execution TLB
>
> I'd rather not see this split this up.  This file is too much of a 
> complicated ifdef mess already.
>
> The window during which you won't be able to use breakpoints is not 
> that large.
> There are other debugging techniques that can be used. 

Can you please share the other techniques. It will help us in future.

> Meanwhile, I wasted time debugging the problems that this extra TLB 
> entry caused -- and every time I try to reason about what's going on 
> in this file, I now have one more thing to consider (which is really 
> several more things due to all the different scenarios).  Not exactly 
> the desired outcome of something meant to help debugging.
>

Debugger is present to help in these scenario for faster and effective 
debugging.
My effort is to make as much as possible u-boot code to be debbug-able.

 From now on, we have to take care of extra temporary debug TLB entry. 
But this should not mean, no new TLB entries can be created depending 
upon requirement.  This TLB entry is required for debugging in AS1 
execution space.

if everything is perfect, debugging is even not required. but if 
something broken in AS1, debugger is best/shortest way to debug.

> Also, we should not be creating this entry at any time during the SPL.

I believe it should not be required as NAND SPL never required.  But 
still need to check once this framework is pushed for mpx85xx platforms

>
>> I made following changes in the patch and tested across P1010RDB for 
>> NOR, NAND-SPL, NAND Ramboot and SPI boot debugging.
>>
>> Please note I used only this patch after replacing MINIMAL_SPL with 
>> CONFIG_NAND_SPL.
>>
>> diff --git a/arch/powerpc/cpu/mpc85xx/start.S 
>> b/arch/powerpc/cpu/mpc85xx/start.S
>> index ac17f9d..c00db4a 100644
>> --- a/arch/powerpc/cpu/mpc85xx/start.S
>> +++ b/arch/powerpc/cpu/mpc85xx/start.S
>> @@ -282,7 +282,7 @@ l2_disabled:
>>         isync
>>         .endm
>>
>> -#if defined(CONFIG_SYS_PPC_E500_DEBUG_TLB) && !defined(CONFIG_NAND_SPL)
>> +#if defined(CONFIG_SYS_PPC_E500_DEBUG_TLB) && !defined(MINIMAL_SPL)
>>  /*
>>   * TLB entry for debuggging in AS1
>>   * Create temporary TLB entry in AS0 to handle debug exception
>> @@ -309,16 +309,6 @@ l2_disabled:
>>                 CONFIG_SYS_MONITOR_BASE, MAS2_I|MAS2_G, \
>>                 CONFIG_SYS_PBI_FLASH_WINDOW, MAS3_SX|MAS3_SW|MAS3_SR, \
>>                 0, r6
>> -#else
>> -/*
>> - * TLB entry is created for IVPR + IVOR15 to map on valid OP code 
>> address
>> - * because "nexti" will resize TLB to 4K
>> - */
>> -       create_tlb1_entry CONFIG_SYS_PPC_E500_DEBUG_TLB, \
>> -               0, BOOKE_PAGESZ_256K, \
>> -               CONFIG_SYS_MONITOR_BASE, MAS2_I, \
>> -               CONFIG_SYS_MONITOR_BASE, MAS3_SX|MAS3_SW|MAS3_SR, \
>> -               0, r6
>>  #endif
>>  #endif
>>
>> @@ -520,6 +510,24 @@ nexti:     mflr    r1              /* R1 = our 
>> PC */
>>         msync
>>         tlbwe
>>
>> +#if defined(CONFIG_SYS_PPC_E500_DEBUG_TLB) && !defined(MINIMAL_SPL)\
>> +   && defined(CONFIG_SYS_RAMBOOT)
>
> Get rid of that CONFIG_SYS_RAMBOOT.  We don't set it anymore for SPL 
> as of this patchset.  And the SPL payload is precisely when I saw 
> problems with this!
>

I paste this just for reference. It should be updated in v2 version of 
this SPL patch series.

>> +/*
>> + * TLB entry for debuggging in AS1
>> + * Create temporary TLB entry in AS0 to handle debug exception
>> + * As on debug exception MSR is cleared i.e. Address space is changed
>> + * to 0. A TLB entry (in AS0) is required to handle debug exception 
>> generated
>> + * in AS1.
>> + * TLB entry is created for IVPR + IVOR15 to map on valid OP code 
>> address
>> + * because "nexti" has resized  execution TLB entry to 4K
>> + */
>> +       create_tlb1_entry CONFIG_SYS_PPC_E500_DEBUG_TLB, \
>> +               0, BOOKE_PAGESZ_256K, \
>> +               CONFIG_SYS_MONITOR_BASE & 0xfffc0000, MAS2_I, \
>> +               CONFIG_SYS_MONITOR_BASE & 0xfffc0000, 
>> MAS3_SX|MAS3_SW|MAS3_SR, \
>> +               0, r6
>> +#endif
>> +
>>  /*
>>   * Clear out any other TLB entries that may exist, to avoid conflicts.
>>   * Our TLB entry is in r14.
>
> You *cannot* put this entry into the TLB until we've cleared out the 
> unknown previous contents of the TLB.
>

OK. We can put it just after clearing TLB entries.


Regards,
Prabhakar