Patchwork [1/2] powerpc/85xx: fix problem that prevents PHYS_64BIT from configurable

login
register
mail settings
Submitter Yang Li
Date Feb. 16, 2012, 12:10 p.m.
Message ID <1329394210-1014-1-git-send-email-leoli@freescale.com>
Download mbox | patch
Permalink /patch/141574/
State Rejected
Delegated to: Kumar Gala
Headers show

Comments

Yang Li - Feb. 16, 2012, 12:10 p.m.
Fix the problem that large physical address support cannot be
disabled when some platforms which only provides 36-bit support
are selected.  According to the philosophy of kernel config
enabling a platform support doesn't mean the kernel is only
running on that platform.  Remove the auto selection of PHYS_64BIT
option for these platforms.  They will need to use a 36bit default
config that selects PHYS_64BIT explicitly.

The reason why we need to keep PHYS_64BIT option configurable is
that enabling it cause negative performance impact on various
aspects like TLB miss and physical address manipulating.  We should
not enable it unless really needed, e.g. use large memory of 4GB
or bigger.

Signed-off-by: Li Yang <leoli@freescale.com>
---
 arch/powerpc/platforms/85xx/Kconfig |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)
Tabi Timur-B04825 - Feb. 16, 2012, 3:56 p.m.
On Thu, Feb 16, 2012 at 6:10 AM, Li Yang <leoli@freescale.com> wrote:
>
> The reason why we need to keep PHYS_64BIT option configurable is
> that enabling it cause negative performance impact on various
> aspects like TLB miss and physical address manipulating.  We should
> not enable it unless really needed, e.g. use large memory of 4GB
> or bigger.

I think we should make 36-bit the only option for the upstream
defconfigs, and if we want a 32-bit "optimized" kernel for the SDK,
then we provide that on the SDK only.
Tabi Timur-B04825 - Feb. 16, 2012, 3:57 p.m.
On Thu, Feb 16, 2012 at 9:56 AM, Tabi Timur-B04825 <b04825@freescale.com> wrote:
> I think we should make 36-bit the only option for the upstream
> defconfigs, and if we want a 32-bit "optimized" kernel for the SDK,
> then we provide that on the SDK only.

Oops, I meant to post this as a reply to "powerpc/85xx: add a 36-bit
corenet default config".
Kumar Gala - Feb. 17, 2012, 1:27 a.m.
On Feb 16, 2012, at 6:10 AM, Li Yang wrote:

> Fix the problem that large physical address support cannot be
> disabled when some platforms which only provides 36-bit support
> are selected.  According to the philosophy of kernel config
> enabling a platform support doesn't mean the kernel is only
> running on that platform.  Remove the auto selection of PHYS_64BIT
> option for these platforms.  They will need to use a 36bit default
> config that selects PHYS_64BIT explicitly.
> 
> The reason why we need to keep PHYS_64BIT option configurable is
> that enabling it cause negative performance impact on various
> aspects like TLB miss and physical address manipulating.  We should
> not enable it unless really needed, e.g. use large memory of 4GB
> or bigger.
> 
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
> arch/powerpc/platforms/85xx/Kconfig |    6 ------
> 1 files changed, 0 insertions(+), 6 deletions(-)

Nak, this isn't correct.

For some of these platforms like P2041RDB, P3041DS, P3060QDS, P4080DS, & P5020DS only a 36-bit physical address map is supported by u-boot and the device tree.  This was a decision that was made to NOT support 32-bit address map for these boards and accept the performance implication of it to reduce the # of builds, etc.

Additionally, outside of maybe P2041RDB I believe the majority of these boards ship with 4G of DDR (but that off the top of my head) and thus require the 36-bit / PHYS_64BIT support to be enabled.

- k
Li Yang-R58472 - Feb. 17, 2012, 4:32 a.m.
>Subject: Re: [PATCH 1/2] powerpc/85xx: fix problem that prevents
>PHYS_64BIT from configurable
>
>
>On Feb 16, 2012, at 6:10 AM, Li Yang wrote:
>
>> Fix the problem that large physical address support cannot be disabled
>> when some platforms which only provides 36-bit support are selected.
>> According to the philosophy of kernel config enabling a platform
>> support doesn't mean the kernel is only running on that platform.
>> Remove the auto selection of PHYS_64BIT option for these platforms.
>> They will need to use a 36bit default config that selects PHYS_64BIT
>> explicitly.
>>
>> The reason why we need to keep PHYS_64BIT option configurable is that
>> enabling it cause negative performance impact on various aspects like
>> TLB miss and physical address manipulating.  We should not enable it
>> unless really needed, e.g. use large memory of 4GB or bigger.
>>
>> Signed-off-by: Li Yang <leoli@freescale.com>
>> ---
>> arch/powerpc/platforms/85xx/Kconfig |    6 ------
>> 1 files changed, 0 insertions(+), 6 deletions(-)
>
>Nak, this isn't correct.
>
>For some of these platforms like P2041RDB, P3041DS, P3060QDS, P4080DS, &
>P5020DS only a 36-bit physical address map is supported by u-boot and the
>device tree.  This was a decision that was made to NOT support 32-bit
>address map for these boards and accept the performance implication of it
>to reduce the # of builds, etc.

