On Thu, Mar 01, 2018 at 03:40:30PM +0100, Niklas Cassel wrote: > On Wed, Feb 28, 2018 at 02:21:48PM +0000, Lorenzo Pieralisi wrote: > > On Tue, Feb 27, 2018 at 12:59:05PM +0100, Niklas Cassel wrote: > > > A 64-bit BAR uses the succeeding BAR for the upper bits, therefore > > > we cannot call pci_epc_set_bar() on a BAR that follows a 64-bit BAR. > > > > > > If pci_epc_set_bar() is called with flag PCI_BASE_ADDRESS_MEM_TYPE_64, > > > > PCI_BASE_ADDRESS_MEM_TYPE_64 is detected through a sizeof(dma_addr_t). > > > > I thought we decided to describe the BARs not as dma_addr_t + size but > > as resources, which would allow you to have flags describing their > > actual size. > > > > Current code has a fixed BAR size defined by the dma_addr_t type size > > and I do not think that's what we really want. > > You suggested to Kishon that the bar member array should be refactored > to be an array of struct resources: > > https://marc.info/?l=linux-pci&m=151851776921594&w=2 > > That refactoring would be a good thing, > but can't it be done after the merge of this patch? This patch does not look right to me - a BAR size should not be determined by sizeof(dma_addr_t) and on top of that this code is not easy to comprehend. > I'm guessing that one of the reasons why you want this > refactoring is so that you can actually call pci_epc_set_bar() > on each array member, so that you don't need my patch: > > > + if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) > > > + bar++; > > However, if we want to omit this, I think that other changes would be needed. > > Let's say that resource[x] is 64-bit, then resource[x+1] must not have > (IORESOURCE_MEM or IORESOURCE_IO) set. > > It is important that BAR[x+1] == 0, for a 64-bit BAR. > > So either pci_epc_set_bar() or epc->ops->set_bar() > must know to not touch its BAR (or BAR mask) register > if neither of those flags are set. > > It's probably easier to change pci_epc_set_bar(), rather than > changing all epc->ops->set_bar() implementations, > here is e.g. part of set_bar() for DesignWare: > > static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, > enum pci_barno bar, > dma_addr_t bar_phys, size_t size, int flags) > { > int ret; > struct dw_pcie_ep *ep = epc_get_drvdata(epc); > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > enum dw_pcie_as_type as_type; > u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar); > > if (!(flags & PCI_BASE_ADDRESS_SPACE)) > as_type = DW_PCIE_AS_MEM; > else > as_type = DW_PCIE_AS_IO; > > > So I still see a point in having this patch, at least until > someone has refactored the code to use resource + modified pci_epc_set_bar(). But the point is that, IIUC (because again - this code path is really confusing) it is up to the specific implementation to turn a struct pci_epf_bar into a *real* BAR and that's implementation specific (even though there must be a 1:1 relationship between struct pci_epf_bar.size and the actual BAR size in the device, ie if struct pci_epf_bar.size exceeds 32-bit space we need a 64-bit BAR to represent it). I assume the problem here is having a 1:1 index mapping between the struct pci_epf.bar[6] array and the BAR index programmed into the device. If that's the case - I see two options: - Use struct epf_bar.size to drive the reg increment (ie reg++ if size > 4G) - Add a return value to struct pci_epc.ops.set_bar() that returns whether a 64-bit BAR was set-up (which is the same __pci_read_base() does in PCI core) Please let me know if I have not got this right so that we can make progress. Thanks, Lorenzo > > How is (in HW I mean) actually the BAR size configured in a given EPF ? > > For the DesignWare PCIe controller (assuming it has been synthesized with > Programmable Masks), to configure a BAR, you set the usual prefetch+type bits > in the standard PCI BAR register, but then the controller also has, for each > bar, a special BAR mask register, which dw_pcie_ep_set_bar() sets to (size-1), > this defines which bits that will be writable by the RC (and the RC can figure > out the size of the BAR by writing all 1's as usual). > > For a 64-bit Memory Space BAR of size 16 GiB (prefetchable), you would set: > BAR[x] = 0000 0000 0000 0000 0000 0000 0000 1100 > BAR[x+1] = 0000 0000 0000 0000 0000 0000 0000 0000 > > BAR_MASK[x] = 1111 1111 1111 1111 1111 1111 1111 1111 > BAR_MASK[x+1] = 0000 0000 0000 0000 0000 0000 0000 0011 > > > > I guess that instead of patch 3/3 in this patch series, > we could do: > > --- a/drivers/pci/dwc/pcie-designware-ep.c > +++ b/drivers/pci/dwc/pcie-designware-ep.c > @@ -136,8 +136,15 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, > return ret; > > dw_pcie_dbi_ro_wr_en(pci); > - dw_pcie_writel_dbi2(pci, reg, size - 1); > - dw_pcie_writel_dbi(pci, reg, flags); > + if (upper_32_bits(size)) { > + dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1)); > + dw_pcie_writel_dbi(pci, reg, flags); > + dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1)); > + dw_pcie_writel_dbi(pci, reg + 4, 0); > + } else { > + dw_pcie_writel_dbi2(pci, reg, size - 1); > + dw_pcie_writel_dbi(pci, reg, flags); > + } > dw_pcie_dbi_ro_wr_dis(pci); > > return 0; > > However, since I don't have access to a 64-bit CPU with loads of RAM, > that has the DesignWare PCIe controller, that patch is completely untested. > But if everyone agrees, we could replace 3/3 of this series with that. > > > > > > Thanks, > > Lorenzo > > > > > it has to be up to the controller driver to write both BAR[x] and BAR[x+1] > > > (and BAR_mask[x] and BAR_mask[x+1]). > > > > > > Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxxx> > > > --- > > > drivers/pci/endpoint/functions/pci-epf-test.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > > > index 64d8a17f8094..ecbf6a7750dc 100644 > > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > > > @@ -382,6 +382,8 @@ static int pci_epf_test_set_bar(struct pci_epf *epf) > > > if (bar == test_reg_bar) > > > return ret; > > > } > > > + if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) > > > + bar++; > > > } > > > > > > return 0; > > > -- > > > 2.14.2 > > >
