[PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Jon Loeliger]

From: James.Yang <James.Yang@freescale.com>

Remove CPU_FTR_NEED_COHERENT for MPC7448 (and single-core MPC86xx).
This prevents needlessly setting M=1 when not SMP.

Signed-off-by: James.Yang <James.Yang@freescale.com>
Acked-by: Jon Loeliger <jdl@freescale.com>
---
 arch/powerpc/kernel/cputable.c |    2 +-
 include/asm-powerpc/cputable.h |    6 ++++++
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
index 9cb24d2..6ef87fb 100644
--- a/arch/powerpc/kernel/cputable.c
+++ b/arch/powerpc/kernel/cputable.c
@@ -836,7 +836,7 @@ static struct cpu_spec cpu_specs[] = {
 		.pvr_mask		= 0xffff0000,
 		.pvr_value		= 0x80040000,
 		.cpu_name		= "7448",
-		.cpu_features		= CPU_FTRS_7447A,
+		.cpu_features		= CPU_FTRS_7448,
 		.cpu_user_features	= COMMON_USER |
 			PPC_FEATURE_HAS_ALTIVEC_COMP | PPC_FEATURE_PPC_LE,
 		.icache_bsize		= 32,
diff --git a/include/asm-powerpc/cputable.h b/include/asm-powerpc/cputable.h
index 4345249..82d595a 100644
--- a/include/asm-powerpc/cputable.h
+++ b/include/asm-powerpc/cputable.h
@@ -302,6 +302,12 @@ extern void do_feature_fixups(unsigned long value, void *fixup_start,
 	    CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_HAS_HIGH_BATS | \
 	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE)
+#define CPU_FTRS_7448	(CPU_FTR_COMMON | CPU_FTR_SPLIT_ID_CACHE | \
+	    CPU_FTR_USE_TB | \
+	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
+	    CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
+	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_HAS_HIGH_BATS | \
+	    CPU_FTR_PPC_LE)
 #define CPU_FTRS_82XX	(CPU_FTR_COMMON | CPU_FTR_SPLIT_ID_CACHE | \
 	    CPU_FTR_MAYBE_CAN_DOZE | CPU_FTR_USE_TB)
 #define CPU_FTRS_G2_LE	(CPU_FTR_SPLIT_ID_CACHE | CPU_FTR_MAYBE_CAN_DOZE | \
-- 
1.5.0.3

Re: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Adrian Cox]

On Wed, 2007-05-02 at 16:34 -0500, Jon Loeliger wrote: 
> From: James.Yang <James.Yang@freescale.com>
> 
> Remove CPU_FTR_NEED_COHERENT for MPC7448 (and single-core MPC86xx).
> This prevents needlessly setting M=1 when not SMP.

There may be side effects to removing this. Most of the 74xx processors
had this flag added because of the L2 prefetch bug (erratum #16 on the
7447A). I see that bug is missing from the 7448 errata.

The problem is that many 32-bit PowerPC machines needed
CPU_FTR_NEED_COHERENT set for a second reason: compatibility with the
cache in the MPC107. This was handled by CPU_FTR_COMMON in cputable.h
before the L2 prefetch bug was known.  There may be other host bridges
that cache, but nobody will have noticed because all the CPUs had
CPU_FTR_NEED_COHERENT set already.

-- 
Adrian Cox <adrian@humboldt.co.uk>

Re: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Benjamin Herrenschmidt]

