Patchwork powerpc: Enable CPU_FTR_NEED_COHERENT for MPC52xx

login
register
mail settings
Submitter Grant Likely
Date March 17, 2009, 4:43 a.m.
Message ID <fa686aa40903162143j5f641d7dob53b96b681737e07@mail.gmail.com>
Download mbox | patch
Permalink /patch/24540/
State Superseded
Delegated to: Grant Likely
Headers show

Comments

Grant Likely - March 17, 2009, 4:43 a.m.
On Mon, Mar 16, 2009 at 9:54 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, Mar 16, 2009 at 4:05 AM, Piotr Ziecik <kosmo@semihalf.com> wrote:
>> BestComm, a DMA engine in MPC52xx SoC, requires snooping when
>> CPU caches are enabled to work properly.
>>
>> Adding CPU_FTR_NEED_COHERENT fixes NFS problems on MPC52xx machines
>> introduced by 'powerpc/mm: Fix handling of _PAGE_COHERENT in BAT setup code'.
>>[...]
>>  #if defined(CONFIG_SMP) || defined(CONFIG_MPC10X_BRIDGE) \
>> -       || defined(CONFIG_PPC_83xx) || defined(CONFIG_8260)
>> +       || defined(CONFIG_PPC_83xx) || defined(CONFIG_8260) \
>> +       || defined(CONFIG_PPC_MPC52xx)
>>  #define CPU_FTR_COMMON                  CPU_FTR_NEED_COHERENT
>>  #else
>>  #define CPU_FTR_COMMON                  0
>
> Aside from the fact that MPC10X_BRIDGE, PPC83xx and 8260 are already
> doing it, adding the feature bit this way isn't multiplatform
> friendly.  Essentially it means that all selected platforms will have
> CPU_FTR_NEED_COHERENT enabled if CONFIG_PPC_MPC52xx is enabled.

Here's my counter-patch.  It contains the change to just G2_LE cores
when MPC52xx is selected.  However, this change will also affect some
of the MPC82xx parts.  However CPU_FTR_NEED_COHERENT shouldn't
actually hurt anything, so maybe it would be better to just enable it
unconditionally for the G2_LE core.

Kumar/Ben, thoughts?

g.

---
Kumar Gala - March 17, 2009, 12:10 p.m.
On Mar 16, 2009, at 11:43 PM, Grant Likely wrote:

> On Mon, Mar 16, 2009 at 9:54 PM, Grant Likely <grant.likely@secretlab.ca 
> > wrote:
>> On Mon, Mar 16, 2009 at 4:05 AM, Piotr Ziecik <kosmo@semihalf.com>  
>> wrote:
>>> BestComm, a DMA engine in MPC52xx SoC, requires snooping when
>>> CPU caches are enabled to work properly.
>>>
>>> Adding CPU_FTR_NEED_COHERENT fixes NFS problems on MPC52xx machines
>>> introduced by 'powerpc/mm: Fix handling of _PAGE_COHERENT in BAT  
>>> setup code'.
>>> [...]
>>>  #if defined(CONFIG_SMP) || defined(CONFIG_MPC10X_BRIDGE) \
>>> -       || defined(CONFIG_PPC_83xx) || defined(CONFIG_8260)
>>> +       || defined(CONFIG_PPC_83xx) || defined(CONFIG_8260) \
>>> +       || defined(CONFIG_PPC_MPC52xx)
>>>  #define CPU_FTR_COMMON                  CPU_FTR_NEED_COHERENT
>>>  #else
>>>  #define CPU_FTR_COMMON                  0
>>
>> Aside from the fact that MPC10X_BRIDGE, PPC83xx and 8260 are already
>> doing it, adding the feature bit this way isn't multiplatform
>> friendly.  Essentially it means that all selected platforms will have
>> CPU_FTR_NEED_COHERENT enabled if CONFIG_PPC_MPC52xx is enabled.
>
> Here's my counter-patch.  It contains the change to just G2_LE cores
> when MPC52xx is selected.  However, this change will also affect some
> of the MPC82xx parts.  However CPU_FTR_NEED_COHERENT shouldn't
> actually hurt anything, so maybe it would be better to just enable it
> unconditionally for the G2_LE core.
>
> Kumar/Ben, thoughts?
>
> g.
>
> ---
>
> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/ 
> include/asm/cput
> index 4911104..48d7f5f 100644
> --- a/arch/powerpc/include/asm/cputable.h
> +++ b/arch/powerpc/include/asm/cputable.h
> @@ -348,8 +348,15 @@ extern const char *powerpc_base_platform;
>            CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
> #define CPU_FTRS_82XX  (CPU_FTR_COMMON | \
>            CPU_FTR_MAYBE_CAN_DOZE | CPU_FTR_USE_TB)
> +
> +#if defined(CONFIG_PPC_MPC52xx)
> +#define CPU_FTRS_G2_LE (CPU_FTR_COMMON | CPU_FTR_MAYBE_CAN_DOZE | \
> +           CPU_FTR_USE_TB | CPU_FTR_MAYBE_CAN_NAP |  
> CPU_FTR_NEED_COHERENT)
> +#else
> #define CPU_FTRS_G2_LE (CPU_FTR_COMMON | CPU_FTR_MAYBE_CAN_DOZE | \
>            CPU_FTR_USE_TB | CPU_FTR_MAYBE_CAN_NAP)
> +#endif
> +
> #define CPU_FTRS_E300  (CPU_FTR_MAYBE_CAN_DOZE | \
>            CPU_FTR_USE_TB | CPU_FTR_MAYBE_CAN_NAP | \
>            CPU_FTR_COMMON)