Ok.  Although personally I don't think # of builds matters that much.

>
>Additionally, outside of maybe P2041RDB I believe the majority of these
>boards ship with 4G of DDR (but that off the top of my head) and thus
>require the 36-bit / PHYS_64BIT support to be enabled.

I know that current support of DPAA boards requires 36-bit.  But I don't think they need to select the PHYS_64BIT option directly and make it not configurable.  It's conflicting with the logic that enabling a platform support doesn't mean the kernel is only running on that platform.  Btw: what's your recommendations on solving this?

- Leo
Benjamin Herrenschmidt - Feb. 17, 2012, 8:42 a.m.
On Thu, 2012-02-16 at 20:10 +0800, Li Yang wrote:
> Fix the problem that large physical address support cannot be
> disabled when some platforms which only provides 36-bit support
> are selected.  According to the philosophy of kernel config
> enabling a platform support doesn't mean the kernel is only
> running on that platform.  Remove the auto selection of PHYS_64BIT
> option for these platforms.  They will need to use a 36bit default
> config that selects PHYS_64BIT explicitly.

No, but unless I'm wrong, with your patch, enabling those platforms will
build the code ... but they won't work unless you -also- enable
PHYS_64BIT one way or another. I thus disagree.

If I enable CONFIG_P1022_DS, I expect those boards to work.

If that's going to negatively impact perfs on other boards that I also
enabled, then so be it (and we should document it in the help text).

Cheers,
Ben.

> The reason why we need to keep PHYS_64BIT option configurable is
> that enabling it cause negative performance impact on various
> aspects like TLB miss and physical address manipulating.  We should
> not enable it unless really needed, e.g. use large memory of 4GB
> or bigger.
> 
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
>  arch/powerpc/platforms/85xx/Kconfig |    6 ------
>  1 files changed, 0 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
> index d7946be..d9bc0bd 100644
> --- a/arch/powerpc/platforms/85xx/Kconfig
> +++ b/arch/powerpc/platforms/85xx/Kconfig
> @@ -80,7 +80,6 @@ config P1010_RDB
>  config P1022_DS
>  	bool "Freescale P1022 DS"
>  	select DEFAULT_UIMAGE
> -	select PHYS_64BIT	# The DTS has 36-bit addresses
>  	select SWIOTLB
>  	help
>  	  This option enables support for the Freescale P1022DS reference board.
> @@ -175,7 +174,6 @@ config P2041_RDB
>  	bool "Freescale P2041 RDB"
>  	select DEFAULT_UIMAGE
>  	select PPC_E500MC
> -	select PHYS_64BIT
>  	select SWIOTLB
>  	select ARCH_REQUIRE_GPIOLIB
>  	select GPIO_MPC8XXX
> @@ -188,7 +186,6 @@ config P3041_DS
>  	bool "Freescale P3041 DS"
>  	select DEFAULT_UIMAGE
>  	select PPC_E500MC
> -	select PHYS_64BIT
>  	select SWIOTLB
>  	select ARCH_REQUIRE_GPIOLIB
>  	select GPIO_MPC8XXX
> @@ -201,7 +198,6 @@ config P3060_QDS
>  	bool "Freescale P3060 QDS"
>  	select DEFAULT_UIMAGE
>  	select PPC_E500MC
> -	select PHYS_64BIT
>  	select SWIOTLB
>  	select GPIO_MPC8XXX
>  	select HAS_RAPIDIO
> @@ -213,7 +209,6 @@ config P4080_DS
>  	bool "Freescale P4080 DS"
>  	select DEFAULT_UIMAGE
>  	select PPC_E500MC
> -	select PHYS_64BIT
>  	select SWIOTLB
>  	select ARCH_REQUIRE_GPIOLIB
>  	select GPIO_MPC8XXX
> @@ -229,7 +224,6 @@ config P5020_DS
>  	select DEFAULT_UIMAGE
>  	select E500
>  	select PPC_E500MC
> -	select PHYS_64BIT
>  	select SWIOTLB
>  	select ARCH_REQUIRE_GPIOLIB
>  	select GPIO_MPC8XXX
Benjamin Herrenschmidt - Feb. 17, 2012, 8:43 a.m.
On Fri, 2012-02-17 at 04:32 +0000, Li Yang-R58472 wrote:
> >Additionally, outside of maybe P2041RDB I believe the majority of
> these
> >boards ship with 4G of DDR (but that off the top of my head) and thus
> >require the 36-bit / PHYS_64BIT support to be enabled.
> 
> I know that current support of DPAA boards requires 36-bit.  But I
> don't think they need to select the PHYS_64BIT option directly and
> make it not configurable.  It's conflicting with the logic that
> enabling a platform support doesn't mean the kernel is only running on
> that platform.  Btw: what's your recommendations on solving this?