On Thu, 2007-05-03 at 10:17 +0000, Adrian Cox wrote:
> On Wed, 2007-05-02 at 16:34 -0500, Jon Loeliger wrote: 
> > From: James.Yang <James.Yang@freescale.com>
> > 
> > Remove CPU_FTR_NEED_COHERENT for MPC7448 (and single-core MPC86xx).
> > This prevents needlessly setting M=1 when not SMP.
> 
> There may be side effects to removing this. Most of the 74xx processors
> had this flag added because of the L2 prefetch bug (erratum #16 on the
> 7447A). I see that bug is missing from the 7448 errata.
> 
> The problem is that many 32-bit PowerPC machines needed
> CPU_FTR_NEED_COHERENT set for a second reason: compatibility with the
> cache in the MPC107. This was handled by CPU_FTR_COMMON in cputable.h
> before the L2 prefetch bug was known.  There may be other host bridges
> that cache, but nobody will have noticed because all the CPUs had
> CPU_FTR_NEED_COHERENT set already.

There are a few options there...

The feature fixup and the hash table init are done after machine probe,
so the machine probe routine can do some last minute fixups of the
features, like detecting an MPC107 or whatever..

Another option would be to add a bit of generic code in
early_init_devtree() to detect the MPC107 and set that feature bit when
it's present, but that would involve having a consistent way to
recognize it via the device-tree which might not be the case currently
(though that only matters for 7448 based-boards so it's still doable).

Cheers,
Ben.

Re: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Paul Mackerras]

Jon Loeliger writes:

> From: James.Yang <James.Yang@freescale.com>
> 
> Remove CPU_FTR_NEED_COHERENT for MPC7448 (and single-core MPC86xx).
> This prevents needlessly setting M=1 when not SMP.

Do you have any benchmark results showing a real performance
improvement from these?

Paul.

Re: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Jon Loeliger]

On Thu, 2007-05-03 at 06:02, Paul Mackerras wrote:

> Do you have any benchmark results showing a real performance
> improvement from these?
> 
> Paul.

Yes.  We have the data to back this claim up.

jdl

Re: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Jon Loeliger]

On Thu, 2007-05-03 at 05:17, Adrian Cox wrote:
> On Wed, 2007-05-02 at 16:34 -0500, Jon Loeliger wrote: 
> > From: James.Yang <James.Yang@freescale.com>
> > 
> > Remove CPU_FTR_NEED_COHERENT for MPC7448 (and single-core MPC86xx).
> > This prevents needlessly setting M=1 when not SMP.
> 
> There may be side effects to removing this. Most of the 74xx processors
> had this flag added because of the L2 prefetch bug (erratum #16 on the
> 7447A). I see that bug is missing from the 7448 errata.
> 
> The problem is that many 32-bit PowerPC machines needed
> CPU_FTR_NEED_COHERENT set for a second reason: compatibility with the
> cache in the MPC107. This was handled by CPU_FTR_COMMON in cputable.h
> before the L2 prefetch bug was known.  There may be other host bridges
> that cache, but nobody will have noticed because all the CPUs had
> CPU_FTR_NEED_COHERENT set already.

Adrian,

Yes, you are correct and your concern is valid.  However,
this case is still being handled by CONFIG_MPC10X_BRIDGE
to deal with the MPC106/MPC107/etc north bridges.

Here:
    include/asm-powerpc/cputable.h:183

/* We need to mark all pages as being coherent if we're SMP or we
 * have a 74[45]x and an MPC107 host bridge. Also 83xx requires
 * it for PCI "streaming/prefetch" to work properly.
 */
#if defined(CONFIG_SMP) || defined(CONFIG_MPC10X_BRIDGE) \
	|| defined(CONFIG_PPC_83xx)
#define CPU_FTR_COMMON                  CPU_FTR_NEED_COHERENT
#else
#define CPU_FTR_COMMON                  0
#endif

The CPU doesn't impose this requirement, the north bridge does.
It might even better be named something like
    CPU_FTR_NORTHBRDIGE_NEEDS_COHERENT.

Thanks,
jdl

Re: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Adrian Cox]

On Thu, 2007-05-03 at 11:13 -0500, Jon Loeliger wrote:
> > The problem is that many 32-bit PowerPC machines needed
> > CPU_FTR_NEED_COHERENT set for a second reason: compatibility with the
> > cache in the MPC107. This was handled by CPU_FTR_COMMON in cputable.h
> > before the L2 prefetch bug was known.  There may be other host bridges
> > that cache, but nobody will have noticed because all the CPUs had
> > CPU_FTR_NEED_COHERENT set already.