Doing this via a static CPU FTR fixup isn't really the best way to  
handle it.  I was thinking about this the other day in my patch to  
actually make G2/e300 cores respect _PAGE_COHERENT.  We really should  
set this via a platform fixup.  Just not sure if that's soon enough.

- k
Grant Likely - March 17, 2009, 1:45 p.m.
On Tue, Mar 17, 2009 at 6:10 AM, Kumar Gala <kumar.gala@freescale.com> wrote:
> On Mar 16, 2009, at 11:43 PM, Grant Likely wrote:
>> diff --git a/arch/powerpc/include/asm/cputable.h
>> b/arch/powerpc/include/asm/cput
>> index 4911104..48d7f5f 100644
>> --- a/arch/powerpc/include/asm/cputable.h
>> +++ b/arch/powerpc/include/asm/cputable.h
>> @@ -348,8 +348,15 @@ extern const char *powerpc_base_platform;
>>           CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
>> #define CPU_FTRS_82XX  (CPU_FTR_COMMON | \
>>           CPU_FTR_MAYBE_CAN_DOZE | CPU_FTR_USE_TB)
>> +
>> +#if defined(CONFIG_PPC_MPC52xx)
>> +#define CPU_FTRS_G2_LE (CPU_FTR_COMMON | CPU_FTR_MAYBE_CAN_DOZE | \
>> +           CPU_FTR_USE_TB | CPU_FTR_MAYBE_CAN_NAP |
>> CPU_FTR_NEED_COHERENT)
>> +#else
>> #define CPU_FTRS_G2_LE (CPU_FTR_COMMON | CPU_FTR_MAYBE_CAN_DOZE | \
>>           CPU_FTR_USE_TB | CPU_FTR_MAYBE_CAN_NAP)
>> +#endif
>> +
>> #define CPU_FTRS_E300  (CPU_FTR_MAYBE_CAN_DOZE | \
>>           CPU_FTR_USE_TB | CPU_FTR_MAYBE_CAN_NAP | \
>>           CPU_FTR_COMMON)
>
> Doing this via a static CPU FTR fixup isn't really the best way to handle
> it.  I was thinking about this the other day in my patch to actually make
> G2/e300 cores respect _PAGE_COHERENT.  We really should set this via a
> platform fixup.  Just not sure if that's soon enough.

I agree, but as Ben pointed out last night on IRC, feature fixup
(early_init) occurs well before platform probe time.  Platform code
cannot fix it up until someone does the work of making platform probe
time earlier.  It's non-trivial.

However, I need to get a fix in for this ASAP, otherwise the 5200 will
be broken in 2.6.29.  I don't see this patch as a final solution, but
it works as a stop-gap until platform probing can be reworked.