But selecting PHYS_64BIT shouldn't prevent running on the other
platforms. If it does, then this needs to be fixed.

Cheers,
Ben.
Tabi Timur-B04825 - Feb. 17, 2012, 4:22 p.m.
On Thu, Feb 16, 2012 at 7:27 PM, Kumar Gala <galak@kernel.crashing.org> wrote:

> For some of these platforms like P2041RDB, P3041DS, P3060QDS, P4080DS, & P5020DS only a 36-bit physical address map is supported by u-boot and the device tree.  This was a decision that was made to NOT support 32-bit address map for these boards and accept the performance implication of it to reduce the # of builds, etc.

Was this a Freescale internal decision, or is this a generic 85xx decision?

For the record, I'm in favor in leaving out support for 32-bit address
map in the upstream kernel, and having it be an option on the SDK
only.  However, in order to do that, we cannot have "select
PHYS_64BIT" in the Kconfigs.  It needs to be in the defconfigs
instead.  Putting it in the defconfig will eliminate the need to have
it in every Kconfig block, so I think that's an improvement.

Then the SDK can include a defconfig that does not have PHYS_64BIT
defined.  And the SDK can include 32-bit U-Boots and 32-bit device
trees for any board where Freescale determines there is a need.

I think Leo's patch simplifies things for everyone.
Benjamin Herrenschmidt - Feb. 17, 2012, 9:10 p.m.
On Fri, 2012-02-17 at 16:22 +0000, Tabi Timur-B04825 wrote:
> Was this a Freescale internal decision, or is this a generic 85xx
> decision?
> 
> For the record, I'm in favor in leaving out support for 32-bit address
> map in the upstream kernel, and having it be an option on the SDK
> only.  However, in order to do that, we cannot have "select
> PHYS_64BIT" in the Kconfigs.  It needs to be in the defconfigs
> instead.  Putting it in the defconfig will eliminate the need to have
> it in every Kconfig block, so I think that's an improvement.
> 
> Then the SDK can include a defconfig that does not have PHYS_64BIT
> defined.  And the SDK can include 32-bit U-Boots and 32-bit device
> trees for any board where Freescale determines there is a need.
> 
> I think Leo's patch simplifies things for everyone. 

Sorry, I fail to see how... it basically makes all those boards
non-functional even when enabled...

What's wrong with the current scheme ?

Ben.
Tabi Timur-B04825 - Feb. 17, 2012, 9:17 p.m.
Benjamin Herrenschmidt wrote:
> Sorry, I fail to see how... it basically makes all those boards
> non-functional even when enabled...

So you're saying that if we allow 32-bit address spacing for a particular
board, then we must provide a 32-bit DTS to go with it?

I was hoping to use the defconfig to force 36-bit, so that we can have a
32-bit option if we want.  Just because we don't put a 32-bit DTS in the
upstream kernel, that doesn't mean that no one is allowed to have a 32-bit
kernel.

> What's wrong with the current scheme ?

It prevents the possibility of creating an "optimized" 32-bit address
environment, for people who have <= 2GB of DDR on the board.  If we want
to ship a 32-bit DTS and 32-bit U-Boot on our BSP, then we'll need to
patch the Kconfig to remove the "select" line.
Tabi Timur-B04825 - Feb. 17, 2012, 11:01 p.m.
So I noticed something else.  PHYS_64BIT is not defined in
mpc85xx_smp_defconfig.  This means that if we want to build a 36-bit
kernel, we need to have "select PHYS_64BIT" in the Kconfig for that board.
 This is what we have today for the P1022DS.

However, that select statement also means that we can't build a 32-bit
kernel.  This is a problem for mpc85xx_defconfig, because that defconfig
includes support for the MPC8540ADS.  The 8540 has an e500v1 core which
doesn't support MAS7 (i.e. no 36-bit physical addresses).

So any patch that removes "select PHYS_64BIT" from an mpc85xx_defconfig
board must also turn on that option in the defconfig.  The patches from me
and Leo don't do that.
Scott Wood - Feb. 18, 2012, 12:56 a.m.
On 02/17/2012 05:01 PM, Timur Tabi wrote:
> So I noticed something else.  PHYS_64BIT is not defined in
> mpc85xx_smp_defconfig.