> Yes, you are correct and your concern is valid.  However,
> this case is still being handled by CONFIG_MPC10X_BRIDGE
> to deal with the MPC106/MPC107/etc north bridges.

My only concern here is that some other Northbridges may have a similar
cache issue to the MPC107, but that we haven't noticed because the
cputable entry has been a crutch for them. If we remove the entry, will
some other 7448 designs quietly stop working?  I think the Tsi108/109
are probably safe, but I don't know about other bridges.

> The CPU doesn't impose this requirement, the north bridge does.
> It might even better be named something like
>     CPU_FTR_NORTHBRDIGE_NEEDS_COHERENT.

Yes - we end up turning on coherency for multiple reasons - SMP, the L2
prefetch bug, or the cache in the MPC107. I quite like Ben H's idea of
doing this in machine_probe().

-- 
Adrian Cox <adrian@humboldt.co.uk>

Re: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Jon Loeliger]

On Thu, 2007-05-03 at 12:07, Adrian Cox wrote:
> On Thu, 2007-05-03 at 11:13 -0500, Jon Loeliger wrote:
> > > The problem is that many 32-bit PowerPC machines needed
> > > CPU_FTR_NEED_COHERENT set for a second reason: compatibility with the
> > > cache in the MPC107. This was handled by CPU_FTR_COMMON in cputable.h
> > > before the L2 prefetch bug was known.  There may be other host bridges
> > > that cache, but nobody will have noticed because all the CPUs had
> > > CPU_FTR_NEED_COHERENT set already.
> 
> > Yes, you are correct and your concern is valid.  However,
> > this case is still being handled by CONFIG_MPC10X_BRIDGE
> > to deal with the MPC106/MPC107/etc north bridges.
> 
> My only concern here is that some other Northbridges may have a similar
> cache issue to the MPC107, but that we haven't noticed because the
> cputable entry has been a crutch for them. If we remove the entry, will
> some other 7448 designs quietly stop working?  I think the Tsi108/109
> are probably safe, but I don't know about other bridges.
> 
> > The CPU doesn't impose this requirement, the north bridge does.
> > It might even better be named something like
> >     CPU_FTR_NORTHBRDIGE_NEEDS_COHERENT.
> 
> Yes - we end up turning on coherency for multiple reasons - SMP, the L2
> prefetch bug, or the cache in the MPC107. I quite like Ben H's idea of
> doing this in machine_probe().

Hmmmm... I see...  Would you prefer something like this instead:

        static int __init mpc86xx_hpcn_probe(void)
        {
        	unsigned long root = of_get_flat_dt_root();
        
        	if (of_flat_dt_is_compatible(root, "mpc86xx")) {
        		/*
        		 * get number_of_cpus() from somewhere
        		 * perhaps the device tree?
        		 */
        		if (number_of_cpus() == 1)
        			cur_cpu_spec->cpu_features &= ~CPU_FTR_NEED_COHERENT;
        
        		return 1;       /* Looks good */
        	}
        
        	return 0;
        }
        
Though, rather than a dynamic test for number_of_cpus() == 1, perhaps
just another #ifndef CONFIG_SMP around that feature removal?

Thanks,
jdl

Re: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Benjamin Herrenschmidt]

