| Message ID | 1433688642-19861-12-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Simon, On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> 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@chromium.org> > --- > > drivers/pci/pci-uclass.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c > index edec93f..4255c02 100644 > --- a/drivers/pci/pci-uclass.c > +++ b/drivers/pci/pci-uclass.c > @@ -495,7 +495,8 @@ 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); > + gd->ram_size < 0x80000000U ? gd->ram_size : 0x80000000U, > + PCI_REGION_MEM | PCI_REGION_SYS_MEMORY); > > return 0; > } > -- I think this is specific to baytrail fsp configuration. It should not be put into a common driver. Regards, Bin
Hi Bin / Simon, On 06/08 10:57, Bin Meng wrote: > Hi Simon, > > On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> 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@chromium.org> > > --- > > > > drivers/pci/pci-uclass.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c > > index edec93f..4255c02 100644 > > --- a/drivers/pci/pci-uclass.c > > +++ b/drivers/pci/pci-uclass.c > > @@ -495,7 +495,8 @@ 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); > > + gd->ram_size < 0x80000000U ? gd->ram_size : 0x80000000U, > > + PCI_REGION_MEM | PCI_REGION_SYS_MEMORY); > > > > return 0; > > } > > -- > > I think this is specific to baytrail fsp configuration. It should not > be put into a common driver. Yes, I agree with Bin, this is likely only a Bay Trail (or maybe a very small number of other processors) configuration. I believe your change here will impact all PCI hosts which have > 2 GiB of RAM. It might be a bit ugly to have an #ifdef in this file, it all seems like nice clean generic code. But maybe it's not a big deal to limit all u-boot to 2 GiB of RAM for this region? Thanks, Andrew
Hi, On 8 June 2015 at 06:32, Andrew Bradford <andrew@bradfordembedded.com> wrote: > Hi Bin / Simon, > > On 06/08 10:57, Bin Meng wrote: >> Hi Simon, >> >> On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> 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@chromium.org> >> > --- >> > >> > drivers/pci/pci-uclass.c | 3 ++- >> > 1 file changed, 2 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c >> > index edec93f..4255c02 100644 >> > --- a/drivers/pci/pci-uclass.c >> > +++ b/drivers/pci/pci-uclass.c >> > @@ -495,7 +495,8 @@ 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); >> > + gd->ram_size < 0x80000000U ? gd->ram_size : 0x80000000U, >> > + PCI_REGION_MEM | PCI_REGION_SYS_MEMORY); >> > >> > return 0; >> > } >> > -- >> >> I think this is specific to baytrail fsp configuration. It should not >> be put into a common driver. > > Yes, I agree with Bin, this is likely only a Bay Trail (or maybe a very > small number of other processors) configuration. I believe your change > here will impact all PCI hosts which have > 2 GiB of RAM. > > It might be a bit ugly to have an #ifdef in this file, it all seems like > nice clean generic code. But maybe it's not a big deal to limit all > u-boot to 2 GiB of RAM for this region? Yes, this will bite us when something else moves PCI to driver model and we may as well fix it now. What is the best option? We could add a pci_max_addr to global_data perhaps? This could be set to ram_size for most machines, and adjusted for x86. I'd prefer to avoid #ifdef CONFIG_X86. Regards, Simon
Hi Simon, On Wed, Jun 24, 2015 at 11:23 AM, Simon Glass <sjg@chromium.org> wrote: > Hi, > > On 8 June 2015 at 06:32, Andrew Bradford <andrew@bradfordembedded.com> wrote: >> Hi Bin / Simon, >> >> On 06/08 10:57, Bin Meng wrote: >>> Hi Simon, >>> >>> On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> 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@chromium.org> >>> > --- >>> > >>> > drivers/pci/pci-uclass.c | 3 ++- >>> > 1 file changed, 2 insertions(+), 1 deletion(-) >>> > >>> > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c >>> > index edec93f..4255c02 100644 >>> > --- a/drivers/pci/pci-uclass.c >>> > +++ b/drivers/pci/pci-uclass.c >>> > @@ -495,7 +495,8 @@ 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); >>> > + gd->ram_size < 0x80000000U ? gd->ram_size : 0x80000000U, >>> > + PCI_REGION_MEM | PCI_REGION_SYS_MEMORY); >>> > >>> > return 0; >>> > } >>> > -- >>> >>> I think this is specific to baytrail fsp configuration. It should not >>> be put into a common driver. >> >> Yes, I agree with Bin, this is likely only a Bay Trail (or maybe a very >> small number of other processors) configuration. I believe your change >> here will impact all PCI hosts which have > 2 GiB of RAM. >> >> It might be a bit ugly to have an #ifdef in this file, it all seems like >> nice clean generic code. But maybe it's not a big deal to limit all >> u-boot to 2 GiB of RAM for this region? > Yep, maybe not a big deal, but it may also break some boards if the allocated buffer for PCI device happens to be above 2 GiB, but that case might be rare (ie: like 3 GiB memory is installed where on most cases we only see boards with 1 GiB, 2 GiB, 4 GiB memory) > Yes, this will bite us when something else moves PCI to driver model > and we may as well fix it now. > > What is the best option? We could add a pci_max_addr to global_data > perhaps? This could be set to ram_size for most machines, and adjusted > for x86. I'd prefer to avoid #ifdef CONFIG_X86. Adding pci_max_addr to global_data sounds good if we want to avoid #ifdefs. It would be good to hear more comments from other domains? Regards, Bin
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index edec93f..4255c02 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -495,7 +495,8 @@ 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); + gd->ram_size < 0x80000000U ? gd->ram_size : 0x80000000U, + PCI_REGION_MEM | PCI_REGION_SYS_MEMORY); return 0; }
Commit afbbd413a fixed this for non-driver-model. Make sure that the driver model code handles this also. Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/pci/pci-uclass.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)