It couldn't have been, since it was selected by another kconfig option.
 defconfigs only hold non-default options.

> However, that select statement also means that we can't build a 32-bit
> kernel.  This is a problem for mpc85xx_defconfig, because that defconfig
> includes support for the MPC8540ADS.  The 8540 has an e500v1 core which
> doesn't support MAS7 (i.e. no 36-bit physical addresses).

It's not a problem for 8540, just lost optimization potential.

> So any patch that removes "select PHYS_64BIT" from an mpc85xx_defconfig
> board must also turn on that option in the defconfig.  The patches from me
> and Leo don't do that.

Yes, or maybe make it "default y", and/or require an "I know what I'm
doing" option to be set for it to be unset if a board otherwise wants it.

The ability to turn it off is potentially useful for any board, since
the address map is determined by boot software which can be changed, but
we shouldn't make it too easy to fail to select it for boards that
normally require it.

-Scott
Benjamin Herrenschmidt - Feb. 18, 2012, 2:19 a.m.
On Fri, 2012-02-17 at 18:56 -0600, Scott Wood wrote:
> Yes, or maybe make it "default y", and/or require an "I know what I'm
> doing" option to be set for it to be unset if a board otherwise wants it.
> 
> The ability to turn it off is potentially useful for any board, since
> the address map is determined by boot software which can be changed, but
> we shouldn't make it too easy to fail to select it for boards that
> normally require it.

Don't we have CONFIG_EMBEDDED to make that sort of option visible ?

Cheers,
Ben.
Scott Wood - Feb. 20, 2012, 8:30 p.m.
On 02/17/2012 08:19 PM, Benjamin Herrenschmidt wrote:
> On Fri, 2012-02-17 at 18:56 -0600, Scott Wood wrote:
>> Yes, or maybe make it "default y", and/or require an "I know what I'm
>> doing" option to be set for it to be unset if a board otherwise wants it.
>>
>> The ability to turn it off is potentially useful for any board, since
>> the address map is determined by boot software which can be changed, but
>> we shouldn't make it too easy to fail to select it for boards that
>> normally require it.
> 
> Don't we have CONFIG_EMBEDDED to make that sort of option visible ?

OK, so:

config WANT_PHYS_64BIT
select PHYS_64BIT if !EMBEDDED

config PHYS_64BIT
default y if WANT_PHYS_64BIT

...and "select WANT_PHYS_64BIT" on boards that normally have 36-bit
address maps?

What about 86xx?  It looks like PHYS_64BIT can't be enabled if it's
sharing a kernel image with 82xx or 83xx (why these specific SoCs but
not 603 in general?).

-Scott

Patch

diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
index d7946be..d9bc0bd 100644
--- a/arch/powerpc/platforms/85xx/Kconfig
+++ b/arch/powerpc/platforms/85xx/Kconfig
@@ -80,7 +80,6 @@  config P1010_RDB
 config P1022_DS
 	bool "Freescale P1022 DS"
 	select DEFAULT_UIMAGE
-	select PHYS_64BIT	# The DTS has 36-bit addresses
 	select SWIOTLB
 	help
 	  This option enables support for the Freescale P1022DS reference board.
@@ -175,7 +174,6 @@  config P2041_RDB
 	bool "Freescale P2041 RDB"
 	select DEFAULT_UIMAGE
 	select PPC_E500MC
-	select PHYS_64BIT
 	select SWIOTLB
 	select ARCH_REQUIRE_GPIOLIB
 	select GPIO_MPC8XXX
@@ -188,7 +186,6 @@  config P3041_DS
 	bool "Freescale P3041 DS"
 	select DEFAULT_UIMAGE
 	select PPC_E500MC
-	select PHYS_64BIT
 	select SWIOTLB
 	select ARCH_REQUIRE_GPIOLIB
 	select GPIO_MPC8XXX
@@ -201,7 +198,6 @@  config P3060_QDS
 	bool "Freescale P3060 QDS"
 	select DEFAULT_UIMAGE
 	select PPC_E500MC
-	select PHYS_64BIT
 	select SWIOTLB
 	select GPIO_MPC8XXX
 	select HAS_RAPIDIO
@@ -213,7 +209,6 @@  config P4080_DS
 	bool "Freescale P4080 DS"
 	select DEFAULT_UIMAGE
 	select PPC_E500MC
-	select PHYS_64BIT
 	select SWIOTLB
 	select ARCH_REQUIRE_GPIOLIB
 	select GPIO_MPC8XXX
@@ -229,7 +224,6 @@  config P5020_DS
 	select DEFAULT_UIMAGE
 	select E500
 	select PPC_E500MC
-	select PHYS_64BIT
 	select SWIOTLB
 	select ARCH_REQUIRE_GPIOLIB
 	select GPIO_MPC8XXX