On Thu, 2007-05-03 at 11:13 -0500, Jon Loeliger wrote:
> On Thu, 2007-05-03 at 05:17, Adrian Cox wrote:
> > On Wed, 2007-05-02 at 16:34 -0500, Jon Loeliger wrote: 
> > > From: James.Yang <James.Yang@freescale.com>
> > > 
> > > Remove CPU_FTR_NEED_COHERENT for MPC7448 (and single-core MPC86xx).
> > > This prevents needlessly setting M=1 when not SMP.
> > 
> > There may be side effects to removing this. Most of the 74xx processors
> > had this flag added because of the L2 prefetch bug (erratum #16 on the
> > 7447A). I see that bug is missing from the 7448 errata.
> > 
> > The problem is that many 32-bit PowerPC machines needed
> > CPU_FTR_NEED_COHERENT set for a second reason: compatibility with the
> > cache in the MPC107. This was handled by CPU_FTR_COMMON in cputable.h
> > before the L2 prefetch bug was known.  There may be other host bridges
> > that cache, but nobody will have noticed because all the CPUs had
> > CPU_FTR_NEED_COHERENT set already.
> 
> Adrian,
> 
> Yes, you are correct and your concern is valid.  However,
> this case is still being handled by CONFIG_MPC10X_BRIDGE
> to deal with the MPC106/MPC107/etc north bridges.

I still maintain it should be a runtime thing tho :-)

Ben.

Re: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Paul Mackerras]

Jon Loeliger writes:

> Yes.  We have the data to back this claim up.

Care to share it with us? :)

Paul.

Re: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Jon Loeliger]

So, like, the other day Paul Mackerras mumbled:
> Jon Loeliger writes:
> 
> > Yes.  We have the data to back this claim up.
> 
> Care to share it with us? :)

To quote our very own Olof Johansson:

    Not at this time, sorry.

jdl

Re: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Jon Loeliger]

So, like, the other day Benjamin Herrenschmidt mumbled:
> 
> I still maintain it should be a runtime thing tho :-)
> 
> Ben.

So, could you comment on my proposed solution doing
things exactly this way?  Speifically, would folks
prefer the dynamic 

    number_of_cpus() == 1 

and possibly checking the device tree or SMP map, or the 

    #ifndef SMP

test guarding the removal of the CPU_FTR_NEED_COHERENT bit?

Thanks,
jdl

Re: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Kumar Gala]

On May 3, 2007, at 5:17 AM, Adrian Cox wrote:

> On Wed, 2007-05-02 at 16:34 -0500, Jon Loeliger wrote:
>> From: James.Yang <James.Yang@freescale.com>
>>
>> Remove CPU_FTR_NEED_COHERENT for MPC7448 (and single-core MPC86xx).
>> This prevents needlessly setting M=1 when not SMP.
>
> There may be side effects to removing this. Most of the 74xx  
> processors
> had this flag added because of the L2 prefetch bug (erratum #16 on the
> 7447A). I see that bug is missing from the 7448 errata.
>
> The problem is that many 32-bit PowerPC machines needed
> CPU_FTR_NEED_COHERENT set for a second reason: compatibility with the
> cache in the MPC107. This was handled by CPU_FTR_COMMON in cputable.h
> before the L2 prefetch bug was known.  There may be other host bridges
> that cache, but nobody will have noticed because all the CPUs had
> CPU_FTR_NEED_COHERENT set already.

While this may be the case for non-10X bridges I don't think we  
should impose the restriction if we don't know its actually needed.

- k

Re: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Benjamin Herrenschmidt]

On Fri, 2007-05-04 at 10:16 -0500, Jon Loeliger wrote:
> So, like, the other day Benjamin Herrenschmidt mumbled:
> > 
> > I still maintain it should be a runtime thing tho :-)
> > 
> > Ben.
> 
> So, could you comment on my proposed solution doing
> things exactly this way?  Speifically, would folks
> prefer the dynamic 
> 
>     number_of_cpus() == 1 

Sorry I don't remember the actual patch, must have missed it... I
suppose we could have generic code in early_init_devtree set the default
for this based on cpu_possible_map() containing more than one bit and
have platforms using one of those broken bridges force the bit in from
their probe routine.

Ben.

Re: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Adrian Cox]