g.
Kumar Gala - March 17, 2009, 1:49 p.m.
On Mar 17, 2009, at 8:45 AM, Grant Likely wrote:

> On Tue, Mar 17, 2009 at 6:10 AM, Kumar Gala  
> <kumar.gala@freescale.com> wrote:
>> On Mar 16, 2009, at 11:43 PM, Grant Likely wrote:
>>> diff --git a/arch/powerpc/include/asm/cputable.h
>>> b/arch/powerpc/include/asm/cput
>>> index 4911104..48d7f5f 100644
>>> --- a/arch/powerpc/include/asm/cputable.h
>>> +++ b/arch/powerpc/include/asm/cputable.h
>>> @@ -348,8 +348,15 @@ extern const char *powerpc_base_platform;
>>>           CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
>>> #define CPU_FTRS_82XX  (CPU_FTR_COMMON | \
>>>           CPU_FTR_MAYBE_CAN_DOZE | CPU_FTR_USE_TB)
>>> +
>>> +#if defined(CONFIG_PPC_MPC52xx)
>>> +#define CPU_FTRS_G2_LE (CPU_FTR_COMMON | CPU_FTR_MAYBE_CAN_DOZE | \
>>> +           CPU_FTR_USE_TB | CPU_FTR_MAYBE_CAN_NAP |
>>> CPU_FTR_NEED_COHERENT)
>>> +#else
>>> #define CPU_FTRS_G2_LE (CPU_FTR_COMMON | CPU_FTR_MAYBE_CAN_DOZE | \
>>>           CPU_FTR_USE_TB | CPU_FTR_MAYBE_CAN_NAP)
>>> +#endif
>>> +
>>> #define CPU_FTRS_E300  (CPU_FTR_MAYBE_CAN_DOZE | \
>>>           CPU_FTR_USE_TB | CPU_FTR_MAYBE_CAN_NAP | \
>>>           CPU_FTR_COMMON)
>>
>> Doing this via a static CPU FTR fixup isn't really the best way to  
>> handle
>> it.  I was thinking about this the other day in my patch to  
>> actually make
>> G2/e300 cores respect _PAGE_COHERENT.  We really should set this  
>> via a
>> platform fixup.  Just not sure if that's soon enough.
>
> I agree, but as Ben pointed out last night on IRC, feature fixup
> (early_init) occurs well before platform probe time.  Platform code
> cannot fix it up until someone does the work of making platform probe
> time earlier.  It's non-trivial.
>
> However, I need to get a fix in for this ASAP, otherwise the 5200 will
> be broken in 2.6.29.  I don't see this patch as a final solution, but
> it works as a stop-gap until platform probing can be reworked.

So for .29 lets just go with the easy solution of adding  
CONFIG_PPC_MPC52xx to the list that set CPU_FTR_NEED_COHERENT (10x,  
83xx, etc..).

- k

Patch

diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cput
index 4911104..48d7f5f 100644
--- a/arch/powerpc/include/asm/cputable.h
+++ b/arch/powerpc/include/asm/cputable.h
@@ -348,8 +348,15 @@  extern const char *powerpc_base_platform;
            CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
 #define CPU_FTRS_82XX  (CPU_FTR_COMMON | \
            CPU_FTR_MAYBE_CAN_DOZE | CPU_FTR_USE_TB)
+
+#if defined(CONFIG_PPC_MPC52xx)
+#define CPU_FTRS_G2_LE (CPU_FTR_COMMON | CPU_FTR_MAYBE_CAN_DOZE | \
+           CPU_FTR_USE_TB | CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_NEED_COHERENT)
+#else
 #define CPU_FTRS_G2_LE (CPU_FTR_COMMON | CPU_FTR_MAYBE_CAN_DOZE | \
            CPU_FTR_USE_TB | CPU_FTR_MAYBE_CAN_NAP)
+#endif
+
 #define CPU_FTRS_E300  (CPU_FTR_MAYBE_CAN_DOZE | \
            CPU_FTR_USE_TB | CPU_FTR_MAYBE_CAN_NAP | \
            CPU_FTR_COMMON)