| Message ID | 20240312105152.3457899-8-cassel@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: endpoint: set prefetchable bit for 64-bit BARs | expand |
[+cc Shawn for Rockchip question] On Tue, Mar 12, 2024 at 11:51:47AM +0100, Niklas Cassel wrote: > Ever since commit f25b5fae29d4 ("PCI: endpoint: Setting a BAR size > 4 GB > is invalid if 64-bit flag is not set") it has been impossible to get the > .set_bar() callback with a BAR size > 4 GB, if the BAR was also not > requested to be configured as a 64-bit BAR. > > Thus, forcing setting the 64-bit flag for BARs larger than 4 GB in the > lower level driver is dead code and can be removed. > > It is however possible that an EPF driver configures a BAR as 64-bit, > even if the requested size is < 4 GB. > > Respect the requested BAR configuration, just like how it is already > repected with regards to the prefetchable bit. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > drivers/pci/controller/cadence/pcie-cadence-ep.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c > index 2d0a8d78bffb..de10e5edd1b0 100644 > --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c > +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c > @@ -99,14 +99,11 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn, > ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_IO_32BITS; > } else { > bool is_prefetch = !!(flags & PCI_BASE_ADDRESS_MEM_PREFETCH); > - bool is_64bits = sz > SZ_2G; > + bool is_64bits = !!(flags & PCI_BASE_ADDRESS_MEM_TYPE_64); > > if (is_64bits && (bar & 1)) > return -EINVAL; Not relevant to *this* patch, but it looks like this code assumes that a 64-bit BAR must consist of an even-numbered DWORD followed by an odd-numbered one, e.g., it could be BAR 0 and BAR 1, but not BAR 1 and BAR 2. I don't think the PCIe spec requires that. Does the Cadence hardware require this? What about Rockchip, which has similar code in rockchip_pcie_ep_set_bar()? FWIW, dw_pcie_ep_set_bar() doesn't enforce this restriction. > - if (is_64bits && !(flags & PCI_BASE_ADDRESS_MEM_TYPE_64)) > - epf_bar->flags |= PCI_BASE_ADDRESS_MEM_TYPE_64; > - > if (is_64bits && is_prefetch) > ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS; > else if (is_prefetch) > -- > 2.44.0 >
Hello Bjorn, On Thu, May 16, 2024 at 03:49:07PM -0500, Bjorn Helgaas wrote: > [+cc Shawn for Rockchip question] > > On Tue, Mar 12, 2024 at 11:51:47AM +0100, Niklas Cassel wrote: > > Ever since commit f25b5fae29d4 ("PCI: endpoint: Setting a BAR size > 4 GB > > is invalid if 64-bit flag is not set") it has been impossible to get the > > .set_bar() callback with a BAR size > 4 GB, if the BAR was also not > > requested to be configured as a 64-bit BAR. > > > > Thus, forcing setting the 64-bit flag for BARs larger than 4 GB in the > > lower level driver is dead code and can be removed. > > > > It is however possible that an EPF driver configures a BAR as 64-bit, > > even if the requested size is < 4 GB. > > > > Respect the requested BAR configuration, just like how it is already > > repected with regards to the prefetchable bit. > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > --- > > drivers/pci/controller/cadence/pcie-cadence-ep.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c > > index 2d0a8d78bffb..de10e5edd1b0 100644 > > --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c > > +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c > > @@ -99,14 +99,11 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn, > > ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_IO_32BITS; > > } else { > > bool is_prefetch = !!(flags & PCI_BASE_ADDRESS_MEM_PREFETCH); > > - bool is_64bits = sz > SZ_2G; > > + bool is_64bits = !!(flags & PCI_BASE_ADDRESS_MEM_TYPE_64); > > > > if (is_64bits && (bar & 1)) > > return -EINVAL; > > Not relevant to *this* patch, but it looks like this code assumes that > a 64-bit BAR must consist of an even-numbered DWORD followed by an > odd-numbered one, e.g., it could be BAR 0 and BAR 1, but not BAR 1 and > BAR 2. > > I don't think the PCIe spec requires that. Does the Cadence hardware > require this? The PCIe spec does not seems to require this: "A Type 0 Configuration Space Header has six DWORD locations allocated for Base Address registers starting at offset 10h in Configuration Space. [...] A Function may use any of the locations to implement Base Address registers. An implemented 64-bit Base Address register consumes two consecutive DWORD locations." > What about Rockchip, which has similar code in > rockchip_pcie_ep_set_bar()? > > FWIW, dw_pcie_ep_set_bar() doesn't enforce this restriction. The Synopsys PCIe controller does have this limitation: From the DWC databook: 3.5.7.1.1 Overview Base Address Registers (Offset: 0x100x24) The controller provides three pairs of 32-bit BARs for each implemented function. Each pair (BARs 0 and 1, BARs 2 and 3, BARs 4 and 5) can be configured as follows: -One 64-bit BAR: For example, BARs 0 and 1 are combined to form a single 64-bit BAR. -Two 32-bit BARs: For example, BARs 0 and 1 are two independent 32-bit BARs. -One 32-bit BAR: For example, BAR0 is a 32-bit BAR and BAR1 is either disabled or removed from the controller altogether to reduce gate count. So dw_pcie_ep_set_bar() should probably be fixed to enforce this requirement. (PCI patches for DWC appear to have a tendency to be redirected to /dev/null, so personally I'm not planning on sending out a patch to enforce this.) Kind regards, Niklas
diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c index 2d0a8d78bffb..de10e5edd1b0 100644 --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c @@ -99,14 +99,11 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn, ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_IO_32BITS; } else { bool is_prefetch = !!(flags & PCI_BASE_ADDRESS_MEM_PREFETCH); - bool is_64bits = sz > SZ_2G; + bool is_64bits = !!(flags & PCI_BASE_ADDRESS_MEM_TYPE_64); if (is_64bits && (bar & 1)) return -EINVAL; - if (is_64bits && !(flags & PCI_BASE_ADDRESS_MEM_TYPE_64)) - epf_bar->flags |= PCI_BASE_ADDRESS_MEM_TYPE_64; - if (is_64bits && is_prefetch) ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS; else if (is_prefetch)
Ever since commit f25b5fae29d4 ("PCI: endpoint: Setting a BAR size > 4 GB is invalid if 64-bit flag is not set") it has been impossible to get the .set_bar() callback with a BAR size > 4 GB, if the BAR was also not requested to be configured as a 64-bit BAR. Thus, forcing setting the 64-bit flag for BARs larger than 4 GB in the lower level driver is dead code and can be removed. It is however possible that an EPF driver configures a BAR as 64-bit, even if the requested size is < 4 GB. Respect the requested BAR configuration, just like how it is already repected with regards to the prefetchable bit. Signed-off-by: Niklas Cassel <cassel@kernel.org> --- drivers/pci/controller/cadence/pcie-cadence-ep.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)