On Sat, 2007-05-05 at 08:25 +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2007-05-04 at 10:16 -0500, Jon Loeliger wrote:
> > So, could you comment on my proposed solution doing
> > things exactly this way?  Speifically, would folks
> > prefer the dynamic 
> > 
> >     number_of_cpus() == 1 
> 
> Sorry I don't remember the actual patch, must have missed it... I
> suppose we could have generic code in early_init_devtree set the default
> for this based on cpu_possible_map() containing more than one bit and
> have platforms using one of those broken bridges force the bit in from
> their probe routine.

Having looked into it further, the MPC106/7 are probably unique in
containing an internal cache which must be coherent with the CPU.
Presumably the designers intended it as a performance enhancement for
other PCI bus masters accessing PowerPC memory, but for most
applications the cost of turning on coherent memory would have
outweighed it.

So I'm now happy about removing CPU_FTR_NEED_COHERENT from the 7448. I
also agree that platforms with this quirk should turn it on during
probing, but there aren't any boards in arch/powerpc that need this. The
static method based on CONFIG_MPC10X_BRIDGE is probably good enough for
arch/ppc. 

-- 
Adrian Cox <adrian@humboldt.co.uk>

RE: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Loeliger Jon-LOELIGER]

> Sorry I don't remember the actual patch, must have missed it... I
> suppose we could have generic code in early_init_devtree set=20
> the default
> for this based on cpu_possible_map() containing more than one bit and
> have platforms using one of those broken bridges force the bit in from
> their probe routine.
>=20
> Ben.

Hi Ben,

Yeah, I had no actual intention of retrofitting
This into all the existing boards.  I was more
Wanting to leave them alone and simply un-set the
Bit on the one board where I wanted it done better.

My proposed solution in the _probe() routine was:

        static int __init mpc86xx_hpcn_probe(void)
        {
        	unsigned long root =3D of_get_flat_dt_root();
       =20
        	if (of_flat_dt_is_compatible(root, "mpc86xx")) {
        		/* get number_of_cpus() from somewhere */
        		if (number_of_cpus() =3D=3D 1) {
        			cur_cpu_spec->cpu_features &=3D
~CPU_FTR_NEED_COHERENT;
        		}
        		return 1;	/* Looks good */
        	}
       =20
        	return 0;
        }

Or perhaps:

        static int __init mpc86xx_hpcn_probe(void)
        {
        	unsigned long root =3D of_get_flat_dt_root();
       =20
        	if (of_flat_dt_is_compatible(root, "mpc86xx")) {
	  #ifndef CONFIG_SMP
        		cur_cpu_spec->cpu_features &=3D
~CPU_FTR_NEED_COHERENT;
	  #endif
        		return 1;	/* Looks good */
        	}
       =20
        	return 0;
        }

Specifically, which notion of multi-CPU-ness do
We want to honor here?

Thanks,
Jdl

Re: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Kumar Gala]

On Wed, 2 May 2007, Jon Loeliger wrote:

> From: James.Yang <James.Yang@freescale.com>
>
> Remove CPU_FTR_NEED_COHERENT for MPC7448 (and single-core MPC86xx).
> This prevents needlessly setting M=1 when not SMP.
>
> Signed-off-by: James.Yang <James.Yang@freescale.com>
> Acked-by: Jon Loeliger <jdl@freescale.com>
> ---
>  arch/powerpc/kernel/cputable.c |    2 +-
>  include/asm-powerpc/cputable.h |    6 ++++++
>  2 files changed, 7 insertions(+), 1 deletions(-)
>

applied.

- k

Re: [PATCH] Remove CPU_FTR_NEED_COHERENT for 7448.

[Jon Loeliger]

On Thu, 2007-05-10 at 11:17, Kumar Gala wrote:

> >
> > Remove CPU_FTR_NEED_COHERENT for MPC7448 (and single-core MPC86xx).
> > This prevents needlessly setting M=1 when not SMP.
> >
> 
> applied.
> 
> - k

Sweet.  Thanks!

jdl