Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1337368 > unrolled thread

[PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values Arnd Bergmann <arnd@arndb.de> - 2016-02-18 15:10 +0100
    Re: [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values Nicolas Pitre <nicolas.pitre@linaro.org> - 2016-02-18 17:10 +0100
      Re: [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values Arnd Bergmann <arnd@arndb.de> - 2016-02-19 09:40 +0100
        RE: [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values Chris Brandt <Chris.Brandt@renesas.com> - 2016-02-19 15:30 +0100
          Re: [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values Arnd Bergmann <arnd@arndb.de> - 2016-02-19 16:40 +0100
            Re: [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values Russell King - ARM Linux <linux@arm.linux.org.uk> - 2016-02-19 17:50 +0100
              RE: [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values Chris Brandt <Chris.Brandt@renesas.com> - 2016-02-19 18:20 +0100
              Re: [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values Nicolas Pitre <nicolas.pitre@linaro.org> - 2016-02-19 19:00 +0100
        Re: [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values Nicolas Pitre <nicolas.pitre@linaro.org> - 2016-02-19 17:20 +0100
          Re: [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values Arnd Bergmann <arnd@arndb.de> - 2016-02-19 17:30 +0100
            Re: [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values Nicolas Pitre <nicolas.pitre@linaro.org> - 2016-02-19 18:40 +0100
              Re: [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values Russell King - ARM Linux <linux@arm.linux.org.uk> - 2016-02-19 19:10 +0100
                Re: [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values Arnd Bergmann <arnd@arndb.de> - 2016-02-19 22:20 +0100

#1337368 — [PATCH 4/9] ARM: add CONFIG_PHYS_OFFSET default values

For platforms that are not yet converted to ARCH_MULTIPLATFORM,
we can disable CONFIG_ARM_PATCH_PHYS_VIRT, which in turn requires
setting a correct address here.

As we actualy know what all the values are supposed to be based
on the old mach/memory.h header file contents (from git history),
we can just add them here.

This also solves a problem in Kconfig where 'make randconfig'
fails to continue if no number is selected for a 'hex' option.
Users can still override the number at configuration time, e.g.
when the memory visible to the kernel starts at a nonstandard
address on some machine, but it should no longer be required
now.

To make this foolproof, another patch is required in mach-davinci
to prevent a configuration with both DMx and DA8xx enabled but
ARM_PATCH_PHYS_VIRT disabled. The two patches however can be
merged independently as there is no direct dependency between
them.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm/Kconfig | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index be00b53f399b..7839c9923709 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -271,15 +271,29 @@ config PHYS_OFFSET
 	depends on !ARM_PATCH_PHYS_VIRT
 	default DRAM_BASE if !MMU
 	default 0x00000000 if ARCH_EBSA110 || \
+			ARCH_DOVE || \
 			ARCH_FOOTBRIDGE || \
+			(ARCH_GEMINI && GEMINI_MEM_SWAP) || \
 			ARCH_INTEGRATOR || \
+			ARCH_IOP33X || \
 			ARCH_IOP13XX || \
+			ARCH_IXP4XX || \
 			ARCH_KS8695 || \
-			(ARCH_REALVIEW && !REALVIEW_HIGH_PHYS_OFFSET)
-	default 0x10000000 if ARCH_OMAP1 || ARCH_RPC
+			(ARCH_REALVIEW && !REALVIEW_HIGH_PHYS_OFFSET) || \
+			ARCH_W90X900
+	default 0x10000000 if (ARCH_GEMINI && !GEMINI_MEM_SWAP) || \
+			ARCH_OMAP1 || \
+			ARCH_RPC
 	default 0x20000000 if ARCH_S5PV210
+	default 0x30000000 if ARCH_S3C24XX
 	default 0x70000000 if REALVIEW_HIGH_PHYS_OFFSET
-	default 0xc0000000 if ARCH_SA1100
+	default 0x80000000 if (ARCH_DAVINCI_DMx && !ARCH_DAVINCI_DA8XX) || \
+			ARCH_NETX || \
+			ARCH_LPC32XX
+	default 0xa0000000 if ARCH_IOP32X || ARCH_PXA
+	default 0xc0000000 if (ARCH_DAVINCI_DA8XX && !ARCH_DAVINCI_DMx) || \
+			ARCH_CLPS711X || \
+			ARCH_SA1100
 	help
 	  Please provide the physical address corresponding to the
 	  location of main memory in your system.
-- 
2.7.0

[toc] | [next] | [standalone]

#1337488

On Thu, 18 Feb 2016, Arnd Bergmann wrote:

> For platforms that are not yet converted to ARCH_MULTIPLATFORM,
> we can disable CONFIG_ARM_PATCH_PHYS_VIRT, which in turn requires
> setting a correct address here.
> 
> As we actualy know what all the values are supposed to be based
> on the old mach/memory.h header file contents (from git history),
> we can just add them here.
> 
> This also solves a problem in Kconfig where 'make randconfig'
> fails to continue if no number is selected for a 'hex' option.
> Users can still override the number at configuration time, e.g.
> when the memory visible to the kernel starts at a nonstandard
> address on some machine, but it should no longer be required
> now.
> 
> To make this foolproof, another patch is required in mach-davinci
> to prevent a configuration with both DMx and DA8xx enabled but
> ARM_PATCH_PHYS_VIRT disabled. The two patches however can be
> merged independently as there is no direct dependency between
> them.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Acked-by: Nicolas Pitre <nico@linaro.org>

Is there a way to provide a default for defaults?


> ---
>  arch/arm/Kconfig | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index be00b53f399b..7839c9923709 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -271,15 +271,29 @@ config PHYS_OFFSET
>  	depends on !ARM_PATCH_PHYS_VIRT
>  	default DRAM_BASE if !MMU
>  	default 0x00000000 if ARCH_EBSA110 || \
> +			ARCH_DOVE || \
>  			ARCH_FOOTBRIDGE || \
> +			(ARCH_GEMINI && GEMINI_MEM_SWAP) || \
>  			ARCH_INTEGRATOR || \
> +			ARCH_IOP33X || \
>  			ARCH_IOP13XX || \
> +			ARCH_IXP4XX || \
>  			ARCH_KS8695 || \
> -			(ARCH_REALVIEW && !REALVIEW_HIGH_PHYS_OFFSET)
> -	default 0x10000000 if ARCH_OMAP1 || ARCH_RPC
> +			(ARCH_REALVIEW && !REALVIEW_HIGH_PHYS_OFFSET) || \
> +			ARCH_W90X900
> +	default 0x10000000 if (ARCH_GEMINI && !GEMINI_MEM_SWAP) || \
> +			ARCH_OMAP1 || \
> +			ARCH_RPC
>  	default 0x20000000 if ARCH_S5PV210
> +	default 0x30000000 if ARCH_S3C24XX
>  	default 0x70000000 if REALVIEW_HIGH_PHYS_OFFSET
> -	default 0xc0000000 if ARCH_SA1100
> +	default 0x80000000 if (ARCH_DAVINCI_DMx && !ARCH_DAVINCI_DA8XX) || \
> +			ARCH_NETX || \
> +			ARCH_LPC32XX
> +	default 0xa0000000 if ARCH_IOP32X || ARCH_PXA
> +	default 0xc0000000 if (ARCH_DAVINCI_DA8XX && !ARCH_DAVINCI_DMx) || \
> +			ARCH_CLPS711X || \
> +			ARCH_SA1100
>  	help
>  	  Please provide the physical address corresponding to the
>  	  location of main memory in your system.
> -- 
> 2.7.0
> 
> 

[toc] | [prev] | [next] | [standalone]

#1337972

On Thursday 18 February 2016 11:02:33 Nicolas Pitre wrote:
> 
> Acked-by: Nicolas Pitre <nico@linaro.org>
> 
> Is there a way to provide a default for defaults?

We could have something like

config PHYS_OFFSET_0
	bool

config PHYS_OFFSET_1
	bool

config PHYS_OFFSET_2
	bool

... (we need 8 of the 16 possible addresses)


config PHYS_OFFSET
	hex "Physical address of main memory" if MMU
	default DRAM_BASE if !MMU
	default 0x00000000 if PHYS_OFFSET_0
	default 0x10000000 if PHYS_OFFSET_1
	default 0x20000000 if PHYS_OFFSET_2
	default 0x30000000 if PHYS_OFFSET_3
	default 0x70000000 if PHYS_OFFSET_7
	default 0x80000000 if PHYS_OFFSET_8
	default 0xa0000000 if PHYS_OFFSET_A
	default 0xc0000000 if PHYS_OFFSET_C


and then select one of the bool symbols from each platform.
Would that address your question?

FWIW, that would also let us do:

config XIP_KERNEL
	bool "Kernel Execute-In-Place from ROM"
	depends on PHYS_OFFSET_0 || PHYS_OFFSET_1 || PHYS_OFFSET_2 ||
		   PHYS_OFFSET_3 || PHYS_OFFSET_7 || PHYS_OFFSET_8 ||
		   PHYS_OFFSET_A || PHYS_OFFSET_C

We can probably come up with a more elaborate way to prevent configurations
that have more than one of these set.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1338228

On 19 Feb 2016, Arnd Bergmann wrote:

> On Thursday 18 February 2016 11:02:33 Nicolas Pitre wrote:
> > 
> > Acked-by: Nicolas Pitre <nico@linaro.org>
> > 
> > Is there a way to provide a default for defaults?
> 
> We could have something like
> 
> config PHYS_OFFSET_0
> 	bool
> 
> config PHYS_OFFSET_1
> 	bool
> 
> config PHYS_OFFSET_2
> 	bool
> 
> ... (we need 8 of the 16 possible addresses)
> 
> 
> config PHYS_OFFSET
> 	hex "Physical address of main memory" if MMU
> 	default DRAM_BASE if !MMU
> 	default 0x00000000 if PHYS_OFFSET_0
> 	default 0x10000000 if PHYS_OFFSET_1
> 	default 0x20000000 if PHYS_OFFSET_2
> 	default 0x30000000 if PHYS_OFFSET_3
> 	default 0x70000000 if PHYS_OFFSET_7
> 	default 0x80000000 if PHYS_OFFSET_8
> 	default 0xa0000000 if PHYS_OFFSET_A
> 	default 0xc0000000 if PHYS_OFFSET_C
> 
> 
> and then select one of the bool symbols from each platform.
> Would that address your question?


Here's a question:

Can we just get rid of PHYS_OFFSET???

If it's only used at boot for XIP systems, we could:

A) pass it in via an unused register like atags and DT
 or
B) just assume that atags or DT is in RAM, so round down to the nearest section and assume that is the start of your RAM

If it is needed after initial boot, then on first boot we save what was passed in from the boot loader for later use.


Chris

[toc] | [prev] | [next] | [standalone]

#1338297

On Friday 19 February 2016 14:29:00 Chris Brandt wrote:
> On 19 Feb 2016, Arnd Bergmann wrote:
> 
> > On Thursday 18 February 2016 11:02:33 Nicolas Pitre wrote:
> > > 
> > > Acked-by: Nicolas Pitre <nico@linaro.org>
> > > 
> > > Is there a way to provide a default for defaults?
> > 
> > We could have something like
> > 
> > config PHYS_OFFSET_0
> > 	bool
> > 
> > config PHYS_OFFSET_1
> > 	bool
> > 
> > config PHYS_OFFSET_2
> > 	bool
> > 
> > ... (we need 8 of the 16 possible addresses)
> > 
> > 
> > config PHYS_OFFSET
> > 	hex "Physical address of main memory" if MMU
> > 	default DRAM_BASE if !MMU
> > 	default 0x00000000 if PHYS_OFFSET_0
> > 	default 0x10000000 if PHYS_OFFSET_1
> > 	default 0x20000000 if PHYS_OFFSET_2
> > 	default 0x30000000 if PHYS_OFFSET_3
> > 	default 0x70000000 if PHYS_OFFSET_7
> > 	default 0x80000000 if PHYS_OFFSET_8
> > 	default 0xa0000000 if PHYS_OFFSET_A
> > 	default 0xc0000000 if PHYS_OFFSET_C
> > 
> > 
> > and then select one of the bool symbols from each platform.
> > Would that address your question?
> 
> 
> Here's a question:
> 
> Can we just get rid of PHYS_OFFSET???
> 
> If it's only used at boot for XIP systems, we could:
> 
> A) pass it in via an unused register like atags and DT
>  or
> B) just assume that atags or DT is in RAM, so round down to the nearest section and assume that is the start of your RAM
> 
> If it is needed after initial boot, then on first boot we save what was passed in from the boot loader for later use.

Hmm, you mean making phys_offset a runtime variable instead
of patching it at early boot time in the instructions?

I have no idea if that works, how much effort it would be,
or how much it would enlarge the kernel image size, but
you can definitely try.

Of course we must not break existing platforms using XIP_KERNEL
already, but the installed base among systems that are upgrading
to modern kernels is very small now, given how all modern platforms
don't support XIP_KERNEL today, or have no MMU to start with.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1338344

On Fri, Feb 19, 2016 at 04:34:51PM +0100, Arnd Bergmann wrote:
> On Friday 19 February 2016 14:29:00 Chris Brandt wrote:
> > On 19 Feb 2016, Arnd Bergmann wrote:
> > 
> > > On Thursday 18 February 2016 11:02:33 Nicolas Pitre wrote:
> > > > 
> > > > Acked-by: Nicolas Pitre <nico@linaro.org>
> > > > 
> > > > Is there a way to provide a default for defaults?
> > > 
> > > We could have something like
> > > 
> > > config PHYS_OFFSET_0
> > > 	bool
> > > 
> > > config PHYS_OFFSET_1
> > > 	bool
> > > 
> > > config PHYS_OFFSET_2
> > > 	bool
> > > 
> > > ... (we need 8 of the 16 possible addresses)
> > > 
> > > 
> > > config PHYS_OFFSET
> > > 	hex "Physical address of main memory" if MMU
> > > 	default DRAM_BASE if !MMU
> > > 	default 0x00000000 if PHYS_OFFSET_0
> > > 	default 0x10000000 if PHYS_OFFSET_1
> > > 	default 0x20000000 if PHYS_OFFSET_2
> > > 	default 0x30000000 if PHYS_OFFSET_3
> > > 	default 0x70000000 if PHYS_OFFSET_7
> > > 	default 0x80000000 if PHYS_OFFSET_8
> > > 	default 0xa0000000 if PHYS_OFFSET_A
> > > 	default 0xc0000000 if PHYS_OFFSET_C
> > > 
> > > 
> > > and then select one of the bool symbols from each platform.
> > > Would that address your question?
> > 
> > 
> > Here's a question:
> > 
> > Can we just get rid of PHYS_OFFSET???
> > 
> > If it's only used at boot for XIP systems, we could:
> > 
> > A) pass it in via an unused register like atags and DT
> >  or
> > B) just assume that atags or DT is in RAM, so round down to the nearest section and assume that is the start of your RAM
> > 
> > If it is needed after initial boot, then on first boot we save what was passed in from the boot loader for later use.
> 
> Hmm, you mean making phys_offset a runtime variable instead
> of patching it at early boot time in the instructions?
> 
> I have no idea if that works, how much effort it would be,
> or how much it would enlarge the kernel image size, but
> you can definitely try.
> 
> Of course we must not break existing platforms using XIP_KERNEL
> already, but the installed base among systems that are upgrading
> to modern kernels is very small now, given how all modern platforms
> don't support XIP_KERNEL today, or have no MMU to start with.

You're all barking up the wrong tree, because you don't understand
why it exists.

ARM_PATCH_PHYS_VIRT exists to support an init-time decided phys offset,
and it supports this by modifying each location that the phys offset
is used.  It determines this by looking at the location that the early
init code is executing, and masking the PC with a value that has been
carefully crafted to fit 99% of the existing platforms.

One of the side effects of ARM_PATCH_PHYS_VIRT is that we hide the
translation from the compiler, so the compiler is unable to optimise
things like virt_to_phys(phys_to_virt(x)) to just 'x' (and yes, such
things do happen.)

PHYS_OFFSET exists to cater for the case where ARM_PATCH_PHYS_VIRT is
disabled, because either ARM_PATCH_PHYS_VIRT does not work for the
platform, or the platform has special requirements and/or requires
better performance.  It switches back to the pre-ARM_PATCH_PHYS_VIRT
situation where the PHYS_OFFSET is a compile time constant.

Obviously, making PHYS_OFFSET a runtime variable is basically what
ARM_PATCH_PHYS_VIRT is doing.  That does not help these cases though,
because the problem cases are not whether it's a runtime variable or
not, it's how to arrive at the value for it in the first place.

Using the DTB location on XIP platforms is a no-goer - the flattened
DTB information can be fixed, so on an XIP platform it makes sense
for this to also be in flash, not in RAM (the whole point of XIP is
to remove constant data from RAM after all, so why would you want to
copy the FDT to RAM?)

Passing it in a register to the kernel image is also not possible:
we've been out of spare registers for some time now for passing
additional information into the kernel.  It wouldn't be a problem
had folk not had this "eww, I don't like ATAGs, lets get rid of them
and use DT instead" attitude: had we kept ATAGs, then we'd have an
in-memory format to pass this kind of information to the kernel.
That would solve soo many problems today it's untrue: stuff like
where a debugging UART is located and the type of it...

When DT was being proposed, my opinion was that's how it should've
been done, but I assumed that the DT folk knew better, and Grant was
very much of the opinion that ATAGs should be completely dropped (I
did touch on it with Grant.)  I regret now not having a discussion
about it and pressing strongly for it.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1338364

On 19 Feb 2016, Russell King wrote:

> Using the DTB location on XIP platforms is a no-goer - the flattened
> DTB information can be fixed, so on an XIP platform it makes sense
> for this to also be in flash, not in RAM (the whole point of XIP is
> to remove constant data from RAM after all, so why would you want to
> copy the FDT to RAM?)

I was under the impression that the DTB had a limited life span, and once booted, it could be clobbered. Hence, RAM would not really be wasted (post boot that is).

For an XIP system (with MMU), if you have your DTB in ROM someplace, you run the risk of getting it cut off after the MMU setup is done because it would fall someplace after the _exiprom marker.


> Passing it in a register to the kernel image is also not possible:
> we've been out of spare registers for some time now for passing
> additional information into the kernel.  It wouldn't be a problem
> had folk not had this "eww, I don't like ATAGs, lets get rid of them
> and use DT instead" attitude: had we kept ATAGs, then we'd have an
> in-memory format to pass this kind of information to the kernel.
> That would solve soo many problems today it's untrue: stuff like
> where a debugging UART is located and the type of it...

Too bad we just couldn't have added an ATAG to point to a DTB. Sigh.


Chris

[toc] | [prev] | [next] | [standalone]

#1338390

On Fri, 19 Feb 2016, Russell King - ARM Linux wrote:

> ARM_PATCH_PHYS_VIRT exists to support an init-time decided phys offset,
> and it supports this by modifying each location that the phys offset
> is used.  It determines this by looking at the location that the early
> init code is executing, and masking the PC with a value that has been
> carefully crafted to fit 99% of the existing platforms.
> 
> One of the side effects of ARM_PATCH_PHYS_VIRT is that we hide the
> translation from the compiler, so the compiler is unable to optimise
> things like virt_to_phys(phys_to_virt(x)) to just 'x' (and yes, such
> things do happen.)
> 
> PHYS_OFFSET exists to cater for the case where ARM_PATCH_PHYS_VIRT is
> disabled, because either ARM_PATCH_PHYS_VIRT does not work for the
> platform, or the platform has special requirements and/or requires
> better performance.  It switches back to the pre-ARM_PATCH_PHYS_VIRT
> situation where the PHYS_OFFSET is a compile time constant.
> 
> Obviously, making PHYS_OFFSET a runtime variable is basically what
> ARM_PATCH_PHYS_VIRT is doing.  That does not help these cases though,
> because the problem cases are not whether it's a runtime variable or
> not, it's how to arrive at the value for it in the first place.

I think what Chris was suggesting is to have the same functionality as 
PATCH_PHYS_VIRT i.e. determining phys offset at run time while being 
XIP.  In theory that could mean that the kernel binary becomes 
independent of the physical location in flash where it executes from.

Arriving at the value for phys offset could be done based on sp instead 
of pc.  That's assuming the bootloader did not clobber sp before calling 
into the kernel.

But this is rather fragile. And normally if you are interested in XIP, 
you certainly have a highly customized kernel config already anyway. And 
having a build-time constant PHYS_OFFSET is of course what performs 
best.


Nicolas

[toc] | [prev] | [next] | [standalone]

#1338324

On Fri, 19 Feb 2016, Arnd Bergmann wrote:

> On Thursday 18 February 2016 11:02:33 Nicolas Pitre wrote:
> > 
> > Acked-by: Nicolas Pitre <nico@linaro.org>
> > 
> > Is there a way to provide a default for defaults?
> 
> We could have something like
> 
> config PHYS_OFFSET_0
> 	bool
> 
> config PHYS_OFFSET_1
> 	bool
> 
> config PHYS_OFFSET_2
> 	bool
> 
> ... (we need 8 of the 16 possible addresses)
> 
> 
> config PHYS_OFFSET
> 	hex "Physical address of main memory" if MMU
> 	default DRAM_BASE if !MMU
> 	default 0x00000000 if PHYS_OFFSET_0
> 	default 0x10000000 if PHYS_OFFSET_1
> 	default 0x20000000 if PHYS_OFFSET_2
> 	default 0x30000000 if PHYS_OFFSET_3
> 	default 0x70000000 if PHYS_OFFSET_7
> 	default 0x80000000 if PHYS_OFFSET_8
> 	default 0xa0000000 if PHYS_OFFSET_A
> 	default 0xc0000000 if PHYS_OFFSET_C
> 
> 
> and then select one of the bool symbols from each platform.
> Would that address your question?

Yes, but the ugliness factor isn't worth it IMHO.

I was wondering if something like this was possible:

config PHYS_OFFSET
        hex "Physical address of main memory" if MMU
        default DRAM_BASE if !MMU
        default 0x10000000 if FOO
        default 0x20000000 if BAR
        default 0x30000000 if BAZ
        default 0x00000000


Nicolas

[toc] | [prev] | [next] | [standalone]

#1338332

On Friday 19 February 2016 11:10:22 Nicolas Pitre wrote:
> On Fri, 19 Feb 2016, Arnd Bergmann wrote:
> 
> > On Thursday 18 February 2016 11:02:33 Nicolas Pitre wrote:
> > > 
> > > Acked-by: Nicolas Pitre <nico@linaro.org>
> > > 
> > > Is there a way to provide a default for defaults?
...
> > and then select one of the bool symbols from each platform.
> > Would that address your question?
> 
> Yes, but the ugliness factor isn't worth it IMHO.
> 
> I was wondering if something like this was possible:
> 
> config PHYS_OFFSET
>         hex "Physical address of main memory" if MMU
>         default DRAM_BASE if !MMU
>         default 0x10000000 if FOO
>         default 0x20000000 if BAR
>         default 0x30000000 if BAZ
>         default 0x00000000
> 

Ah, that was my previous approach, but Russell didn't like
how it makes it easier to fall back to an incorrect address
instead of forcing a build error when the address is not
configured.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1338381

On Fri, 19 Feb 2016, Arnd Bergmann wrote:

> On Friday 19 February 2016 11:10:22 Nicolas Pitre wrote:
> > On Fri, 19 Feb 2016, Arnd Bergmann wrote:
> > 
> > > On Thursday 18 February 2016 11:02:33 Nicolas Pitre wrote:
> > > > 
> > > > Acked-by: Nicolas Pitre <nico@linaro.org>
> > > > 
> > > > Is there a way to provide a default for defaults?
> ...
> > > and then select one of the bool symbols from each platform.
> > > Would that address your question?
> > 
> > Yes, but the ugliness factor isn't worth it IMHO.
> > 
> > I was wondering if something like this was possible:
> > 
> > config PHYS_OFFSET
> >         hex "Physical address of main memory" if MMU
> >         default DRAM_BASE if !MMU
> >         default 0x10000000 if FOO
> >         default 0x20000000 if BAR
> >         default 0x30000000 if BAZ
> >         default 0x00000000
> > 
> 
> Ah, that was my previous approach, but Russell didn't like
> how it makes it easier to fall back to an incorrect address
> instead of forcing a build error when the address is not
> configured.

Makes sense.

Yet, the only reason for a default here is to accommodate automatic 
build tests like randconfig, right?

If so then this should be "fixed" by having the config system provide 
built-in symbols that can be tested from kconfig files.  This way you 
could terminate the above list with:

	default 0x00000000 if RANDCONFIG || ALLYESCONFIG

or the like.


Nicolas

[toc] | [prev] | [next] | [standalone]

#1338401

On Fri, Feb 19, 2016 at 12:31:02PM -0500, Nicolas Pitre wrote:
> Yet, the only reason for a default here is to accommodate automatic 
> build tests like randconfig, right?
> 
> If so then this should be "fixed" by having the config system provide 
> built-in symbols that can be tested from kconfig files.  This way you 
> could terminate the above list with:
> 
> 	default 0x00000000 if RANDCONFIG || ALLYESCONFIG
> 
> or the like.

I've suggested in the past that we have kconf read a seed file for
these configurations.  kconf already has most of the required support
for this, we just need to teach it where to read it from.  Maybe
something like this.

 arch/arm/allrandom.config |  1 +
 scripts/kconfig/conf.c    | 61 ++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/arch/arm/allrandom.config b/arch/arm/allrandom.config
index e69de29bb2d1..5a70ef5926f5 100644
--- a/arch/arm/allrandom.config
+++ b/arch/arm/allrandom.config
@@ -0,0 +1 @@
+CONFIG_PHYS_OFFSET=0
diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 866369f10ff8..5a4b2d8bbf9a 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -12,6 +12,7 @@
 #include <time.h>
 #include <unistd.h>
 #include <getopt.h>
+#include <sys/fcntl.h>
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <errno.h>
@@ -489,10 +490,53 @@ static void conf_usage(const char *progname)
 	printf("  --randconfig            New config with random answer to all options\n");
 }
 
+static int try_allconfig(int try_arch, int input_mode)
+{
+	const char *name = NULL;
+	int fd = -1, ret;
+
+	if (try_arch) {
+		const char *srctree = getenv("srctree");
+		const char *arch = getenv("ARCH");
+		
+		fd = open(".", O_DIRECTORY);
+		if (fd == -1) {
+			perror("opening .");
+			return -1;
+		}
+		if (chdir(srctree) == -1 ||
+		    chdir("arch") == -1 ||
+		    chdir(arch) == -1) {
+			perror("chdir");
+			return -1;
+		}
+	}
+
+	switch (input_mode) {
+	case allnoconfig:	name = "allno.config"; break;
+	case allyesconfig:	name = "allyes.config"; break;
+	case allmodconfig:	name = "allmod.config"; break;
+	case alldefconfig:	name = "alldef.config"; break;
+	case randconfig:	name = "allrandom.config"; break;
+	default: break;
+	}
+
+	ret = name ? conf_read_simple(name, S_DEF_USER) : 1;
+	if (ret)
+		ret = conf_read_simple("all.config", S_DEF_USER);
+
+	if (fd >= 0) {
+		fchdir(fd);
+		close(fd);
+	}
+
+	return ret;
+}
+
 int main(int ac, char **av)
 {
 	const char *progname = av[0];
-	int opt;
+	int opt, ret;
 	const char *name, *defconfig_file = NULL /* gcc uninit */;
 	struct stat tmpstat;
 
@@ -601,6 +645,9 @@ int main(int ac, char **av)
 	case allmodconfig:
 	case alldefconfig:
 	case randconfig:
+		ret = try_allconfig(1, input_mode);
+		if (ret < 0)
+			exit(1);
 		name = getenv("KCONFIG_ALLCONFIG");
 		if (!name)
 			break;
@@ -613,16 +660,8 @@ int main(int ac, char **av)
 			}
 			break;
 		}
-		switch (input_mode) {
-		case allnoconfig:	name = "allno.config"; break;
-		case allyesconfig:	name = "allyes.config"; break;
-		case allmodconfig:	name = "allmod.config"; break;
-		case alldefconfig:	name = "alldef.config"; break;
-		case randconfig:	name = "allrandom.config"; break;
-		default: break;
-		}
-		if (conf_read_simple(name, S_DEF_USER) &&
-		    conf_read_simple("all.config", S_DEF_USER)) {
+		ret = try_allconfig(0, input_mode);
+		if (ret) {
 			fprintf(stderr,
 				_("*** KCONFIG_ALLCONFIG set, but no \"%s\" or \"all.config\" file found\n"),
 				name);

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1338474

On Friday 19 February 2016 18:07:25 Russell King - ARM Linux wrote:
> On Fri, Feb 19, 2016 at 12:31:02PM -0500, Nicolas Pitre wrote:
> > Yet, the only reason for a default here is to accommodate automatic 
> > build tests like randconfig, right?
> > 
> > If so then this should be "fixed" by having the config system provide 
> > built-in symbols that can be tested from kconfig files.  This way you 
> > could terminate the above list with:
> > 
> > 	default 0x00000000 if RANDCONFIG || ALLYESCONFIG
> > 
> > or the like.
> 
> I've suggested in the past that we have kconf read a seed file for
> these configurations.  kconf already has most of the required support
> for this, we just need to teach it where to read it from.  Maybe
> something like this.
> 
>  arch/arm/allrandom.config |  1 +
>  scripts/kconfig/conf.c    | 61 ++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 51 insertions(+), 11 deletions(-)

Interesting, I had never noticed that we had the infrastructure to have
separate presets for allno/allmod/allyes/...config by file name,
aside from the ${KCONFIG_ALLCONFIG}, I think your extension to make
it architecture specific is a very good idea, and it can solve a
couple of other problems as well, such as new toolchains barfing
on -march=armv3 and OABI support.

There is a bit of overlap with the Kconfig fragments, which are
defined in a similar way:

> diff --git a/arch/arm/allrandom.config b/arch/arm/allrandom.config
> index e69de29bb2d1..5a70ef5926f5 100644
> --- a/arch/arm/allrandom.config
> +++ b/arch/arm/allrandom.config
> @@ -0,0 +1 @@
> +CONFIG_PHYS_OFFSET=0

With the recently added Kconfig fragments support, you could do
(almost) the same thing by specifying

make randconfig allrandom.config

"almost", because

- The fragments use a search path including kernel/config/*.config
  and arch/*/configs/*.config, rather than arch/*/*.config
  I would prefer using the search path we have for the fragments now.

- The current implementation does not start out with the symbols
  from the fragment but instead applies the fragments one by one
  after the initial config, so the example above is the same as

	make randconfig
	make allrandom.config

  which does not have the same results. For this, I think starting
  with the fragment makes more sense, but that unfortunately requires
  changing the command line interface if we want to generalize it.

	Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web