| Message ID | 1309920457-21913-1-git-send-email-nicolas.pitre@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/05/2011 07:46 PM, Nicolas Pitre wrote: > diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h > index 7b79a00..1bdf6e1 100644 > --- a/arch/arm/include/asm/mach/arch.h > +++ b/arch/arm/include/asm/mach/arch.h > @@ -18,6 +18,7 @@ struct machine_desc { > unsigned int nr; /* architecture number */ > const char *name; /* architecture name */ > unsigned long boot_params; /* tagged list */ > + unsigned long atag_offset; /* tagged list (relative) */ > const char **dt_compat; /* array of device tree > * 'compatible' strings */ > > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c > index e0db84d..4cc3e2b 100644 > --- a/arch/arm/kernel/setup.c > +++ b/arch/arm/kernel/setup.c > @@ -816,6 +816,8 @@ static struct machine_desc * __init setup_machine_tags(unsigned int nr) > > if (__atags_pointer) > tags = phys_to_virt(__atags_pointer); > + else if(mdesc->atag_offset) > + tags = PAGE_OFFSET + mdesc->atag_offset; > else if (mdesc->boot_params) { > #ifdef CONFIG_MMU > /* Would it make sense to drop the atag_offset in most board files and default it to 0x100 in the MACHINE_START macro itself? It's almost a universal value and would cut down on hundreds of lines.
On Tue, 5 Jul 2011, Stephen Boyd wrote: > On 07/05/2011 07:46 PM, Nicolas Pitre wrote: > > diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h > > index 7b79a00..1bdf6e1 100644 > > --- a/arch/arm/include/asm/mach/arch.h > > +++ b/arch/arm/include/asm/mach/arch.h > > @@ -18,6 +18,7 @@ struct machine_desc { > > unsigned int nr; /* architecture number */ > > const char *name; /* architecture name */ > > unsigned long boot_params; /* tagged list */ > > + unsigned long atag_offset; /* tagged list (relative) */ > > const char **dt_compat; /* array of device tree > > * 'compatible' strings */ > > > > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c > > index e0db84d..4cc3e2b 100644 > > --- a/arch/arm/kernel/setup.c > > +++ b/arch/arm/kernel/setup.c > > @@ -816,6 +816,8 @@ static struct machine_desc * __init setup_machine_tags(unsigned int nr) > > > > if (__atags_pointer) > > tags = phys_to_virt(__atags_pointer); > > + else if(mdesc->atag_offset) > > + tags = PAGE_OFFSET + mdesc->atag_offset; > > else if (mdesc->boot_params) { > > #ifdef CONFIG_MMU > > /* > > Would it make sense to drop the atag_offset in most board files and > default it to 0x100 in the MACHINE_START macro itself? It's almost a > universal value and would cut down on hundreds of lines. Well, no more than 50 lines actually. And there are those cases where that value is not set to anything, and therefore I'd prefer we don't start always assuming something at 0x*100 for them. IOW, this is meant to be functionally transparent. Nicolas
On 07/06/2011 05:54 AM, Nicolas Pitre wrote: > On Tue, 5 Jul 2011, Stephen Boyd wrote: > >> >> Would it make sense to drop the atag_offset in most board files and >> default it to 0x100 in the MACHINE_START macro itself? It's almost a >> universal value and would cut down on hundreds of lines. > > Well, no more than 50 lines actually. > > And there are those cases where that value is not set to anything, and > therefore I'd prefer we don't start always assuming something at 0x*100 > for them. IOW, this is meant to be functionally transparent. Fair enough. Probably too magical anyway.
On Tuesday, July 05, 2011 7:47 PM, Nicolas Pitre wrote: > The boot_params member of the mdesc structure is used to provide a > default physical address for the ATAG list. Since this value is fixed > at compile time and often based on ARCH_PHYS_OFFSET, it gets in the way > of runtime PHYS_OFFSET usage. > > Let's introduce atag_offset which should contains only the relative > offset from start of memory instead of an absolute value, in preparation > to move all instance of boot_params over to it. > > Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org> > --- > arch/arm/include/asm/mach/arch.h | 1 + > arch/arm/kernel/setup.c | 2 ++ > 2 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h > index 7b79a00..1bdf6e1 100644 > --- a/arch/arm/include/asm/mach/arch.h > +++ b/arch/arm/include/asm/mach/arch.h > @@ -18,6 +18,7 @@ struct machine_desc { > unsigned int nr; /* architecture number */ > const char *name; /* architecture name */ > unsigned long boot_params; /* tagged list */ > + unsigned long atag_offset; /* tagged list (relative) */ > const char **dt_compat; /* array of device tree > * 'compatible' strings */ > > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c > index e0db84d..4cc3e2b 100644 > --- a/arch/arm/kernel/setup.c > +++ b/arch/arm/kernel/setup.c > @@ -816,6 +816,8 @@ static struct machine_desc * __init setup_machine_tags(unsigned int nr) > > if (__atags_pointer) > tags = phys_to_virt(__atags_pointer); > + else if(mdesc->atag_offset) > + tags = PAGE_OFFSET + mdesc->atag_offset; This results in a warning... arch/arm/kernel/setup.c: In function 'setup_arch': arch/arm/kernel/setup.c:820: warning: assignment makes pointer from integer without a cast Regards, Hartley
On Wed, 6 Jul 2011, H Hartley Sweeten wrote: > On Tuesday, July 05, 2011 7:47 PM, Nicolas Pitre wrote: > > The boot_params member of the mdesc structure is used to provide a > > default physical address for the ATAG list. Since this value is fixed > > at compile time and often based on ARCH_PHYS_OFFSET, it gets in the way > > of runtime PHYS_OFFSET usage. > > > > Let's introduce atag_offset which should contains only the relative > > offset from start of memory instead of an absolute value, in preparation > > to move all instance of boot_params over to it. > > > > Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org> > > --- > > arch/arm/include/asm/mach/arch.h | 1 + > > arch/arm/kernel/setup.c | 2 ++ > > 2 files changed, 3 insertions(+), 0 deletions(-) > > > > diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h > > index 7b79a00..1bdf6e1 100644 > > --- a/arch/arm/include/asm/mach/arch.h > > +++ b/arch/arm/include/asm/mach/arch.h > > @@ -18,6 +18,7 @@ struct machine_desc { > > unsigned int nr; /* architecture number */ > > const char *name; /* architecture name */ > > unsigned long boot_params; /* tagged list */ > > + unsigned long atag_offset; /* tagged list (relative) */ > > const char **dt_compat; /* array of device tree > > * 'compatible' strings */ > > > > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c > > index e0db84d..4cc3e2b 100644 > > --- a/arch/arm/kernel/setup.c > > +++ b/arch/arm/kernel/setup.c > > @@ -816,6 +816,8 @@ static struct machine_desc * __init setup_machine_tags(unsigned int nr) > > > > if (__atags_pointer) > > tags = phys_to_virt(__atags_pointer); > > + else if(mdesc->atag_offset) > > + tags = PAGE_OFFSET + mdesc->atag_offset; > > This results in a warning... > > arch/arm/kernel/setup.c: In function 'setup_arch': > arch/arm/kernel/setup.c:820: warning: assignment makes pointer from integer without a cast Bleh. Fixed, thanks. Nicolas
Hello. On 06-07-2011 6:46, Nicolas Pitre wrote: > The boot_params member of the mdesc structure is used to provide a > default physical address for the ATAG list. Since this value is fixed > at compile time and often based on ARCH_PHYS_OFFSET, it gets in the way > of runtime PHYS_OFFSET usage. > Let's introduce atag_offset which should contains only the relative > offset from start of memory instead of an absolute value, in preparation > to move all instance of boot_params over to it. > Signed-off-by: Nicolas Pitre<nicolas.pitre@linaro.org> [...] > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c > index e0db84d..4cc3e2b 100644 > --- a/arch/arm/kernel/setup.c > +++ b/arch/arm/kernel/setup.c > @@ -816,6 +816,8 @@ static struct machine_desc * __init setup_machine_tags(unsigned int nr) > > if (__atags_pointer) > tags = phys_to_virt(__atags_pointer); > + else if(mdesc->atag_offset) Should be space after *if* -- checkpatch.pl should have complained. WBR, Sergei
On Thu, 7 Jul 2011, Sergei Shtylyov wrote: > Hello. > > On 06-07-2011 6:46, Nicolas Pitre wrote: > > > The boot_params member of the mdesc structure is used to provide a > > default physical address for the ATAG list. Since this value is fixed > > at compile time and often based on ARCH_PHYS_OFFSET, it gets in the way > > of runtime PHYS_OFFSET usage. > > > Let's introduce atag_offset which should contains only the relative > > offset from start of memory instead of an absolute value, in preparation > > to move all instance of boot_params over to it. > > > Signed-off-by: Nicolas Pitre<nicolas.pitre@linaro.org> > [...] > > > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c > > index e0db84d..4cc3e2b 100644 > > --- a/arch/arm/kernel/setup.c > > +++ b/arch/arm/kernel/setup.c > > @@ -816,6 +816,8 @@ static struct machine_desc * __init > > setup_machine_tags(unsigned int nr) > > > > if (__atags_pointer) > > tags = phys_to_virt(__atags_pointer); > > + else if(mdesc->atag_offset) > > Should be space after *if* -- checkpatch.pl should have complained. Indeed, thanks. Nicolas
On Wednesday, July 06, 2011 9:04 PM, Nicolas Pitre wrote: > On Wed, 6 Jul 2011, H Hartley Sweeten wrote: >>> + else if(mdesc->atag_offset) >>> + tags = PAGE_OFFSET + mdesc->atag_offset; >> >> This results in a warning... >> >> arch/arm/kernel/setup.c: In function 'setup_arch': >> arch/arm/kernel/setup.c:820: warning: assignment makes pointer from integer without a cast > > Bleh. Fixed, thanks. Thanks. Tested-by: H Hartley Sweeten <hsweeten@visionengravers.com>
On Tue, Jul 05, 2011 at 10:46:46PM -0400, Nicolas Pitre wrote: > The boot_params member of the mdesc structure is used to provide a > default physical address for the ATAG list. Since this value is fixed > at compile time and often based on ARCH_PHYS_OFFSET, it gets in the way > of runtime PHYS_OFFSET usage. > > Let's introduce atag_offset which should contains only the relative > offset from start of memory instead of an absolute value, in preparation > to move all instance of boot_params over to it. I don't like this. While I understand the issue (the oops when we try to dereference the absolute address), the fact is that's how it is. Take a moment to think about this. The ATAG list is normally setup at a fixed physical address by the boot loader. While there are some boot loaders which allow you to load the kernel at different physical addresses, they still place the ATAG list at that same physical address. That means if you change this into an offset from the kernels idea of PHYS_OFFSET, it's not going to find the ATAGs. So we're probably better off checking whether the platform provided ATAG pointer is below our idea of PHYS_OFFSET, and if it is to ignore that. The ultimate effect will be the same - the kernel won't find the ATAGs and won't crash instead. And no need for all this churn either.
On Fri, 8 Jul 2011, Russell King - ARM Linux wrote: > On Tue, Jul 05, 2011 at 10:46:46PM -0400, Nicolas Pitre wrote: > > The boot_params member of the mdesc structure is used to provide a > > default physical address for the ATAG list. Since this value is fixed > > at compile time and often based on ARCH_PHYS_OFFSET, it gets in the way > > of runtime PHYS_OFFSET usage. > > > > Let's introduce atag_offset which should contains only the relative > > offset from start of memory instead of an absolute value, in preparation > > to move all instance of boot_params over to it. > > I don't like this. > > While I understand the issue (the oops when we try to dereference the > absolute address), the fact is that's how it is. > > Take a moment to think about this. The ATAG list is normally setup at a > fixed physical address by the boot loader. While there are some boot > loaders which allow you to load the kernel at different physical addresses, > they still place the ATAG list at that same physical address. Would they really load the kernel at a different address? > That means if you change this into an offset from the kernels idea of > PHYS_OFFSET, it's not going to find the ATAGs. Is this going to be a real issue in practice? > So we're probably better off checking whether the platform provided ATAG > pointer is below our idea of PHYS_OFFSET, and if it is to ignore that. The code already does that. Alternatively, I can make the boot_params values into numeric values instead of PLAT_PHYS_OFFSET or any other symbolic define I'd like to get rid of. What do you think? Nicolas
On Fri, 8 Jul 2011, Nicolas Pitre wrote: > On Fri, 8 Jul 2011, Russell King - ARM Linux wrote: > > > On Tue, Jul 05, 2011 at 10:46:46PM -0400, Nicolas Pitre wrote: > > > The boot_params member of the mdesc structure is used to provide a > > > default physical address for the ATAG list. Since this value is fixed > > > at compile time and often based on ARCH_PHYS_OFFSET, it gets in the way > > > of runtime PHYS_OFFSET usage. > > > > > > Let's introduce atag_offset which should contains only the relative > > > offset from start of memory instead of an absolute value, in preparation > > > to move all instance of boot_params over to it. > > > > I don't like this. > > > > While I understand the issue (the oops when we try to dereference the > > absolute address), the fact is that's how it is. > > > > Take a moment to think about this. The ATAG list is normally setup at a > > fixed physical address by the boot loader. While there are some boot > > loaders which allow you to load the kernel at different physical addresses, > > they still place the ATAG list at that same physical address. > > Would they really load the kernel at a different address? > > > That means if you change this into an offset from the kernels idea of > > PHYS_OFFSET, it's not going to find the ATAGs. > > Is this going to be a real issue in practice? > > > So we're probably better off checking whether the platform provided ATAG > > pointer is below our idea of PHYS_OFFSET, and if it is to ignore that. > > The code already does that. > > Alternatively, I can make the boot_params values into numeric values > instead of PLAT_PHYS_OFFSET or any other symbolic define I'd like to get > rid of. What do you think? After looking at it some more, this can't work either as on ep93xx the RAM offset is currently selected a kernel config time and we do want to get rid of that. Furthermore, the code in setup_machine_tags() always validates the atags pointer with ATAG_CORE and assigns it the address of init_tags otherwise. Given that the atag offset is always within the first megabyte which is always mapped, the kernel won't crash even if the ATAGs are not there. Nicolas
On Monday, July 11, 2011 7:59 AM, Nicolas Pitre wrote: > After looking at it some more, this can't work either as on ep93xx the > RAM offset is currently selected a kernel config time and we do want to > get rid of that. The ep93xx actually uses the same relative offset for the boot_params (+0x100) for all memory configurations. The issue is the base physical address of the system memory. There are four sdram chip selects available on the ep93xx. These, along with the ASDO boot option pin, define the base address for the sdram. nSDCS3, ASDO=1 0x00000000 nSDCS3, ASDO=0 0xf0000000 nSDCS2, ASDO=x 0xe0000000 nSDCS1, ASDO=x 0xd0000000 nSDCS0, ASDO=x 0xc0000000 Unfortunately, all the ep93xx boards I have are designed with the sdram using nSDCS0 so I can't test the other boot configurations. Regardless, with these patches, and having ARM_PATCH_PHYS_VIRT enabled, I am able to boot all of my boards successfully. The EDB9315 dev board can be jumper selected so that the sdram is connected to either nSDSC0 or nSDCS3 (default). The EDB9312 dev board can be jumper selected to use any of the chip selects for the sdram. If anyone has access to either of these boards and could test these patches it would be a great help. Before the ARM_PATCH_PHYS_VIRT patch the Kconfig options to select the "EP93xx first SDRAM bank selection" was needed to limit the board selections so that only board which shared an SDRAM chip select configuration could be selected. With ARM_PATCH_PHYS_VIRT I don't think this is needed any longer. But, if the boot_params are still going to be used, the bank selection is still needed so that the physical address of the boot_params is correct. Changing it to an atag_offset makes it work on the ep93xx. Just my 2 cents.... Regards, Hartley
H Hartley Sweeten <hartleys@visionengravers.com> [2011-07-11 12:17:25]: > Unfortunately, all the ep93xx boards I have are designed with the sdram using > nSDCS0 so I can't test the other boot configurations. I've tested that actual patchset from nico/redux git branch[1,2] on ts-7250/32M and ts-7300/128M (both nSDCS3) and it boots, so you can add my tested-by if you like: Tested-by: Petr Štetiar <ynezz@true.cz> 1. git://git.linaro.org/people/nico/linux.git redux 2. git hash bc869e434e411ee7f3fcfed7594ee64b1d56b3c9 -- ynezz
On Tuesday, July 12, 2011 1:48 PM, Petr Štetiar wrote: > H Hartley Sweeten <hartleys@visionengravers.com> [2011-07-11 12:17:25]: > >> Unfortunately, all the ep93xx boards I have are designed with the sdram using >> nSDCS0 so I can't test the other boot configurations. > > I've tested that actual patchset from nico/redux git branch[1,2] on > ts-7250/32M and ts-7300/128M (both nSDCS3) and it boots, so you can add my > tested-by if you like: > > Tested-by: Petr Štetiar <ynezz@true.cz> > > 1. git://git.linaro.org/people/nico/linux.git redux > 2. git hash bc869e434e411ee7f3fcfed7594ee64b1d56b3c9 So, I guess the big test would be to see if a kernel that boots on your boards using nSDCS3 will also boot on mine using nSDCS0. Could you rebuild your kernel with MACH_EDB9307A also selected in your config? After verifying that it still boots please either send me the zImage or place it somewhere I can grab it and I will make sure it boots on mine. Regards, Hartley
On Tue, 12 Jul 2011, Petr Štetiar wrote: > H Hartley Sweeten <hartleys@visionengravers.com> [2011-07-11 12:17:25]: > > > Unfortunately, all the ep93xx boards I have are designed with the sdram using > > nSDCS0 so I can't test the other boot configurations. > > I've tested that actual patchset from nico/redux git branch[1,2] on > ts-7250/32M and ts-7300/128M (both nSDCS3) and it boots, so you can add my > tested-by if you like: > > Tested-by: Petr Štetiar <ynezz@true.cz> Thanks. Nicolas
H Hartley Sweeten <hartleys@visionengravers.com> [2011-07-12 16:39:47]: > Could you rebuild your kernel with MACH_EDB9307A also selected in your > config? After verifying that it still boots please either send me the zImage > or place it somewhere I can grab it and I will make sure it boots on mine. Verified, kernel[1,2] boots. Source tree[3] used to build it (including config etc). 1. http://ynezz.true.cz/redux/zImage 2. http://ynezz.true.cz/redux/zImage.md5sum 3. https://github.com/ynezz/linux-2.6/commits/test-redux -- ynezz
On Thursday, July 14, 2011 11:49 PM, Petr Štetiar wrote: > H Hartley Sweeten <hartleys@visionengravers.com> [2011-07-12 16:39:47]: > >> Could you rebuild your kernel with MACH_EDB9307A also selected in your >> config? After verifying that it still boots please either send me the zImage >> or place it somewhere I can grab it and I will make sure it boots on mine. > > Verified, kernel[1,2] boots. Source tree[3] used to build it (including config etc). > > 1. http://ynezz.true.cz/redux/zImage > 2. http://ynezz.true.cz/redux/zImage.md5sum > 3. https://github.com/ynezz/linux-2.6/commits/test-redux Well... Partial success... The kernel does boot, but then panic's when trying to mount the root fs. All my development boards are setup to pull the rootfs from a tftp server and then use root=/dev/ram as the rootfs. This kernel must be missing something for that to work. Regardless, the kernel _does_ boot so the phys offset is correctly being worked out at run-time and the memory configuration looks correct so the atags offset must be working. So, as far as the ep39xx goes, for this patchset you can have either of these: Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com> Tested-by: H Hartley Sweeten <hsweeten@visionengravers.com> Regards, Hartley
On Fri, 15 Jul 2011, H Hartley Sweeten wrote: > On Thursday, July 14, 2011 11:49 PM, Petr Štetiar wrote: > > H Hartley Sweeten <hartleys@visionengravers.com> [2011-07-12 16:39:47]: > > > >> Could you rebuild your kernel with MACH_EDB9307A also selected in your > >> config? After verifying that it still boots please either send me the zImage > >> or place it somewhere I can grab it and I will make sure it boots on mine. > > > > Verified, kernel[1,2] boots. Source tree[3] used to build it (including config etc). > > > > 1. http://ynezz.true.cz/redux/zImage > > 2. http://ynezz.true.cz/redux/zImage.md5sum > > 3. https://github.com/ynezz/linux-2.6/commits/test-redux > > Well... Partial success... > > The kernel does boot, but then panic's when trying to mount the root fs. > > All my development boards are setup to pull the rootfs from a tftp server > and then use root=/dev/ram as the rootfs. This kernel must be missing something > for that to work. > > Regardless, the kernel _does_ boot so the phys offset is correctly being > worked out at run-time and the memory configuration looks correct so the > atags offset must be working. > > So, as far as the ep39xx goes, for this patchset you can have either of these: > > Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com> > > Tested-by: H Hartley Sweeten <hsweeten@visionengravers.com> Thanks a lot for the testing. This validates the basic concept. Nicolas
diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h index 7b79a00..1bdf6e1 100644 --- a/arch/arm/include/asm/mach/arch.h +++ b/arch/arm/include/asm/mach/arch.h @@ -18,6 +18,7 @@ struct machine_desc { unsigned int nr; /* architecture number */ const char *name; /* architecture name */ unsigned long boot_params; /* tagged list */ + unsigned long atag_offset; /* tagged list (relative) */ const char **dt_compat; /* array of device tree * 'compatible' strings */ diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c index e0db84d..4cc3e2b 100644 --- a/arch/arm/kernel/setup.c +++ b/arch/arm/kernel/setup.c @@ -816,6 +816,8 @@ static struct machine_desc * __init setup_machine_tags(unsigned int nr) if (__atags_pointer) tags = phys_to_virt(__atags_pointer); + else if(mdesc->atag_offset) + tags = PAGE_OFFSET + mdesc->atag_offset; else if (mdesc->boot_params) { #ifdef CONFIG_MMU /*
The boot_params member of the mdesc structure is used to provide a default physical address for the ATAG list. Since this value is fixed at compile time and often based on ARCH_PHYS_OFFSET, it gets in the way of runtime PHYS_OFFSET usage. Let's introduce atag_offset which should contains only the relative offset from start of memory instead of an absolute value, in preparation to move all instance of boot_params over to it. Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org> --- arch/arm/include/asm/mach/arch.h | 1 + arch/arm/kernel/setup.c | 2 ++ 2 files changed, 3 insertions(+), 0 deletions(-)