| Message ID | 20160926182917.27531-8-paul.burton@imgtec.com |
|---|---|
| State | Deferred |
| Delegated to: | Daniel Schwierzeck |
| Headers | show |
Hi Paul, On 26 September 2016 at 12:29, Paul Burton <paul.burton@imgtec.com> wrote: > The decode_regions() function in the PCI code presumes that > CONFIG_SYS_SDRAM_BASE is a physical address, which seems reasonable > given that README states that it should be. > > However there is also common code which expects CONFIG_SYS_SDRAM_BASE to > be an address accessible by the CPU, ie. a valid virtual address - > notably gd->ram_top is set to it & various pieces of data are located > relative to that, and getenv_bootm_low() defaults to > CONFIG_SYS_SDRAM_BASE as the lower bound on addresses to load into. Thus > on MIPS CONFIG_SYS_SDRAM_BASE is a virtual address. > > This patch takes the simple approach to fixing this & converts > CONFIG_SYS_SDRAM_BASE to a physical address for use by the PCI code when > built for MIPS. > > Signed-off-by: Paul Burton <paul.burton@imgtec.com> > > --- > > drivers/pci/pci-uclass.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c > index 415c632..26fe0f4 100644 > --- a/drivers/pci/pci-uclass.c > +++ b/drivers/pci/pci-uclass.c > @@ -848,7 +848,9 @@ static int decode_regions(struct pci_controller *hose, const void *blob, > > /* Add a region for our local memory */ > size = gd->ram_size; > -#ifdef CONFIG_SYS_SDRAM_BASE > +#if defined(CONFIG_MIPS) > + base = virt_to_phys((void *)CONFIG_SYS_SDRAM_BASE); > +#elif defined(CONFIG_SYS_SDRAM_BASE) > base = CONFIG_SYS_SDRAM_BASE; > #endif > if (gd->pci_ram_top && gd->pci_ram_top < base + size) > -- > 2.10.0 > Can you find another way? We don't want arch- or board-specific #ifdefs in uclasses or driver code. Regards, Simon
2016-09-27 2:35 GMT+02:00 Simon Glass <sjg@chromium.org>: > Hi Paul, > > On 26 September 2016 at 12:29, Paul Burton <paul.burton@imgtec.com> wrote: >> The decode_regions() function in the PCI code presumes that >> CONFIG_SYS_SDRAM_BASE is a physical address, which seems reasonable >> given that README states that it should be. >> >> However there is also common code which expects CONFIG_SYS_SDRAM_BASE to >> be an address accessible by the CPU, ie. a valid virtual address - >> notably gd->ram_top is set to it & various pieces of data are located >> relative to that, and getenv_bootm_low() defaults to >> CONFIG_SYS_SDRAM_BASE as the lower bound on addresses to load into. Thus >> on MIPS CONFIG_SYS_SDRAM_BASE is a virtual address. >> >> This patch takes the simple approach to fixing this & converts >> CONFIG_SYS_SDRAM_BASE to a physical address for use by the PCI code when >> built for MIPS. >> >> Signed-off-by: Paul Burton <paul.burton@imgtec.com> >> >> --- >> >> drivers/pci/pci-uclass.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c >> index 415c632..26fe0f4 100644 >> --- a/drivers/pci/pci-uclass.c >> +++ b/drivers/pci/pci-uclass.c >> @@ -848,7 +848,9 @@ static int decode_regions(struct pci_controller *hose, const void *blob, >> >> /* Add a region for our local memory */ >> size = gd->ram_size; >> -#ifdef CONFIG_SYS_SDRAM_BASE >> +#if defined(CONFIG_MIPS) >> + base = virt_to_phys((void *)CONFIG_SYS_SDRAM_BASE); >> +#elif defined(CONFIG_SYS_SDRAM_BASE) >> base = CONFIG_SYS_SDRAM_BASE; >> #endif >> if (gd->pci_ram_top && gd->pci_ram_top < base + size) >> -- >> 2.10.0 >> > > Can you find another way? We don't want arch- or board-specific > #ifdefs in uclasses or driver code. > I suggest that we fix all MIPS boards to configure CONFIG_SYS_SDRAM_BASE as physical address. I guess that would be simply 0 for all current supported MIPS boards. In "arch/mips/" there are only two users ("arch/mips/cpu/start.S" and "arch/mips/lib/bootm.c") which could be easily fixed to map CONFIG_SYS_SDRAM_BASE to KSEG0.
On Tuesday, 27 September 2016 12:01:32 BST Daniel Schwierzeck wrote: > 2016-09-27 2:35 GMT+02:00 Simon Glass <sjg@chromium.org>: > > Hi Paul, > > > > On 26 September 2016 at 12:29, Paul Burton <paul.burton@imgtec.com> wrote: > >> The decode_regions() function in the PCI code presumes that > >> CONFIG_SYS_SDRAM_BASE is a physical address, which seems reasonable > >> given that README states that it should be. > >> > >> However there is also common code which expects CONFIG_SYS_SDRAM_BASE to > >> be an address accessible by the CPU, ie. a valid virtual address - > >> notably gd->ram_top is set to it & various pieces of data are located > >> relative to that, and getenv_bootm_low() defaults to > >> CONFIG_SYS_SDRAM_BASE as the lower bound on addresses to load into. Thus > >> on MIPS CONFIG_SYS_SDRAM_BASE is a virtual address. > >> > >> This patch takes the simple approach to fixing this & converts > >> CONFIG_SYS_SDRAM_BASE to a physical address for use by the PCI code when > >> built for MIPS. > >> > >> Signed-off-by: Paul Burton <paul.burton@imgtec.com> > >> > >> --- > >> > >> drivers/pci/pci-uclass.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c > >> index 415c632..26fe0f4 100644 > >> --- a/drivers/pci/pci-uclass.c > >> +++ b/drivers/pci/pci-uclass.c > >> @@ -848,7 +848,9 @@ static int decode_regions(struct pci_controller > >> *hose, const void *blob,>> > >> /* Add a region for our local memory */ > >> size = gd->ram_size; > >> > >> -#ifdef CONFIG_SYS_SDRAM_BASE > >> +#if defined(CONFIG_MIPS) > >> + base = virt_to_phys((void *)CONFIG_SYS_SDRAM_BASE); > >> +#elif defined(CONFIG_SYS_SDRAM_BASE) > >> > >> base = CONFIG_SYS_SDRAM_BASE; > >> > >> #endif > >> > >> if (gd->pci_ram_top && gd->pci_ram_top < base + size) > >> > >> -- > >> 2.10.0 > > > > Can you find another way? We don't want arch- or board-specific > > #ifdefs in uclasses or driver code. > > I suggest that we fix all MIPS boards to configure > CONFIG_SYS_SDRAM_BASE as physical address. I guess that would be > simply 0 for all current supported MIPS boards. In "arch/mips/" there > are only two users ("arch/mips/cpu/start.S" and > "arch/mips/lib/bootm.c") which could be easily fixed to map > CONFIG_SYS_SDRAM_BASE to KSEG0. Hi Daniel & Simon, So doing that led to the 27 patch series I just sent out. It's never simple :) It's not so much the 2 users in arch/mips/ as the generic uses which cause the trouble - and avoiding them needs something like phys_to_virt() so the bulk of that series is about providing that for all architectures. FYI CONFIG_SYS_SDRAM_BASE is zero for all MIPS boards except the PIC32 one. I've tested the series on a 32 bit Malta & a 64 bit boston, and build tested a bunch of other boards. The only thing to really take note of is that for MIPS64 systems using a physical CONFIG_SYS_SDRAM_BASE and phys_to_virt we end up accessing memory through xkphys instead of ckseg0. Nothing seems to have exploded too badly from that (just the boston board_get_usable_ram_top() needed adjusting) but it's probably worth pointing out. Thanks, Paul
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index 415c632..26fe0f4 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -848,7 +848,9 @@ static int decode_regions(struct pci_controller *hose, const void *blob, /* Add a region for our local memory */ size = gd->ram_size; -#ifdef CONFIG_SYS_SDRAM_BASE +#if defined(CONFIG_MIPS) + base = virt_to_phys((void *)CONFIG_SYS_SDRAM_BASE); +#elif defined(CONFIG_SYS_SDRAM_BASE) base = CONFIG_SYS_SDRAM_BASE; #endif if (gd->pci_ram_top && gd->pci_ram_top < base + size)
The decode_regions() function in the PCI code presumes that CONFIG_SYS_SDRAM_BASE is a physical address, which seems reasonable given that README states that it should be. However there is also common code which expects CONFIG_SYS_SDRAM_BASE to be an address accessible by the CPU, ie. a valid virtual address - notably gd->ram_top is set to it & various pieces of data are located relative to that, and getenv_bootm_low() defaults to CONFIG_SYS_SDRAM_BASE as the lower bound on addresses to load into. Thus on MIPS CONFIG_SYS_SDRAM_BASE is a virtual address. This patch takes the simple approach to fixing this & converts CONFIG_SYS_SDRAM_BASE to a physical address for use by the PCI code when built for MIPS. Signed-off-by: Paul Burton <paul.burton@imgtec.com> --- drivers/pci/pci-uclass.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)