[U-Boot] [PATCH v2 7/7] dm: x86: baytrail: Correct PCI region 3 when driver model is used

[Simon Glass]

Hi Andrew,

On 29 June 2015 at 12:44, Andrew Bradford <andrew at bradfordembedded.com> wrote:
>
> Hi Simon,
>
> Probably I'm not fully understanding the startup flow and how PCI fits
> into it yet, so can you please help me to understand this patch a little
> more?
>
> On 06/25 11:55, Simon Glass wrote:
> > Commit afbbd413a fixed this for non-driver-model. Make sure that the driver
> > model code handles this also.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > Changes in v2:
> > - Only limit the PCI system memory region on x86 machines
> >
> >  arch/x86/cpu/cpu.c                | 1 +
> >  common/board_f.c                  | 4 ++++
> >  drivers/pci/pci-uclass.c          | 8 ++++++--
> >  include/asm-generic/global_data.h | 1 +
> >  4 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c
> > index d108ee5..936b6ee 100644
> > --- a/arch/x86/cpu/cpu.c
> > +++ b/arch/x86/cpu/cpu.c
> > @@ -351,6 +351,7 @@ int x86_cpu_init_f(void)
> >
> >               gd->arch.has_mtrr = has_mtrr();
> >       }
> > +     gd->pci_ram_top = 0x80000000U;
>
> So this sets gd->pci_ram_top to 2 GiB, then...
>
> >
> >       return 0;
> >  }
> > diff --git a/common/board_f.c b/common/board_f.c
> > index 21be26f..cb85382 100644
> > --- a/common/board_f.c
> > +++ b/common/board_f.c
> > @@ -350,6 +350,10 @@ static int setup_dest_addr(void)
> >               debug("Reserving MP boot page to %08lx\n", gd->relocaddr);
> >       }
> >  #endif
> > +#ifdef CONFIG_PCI
> > +     gd->pci_ram_top = gd->ram_top;
>
> This can set gd->pci_ram_top to be the actual size of memory later in
> boot, which on E3800 with 4 GiB of SDRAM will be 4 GiB (right?), then...
>
> > +#endif
> > +
> >       return 0;
> >  }
> >
> > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> > index edec93f..5b91fe3 100644
> > --- a/drivers/pci/pci-uclass.c
> > +++ b/drivers/pci/pci-uclass.c
> > @@ -444,6 +444,7 @@ static int decode_regions(struct pci_controller *hose, const void *blob,
> >  {
> >       int pci_addr_cells, addr_cells, size_cells;
> >       int cells_per_record;
> > +     phys_addr_t addr;
> >       const u32 *prop;
> >       int len;
> >       int i;
> > @@ -494,8 +495,11 @@ static int decode_regions(struct pci_controller *hose, const void *blob,
> >       }
> >
> >       /* Add a region for our local memory */
> > -     pci_set_region(hose->regions + hose->region_count++, 0, 0,
> > -                    gd->ram_size, PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
> > +     addr = gd->ram_size;
> > +     if (gd->pci_ram_top && gd->pci_ram_top < addr)
> > +             addr = gd->pci_ram_top;
>
> This sets addr to be the lesser of gd->ram_size and gd->pci_ram_top when
> we enumerate a PCI device.  Right?
>
> What is the flow if, for example, a Baytrail E3800 has 4 GiB of RAM (and
> the physical memory hole from 2 GiB to 4 GiB exists), then does this
> code here actually set addr to be 2 GiB like it should?
>
> I don't fear that memory sizes of 2 GiB or less will work fine but I
> don't quite understand if this patch works how I expect for memory sizes
> larger than 2 GiB if the processor has a physical memory hole like E3800
> does.
>
> Why wouldn't the assignment in board_f.c interfere and set
> gd->pci_ram_top to be 4 GiB instead of 2 GiB like it should be?

I think the assignment in board_f.c should be removed. Since there is
a zero check, it is not needed, and it will interfere as you say.
Thanks for picking this up. I'll respin it.

Regards,
Simon