From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Wed, 5 Aug 2020 11:12:01 +0200 Subject: [PATCH v1 03/24] pci: pci-uclass: Dynamically allocate the PCI regions In-Reply-To: References: <20200724100856.1482324-1-sr@denx.de> <20200724100856.1482324-4-sr@denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Marek, On 30.07.20 17:16, Stefan Roese wrote: > Hi Simon, > > On 28.07.20 21:01, Simon Glass wrote: >> Hi Stefan, >> >> On Fri, 24 Jul 2020 at 04:09, Stefan Roese wrote: >>> >>> Instead of using a fixed length pre-allocated array of regions, this >>> patch moves to dynamically allocating the regions based on the number >>> of available regions plus the necessary regions for DRAM banks. >>> >>> Since MAX_PCI_REGIONS is not needed any more, its removed completely >>> with this patch. >>> >>> Signed-off-by: Stefan Roese >>> Cc: Simon Glass >>> Cc: Bin Meng >>> Cc: Thierry Reding >>> Cc: Marek Vasut >>> >>> --- >>> >>> Changes in v1: >>> - New patch, replaces increase of MAX_PCI_REGIONS to 10 >>> >>> ? board/renesas/rcar-common/common.c | 10 +++++----- >>> ? drivers/pci/pci-uclass.c?????????? | 14 ++++++++------ >>> ? include/pci.h????????????????????? |? 4 +--- >>> ? 3 files changed, 14 insertions(+), 14 deletions(-) >>> >> >> Can you please split out the generic PCI changes into a separate patch? > > Okay, will do. > >>> diff --git a/board/renesas/rcar-common/common.c >>> b/board/renesas/rcar-common/common.c >>> index 83dd288847..83440c11ef 100644 >>> --- a/board/renesas/rcar-common/common.c >>> +++ b/board/renesas/rcar-common/common.c >>> @@ -58,12 +58,12 @@ int ft_board_setup(void *blob, struct bd_info *bd) >>> ???????? uclass_foreach_dev(dev, uc) { >>> ???????????????? struct pci_controller hose = { 0 }; >>> >>> -?????????????? for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) { >>> -?????????????????????? if (hose.region_count == MAX_PCI_REGIONS) { >>> -?????????????????????????????? printf("maximum number of regions >>> parsed, aborting\n"); >>> -?????????????????????????????? break; >>> -?????????????????????? } >>> +?????????????? /* Dynamically allocate the regions array */ >> >> Why is the driver allocating this? Shouldn't it happen in pci-uclass.c ? > > I'm not sure, if the PCI init code has been run before this function > is called in all cases. Marek, do you have an answer on this? Marek, again could you please take a look and let us know, why this code is necessary here? And if it is okay to allocate the structs here as well then? Thanks, Stefan > > Thanks, > Stefan > >> >>> +?????????????? hose.regions = (struct pci_region *) >>> +?????????????????????? calloc(1, CONFIG_NR_DRAM_BANKS * >>> +????????????????????????????? sizeof(struct pci_region)); >>> >>> +?????????????? for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) { >>> ???????????????????????? if (bd->bi_dram[i].size) { >>> >>> pci_set_region(&hose.regions[hose.region_count++], >>> ??????????????????????????????????????????????? bd->bi_dram[i].start, >>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c >>> index 69fb46d3f4..0fbbef70c8 100644 >>> --- a/drivers/pci/pci-uclass.c >>> +++ b/drivers/pci/pci-uclass.c >>> @@ -874,6 +874,7 @@ static void decode_regions(struct pci_controller >>> *hose, ofnode parent_node, >>> ???????? struct bd_info *bd = gd->bd; >>> ???????? int cells_per_record; >>> ???????? const u32 *prop; >>> +?????? int max_regions; >>> ???????? int len; >>> ???????? int i; >>> >>> @@ -893,7 +894,13 @@ static void decode_regions(struct pci_controller >>> *hose, ofnode parent_node, >>> ???????? hose->region_count = 0; >>> ???????? debug("%s: len=%d, cells_per_record=%d\n", __func__, len, >>> ?????????????? cells_per_record); >>> -?????? for (i = 0; i < MAX_PCI_REGIONS; i++, len -= cells_per_record) { >>> + >>> +?????? /* Dynamically allocate the regions array */ >>> +?????? max_regions = len / cells_per_record + CONFIG_NR_DRAM_BANKS; >>> +?????? hose->regions = (struct pci_region *) >>> +?????????????? calloc(1, max_regions * sizeof(struct pci_region)); >>> + >>> +?????? for (i = 0; i < max_regions; i++, len -= cells_per_record) { >>> ???????????????? u64 pci_addr, addr, size; >>> ???????????????? int space_code; >>> ???????????????? u32 flags; >>> @@ -943,11 +950,6 @@ static void decode_regions(struct pci_controller >>> *hose, ofnode parent_node, >>> ???????????????? return; >>> >>> ???????? for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) { >>> -?????????????? if (hose->region_count == MAX_PCI_REGIONS) { >>> -?????????????????????? pr_err("maximum number of regions parsed, >>> aborting\n"); >>> -?????????????????????? break; >>> -?????????????? } >>> - >>> ???????????????? if (bd->bi_dram[i].size) { >>> ???????????????????????? pci_set_region(hose->regions + >>> hose->region_count++, >>> ??????????????????????????????????????? bd->bi_dram[i].start, >>> diff --git a/include/pci.h b/include/pci.h >>> index 281f353916..53f1386fd4 100644 >>> --- a/include/pci.h >>> +++ b/include/pci.h >>> @@ -590,8 +590,6 @@ extern void pci_cfgfunc_do_nothing(struct >>> pci_controller* hose, pci_dev_t dev, >>> ? extern void pci_cfgfunc_config_device(struct pci_controller* hose, >>> pci_dev_t dev, >>> ?????????????????????????????????????? struct pci_config_table *); >>> >>> -#define MAX_PCI_REGIONS??????????????? 7 >>> - >>> ? #define INDIRECT_TYPE_NO_PCIE_LINK???? 1 >>> >>> ? /** >>> @@ -632,7 +630,7 @@ struct pci_controller { >>> ????????? * for PCI controllers and a separate UCLASS (or perhaps >>> ????????? * UCLASS_PCI_GENERIC) is used for bridges. >>> ????????? */ >>> -?????? struct pci_region regions[MAX_PCI_REGIONS]; >>> +?????? struct pci_region *regions; >>> ???????? int region_count; >>> >>> ???????? struct pci_config_table *config_table; >>> -- >>> 2.27.0 >>> > > > Viele Gr??e, > Stefan > Viele Gr??e, Stefan -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de