PCI endpoint fixes to improve the way 64-bit BARs are handled.
There are still future improvements that could be made:
pci-epf-test.c always allocates space for
6 BARs, even when using 64-bit BARs (which
really only requires us to allocate 3 BARs).
pcitest.sh will print "NOT OKAY" for BAR1,
BAR3, and BAR5 when using 64-bit BARs.
This could probably be improved to say
something like "N/A (64-bit BAR)".
Niklas Cassel (12):
PCI: endpoint: BAR width should not depend on sizeof dma_addr_t
PCI: endpoint: Simplify epc->ops->set_bar()/pci_epc_set_bar()
PCI: endpoint: Setting BAR_5 to 64-bits wide is invalid
PCI: endpoint: Setting 64-bit/prefetch bit is invalid when IO is set
PCI: endpoint: Setting a BAR size > 4 GB is invalid if 64-bit flag is
not set
PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs
properly
PCI: cadence: Set PCI_BASE_ADDRESS_MEM_TYPE_64 if a 64-bit BAR was
set-up
PCI: endpoint: Handle 64-bit BARs properly
PCI: endpoint: Make epc->ops->clear_bar()/pci_epc_clear_bar() take
struct *epf_bar
PCI: endpoint: Make sure that BAR_5 does not have 64-bit flag set when
clearing
PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs
properly
misc: pci_endpoint_test: Handle 64-bit BARs properly
drivers/misc/pci_endpoint_test.c | 12 +++++----
drivers/pci/cadence/pcie-cadence-ep.c | 15 ++++++++---
drivers/pci/dwc/pcie-designware-ep.c | 36 +++++++++++++++++++++------
drivers/pci/endpoint/functions/pci-epf-test.c | 28 +++++++++++++--------
drivers/pci/endpoint/pci-epc-core.c | 32 +++++++++++++++---------
drivers/pci/endpoint/pci-epf-core.c | 4 +++
include/linux/pci-epc.h | 11 ++++----
include/linux/pci-epf.h | 2 ++
8 files changed, 95 insertions(+), 45 deletions(-)
--
2.14.2
If a BAR supports 64-bit width or not depends on the hardware,
and should thus not depend on sizeof(dma_addr_t).
If a certain hardware doesn't support 64-bit BARs, its
epc->ops->set_bar() implementation should return -EINVAL
when PCI_BASE_ADDRESS_MEM_TYPE_64 is set.
We can't change pci_epc_set_bar() to only set
PCI_BASE_ADDRESS_MEM_TYPE_64 based on size, since if the user,
for some reason, wants to configure a BAR with a 64-bit width,
even though the BAR size is less than 4 GB, he should be able
to do that.
However, since pci-epf-test is simply a test and not an API,
we can set PCI_BASE_ADDRESS_MEM_TYPE_64 in pci-epf-test itself
only based on size.
Signed-off-by: Niklas Cassel <[email protected]>
---
drivers/pci/endpoint/functions/pci-epf-test.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 64d8a17f8094..f6c0c59b1bc8 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -70,7 +70,7 @@ struct pci_epf_test_data {
bool linkup_notifier;
};
-static int bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
+static size_t bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
static int pci_epf_test_copy(struct pci_epf_test *epf_test)
{
@@ -367,12 +367,14 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
struct pci_epf_test *epf_test = epf_get_drvdata(epf);
enum pci_barno test_reg_bar = epf_test->test_reg_bar;
- flags = PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32;
- if (sizeof(dma_addr_t) == 0x8)
- flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
-
for (bar = BAR_0; bar <= BAR_5; bar++) {
epf_bar = &epf->bar[bar];
+
+ flags = PCI_BASE_ADDRESS_SPACE_MEMORY;
+ flags |= upper_32_bits(epf_bar->size) ?
+ PCI_BASE_ADDRESS_MEM_TYPE_64 :
+ PCI_BASE_ADDRESS_MEM_TYPE_32;
+
ret = pci_epc_set_bar(epc, epf->func_no, bar,
epf_bar->phys_addr,
epf_bar->size, flags);
--
2.14.2
If a 64-bit BAR was set-up, we need to skip a BAR,
since a 64-bit BAR consists of a BAR pair.
We need to check what BAR width the epc->ops->set_bar() specific
implementation actually did set-up, since some drivers, like the
Cadence EP controller, sometimes sets up a 64-bit BAR, even though
a 32-bit BAR was requested.
Signed-off-by: Niklas Cassel <[email protected]>
---
drivers/pci/endpoint/functions/pci-epf-test.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 91274779e59f..d46e3ebabb8e 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -380,6 +380,13 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
if (bar == test_reg_bar)
return ret;
}
+ /*
+ * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
+ * if the specific implementation required a 64-bit BAR,
+ * even if we only requested a 32-bit BAR.
+ */
+ if (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
+ bar++;
}
return 0;
--
2.14.2
Since a 64-bit BAR consists of a BAR pair, and since there is no
BAR after BAR_5, BAR_5 cannot be 64-bits wide.
This sanity check is done in pci_epc_set_bar(), so that we don't need
to do this sanity check in all epc->ops->set_bar() implementations.
Signed-off-by: Niklas Cassel <[email protected]>
---
drivers/pci/endpoint/pci-epc-core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index 784e33d6f229..109d75f0b7d2 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -310,7 +310,9 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
int ret;
unsigned long irq_flags;
- if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
+ if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
+ (epf_bar->barno == BAR_5 &&
+ epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
return -EINVAL;
if (!epc->ops->set_bar)
--
2.14.2
cdns_pcie_ep_set_bar() does some round-up of the BAR size, which means
that a 64-bit BAR can be set-up, even when the flag
PCI_BASE_ADDRESS_MEM_TYPE_64 isn't set.
If a 64-bit BAR was set-up, set the flag PCI_BASE_ADDRESS_MEM_TYPE_64,
so that the calling function can know what BAR width that was actually
set-up.
I'm not sure why cdns_pcie_ep_set_bar() doesn't obey the flag
PCI_BASE_ADDRESS_MEM_TYPE_64, but I leave this for the MAINTAINER to
fix, since there might be a reason why this flag is ignored.
Signed-off-by: Niklas Cassel <[email protected]>
---
drivers/pci/cadence/pcie-cadence-ep.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
index cef36cd6b710..2905e098678c 100644
--- a/drivers/pci/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/cadence/pcie-cadence-ep.c
@@ -106,6 +106,9 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
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)
--
2.14.2
Make epc->ops->clear_bar()/pci_epc_clear_bar() take struct *epf_bar.
This is needed so that epc->ops->clear_bar() can clear the BAR pair,
if the BAR is 64-bits wide.
This also makes it possible for pci_epc_clear_bar() to sanity check
the flags.
Signed-off-by: Niklas Cassel <[email protected]>
---
drivers/pci/cadence/pcie-cadence-ep.c | 3 ++-
drivers/pci/dwc/pcie-designware-ep.c | 13 ++++++++++---
drivers/pci/endpoint/functions/pci-epf-test.c | 5 ++++-
drivers/pci/endpoint/pci-epc-core.c | 7 ++++---
include/linux/pci-epc.h | 5 +++--
5 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
index 2905e098678c..3d8283e450a9 100644
--- a/drivers/pci/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/cadence/pcie-cadence-ep.c
@@ -145,10 +145,11 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
}
static void cdns_pcie_ep_clear_bar(struct pci_epc *epc, u8 fn,
- enum pci_barno bar)
+ struct pci_epf_bar *epf_bar)
{
struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
struct cdns_pcie *pcie = &ep->pcie;
+ enum pci_barno bar = epf_bar->barno;
u32 reg, cfg, b, ctrl;
if (bar < BAR_4) {
diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index 571b90f88d84..cc4d8381c1dc 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -19,7 +19,8 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
pci_epc_linkup(epc);
}
-void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
+static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
+ int flags)
{
u32 reg;
@@ -30,6 +31,11 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
dw_pcie_dbi_ro_wr_dis(pci);
}
+void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
+{
+ __dw_pcie_ep_reset_bar(pci, bar, 0);
+}
+
static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
struct pci_epf_header *hdr)
{
@@ -104,13 +110,14 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, phys_addr_t phys_addr,
}
static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no,
- enum pci_barno bar)
+ struct pci_epf_bar *epf_bar)
{
struct dw_pcie_ep *ep = epc_get_drvdata(epc);
struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+ enum pci_barno bar = epf_bar->barno;
u32 atu_index = ep->bar_to_atu[bar];
- dw_pcie_ep_reset_bar(pci, bar);
+ __dw_pcie_ep_reset_bar(pci, bar, epf_bar->flags);
dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND);
clear_bit(atu_index, ep->ib_window_map);
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index d46e3ebabb8e..7cef85124325 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -344,14 +344,17 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
{
struct pci_epf_test *epf_test = epf_get_drvdata(epf);
struct pci_epc *epc = epf->epc;
+ struct pci_epf_bar *epf_bar;
int bar;
cancel_delayed_work(&epf_test->cmd_handler);
pci_epc_stop(epc);
for (bar = BAR_0; bar <= BAR_5; bar++) {
+ epf_bar = &epf->bar[bar];
+
if (epf_test->reg[bar]) {
pci_epf_free_space(epf, epf_test->reg[bar], bar);
- pci_epc_clear_bar(epc, epf->func_no, bar);
+ pci_epc_clear_bar(epc, epf->func_no, epf_bar);
}
}
}
diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index 8637822605ff..eccc942043cb 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -276,11 +276,12 @@ EXPORT_SYMBOL_GPL(pci_epc_map_addr);
* pci_epc_clear_bar() - reset the BAR
* @epc: the EPC device for which the BAR has to be cleared
* @func_no: the endpoint function number in the EPC device
- * @bar: the BAR number that has to be reset
+ * @epf_bar: the struct epf_bar that contains the BAR information
*
* Invoke to reset the BAR of the endpoint device.
*/
-void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar)
+void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no,
+ struct pci_epf_bar *epf_bar)
{
unsigned long flags;
@@ -291,7 +292,7 @@ void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar)
return;
spin_lock_irqsave(&epc->lock, flags);
- epc->ops->clear_bar(epc, func_no, bar);
+ epc->ops->clear_bar(epc, func_no, epf_bar);
spin_unlock_irqrestore(&epc->lock, flags);
}
EXPORT_SYMBOL_GPL(pci_epc_clear_bar);
diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
index 75bae8aabbf9..af657ca58b70 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -41,7 +41,7 @@ struct pci_epc_ops {
int (*set_bar)(struct pci_epc *epc, u8 func_no,
struct pci_epf_bar *epf_bar);
void (*clear_bar)(struct pci_epc *epc, u8 func_no,
- enum pci_barno bar);
+ struct pci_epf_bar *epf_bar);
int (*map_addr)(struct pci_epc *epc, u8 func_no,
phys_addr_t addr, u64 pci_addr, size_t size);
void (*unmap_addr)(struct pci_epc *epc, u8 func_no,
@@ -127,7 +127,8 @@ int pci_epc_write_header(struct pci_epc *epc, u8 func_no,
struct pci_epf_header *hdr);
int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
struct pci_epf_bar *epf_bar);
-void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar);
+void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no,
+ struct pci_epf_bar *epf_bar);
int pci_epc_map_addr(struct pci_epc *epc, u8 func_no,
phys_addr_t phys_addr,
u64 pci_addr, size_t size);
--
2.14.2
A 64-bit BAR consists of a BAR pair, where the second BAR has the
upper bits, so we cannot simply call pci_ioremap_bar() on every single
BAR index.
The second BAR in a BAR pair will not have the IORESOURCE_MEM resource
flag set. Only call ioremap on BARs that have the IORESOURCE_MEM
resource flag set.
pci 0000:01:00.0: BAR 4: assigned [mem 0xc0300000-0xc031ffff 64bit]
pci 0000:01:00.0: BAR 2: assigned [mem 0xc0320000-0xc03203ff 64bit]
pci 0000:01:00.0: BAR 0: assigned [mem 0xc0320400-0xc03204ff 64bit]
pci-endpoint-test 0000:01:00.0: can't ioremap BAR 1: [??? 0x00000000 flags 0x0]
pci-endpoint-test 0000:01:00.0: failed to read BAR1
pci-endpoint-test 0000:01:00.0: can't ioremap BAR 3: [??? 0x00000000 flags 0x0]
pci-endpoint-test 0000:01:00.0: failed to read BAR3
pci-endpoint-test 0000:01:00.0: can't ioremap BAR 5: [??? 0x00000000 flags 0x0]
pci-endpoint-test 0000:01:00.0: failed to read BAR5
Signed-off-by: Niklas Cassel <[email protected]>
Acked-by: Kishon Vijay Abraham I <[email protected]>
---
drivers/misc/pci_endpoint_test.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 320276f42653..fe8897e64635 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -534,12 +534,14 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
}
for (bar = BAR_0; bar <= BAR_5; bar++) {
- base = pci_ioremap_bar(pdev, bar);
- if (!base) {
- dev_err(dev, "failed to read BAR%d\n", bar);
- WARN_ON(bar == test_reg_bar);
+ if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) {
+ base = pci_ioremap_bar(pdev, bar);
+ if (!base) {
+ dev_err(dev, "failed to read BAR%d\n", bar);
+ WARN_ON(bar == test_reg_bar);
+ }
+ test->bar[bar] = base;
}
- test->bar[bar] = base;
}
test->base = test->bar[test_reg_bar];
--
2.14.2
Since a 64-bit BAR consists of a BAR pair, we need to write to both
BARs in the BAR pair to clear the BAR properly.
Signed-off-by: Niklas Cassel <[email protected]>
---
drivers/pci/dwc/pcie-designware-ep.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index cc4d8381c1dc..4d304e3ccf24 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -28,6 +28,10 @@ static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
dw_pcie_dbi_ro_wr_en(pci);
dw_pcie_writel_dbi2(pci, reg, 0x0);
dw_pcie_writel_dbi(pci, reg, 0x0);
+ if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
+ dw_pcie_writel_dbi2(pci, reg + 4, 0x0);
+ dw_pcie_writel_dbi(pci, reg + 4, 0x0);
+ }
dw_pcie_dbi_ro_wr_dis(pci);
}
--
2.14.2
Since a 64-bit BAR consists of a BAR pair, and since there is no
BAR after BAR_5, BAR_5 cannot be 64-bits wide.
This sanity check is done in pci_epc_clear_bar(), so that we don't need
to do this sanity check in all epc->ops->clear_bar() implementations.
Signed-off-by: Niklas Cassel <[email protected]>
---
Kishon made a review comment that he wanted this:
https://marc.info/?l=linux-kernel&m=152161168226203
Personally, I don't think that this check is needed,
since pci_epc_set_bar() already does this check,
and no one should modify the flags after pci_epc_set_bar()
has been called.
If everyone agrees, then this patch could be dropped.
drivers/pci/endpoint/pci-epc-core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index eccc942043cb..b0ee42739c3c 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -285,7 +285,9 @@ void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no,
{
unsigned long flags;
- if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
+ if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
+ (epf_bar->barno == BAR_5 &&
+ epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
return;
if (!epc->ops->clear_bar)
--
2.14.2
Since a 64-bit BAR consists of a BAR pair, we need to write to both
BARs in the BAR pair to setup the BAR properly.
Signed-off-by: Niklas Cassel <[email protected]>
---
drivers/pci/dwc/pcie-designware-ep.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index 5a0bb53c795c..571b90f88d84 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -138,8 +138,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 (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
+ 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;
--
2.14.2
Setting a BAR size > 4 GB is invalid if PCI_BASE_ADDRESS_MEM_TYPE_64
flag is not set.
This sanity check is done in pci_epc_set_bar(), so that we don't need
to do this sanity check in all epc->ops->set_bar() implementations.
Signed-off-by: Niklas Cassel <[email protected]>
---
drivers/pci/endpoint/pci-epc-core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index 40eea20d21f9..8637822605ff 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -315,7 +315,9 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
(epf_bar->barno == BAR_5 &&
flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ||
(flags & PCI_BASE_ADDRESS_SPACE_IO &&
- flags & PCI_BASE_ADDRESS_IO_MASK))
+ flags & PCI_BASE_ADDRESS_IO_MASK) ||
+ (upper_32_bits(epf_bar->size) &&
+ !(flags & PCI_BASE_ADDRESS_MEM_TYPE_64)))
return -EINVAL;
if (!epc->ops->set_bar)
--
2.14.2
Add barno and flags to struct epf_bar.
That way we can simplify epc->ops->set_bar()/pci_epc_set_bar()
by passing a struct *epf_bar instead of a whole lot of arguments.
This is needed so that epc->ops->set_bar() implementations can
modify BAR flags. Will be utilized in a succeeding patch.
Signed-off-by: Niklas Cassel <[email protected]>
---
It is not trivial to convert the bar_size + bar_flags +
struct pci_epf->bar member array to an array of struct resources,
since we need to be able to store the addresses returned
by dma_alloc_coherent(), which is of type dma_addr_t.
struct resource uses resource_size_t, which is defined as phys_addr_t.
E.g. ARTPEC-7 uses 64-bit dma_addr_t, but only 32-bit phys_addr_t.
drivers/pci/cadence/pcie-cadence-ep.c | 9 ++++++---
drivers/pci/dwc/pcie-designware-ep.c | 8 +++++---
drivers/pci/endpoint/functions/pci-epf-test.c | 8 ++------
drivers/pci/endpoint/pci-epc-core.c | 10 ++++------
drivers/pci/endpoint/pci-epf-core.c | 4 ++++
include/linux/pci-epc.h | 6 ++----
include/linux/pci-epf.h | 2 ++
7 files changed, 25 insertions(+), 22 deletions(-)
diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
index 3c3a97743453..cef36cd6b710 100644
--- a/drivers/pci/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/cadence/pcie-cadence-ep.c
@@ -77,16 +77,19 @@ static int cdns_pcie_ep_write_header(struct pci_epc *epc, u8 fn,
return 0;
}
-static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, enum pci_barno bar,
- dma_addr_t bar_phys, size_t size, int flags)
+static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
+ struct pci_epf_bar *epf_bar)
{
struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
struct cdns_pcie *pcie = &ep->pcie;
+ dma_addr_t bar_phys = epf_bar->phys_addr;
+ enum pci_barno bar = epf_bar->barno;
+ int flags = epf_bar->flags;
u32 addr0, addr1, reg, cfg, b, aperture, ctrl;
u64 sz;
/* BAR size is 2^(aperture + 7) */
- sz = max_t(size_t, size, CDNS_PCIE_EP_MIN_APERTURE);
+ sz = max_t(size_t, epf_bar->size, CDNS_PCIE_EP_MIN_APERTURE);
/*
* roundup_pow_of_two() returns an unsigned long, which is not suited
* for 64bit values.
diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index 9236b998327f..5a0bb53c795c 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -117,12 +117,14 @@ static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no,
}
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)
+ struct pci_epf_bar *epf_bar)
{
int ret;
struct dw_pcie_ep *ep = epc_get_drvdata(epc);
struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+ enum pci_barno bar = epf_bar->barno;
+ size_t size = epf_bar->size;
+ int flags = epf_bar->flags;
enum dw_pcie_as_type as_type;
u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
@@ -131,7 +133,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
else
as_type = DW_PCIE_AS_IO;
- ret = dw_pcie_ep_inbound_atu(ep, bar, bar_phys, as_type);
+ ret = dw_pcie_ep_inbound_atu(ep, bar, epf_bar->phys_addr, as_type);
if (ret)
return ret;
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index f6c0c59b1bc8..91274779e59f 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -358,7 +358,6 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
static int pci_epf_test_set_bar(struct pci_epf *epf)
{
- int flags;
int bar;
int ret;
struct pci_epf_bar *epf_bar;
@@ -370,14 +369,11 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
for (bar = BAR_0; bar <= BAR_5; bar++) {
epf_bar = &epf->bar[bar];
- flags = PCI_BASE_ADDRESS_SPACE_MEMORY;
- flags |= upper_32_bits(epf_bar->size) ?
+ epf_bar->flags |= upper_32_bits(epf_bar->size) ?
PCI_BASE_ADDRESS_MEM_TYPE_64 :
PCI_BASE_ADDRESS_MEM_TYPE_32;
- ret = pci_epc_set_bar(epc, epf->func_no, bar,
- epf_bar->phys_addr,
- epf_bar->size, flags);
+ ret = pci_epc_set_bar(epc, epf->func_no, epf_bar);
if (ret) {
pci_epf_free_space(epf, epf_test->reg[bar], bar);
dev_err(dev, "failed to set BAR%d\n", bar);
diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index e245bba0ab53..784e33d6f229 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -300,14 +300,12 @@ EXPORT_SYMBOL_GPL(pci_epc_clear_bar);
* pci_epc_set_bar() - configure BAR in order for host to assign PCI addr space
* @epc: the EPC device on which BAR has to be configured
* @func_no: the endpoint function number in the EPC device
- * @bar: the BAR number that has to be configured
- * @size: the size of the addr space
- * @flags: specify memory allocation/io allocation/32bit address/64 bit address
+ * @epf_bar: the struct epf_bar that contains the BAR information
*
* Invoke to configure the BAR of the endpoint device.
*/
-int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, enum pci_barno bar,
- dma_addr_t bar_phys, size_t size, int flags)
+int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
+ struct pci_epf_bar *epf_bar)
{
int ret;
unsigned long irq_flags;
@@ -319,7 +317,7 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, enum pci_barno bar,
return 0;
spin_lock_irqsave(&epc->lock, irq_flags);
- ret = epc->ops->set_bar(epc, func_no, bar, bar_phys, size, flags);
+ ret = epc->ops->set_bar(epc, func_no, epf_bar);
spin_unlock_irqrestore(&epc->lock, irq_flags);
return ret;
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 59ed29e550e9..465b5f058b6d 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -98,6 +98,8 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar)
epf->bar[bar].phys_addr = 0;
epf->bar[bar].size = 0;
+ epf->bar[bar].barno = 0;
+ epf->bar[bar].flags = 0;
}
EXPORT_SYMBOL_GPL(pci_epf_free_space);
@@ -126,6 +128,8 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar)
epf->bar[bar].phys_addr = phys_addr;
epf->bar[bar].size = size;
+ epf->bar[bar].barno = bar;
+ epf->bar[bar].flags = PCI_BASE_ADDRESS_SPACE_MEMORY;
return space;
}
diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
index a1a5e5df0f66..75bae8aabbf9 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -39,8 +39,7 @@ struct pci_epc_ops {
int (*write_header)(struct pci_epc *epc, u8 func_no,
struct pci_epf_header *hdr);
int (*set_bar)(struct pci_epc *epc, u8 func_no,
- enum pci_barno bar,
- dma_addr_t bar_phys, size_t size, int flags);
+ struct pci_epf_bar *epf_bar);
void (*clear_bar)(struct pci_epc *epc, u8 func_no,
enum pci_barno bar);
int (*map_addr)(struct pci_epc *epc, u8 func_no,
@@ -127,8 +126,7 @@ void pci_epc_remove_epf(struct pci_epc *epc, struct pci_epf *epf);
int pci_epc_write_header(struct pci_epc *epc, u8 func_no,
struct pci_epf_header *hdr);
int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
- enum pci_barno bar,
- dma_addr_t bar_phys, size_t size, int flags);
+ struct pci_epf_bar *epf_bar);
void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar);
int pci_epc_map_addr(struct pci_epc *epc, u8 func_no,
phys_addr_t phys_addr,
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index e897bf076701..f7d6f4883f8b 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -97,6 +97,8 @@ struct pci_epf_driver {
struct pci_epf_bar {
dma_addr_t phys_addr;
size_t size;
+ enum pci_barno barno;
+ int flags;
};
/**
--
2.14.2
If flag PCI_BASE_ADDRESS_SPACE_IO is set, also having any
PCI_BASE_ADDRESS_MEM_* bit set is invalid.
This sanity check is done in pci_epc_set_bar(), so that we don't need
to do this sanity check in all epc->ops->set_bar() implementations.
Signed-off-by: Niklas Cassel <[email protected]>
---
drivers/pci/endpoint/pci-epc-core.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index 109d75f0b7d2..40eea20d21f9 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -309,10 +309,13 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
{
int ret;
unsigned long irq_flags;
+ int flags = epf_bar->flags;
if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
(epf_bar->barno == BAR_5 &&
- epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
+ flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ||
+ (flags & PCI_BASE_ADDRESS_SPACE_IO &&
+ flags & PCI_BASE_ADDRESS_IO_MASK))
return -EINVAL;
if (!epc->ops->set_bar)
--
2.14.2
Hi Niklas,
On 28/03/2018 12:50, Niklas Cassel wrote:
> Since a 64-bit BAR consists of a BAR pair, we need to write to both
> BARs in the BAR pair to setup the BAR properly.
>
> Signed-off-by: Niklas Cassel <[email protected]>
> ---
> drivers/pci/dwc/pcie-designware-ep.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> index 5a0bb53c795c..571b90f88d84 100644
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -138,8 +138,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 (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> + 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;
>
Seems good to me. :)
Reviewed-by: Gustavo Pimentel <[email protected]>
Hi Niklas,
On 28/03/2018 12:50, Niklas Cassel wrote:
> Make epc->ops->clear_bar()/pci_epc_clear_bar() take struct *epf_bar.
>
> This is needed so that epc->ops->clear_bar() can clear the BAR pair,
> if the BAR is 64-bits wide.
>
> This also makes it possible for pci_epc_clear_bar() to sanity check
> the flags.
>
> Signed-off-by: Niklas Cassel <[email protected]>
> ---
> drivers/pci/cadence/pcie-cadence-ep.c | 3 ++-
> drivers/pci/dwc/pcie-designware-ep.c | 13 ++++++++++---
> drivers/pci/endpoint/functions/pci-epf-test.c | 5 ++++-
> drivers/pci/endpoint/pci-epc-core.c | 7 ++++---
> include/linux/pci-epc.h | 5 +++--
> 5 files changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
> index 2905e098678c..3d8283e450a9 100644
> --- a/drivers/pci/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/cadence/pcie-cadence-ep.c
> @@ -145,10 +145,11 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
> }
>
> static void cdns_pcie_ep_clear_bar(struct pci_epc *epc, u8 fn,
> - enum pci_barno bar)
> + struct pci_epf_bar *epf_bar)
> {
> struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
> struct cdns_pcie *pcie = &ep->pcie;
> + enum pci_barno bar = epf_bar->barno;
> u32 reg, cfg, b, ctrl;
>
> if (bar < BAR_4) {
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> index 571b90f88d84..cc4d8381c1dc 100644
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -19,7 +19,8 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> pci_epc_linkup(epc);
> }
>
> -void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
> +static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
> + int flags)
> {
> u32 reg;
>
> @@ -30,6 +31,11 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
> dw_pcie_dbi_ro_wr_dis(pci);
> }
>
> +void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
> +{
> + __dw_pcie_ep_reset_bar(pci, bar, 0);
> +}
> +
> static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
> struct pci_epf_header *hdr)
> {
> @@ -104,13 +110,14 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, phys_addr_t phys_addr,
> }
>
> static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no,
> - enum pci_barno bar)
> + struct pci_epf_bar *epf_bar)
> {
> struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> + enum pci_barno bar = epf_bar->barno;
> u32 atu_index = ep->bar_to_atu[bar];
>
> - dw_pcie_ep_reset_bar(pci, bar);
> + __dw_pcie_ep_reset_bar(pci, bar, epf_bar->flags);
>
> dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND);
> clear_bit(atu_index, ep->ib_window_map);
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index d46e3ebabb8e..7cef85124325 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -344,14 +344,17 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
> {
> struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> struct pci_epc *epc = epf->epc;
> + struct pci_epf_bar *epf_bar;
> int bar;
>
> cancel_delayed_work(&epf_test->cmd_handler);
> pci_epc_stop(epc);
> for (bar = BAR_0; bar <= BAR_5; bar++) {
> + epf_bar = &epf->bar[bar];
> +
> if (epf_test->reg[bar]) {
> pci_epf_free_space(epf, epf_test->reg[bar], bar);
> - pci_epc_clear_bar(epc, epf->func_no, bar);
> + pci_epc_clear_bar(epc, epf->func_no, epf_bar);
> }
> }
> }
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 8637822605ff..eccc942043cb 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -276,11 +276,12 @@ EXPORT_SYMBOL_GPL(pci_epc_map_addr);
> * pci_epc_clear_bar() - reset the BAR
> * @epc: the EPC device for which the BAR has to be cleared
> * @func_no: the endpoint function number in the EPC device
> - * @bar: the BAR number that has to be reset
> + * @epf_bar: the struct epf_bar that contains the BAR information
> *
> * Invoke to reset the BAR of the endpoint device.
> */
> -void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar)
> +void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no,
> + struct pci_epf_bar *epf_bar)
> {
> unsigned long flags;
>
> @@ -291,7 +292,7 @@ void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar)
> return;
>
> spin_lock_irqsave(&epc->lock, flags);
> - epc->ops->clear_bar(epc, func_no, bar);
> + epc->ops->clear_bar(epc, func_no, epf_bar);
> spin_unlock_irqrestore(&epc->lock, flags);
> }
> EXPORT_SYMBOL_GPL(pci_epc_clear_bar);
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index 75bae8aabbf9..af657ca58b70 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -41,7 +41,7 @@ struct pci_epc_ops {
> int (*set_bar)(struct pci_epc *epc, u8 func_no,
> struct pci_epf_bar *epf_bar);
> void (*clear_bar)(struct pci_epc *epc, u8 func_no,
> - enum pci_barno bar);
> + struct pci_epf_bar *epf_bar);
> int (*map_addr)(struct pci_epc *epc, u8 func_no,
> phys_addr_t addr, u64 pci_addr, size_t size);
> void (*unmap_addr)(struct pci_epc *epc, u8 func_no,
> @@ -127,7 +127,8 @@ int pci_epc_write_header(struct pci_epc *epc, u8 func_no,
> struct pci_epf_header *hdr);
> int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
> struct pci_epf_bar *epf_bar);
> -void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar);
> +void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no,
> + struct pci_epf_bar *epf_bar);
> int pci_epc_map_addr(struct pci_epc *epc, u8 func_no,
> phys_addr_t phys_addr,
> u64 pci_addr, size_t size);
>
Seems good to me. :)
Reviewed-by: Gustavo Pimentel <[email protected]>
Hi Niklas,
On 28/03/2018 12:50, Niklas Cassel wrote:
> Since a 64-bit BAR consists of a BAR pair, we need to write to both
> BARs in the BAR pair to clear the BAR properly.
>
> Signed-off-by: Niklas Cassel <[email protected]>
> ---
> drivers/pci/dwc/pcie-designware-ep.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> index cc4d8381c1dc..4d304e3ccf24 100644
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -28,6 +28,10 @@ static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
> dw_pcie_dbi_ro_wr_en(pci);
> dw_pcie_writel_dbi2(pci, reg, 0x0);
> dw_pcie_writel_dbi(pci, reg, 0x0);
> + if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> + dw_pcie_writel_dbi2(pci, reg + 4, 0x0);
> + dw_pcie_writel_dbi(pci, reg + 4, 0x0);
> + }
> dw_pcie_dbi_ro_wr_dis(pci);
> }
>
>
Seems good to me. :)
Reviewed-by: Gustavo Pimentel <[email protected]>
> On 28/03/2018 12:51, Niklas Cassel wrote:
> cdns_pcie_ep_set_bar() does some round-up of the BAR size, which means that a 64-bit BAR can be set-up, even when the flag
> PCI_BASE_ADDRESS_MEM_TYPE_64 isn't set.
> If a 64-bit BAR was set-up, set the flag PCI_BASE_ADDRESS_MEM_TYPE_64, so that the calling function can know what BAR width that was actually set-up.
> I'm not sure why cdns_pcie_ep_set_bar() doesn't obey the flag PCI_BASE_ADDRESS_MEM_TYPE_64, but I leave this for the MAINTAINER to fix, since there might be a reason why > this flag is ignored.
Will investigate and fix this in future patch
> Signed-off-by: Niklas Cassel <[email protected]>
> ---
> drivers/pci/cadence/pcie-cadence-ep.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
> index cef36cd6b710..2905e098678c 100644
> --- a/drivers/pci/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/cadence/pcie-cadence-ep.c
> @@ -106,6 +106,9 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
> 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)
> --
> 2.14.2
Change looks good to me.
Hi Niklas,
On 28/03/2018 12:50, Niklas Cassel wrote:
> Add barno and flags to struct epf_bar.
> That way we can simplify epc->ops->set_bar()/pci_epc_set_bar()
> by passing a struct *epf_bar instead of a whole lot of arguments.
>
> This is needed so that epc->ops->set_bar() implementations can
> modify BAR flags. Will be utilized in a succeeding patch.
>
> Signed-off-by: Niklas Cassel <[email protected]>
> ---
> It is not trivial to convert the bar_size + bar_flags +
> struct pci_epf->bar member array to an array of struct resources,
> since we need to be able to store the addresses returned
> by dma_alloc_coherent(), which is of type dma_addr_t.
> struct resource uses resource_size_t, which is defined as phys_addr_t.
> E.g. ARTPEC-7 uses 64-bit dma_addr_t, but only 32-bit phys_addr_t.
>
> drivers/pci/cadence/pcie-cadence-ep.c | 9 ++++++---
> drivers/pci/dwc/pcie-designware-ep.c | 8 +++++---
> drivers/pci/endpoint/functions/pci-epf-test.c | 8 ++------
> drivers/pci/endpoint/pci-epc-core.c | 10 ++++------
> drivers/pci/endpoint/pci-epf-core.c | 4 ++++
> include/linux/pci-epc.h | 6 ++----
> include/linux/pci-epf.h | 2 ++
> 7 files changed, 25 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
> index 3c3a97743453..cef36cd6b710 100644
> --- a/drivers/pci/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/cadence/pcie-cadence-ep.c
> @@ -77,16 +77,19 @@ static int cdns_pcie_ep_write_header(struct pci_epc *epc, u8 fn,
> return 0;
> }
>
> -static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, enum pci_barno bar,
> - dma_addr_t bar_phys, size_t size, int flags)
> +static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
> + struct pci_epf_bar *epf_bar)
> {
> struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
> struct cdns_pcie *pcie = &ep->pcie;
> + dma_addr_t bar_phys = epf_bar->phys_addr;
> + enum pci_barno bar = epf_bar->barno;
> + int flags = epf_bar->flags;
> u32 addr0, addr1, reg, cfg, b, aperture, ctrl;
> u64 sz;
>
> /* BAR size is 2^(aperture + 7) */
> - sz = max_t(size_t, size, CDNS_PCIE_EP_MIN_APERTURE);
> + sz = max_t(size_t, epf_bar->size, CDNS_PCIE_EP_MIN_APERTURE);
> /*
> * roundup_pow_of_two() returns an unsigned long, which is not suited
> * for 64bit values.
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> index 9236b998327f..5a0bb53c795c 100644
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -117,12 +117,14 @@ static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no,
> }
>
> 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)
> + struct pci_epf_bar *epf_bar)
> {
> int ret;
> struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> + enum pci_barno bar = epf_bar->barno;
> + size_t size = epf_bar->size;
> + int flags = epf_bar->flags;
> enum dw_pcie_as_type as_type;
> u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>
> @@ -131,7 +133,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
> else
> as_type = DW_PCIE_AS_IO;
>
> - ret = dw_pcie_ep_inbound_atu(ep, bar, bar_phys, as_type);
> + ret = dw_pcie_ep_inbound_atu(ep, bar, epf_bar->phys_addr, as_type);
> if (ret)
> return ret;
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index f6c0c59b1bc8..91274779e59f 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -358,7 +358,6 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
>
> static int pci_epf_test_set_bar(struct pci_epf *epf)
> {
> - int flags;
> int bar;
> int ret;
> struct pci_epf_bar *epf_bar;
> @@ -370,14 +369,11 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
> for (bar = BAR_0; bar <= BAR_5; bar++) {
> epf_bar = &epf->bar[bar];
>
> - flags = PCI_BASE_ADDRESS_SPACE_MEMORY;
> - flags |= upper_32_bits(epf_bar->size) ?
> + epf_bar->flags |= upper_32_bits(epf_bar->size) ?
> PCI_BASE_ADDRESS_MEM_TYPE_64 :
> PCI_BASE_ADDRESS_MEM_TYPE_32;
>
> - ret = pci_epc_set_bar(epc, epf->func_no, bar,
> - epf_bar->phys_addr,
> - epf_bar->size, flags);
> + ret = pci_epc_set_bar(epc, epf->func_no, epf_bar);
> if (ret) {
> pci_epf_free_space(epf, epf_test->reg[bar], bar);
> dev_err(dev, "failed to set BAR%d\n", bar);
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index e245bba0ab53..784e33d6f229 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -300,14 +300,12 @@ EXPORT_SYMBOL_GPL(pci_epc_clear_bar);
> * pci_epc_set_bar() - configure BAR in order for host to assign PCI addr space
> * @epc: the EPC device on which BAR has to be configured
> * @func_no: the endpoint function number in the EPC device
> - * @bar: the BAR number that has to be configured
> - * @size: the size of the addr space
> - * @flags: specify memory allocation/io allocation/32bit address/64 bit address
> + * @epf_bar: the struct epf_bar that contains the BAR information
> *
> * Invoke to configure the BAR of the endpoint device.
> */
> -int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, enum pci_barno bar,
> - dma_addr_t bar_phys, size_t size, int flags)
> +int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
> + struct pci_epf_bar *epf_bar)
> {
> int ret;
> unsigned long irq_flags;
> @@ -319,7 +317,7 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, enum pci_barno bar,
> return 0;
>
> spin_lock_irqsave(&epc->lock, irq_flags);
> - ret = epc->ops->set_bar(epc, func_no, bar, bar_phys, size, flags);
> + ret = epc->ops->set_bar(epc, func_no, epf_bar);
> spin_unlock_irqrestore(&epc->lock, irq_flags);
>
> return ret;
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index 59ed29e550e9..465b5f058b6d 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -98,6 +98,8 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar)
>
> epf->bar[bar].phys_addr = 0;
> epf->bar[bar].size = 0;
> + epf->bar[bar].barno = 0;
> + epf->bar[bar].flags = 0;
> }
> EXPORT_SYMBOL_GPL(pci_epf_free_space);
>
> @@ -126,6 +128,8 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar)
>
> epf->bar[bar].phys_addr = phys_addr;
> epf->bar[bar].size = size;
> + epf->bar[bar].barno = bar;
> + epf->bar[bar].flags = PCI_BASE_ADDRESS_SPACE_MEMORY;
>
> return space;
> }
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index a1a5e5df0f66..75bae8aabbf9 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -39,8 +39,7 @@ struct pci_epc_ops {
> int (*write_header)(struct pci_epc *epc, u8 func_no,
> struct pci_epf_header *hdr);
> int (*set_bar)(struct pci_epc *epc, u8 func_no,
> - enum pci_barno bar,
> - dma_addr_t bar_phys, size_t size, int flags);
> + struct pci_epf_bar *epf_bar);
> void (*clear_bar)(struct pci_epc *epc, u8 func_no,
> enum pci_barno bar);
> int (*map_addr)(struct pci_epc *epc, u8 func_no,
> @@ -127,8 +126,7 @@ void pci_epc_remove_epf(struct pci_epc *epc, struct pci_epf *epf);
> int pci_epc_write_header(struct pci_epc *epc, u8 func_no,
> struct pci_epf_header *hdr);
> int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
> - enum pci_barno bar,
> - dma_addr_t bar_phys, size_t size, int flags);
> + struct pci_epf_bar *epf_bar);
> void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar);
> int pci_epc_map_addr(struct pci_epc *epc, u8 func_no,
> phys_addr_t phys_addr,
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index e897bf076701..f7d6f4883f8b 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -97,6 +97,8 @@ struct pci_epf_driver {
> struct pci_epf_bar {
> dma_addr_t phys_addr;
> size_t size;
> + enum pci_barno barno;
> + int flags;
> };
>
> /**
>
Seems good to me. :)
Reviewed-by: Gustavo Pimentel <[email protected]>
[+cc Lorenzo]
On Wed, Mar 28, 2018 at 01:24:10PM +0000, Alan Douglas wrote:
> > On 28/03/2018 12:51, Niklas Cassel wrote:
> > cdns_pcie_ep_set_bar() does some round-up of the BAR size, which means that a 64-bit BAR can be set-up, even when the flag
> > PCI_BASE_ADDRESS_MEM_TYPE_64 isn't set.
>
> > If a 64-bit BAR was set-up, set the flag PCI_BASE_ADDRESS_MEM_TYPE_64, so that the calling function can know what BAR width that was actually set-up.
>
> > I'm not sure why cdns_pcie_ep_set_bar() doesn't obey the flag PCI_BASE_ADDRESS_MEM_TYPE_64, but I leave this for the MAINTAINER to fix, since there might be a reason why > this flag is ignored.
> Will investigate and fix this in future patch
>
> > Signed-off-by: Niklas Cassel <[email protected]>
> > ---
> > drivers/pci/cadence/pcie-cadence-ep.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
> > index cef36cd6b710..2905e098678c 100644
> > --- a/drivers/pci/cadence/pcie-cadence-ep.c
> > +++ b/drivers/pci/cadence/pcie-cadence-ep.c
> > @@ -106,6 +106,9 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
> > 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)
> > --
> > 2.14.2
> Change looks good to me.
It will be helpful to Lorenzo if you spell this out, e.g.,
Acked-by: Alan Douglas <[email protected]>
Also, it looks like we need a MAINTAINERS update to add
drivers/pci/cadence/ to the "PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS"
filename patterns.
On Wednesday 28 March 2018 05:20 PM, Niklas Cassel wrote:
> If a BAR supports 64-bit width or not depends on the hardware,
> and should thus not depend on sizeof(dma_addr_t).
>
> If a certain hardware doesn't support 64-bit BARs, its
> epc->ops->set_bar() implementation should return -EINVAL
> when PCI_BASE_ADDRESS_MEM_TYPE_64 is set.
>
> We can't change pci_epc_set_bar() to only set
> PCI_BASE_ADDRESS_MEM_TYPE_64 based on size, since if the user,
> for some reason, wants to configure a BAR with a 64-bit width,
> even though the BAR size is less than 4 GB, he should be able
> to do that.
>
> However, since pci-epf-test is simply a test and not an API,
> we can set PCI_BASE_ADDRESS_MEM_TYPE_64 in pci-epf-test itself
> only based on size.
>
> Signed-off-by: Niklas Cassel <[email protected]>
Acked-by: Kishon Vijay Abraham I <[email protected]>
> ---
> drivers/pci/endpoint/functions/pci-epf-test.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 64d8a17f8094..f6c0c59b1bc8 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -70,7 +70,7 @@ struct pci_epf_test_data {
> bool linkup_notifier;
> };
>
> -static int bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
> +static size_t bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
>
> static int pci_epf_test_copy(struct pci_epf_test *epf_test)
> {
> @@ -367,12 +367,14 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
> struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>
> - flags = PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32;
> - if (sizeof(dma_addr_t) == 0x8)
> - flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> -
> for (bar = BAR_0; bar <= BAR_5; bar++) {
> epf_bar = &epf->bar[bar];
> +
> + flags = PCI_BASE_ADDRESS_SPACE_MEMORY;
> + flags |= upper_32_bits(epf_bar->size) ?
> + PCI_BASE_ADDRESS_MEM_TYPE_64 :
> + PCI_BASE_ADDRESS_MEM_TYPE_32;
> +
> ret = pci_epc_set_bar(epc, epf->func_no, bar,
> epf_bar->phys_addr,
> epf_bar->size, flags);
>
On Wednesday 28 March 2018 05:20 PM, Niklas Cassel wrote:
> Add barno and flags to struct epf_bar.
> That way we can simplify epc->ops->set_bar()/pci_epc_set_bar()
> by passing a struct *epf_bar instead of a whole lot of arguments.
>
> This is needed so that epc->ops->set_bar() implementations can
> modify BAR flags. Will be utilized in a succeeding patch.
>
> Signed-off-by: Niklas Cassel <[email protected]>
Acked-by: Kishon Vijay Abraham I <[email protected]>
> ---
> It is not trivial to convert the bar_size + bar_flags +
> struct pci_epf->bar member array to an array of struct resources,
> since we need to be able to store the addresses returned
> by dma_alloc_coherent(), which is of type dma_addr_t.
> struct resource uses resource_size_t, which is defined as phys_addr_t.
> E.g. ARTPEC-7 uses 64-bit dma_addr_t, but only 32-bit phys_addr_t.
>
> drivers/pci/cadence/pcie-cadence-ep.c | 9 ++++++---
> drivers/pci/dwc/pcie-designware-ep.c | 8 +++++---
> drivers/pci/endpoint/functions/pci-epf-test.c | 8 ++------
> drivers/pci/endpoint/pci-epc-core.c | 10 ++++------
> drivers/pci/endpoint/pci-epf-core.c | 4 ++++
> include/linux/pci-epc.h | 6 ++----
> include/linux/pci-epf.h | 2 ++
> 7 files changed, 25 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
> index 3c3a97743453..cef36cd6b710 100644
> --- a/drivers/pci/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/cadence/pcie-cadence-ep.c
> @@ -77,16 +77,19 @@ static int cdns_pcie_ep_write_header(struct pci_epc *epc, u8 fn,
> return 0;
> }
>
> -static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, enum pci_barno bar,
> - dma_addr_t bar_phys, size_t size, int flags)
> +static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
> + struct pci_epf_bar *epf_bar)
> {
> struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
> struct cdns_pcie *pcie = &ep->pcie;
> + dma_addr_t bar_phys = epf_bar->phys_addr;
> + enum pci_barno bar = epf_bar->barno;
> + int flags = epf_bar->flags;
> u32 addr0, addr1, reg, cfg, b, aperture, ctrl;
> u64 sz;
>
> /* BAR size is 2^(aperture + 7) */
> - sz = max_t(size_t, size, CDNS_PCIE_EP_MIN_APERTURE);
> + sz = max_t(size_t, epf_bar->size, CDNS_PCIE_EP_MIN_APERTURE);
> /*
> * roundup_pow_of_two() returns an unsigned long, which is not suited
> * for 64bit values.
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> index 9236b998327f..5a0bb53c795c 100644
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -117,12 +117,14 @@ static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no,
> }
>
> 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)
> + struct pci_epf_bar *epf_bar)
> {
> int ret;
> struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> + enum pci_barno bar = epf_bar->barno;
> + size_t size = epf_bar->size;
> + int flags = epf_bar->flags;
> enum dw_pcie_as_type as_type;
> u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>
> @@ -131,7 +133,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
> else
> as_type = DW_PCIE_AS_IO;
>
> - ret = dw_pcie_ep_inbound_atu(ep, bar, bar_phys, as_type);
> + ret = dw_pcie_ep_inbound_atu(ep, bar, epf_bar->phys_addr, as_type);
> if (ret)
> return ret;
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index f6c0c59b1bc8..91274779e59f 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -358,7 +358,6 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
>
> static int pci_epf_test_set_bar(struct pci_epf *epf)
> {
> - int flags;
> int bar;
> int ret;
> struct pci_epf_bar *epf_bar;
> @@ -370,14 +369,11 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
> for (bar = BAR_0; bar <= BAR_5; bar++) {
> epf_bar = &epf->bar[bar];
>
> - flags = PCI_BASE_ADDRESS_SPACE_MEMORY;
> - flags |= upper_32_bits(epf_bar->size) ?
> + epf_bar->flags |= upper_32_bits(epf_bar->size) ?
> PCI_BASE_ADDRESS_MEM_TYPE_64 :
> PCI_BASE_ADDRESS_MEM_TYPE_32;
>
> - ret = pci_epc_set_bar(epc, epf->func_no, bar,
> - epf_bar->phys_addr,
> - epf_bar->size, flags);
> + ret = pci_epc_set_bar(epc, epf->func_no, epf_bar);
> if (ret) {
> pci_epf_free_space(epf, epf_test->reg[bar], bar);
> dev_err(dev, "failed to set BAR%d\n", bar);
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index e245bba0ab53..784e33d6f229 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -300,14 +300,12 @@ EXPORT_SYMBOL_GPL(pci_epc_clear_bar);
> * pci_epc_set_bar() - configure BAR in order for host to assign PCI addr space
> * @epc: the EPC device on which BAR has to be configured
> * @func_no: the endpoint function number in the EPC device
> - * @bar: the BAR number that has to be configured
> - * @size: the size of the addr space
> - * @flags: specify memory allocation/io allocation/32bit address/64 bit address
> + * @epf_bar: the struct epf_bar that contains the BAR information
> *
> * Invoke to configure the BAR of the endpoint device.
> */
> -int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, enum pci_barno bar,
> - dma_addr_t bar_phys, size_t size, int flags)
> +int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
> + struct pci_epf_bar *epf_bar)
> {
> int ret;
> unsigned long irq_flags;
> @@ -319,7 +317,7 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, enum pci_barno bar,
> return 0;
>
> spin_lock_irqsave(&epc->lock, irq_flags);
> - ret = epc->ops->set_bar(epc, func_no, bar, bar_phys, size, flags);
> + ret = epc->ops->set_bar(epc, func_no, epf_bar);
> spin_unlock_irqrestore(&epc->lock, irq_flags);
>
> return ret;
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index 59ed29e550e9..465b5f058b6d 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -98,6 +98,8 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar)
>
> epf->bar[bar].phys_addr = 0;
> epf->bar[bar].size = 0;
> + epf->bar[bar].barno = 0;
> + epf->bar[bar].flags = 0;
> }
> EXPORT_SYMBOL_GPL(pci_epf_free_space);
>
> @@ -126,6 +128,8 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar)
>
> epf->bar[bar].phys_addr = phys_addr;
> epf->bar[bar].size = size;
> + epf->bar[bar].barno = bar;
> + epf->bar[bar].flags = PCI_BASE_ADDRESS_SPACE_MEMORY;
>
> return space;
> }
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index a1a5e5df0f66..75bae8aabbf9 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -39,8 +39,7 @@ struct pci_epc_ops {
> int (*write_header)(struct pci_epc *epc, u8 func_no,
> struct pci_epf_header *hdr);
> int (*set_bar)(struct pci_epc *epc, u8 func_no,
> - enum pci_barno bar,
> - dma_addr_t bar_phys, size_t size, int flags);
> + struct pci_epf_bar *epf_bar);
> void (*clear_bar)(struct pci_epc *epc, u8 func_no,
> enum pci_barno bar);
> int (*map_addr)(struct pci_epc *epc, u8 func_no,
> @@ -127,8 +126,7 @@ void pci_epc_remove_epf(struct pci_epc *epc, struct pci_epf *epf);
> int pci_epc_write_header(struct pci_epc *epc, u8 func_no,
> struct pci_epf_header *hdr);
> int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
> - enum pci_barno bar,
> - dma_addr_t bar_phys, size_t size, int flags);
> + struct pci_epf_bar *epf_bar);
> void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, int bar);
> int pci_epc_map_addr(struct pci_epc *epc, u8 func_no,
> phys_addr_t phys_addr,
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index e897bf076701..f7d6f4883f8b 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -97,6 +97,8 @@ struct pci_epf_driver {
> struct pci_epf_bar {
> dma_addr_t phys_addr;
> size_t size;
> + enum pci_barno barno;
> + int flags;
> };
>
> /**
>
On Wednesday 28 March 2018 05:20 PM, Niklas Cassel wrote:
> Since a 64-bit BAR consists of a BAR pair, and since there is no
> BAR after BAR_5, BAR_5 cannot be 64-bits wide.
>
> This sanity check is done in pci_epc_set_bar(), so that we don't need
> to do this sanity check in all epc->ops->set_bar() implementations.
>
> Signed-off-by: Niklas Cassel <[email protected]>
> ---
> drivers/pci/endpoint/pci-epc-core.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 784e33d6f229..109d75f0b7d2 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -310,7 +310,9 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
> int ret;
> unsigned long irq_flags;
>
> - if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> + if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
> + (epf_bar->barno == BAR_5 &&
> + epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
> return -EINVAL;
It's getting a bit lengthy. I'd prefer two separate ifs as that might be
legible. But otherwise
Acked-by: Kishon Vijay Abraham I <[email protected]>
>
> if (!epc->ops->set_bar)
>
On Wednesday 28 March 2018 05:20 PM, Niklas Cassel wrote:
> If flag PCI_BASE_ADDRESS_SPACE_IO is set, also having any
> PCI_BASE_ADDRESS_MEM_* bit set is invalid.
>
> This sanity check is done in pci_epc_set_bar(), so that we don't need
> to do this sanity check in all epc->ops->set_bar() implementations.
>
> Signed-off-by: Niklas Cassel <[email protected]>
> ---
> drivers/pci/endpoint/pci-epc-core.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 109d75f0b7d2..40eea20d21f9 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -309,10 +309,13 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
> {
> int ret;
> unsigned long irq_flags;
> + int flags = epf_bar->flags;
>
> if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
> (epf_bar->barno == BAR_5 &&
> - epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
> + flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ||
> + (flags & PCI_BASE_ADDRESS_SPACE_IO &&
> + flags & PCI_BASE_ADDRESS_IO_MASK))
> return -EINVAL;
If possible I'd like this to be split. But otherwise
Acked-by: Kishon Vijay Abraham I <[email protected]>
>
> if (!epc->ops->set_bar)
>
On Wednesday 28 March 2018 05:20 PM, Niklas Cassel wrote:
> Setting a BAR size > 4 GB is invalid if PCI_BASE_ADDRESS_MEM_TYPE_64
> flag is not set.
>
> This sanity check is done in pci_epc_set_bar(), so that we don't need
> to do this sanity check in all epc->ops->set_bar() implementations.
>
> Signed-off-by: Niklas Cassel <[email protected]>
Acked-by: Kishon Vijay Abraham I <[email protected]>
> ---
> drivers/pci/endpoint/pci-epc-core.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 40eea20d21f9..8637822605ff 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -315,7 +315,9 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no,
> (epf_bar->barno == BAR_5 &&
> flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ||
> (flags & PCI_BASE_ADDRESS_SPACE_IO &&
> - flags & PCI_BASE_ADDRESS_IO_MASK))
> + flags & PCI_BASE_ADDRESS_IO_MASK) ||
> + (upper_32_bits(epf_bar->size) &&
> + !(flags & PCI_BASE_ADDRESS_MEM_TYPE_64)))
> return -EINVAL;
>
> if (!epc->ops->set_bar)
>
Hi,
On Wednesday 28 March 2018 05:20 PM, Niklas Cassel wrote:
> Since a 64-bit BAR consists of a BAR pair, we need to write to both
> BARs in the BAR pair to setup the BAR properly.
>
> Signed-off-by: Niklas Cassel <[email protected]>
> ---
> drivers/pci/dwc/pcie-designware-ep.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> index 5a0bb53c795c..571b90f88d84 100644
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -138,8 +138,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 (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> + 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);
> + }
I think this should work too?
dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
dw_pcie_writel_dbi(pci, reg, flags);
if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
dw_pcie_writel_dbi(pci, reg + 4, 0);
}
Thanks
Kishon
On Wednesday 28 March 2018 05:20 PM, Niklas Cassel wrote:
> If a 64-bit BAR was set-up, we need to skip a BAR,
> since a 64-bit BAR consists of a BAR pair.
>
> We need to check what BAR width the epc->ops->set_bar() specific
> implementation actually did set-up, since some drivers, like the
> Cadence EP controller, sometimes sets up a 64-bit BAR, even though
> a 32-bit BAR was requested.
>
> Signed-off-by: Niklas Cassel <[email protected]>
Acked-by: Kishon Vijay Abraham I <[email protected]>
> ---
> drivers/pci/endpoint/functions/pci-epf-test.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 91274779e59f..d46e3ebabb8e 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -380,6 +380,13 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
> if (bar == test_reg_bar)
> return ret;
> }
> + /*
> + * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
> + * if the specific implementation required a 64-bit BAR,
> + * even if we only requested a 32-bit BAR.
> + */
> + if (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
> + bar++;
> }
>
> return 0;
>
Hi Niklas,
On Wednesday 28 March 2018 05:20 PM, Niklas Cassel wrote:
> Make epc->ops->clear_bar()/pci_epc_clear_bar() take struct *epf_bar.
>
> This is needed so that epc->ops->clear_bar() can clear the BAR pair,
> if the BAR is 64-bits wide.
>
> This also makes it possible for pci_epc_clear_bar() to sanity check
> the flags.
>
> Signed-off-by: Niklas Cassel <[email protected]>
> ---
> drivers/pci/cadence/pcie-cadence-ep.c | 3 ++-
> drivers/pci/dwc/pcie-designware-ep.c | 13 ++++++++++---
> drivers/pci/endpoint/functions/pci-epf-test.c | 5 ++++-
> drivers/pci/endpoint/pci-epc-core.c | 7 ++++---
> include/linux/pci-epc.h | 5 +++--
> 5 files changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
> index 2905e098678c..3d8283e450a9 100644
> --- a/drivers/pci/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/cadence/pcie-cadence-ep.c
> @@ -145,10 +145,11 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
> }
>
> static void cdns_pcie_ep_clear_bar(struct pci_epc *epc, u8 fn,
> - enum pci_barno bar)
> + struct pci_epf_bar *epf_bar)
> {
> struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
> struct cdns_pcie *pcie = &ep->pcie;
> + enum pci_barno bar = epf_bar->barno;
> u32 reg, cfg, b, ctrl;
>
> if (bar < BAR_4) {
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> index 571b90f88d84..cc4d8381c1dc 100644
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -19,7 +19,8 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> pci_epc_linkup(epc);
> }
>
> -void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
> +static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
> + int flags)
Looks like the 'flags' are not used anywhere here?
Thanks
Kishon
On Wednesday 28 March 2018 05:20 PM, Niklas Cassel wrote:
> Since a 64-bit BAR consists of a BAR pair, and since there is no
> BAR after BAR_5, BAR_5 cannot be 64-bits wide.
>
> This sanity check is done in pci_epc_clear_bar(), so that we don't need
> to do this sanity check in all epc->ops->clear_bar() implementations.
>
> Signed-off-by: Niklas Cassel <[email protected]>
Acked-by: Kishon Vijay Abraham I <[email protected]>
> ---
> Kishon made a review comment that he wanted this:
> https://marc.info/?l=linux-kernel&m=152161168226203
>
> Personally, I don't think that this check is needed,
> since pci_epc_set_bar() already does this check,
> and no one should modify the flags after pci_epc_set_bar()
> has been called.
>
> If everyone agrees, then this patch could be dropped.
>
> drivers/pci/endpoint/pci-epc-core.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index eccc942043cb..b0ee42739c3c 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -285,7 +285,9 @@ void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no,
> {
> unsigned long flags;
>
> - if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> + if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
> + (epf_bar->barno == BAR_5 &&
> + epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
> return;
>
> if (!epc->ops->clear_bar)
>
On Wednesday 28 March 2018 05:20 PM, Niklas Cassel wrote:
> Since a 64-bit BAR consists of a BAR pair, we need to write to both
> BARs in the BAR pair to clear the BAR properly.
>
> Signed-off-by: Niklas Cassel <[email protected]>
Acked-by: Kishon Vijay Abraham I <[email protected]>
> ---
> drivers/pci/dwc/pcie-designware-ep.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> index cc4d8381c1dc..4d304e3ccf24 100644
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -28,6 +28,10 @@ static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
> dw_pcie_dbi_ro_wr_en(pci);
> dw_pcie_writel_dbi2(pci, reg, 0x0);
> dw_pcie_writel_dbi(pci, reg, 0x0);
> + if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> + dw_pcie_writel_dbi2(pci, reg + 4, 0x0);
> + dw_pcie_writel_dbi(pci, reg + 4, 0x0);
> + }
> dw_pcie_dbi_ro_wr_dis(pci);
> }
>
>
Hi Niklas,
On 28/03/2018 12:50, Niklas Cassel wrote:
> PCI endpoint fixes to improve the way 64-bit BARs are handled.
>
>
> There are still future improvements that could be made:
>
> pci-epf-test.c always allocates space for
> 6 BARs, even when using 64-bit BARs (which
> really only requires us to allocate 3 BARs).
>
> pcitest.sh will print "NOT OKAY" for BAR1,
> BAR3, and BAR5 when using 64-bit BARs.
> This could probably be improved to say
> something like "N/A (64-bit BAR)".
>
> Niklas Cassel (12):
> PCI: endpoint: BAR width should not depend on sizeof dma_addr_t
> PCI: endpoint: Simplify epc->ops->set_bar()/pci_epc_set_bar()
> PCI: endpoint: Setting BAR_5 to 64-bits wide is invalid
> PCI: endpoint: Setting 64-bit/prefetch bit is invalid when IO is set
> PCI: endpoint: Setting a BAR size > 4 GB is invalid if 64-bit flag is
> not set
> PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs
> properly
> PCI: cadence: Set PCI_BASE_ADDRESS_MEM_TYPE_64 if a 64-bit BAR was
> set-up
> PCI: endpoint: Handle 64-bit BARs properly
> PCI: endpoint: Make epc->ops->clear_bar()/pci_epc_clear_bar() take
> struct *epf_bar
> PCI: endpoint: Make sure that BAR_5 does not have 64-bit flag set when
> clearing
> PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs
> properly
> misc: pci_endpoint_test: Handle 64-bit BARs properly
>
> drivers/misc/pci_endpoint_test.c | 12 +++++----
> drivers/pci/cadence/pcie-cadence-ep.c | 15 ++++++++---
> drivers/pci/dwc/pcie-designware-ep.c | 36 +++++++++++++++++++++------
> drivers/pci/endpoint/functions/pci-epf-test.c | 28 +++++++++++++--------
> drivers/pci/endpoint/pci-epc-core.c | 32 +++++++++++++++---------
> drivers/pci/endpoint/pci-epf-core.c | 4 +++
> include/linux/pci-epc.h | 11 ++++----
> include/linux/pci-epf.h | 2 ++
> 8 files changed, 95 insertions(+), 45 deletions(-)
>
For the whole series:
Tested-by: Gustavo Pimentel <[email protected]>
Regards,
Gustavo
On Wed, Mar 28, 2018 at 20:37, Bjorn Helgaas wrote:
> On Wed, Mar 28, 2018 at 01:24:10PM +0000, Alan Douglas wrote:
> > > On 28/03/2018 12:51, Niklas Cassel wrote:
> > > cdns_pcie_ep_set_bar() does some round-up of the BAR size, which
> > > means that a 64-bit BAR can be set-up, even when the flag
> > > PCI_BASE_ADDRESS_MEM_TYPE_64 isn't set.
> >
> > > If a 64-bit BAR was set-up, set the flag
> PCI_BASE_ADDRESS_MEM_TYPE_64, so that the calling function can know
> what BAR width that was actually set-up.
> >
> > > I'm not sure why cdns_pcie_ep_set_bar() doesn't obey the flag
> PCI_BASE_ADDRESS_MEM_TYPE_64, but I leave this for the MAINTAINER to
> fix, since there might be a reason why > this flag is ignored.
> > Will investigate and fix this in future patch
> >
> > > Signed-off-by: Niklas Cassel <[email protected]>
> > > ---
> > > drivers/pci/cadence/pcie-cadence-ep.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > Change looks good to me.
>
> It will be helpful to Lorenzo if you spell this out, e.g.,
>
> Acked-by: Alan Douglas <[email protected]>
Acked-by: Alan Douglas <[email protected]>
>
> Also, it looks like we need a MAINTAINERS update to add drivers/pci/cadence/
> to the "PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS"
> filename patterns.
Lorenzo please let me know if I should create a patch for this, it sounds good to me.
On Thu, Mar 29, 2018 at 03:17:11PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Wednesday 28 March 2018 05:20 PM, Niklas Cassel wrote:
> > Since a 64-bit BAR consists of a BAR pair, we need to write to both
> > BARs in the BAR pair to setup the BAR properly.
> >
> > Signed-off-by: Niklas Cassel <[email protected]>
> > ---
> > drivers/pci/dwc/pcie-designware-ep.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> > index 5a0bb53c795c..571b90f88d84 100644
> > --- a/drivers/pci/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/dwc/pcie-designware-ep.c
> > @@ -138,8 +138,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 (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > + 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);
> > + }
>
>
> I think this should work too?
> dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
> dw_pcie_writel_dbi(pci, reg, flags);
>
> if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
> dw_pcie_writel_dbi(pci, reg + 4, 0);
> }
>
Hello Kishon,
I agree, your suggestion is more neat.
Kishon, please tell me if you insist that the long if-statement
in pci_epc_set_bar() should be split, since there are 5 different
conditions. Because imho, having 5 succeeding if-statements isn't
clearer than having 1 long if-statement.
If Kishon agrees with me, then the review comment in this mail
seems to be the only review comment.
And in that case, perhaps Lorenzo wouldn't mind fixing this up.
Or perhaps Lorenzo prefers if I reroll the whole patch series?
Kind regards,
Niklas
On Thu, Mar 29, 2018 at 02:52:56PM +0100, Gustavo Pimentel wrote:
> Hi Niklas,
>
> On 28/03/2018 12:50, Niklas Cassel wrote:
> > PCI endpoint fixes to improve the way 64-bit BARs are handled.
> >
> >
> > There are still future improvements that could be made:
> >
> > pci-epf-test.c always allocates space for
> > 6 BARs, even when using 64-bit BARs (which
> > really only requires us to allocate 3 BARs).
> >
> > pcitest.sh will print "NOT OKAY" for BAR1,
> > BAR3, and BAR5 when using 64-bit BARs.
> > This could probably be improved to say
> > something like "N/A (64-bit BAR)".
> >
> > Niklas Cassel (12):
> > PCI: endpoint: BAR width should not depend on sizeof dma_addr_t
> > PCI: endpoint: Simplify epc->ops->set_bar()/pci_epc_set_bar()
> > PCI: endpoint: Setting BAR_5 to 64-bits wide is invalid
> > PCI: endpoint: Setting 64-bit/prefetch bit is invalid when IO is set
> > PCI: endpoint: Setting a BAR size > 4 GB is invalid if 64-bit flag is
> > not set
> > PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs
> > properly
> > PCI: cadence: Set PCI_BASE_ADDRESS_MEM_TYPE_64 if a 64-bit BAR was
> > set-up
> > PCI: endpoint: Handle 64-bit BARs properly
> > PCI: endpoint: Make epc->ops->clear_bar()/pci_epc_clear_bar() take
> > struct *epf_bar
> > PCI: endpoint: Make sure that BAR_5 does not have 64-bit flag set when
> > clearing
> > PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs
> > properly
> > misc: pci_endpoint_test: Handle 64-bit BARs properly
> >
> > drivers/misc/pci_endpoint_test.c | 12 +++++----
> > drivers/pci/cadence/pcie-cadence-ep.c | 15 ++++++++---
> > drivers/pci/dwc/pcie-designware-ep.c | 36 +++++++++++++++++++++------
> > drivers/pci/endpoint/functions/pci-epf-test.c | 28 +++++++++++++--------
> > drivers/pci/endpoint/pci-epc-core.c | 32 +++++++++++++++---------
> > drivers/pci/endpoint/pci-epf-core.c | 4 +++
> > include/linux/pci-epc.h | 11 ++++----
> > include/linux/pci-epf.h | 2 ++
> > 8 files changed, 95 insertions(+), 45 deletions(-)
> >
>
> For the whole series:
>
> Tested-by: Gustavo Pimentel <[email protected]>
Hello Gustavo,
Thanks a lot for testing!
Kind regards,
Niklas
On Thu, Mar 29, 2018 at 03:30:23PM +0530, Kishon Vijay Abraham I wrote:
> Hi Niklas,
>
> On Wednesday 28 March 2018 05:20 PM, Niklas Cassel wrote:
> > Make epc->ops->clear_bar()/pci_epc_clear_bar() take struct *epf_bar.
> >
> > This is needed so that epc->ops->clear_bar() can clear the BAR pair,
> > if the BAR is 64-bits wide.
> >
> > This also makes it possible for pci_epc_clear_bar() to sanity check
> > the flags.
> >
> > Signed-off-by: Niklas Cassel <[email protected]>
> > ---
> > drivers/pci/cadence/pcie-cadence-ep.c | 3 ++-
> > drivers/pci/dwc/pcie-designware-ep.c | 13 ++++++++++---
> > drivers/pci/endpoint/functions/pci-epf-test.c | 5 ++++-
> > drivers/pci/endpoint/pci-epc-core.c | 7 ++++---
> > include/linux/pci-epc.h | 5 +++--
> > 5 files changed, 23 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
> > index 2905e098678c..3d8283e450a9 100644
> > --- a/drivers/pci/cadence/pcie-cadence-ep.c
> > +++ b/drivers/pci/cadence/pcie-cadence-ep.c
> > @@ -145,10 +145,11 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn,
> > }
> >
> > static void cdns_pcie_ep_clear_bar(struct pci_epc *epc, u8 fn,
> > - enum pci_barno bar)
> > + struct pci_epf_bar *epf_bar)
> > {
> > struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
> > struct cdns_pcie *pcie = &ep->pcie;
> > + enum pci_barno bar = epf_bar->barno;
> > u32 reg, cfg, b, ctrl;
> >
> > if (bar < BAR_4) {
> > diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> > index 571b90f88d84..cc4d8381c1dc 100644
> > --- a/drivers/pci/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/dwc/pcie-designware-ep.c
> > @@ -19,7 +19,8 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> > pci_epc_linkup(epc);
> > }
> >
> > -void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
> > +static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
> > + int flags)
>
> Looks like the 'flags' are not used anywhere here?
Hello Kishon,
That is correct, this patch is simply refactoring, flags is first used
in patch 11/12, since I didn't want to refactor + add new code in the
same commit.
Kind regards,
Niklas
On Tuesday 03 April 2018 01:07 AM, Niklas Cassel wrote:
> On Thu, Mar 29, 2018 at 03:17:11PM +0530, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Wednesday 28 March 2018 05:20 PM, Niklas Cassel wrote:
>>> Since a 64-bit BAR consists of a BAR pair, we need to write to both
>>> BARs in the BAR pair to setup the BAR properly.
>>>
>>> Signed-off-by: Niklas Cassel <[email protected]>
>>> ---
>>> drivers/pci/dwc/pcie-designware-ep.c | 11 +++++++++--
>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
>>> index 5a0bb53c795c..571b90f88d84 100644
>>> --- a/drivers/pci/dwc/pcie-designware-ep.c
>>> +++ b/drivers/pci/dwc/pcie-designware-ep.c
>>> @@ -138,8 +138,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 (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
>>> + 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);
>>> + }
>>
>>
>> I think this should work too?
>> dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
>> dw_pcie_writel_dbi(pci, reg, flags);
>>
>> if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
>> dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
>> dw_pcie_writel_dbi(pci, reg + 4, 0);
>> }
>>
>
> Hello Kishon,
>
> I agree, your suggestion is more neat.
>
>
> Kishon, please tell me if you insist that the long if-statement
> in pci_epc_set_bar() should be split, since there are 5 different
> conditions. Because imho, having 5 succeeding if-statements isn't
I'm okay as it is as well if Lorenzo/Bjorn is also fine with it.
Thanks
Kishon
On Mon, Apr 02, 2018 at 09:37:03PM +0200, Niklas Cassel wrote:
> On Thu, Mar 29, 2018 at 03:17:11PM +0530, Kishon Vijay Abraham I wrote:
> > Hi,
> >
> > On Wednesday 28 March 2018 05:20 PM, Niklas Cassel wrote:
> > > Since a 64-bit BAR consists of a BAR pair, we need to write to both
> > > BARs in the BAR pair to setup the BAR properly.
> > >
> > > Signed-off-by: Niklas Cassel <[email protected]>
> > > ---
> > > drivers/pci/dwc/pcie-designware-ep.c | 11 +++++++++--
> > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> > > index 5a0bb53c795c..571b90f88d84 100644
> > > --- a/drivers/pci/dwc/pcie-designware-ep.c
> > > +++ b/drivers/pci/dwc/pcie-designware-ep.c
> > > @@ -138,8 +138,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 (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > > + 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);
> > > + }
> >
> >
> > I think this should work too?
> > dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
> > dw_pcie_writel_dbi(pci, reg, flags);
> >
> > if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
> > dw_pcie_writel_dbi(pci, reg + 4, 0);
> > }
> >
>
> Hello Kishon,
>
> I agree, your suggestion is more neat.
>
>
> Kishon, please tell me if you insist that the long if-statement
> in pci_epc_set_bar() should be split, since there are 5 different
> conditions. Because imho, having 5 succeeding if-statements isn't
> clearer than having 1 long if-statement.
>
> If Kishon agrees with me, then the review comment in this mail
> seems to be the only review comment.
> And in that case, perhaps Lorenzo wouldn't mind fixing this up.
> Or perhaps Lorenzo prefers if I reroll the whole patch series?
I updated it myself in my pci/endpoint branch, please have a look, I
can't guarantee we can merge this for this cycle though, I will ask
Bjorn; apologies I could not be online for a while.
Lorenzo
> Kind regards,
> Niklas
On Tue, Apr 03, 2018 at 01:53:12PM +0100, Lorenzo Pieralisi wrote:
> On Mon, Apr 02, 2018 at 09:37:03PM +0200, Niklas Cassel wrote:
> > On Thu, Mar 29, 2018 at 03:17:11PM +0530, Kishon Vijay Abraham I wrote:
> > > Hi,
> > >
> > > On Wednesday 28 March 2018 05:20 PM, Niklas Cassel wrote:
> > > > Since a 64-bit BAR consists of a BAR pair, we need to write to both
> > > > BARs in the BAR pair to setup the BAR properly.
> > > >
> > > > Signed-off-by: Niklas Cassel <[email protected]>
> > > > ---
> > > > drivers/pci/dwc/pcie-designware-ep.c | 11 +++++++++--
> > > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> > > > index 5a0bb53c795c..571b90f88d84 100644
> > > > --- a/drivers/pci/dwc/pcie-designware-ep.c
> > > > +++ b/drivers/pci/dwc/pcie-designware-ep.c
> > > > @@ -138,8 +138,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 (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > > > + 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);
> > > > + }
> > >
> > >
> > > I think this should work too?
> > > dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
> > > dw_pcie_writel_dbi(pci, reg, flags);
> > >
> > > if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > > dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
> > > dw_pcie_writel_dbi(pci, reg + 4, 0);
> > > }
> > >
> >
> > Hello Kishon,
> >
> > I agree, your suggestion is more neat.
> >
> >
> > Kishon, please tell me if you insist that the long if-statement
> > in pci_epc_set_bar() should be split, since there are 5 different
> > conditions. Because imho, having 5 succeeding if-statements isn't
> > clearer than having 1 long if-statement.
> >
> > If Kishon agrees with me, then the review comment in this mail
> > seems to be the only review comment.
> > And in that case, perhaps Lorenzo wouldn't mind fixing this up.
> > Or perhaps Lorenzo prefers if I reroll the whole patch series?
>
> I updated it myself in my pci/endpoint branch, please have a look, I
> can't guarantee we can merge this for this cycle though, I will ask
> Bjorn; apologies I could not be online for a while.
Hello Lorenzo,
your pci/endpoint branch looks good.
There is no rush, merge it whenever you think is best.
Have in mind that there is EP support @ patchwork for Rockchip
and for pcie-designware-plat, so make sure to juggle all the
branches with care :)
Kind regards,
Niklas