| Message ID | 1329394210-1014-1-git-send-email-leoli@freescale.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Kumar Gala |
| Headers | show |
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.
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".
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
>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
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
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.
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.
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.
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.
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.
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
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.
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
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
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(-)