| Message ID | 20240610235048.319266-1-shyamsaini@linux.microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] drivers: pci: dwc: configure multiple atu regions | expand |
On Mon, Jun 10, 2024 at 04:50:48PM -0700, Shyam Saini wrote: Subject should be 'PCI: dwc: ...' > Before this change, the dwc PCIe driver configures only 1 ATU region, > which is sufficient for the devices with PCIe memory <= 4GB. However, > the driver probe fails when device uses more than 4GB of pcie memory. > Something is not clear... This commit message implies that the driver used to work on your hardware (you haven't mentioned which one it is) and broken by the commit from Sergey. As per Sergey's commit, we auto detect the dw_pcie::region_limit. If the IP is < 4.60, then the limit is 4G, otherwise depends on CX_ATU_MAX_REGION_SIZE set in hw. So if your IP is < 4.60, you cannot map > 4GB of outbound memory anyway. But if it is > 4.60, you shouldn't see the failure that you reported for > 4G space (well you can see the failure if the limit is less than the region size). In the previous thread you mentioned that dw_pcie::region_limit is set to 4G. So how come your driver was working previously? PS: When reporting issue like this, please add info about your hardware also (platform, controller driver used etc...). - Mani > Fix this by configuring multiple ATU regions for the devices which > use more than 4GB of PCIe memory. > > Given each 4GB block of memory requires a new ATU region, the total > number of ATU regions are calculated using the size of PCIe device > tree node's MEM64 pref range size. > > Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com> > --- > .../pci/controller/dwc/pcie-designware-host.c | 38 +++++++++++++++++-- > 1 file changed, 34 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index d15a5c2d5b48..bed0b189b6ad 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -652,6 +652,33 @@ static struct pci_ops dw_pcie_ops = { > .write = pci_generic_config_write, > }; > > +static int dw_pcie_num_atu_regions(struct resource_entry *entry) > +{ > + return DIV_ROUND_UP(resource_size(entry->res), SZ_4G); > +} > + > +static int dw_pcie_prog_outbound_atu_multi(struct dw_pcie *pci, int type, > + struct resource_entry *entry) > +{ > + int idx, ret, num_regions; > + > + num_regions = dw_pcie_num_atu_regions(entry); > + > + for (idx = 0; idx < num_regions; idx++) { > + dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, idx); > + ret = dw_pcie_prog_outbound_atu(pci, idx, PCIE_ATU_TYPE_MEM, > + entry->res->start, > + entry->res->start - entry->offset, > + resource_size(entry->res)/4); > + > + if (ret) > + goto err; > + } > + > +err: > + return ret; > +} > + > static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > @@ -682,10 +709,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp) > if (pci->num_ob_windows <= ++i) > break; > > - ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM, > - entry->res->start, > - entry->res->start - entry->offset, > - resource_size(entry->res)); > + if (resource_size(entry->res) > SZ_4G) > + ret = dw_pcie_prog_outbound_atu_multi(pci, PCIE_ATU_TYPE_MEM, entry); > + else > + ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM, > + entry->res->start, > + entry->res->start - entry->offset, > + resource_size(entry->res)); > if (ret) { > dev_err(pci->dev, "Failed to set MEM range %pr\n", > entry->res); > -- > 2.34.1 >
Hi Manivannan, >> Before this change, the dwc PCIe driver configures only 1 ATU region, >> which is sufficient for the devices with PCIe memory <= 4GB. However, >> the driver probe fails when device uses more than 4GB of pcie memory. >> > Something is not clear... This commit message implies that the driver used to > work on your hardware (you haven't mentioned which one it is) and broken by the > commit from Sergey. sorry for any confusion, the driver use to work in v5.10 kernel, with v6.0 kernel it fails to probe with following messages: layerscape-pcie xx00000.pcie: Failed to set MEM range ... layerscape-pcie: probe of xx00000.pcie failed with error -22 By tracing code, I found that the probe was failing on [1] this check, which was added in [2] upstream commit from Serge and finally, the ATU upper bound limit was set to 4G in [3] commit pci driver probe succeeds either when 1) I remove [1] check 2) or by setting the ATU limit to the size of Mem64 range (my original patch [4]) > As per Sergey's commit, we auto detect the dw_pcie::region_limit. If the IP is < > 4.60, then the limit is 4G, otherwise depends on CX_ATU_MAX_REGION_SIZE set in > hw. I couldn't find any reference of CX_ATU_MAX_REGION_SIZE in my PCIe TRM, perhaps because it is older than v4.60 > So if your IP is < 4.60, you cannot map > 4GB of outbound memory anyway. But if > it is > 4.60, you shouldn't see the failure that you reported for > 4G space > (well you can see the failure if the limit is less than the region size). In the > previous thread you mentioned that dw_pcie::region_limit is set to 4G. So how > come your driver was working previously? The PCIe IP is from Synopsys and is older than 4.60, the board is from Freescale LX2 family. The board uses drivers/pci/controller/dwc/pci-layerscape.c driver Given PCIe IP is older than 4.60, I am not sure why it was working earlier for a memory range larger than 4G Highly appreciate your guidance and help on this. Best regards, Shyam [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-designware.c?h=v6.10-rc3#n480 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/pci/controller/dwc?h=v6.10-rc3&id=edf408b946d37cc70fbf0db6ab85bbf67dae1894 [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/pci/controller/dwc?h=v6.10-rc3&id=89473aa9ab261948ed13b16effe841a675efed77 [4] https://lore.kernel.org/linux-pci/20240523001046.1413579-1-shyamsaini@linux.microsoft.com/
Hi Shyam On Thu, Jun 13, 2024 at 03:47:47PM -0700, Shyam Saini wrote: > Hi Manivannan, > > >> Before this change, the dwc PCIe driver configures only 1 ATU region, > >> which is sufficient for the devices with PCIe memory <= 4GB. However, > >> the driver probe fails when device uses more than 4GB of pcie memory. > >> > > Something is not clear... This commit message implies that the driver used to > > work on your hardware (you haven't mentioned which one it is) and broken by the > > commit from Sergey. > > sorry for any confusion, the driver use to work in v5.10 kernel, with v6.0 kernel it > fails to probe with following messages: > layerscape-pcie xx00000.pcie: Failed to set MEM range ... > layerscape-pcie: probe of xx00000.pcie failed with error -22 > > By tracing code, I found that the probe was failing on [1] this check, > which was added in [2] upstream commit from Serge and finally, the ATU upper bound limit was > set to 4G in [3] commit > > pci driver probe succeeds either when > 1) I remove [1] check > 2) or by setting the ATU limit to the size of Mem64 range (my original patch [4]) > > > As per Sergey's commit, we auto detect the dw_pcie::region_limit. If the IP is < > > 4.60, then the limit is 4G, otherwise depends on CX_ATU_MAX_REGION_SIZE set in > > hw. > > I couldn't find any reference of CX_ATU_MAX_REGION_SIZE in my PCIe TRM, perhaps because it > is older than v4.60 Please find the line containing "iATU: unroll " in the boot log and copy it here. Also please provide a content of the dw_pcie::version field _after_ the dw_pcie_version_detect() method is executed. > > > So if your IP is < 4.60, you cannot map > 4GB of outbound memory anyway. But if > > it is > 4.60, you shouldn't see the failure that you reported for > 4G space > > (well you can see the failure if the limit is less than the region size). In the > > previous thread you mentioned that dw_pcie::region_limit is set to 4G. So how > > come your driver was working previously? > > The PCIe IP is from Synopsys and is older than 4.60, If you know what the actual IP-core version is and it's older than 4.70a, then it's better to pre-define the version in the drivers/pci/controller/dwc/pci-layerscape.c probe procedure (like it's done in drivers/pci/controller/dwc/pci-keystone.c, drivers/pci/controller/dwc/pcie-bt1.c). > the board is from Freescale LX2 > family. I failed to find the LX2 PCIe controller support in the drivers/pci/controller/dwc/pci-layerscape.c driver. AFAICS the drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c driver is responsible for working with the LX2 PCIe host. Am I missing something? Anyway I've checked all the DT-nodes defined for the Freescale Layerscape PCIe host controllers. None of them have the PCIe ranges defined with the window size greater than 4G (actually all of them of the 1G size). So the next question is: what DT source have you been using on your platform and what is the DT-node defined for the PCIe host controller in there? > The board uses drivers/pci/controller/dwc/pci-layerscape.c driver > Given PCIe IP is older than 4.60, I am not sure why it was working earlier for a memory range larger than 4G > Highly appreciate your guidance and help on this. It has been working earlier because the kernel 5.10 didn't have PCI ranges check for not exceeding the iATU regions maximum limit. But once again, the DW PCIe Root-port driver has been designed to allocate a _single_ iATU region for each PCIe memory ranges. Thus if the ranges exceeds the maximum limit, the mapping won't work for the addresses greater than the maximum limit. I've already explained it here: https://lore.kernel.org/linux-pci/hxluc6qth4temdyxloekbhoy4iielyvxmmhp3u47qwtcxb5t5v@v5hdzvqmrsyv Moreover the IP-core databook not only explicitly prohibits to define the iATU ranges greater than 4GB on the DW PCIe device older than 4.60a (text from v4.21a databook), but has more strict constraint of the regions not crossing the 4GB boundary: "The upper 32 bits of the Target Address register always forms the upper 32 bits of the translated address because: ■ The maximum region size is 4 GB. ■ A region must not cross a 4 GB boundary." (It's obvious though since the iATU Limit Address register is of the 32-bit size on the old controllers.) So you either need to allocate several iATU regions to cover the requested PCIe ranges, or fix the PCIe controller DT-node to having the ranges not exceeding the maximum limit. -Serge(y) > > Best regards, > Shyam > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-designware.c?h=v6.10-rc3#n480 > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/pci/controller/dwc?h=v6.10-rc3&id=edf408b946d37cc70fbf0db6ab85bbf67dae1894 > [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/pci/controller/dwc?h=v6.10-rc3&id=89473aa9ab261948ed13b16effe841a675efed77 > [4] https://lore.kernel.org/linux-pci/20240523001046.1413579-1-shyamsaini@linux.microsoft.com/ > > > >
On Mon, Jun 10, 2024 at 04:50:48PM -0700, Shyam Saini wrote: > Before this change, the dwc PCIe driver configures only 1 ATU region, > which is sufficient for the devices with PCIe memory <= 4GB. However, > the driver probe fails when device uses more than 4GB of pcie memory. > > Fix this by configuring multiple ATU regions for the devices which > use more than 4GB of PCIe memory. > > Given each 4GB block of memory requires a new ATU region, the total > number of ATU regions are calculated using the size of PCIe device > tree node's MEM64 pref range size. > > Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com> > --- > .../pci/controller/dwc/pcie-designware-host.c | 38 +++++++++++++++++-- > 1 file changed, 34 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index d15a5c2d5b48..bed0b189b6ad 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -652,6 +652,33 @@ static struct pci_ops dw_pcie_ops = { > .write = pci_generic_config_write, > }; > > +static int dw_pcie_num_atu_regions(struct resource_entry *entry) > +{ > + return DIV_ROUND_UP(resource_size(entry->res), SZ_4G); > +} > + > +static int dw_pcie_prog_outbound_atu_multi(struct dw_pcie *pci, int type, > + struct resource_entry *entry) > +{ > + int idx, ret, num_regions; > + > + num_regions = dw_pcie_num_atu_regions(entry); > + > + for (idx = 0; idx < num_regions; idx++) { > + dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, idx); > + ret = dw_pcie_prog_outbound_atu(pci, idx, PCIE_ATU_TYPE_MEM, > + entry->res->start, > + entry->res->start - entry->offset, > + resource_size(entry->res)/4); > + > + if (ret) > + goto err; > + } > + > +err: > + return ret; > +} > + > static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > @@ -682,10 +709,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp) > if (pci->num_ob_windows <= ++i) > break; > > - ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM, > - entry->res->start, > - entry->res->start - entry->offset, > - resource_size(entry->res)); > + if (resource_size(entry->res) > SZ_4G) > + ret = dw_pcie_prog_outbound_atu_multi(pci, PCIE_ATU_TYPE_MEM, entry); > + else > + ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM, > + entry->res->start, > + entry->res->start - entry->offset, > + resource_size(entry->res)); Sorry, but the change doesn't look correct. First of all, you suggest to split up _all_ the PCIe ranges greater than 4GB. As I already mentioned several times the DW PCIe controller of v4.60a and younger may have the maximum limit address greater than 4GB. So the change is wrong for the modern IP-core which have the CX_ATU_MAX_REGION_SIZE synthesis parameter set with a number greater than 4GB. Secondly the dw_pcie_iatu_setup() method is looping around all the PCIe memory ranges and allocates the iATU regions one after another. So should the dw_pcie_prog_outbound_atu_multi() allocated more than one region, the second, third, etc iATU regions setup will be overwritten with the subsequent PCIe memory ranges data. Thirdly your dw_pcie_prog_outbound_atu_multi() initializes the iATU regions starting from zero each time it's called. So each next call will override the initialization performed in the framework of the previous one. AFAICS it's not that easy as it seemed at the first glance. One of the possible solution is to fix the __dw_pcie_prog_outbound_atu() method in a way so instead of failing on the ((limit_addr & ~pci->region_limit) != (pci_addr & ~pci->region_limit)) conditional statement the method would initialize the requested region from cpu_addr up to limit_addr = clamp(limit_addr, cpu_addr, cpu_addr | pci->region_limit) and returned (limit_addr - cpu_addr + 1) from __dw_pcie_prog_outbound_atu(). The dw_pcie_prog_outbound_atu() caller shall check the returned value. If it's negative then error happened. If the size is the same as the requested one, then the memory window region was successfully initialized. If the returned value is smaller than the requested one, then the memory window only partly covers the requested region and the next free outbound iATU region needs to be allocated and initialized. Note 1. The suggested semantics need to be propagated to all the dw_pcie_prog_outbound_atu() and dw_pcie_prog_ep_outbound_atu() callers. Note 2. The same procedure can be implemented for the inbound iATU regions initialization procedure in dw_pcie_prog_inbound_atu(). The difference is only in the untranslated address argument. In dw_pcie_prog_inbound_atu() it's pci_addr. So the initialization would be based on the PCI-address space from pci_addr up to limit_addr = clamp(limit_addr, pci_addr, pci_addr | pci->region_limit) with returning (limit_addr - pci_addr + 1). -Serge(y) > if (ret) { > dev_err(pci->dev, "Failed to set MEM range %pr\n", > entry->res); > -- > 2.34.1 >
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index d15a5c2d5b48..bed0b189b6ad 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -652,6 +652,33 @@ static struct pci_ops dw_pcie_ops = { .write = pci_generic_config_write, }; +static int dw_pcie_num_atu_regions(struct resource_entry *entry) +{ + return DIV_ROUND_UP(resource_size(entry->res), SZ_4G); +} + +static int dw_pcie_prog_outbound_atu_multi(struct dw_pcie *pci, int type, + struct resource_entry *entry) +{ + int idx, ret, num_regions; + + num_regions = dw_pcie_num_atu_regions(entry); + + for (idx = 0; idx < num_regions; idx++) { + dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, idx); + ret = dw_pcie_prog_outbound_atu(pci, idx, PCIE_ATU_TYPE_MEM, + entry->res->start, + entry->res->start - entry->offset, + resource_size(entry->res)/4); + + if (ret) + goto err; + } + +err: + return ret; +} + static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp) { struct dw_pcie *pci = to_dw_pcie_from_pp(pp); @@ -682,10 +709,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp) if (pci->num_ob_windows <= ++i) break; - ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM, - entry->res->start, - entry->res->start - entry->offset, - resource_size(entry->res)); + if (resource_size(entry->res) > SZ_4G) + ret = dw_pcie_prog_outbound_atu_multi(pci, PCIE_ATU_TYPE_MEM, entry); + else + ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM, + entry->res->start, + entry->res->start - entry->offset, + resource_size(entry->res)); if (ret) { dev_err(pci->dev, "Failed to set MEM range %pr\n", entry->res);
Before this change, the dwc PCIe driver configures only 1 ATU region, which is sufficient for the devices with PCIe memory <= 4GB. However, the driver probe fails when device uses more than 4GB of pcie memory. Fix this by configuring multiple ATU regions for the devices which use more than 4GB of PCIe memory. Given each 4GB block of memory requires a new ATU region, the total number of ATU regions are calculated using the size of PCIe device tree node's MEM64 pref range size. Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com> --- .../pci/controller/dwc/pcie-designware-host.c | 38 +++++++++++++++++-- 1 file changed, 34 insertions(+), 4 deletions(-)