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 (5):
PCI: endpoint: BAR width should not depend on sizeof dma_addr_t
PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs
properly
PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs
properly
PCI: endpoint: Make pci_epc_set_bar() return the BAR width that was
set-up
misc: pci_endpoint_test: Handle 64-bit BARs properly
drivers/misc/pci_endpoint_test.c | 12 +++++++-----
drivers/pci/cadence/pcie-cadence-ep.c | 2 +-
drivers/pci/dwc/pcie-designware-ep.c | 22 ++++++++++++++++++----
drivers/pci/endpoint/functions/pci-epf-test.c | 22 +++++++++++++++-------
4 files changed, 41 insertions(+), 17 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).
Since this driver is generic, default to always using BAR width
of 32-bits. 64-bit BARs can easily be tested by replacing
PCI_BASE_ADDRESS_MEM_TYPE_32 with PCI_BASE_ADDRESS_MEM_TYPE_64
in bar_flags.
Signed-off-by: Niklas Cassel <[email protected]>
---
Note to Lorenzo/Bjorn:
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/endpoint/functions/pci-epf-test.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 800da09d9005..7c70433b11a7 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -71,6 +71,14 @@ struct pci_epf_test_data {
};
static int bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
+static int bar_flags[] = {
+ PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
+ PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
+ PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
+ PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
+ PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
+ PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32
+};
static int pci_epf_test_copy(struct pci_epf_test *epf_test)
{
@@ -358,7 +366,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;
@@ -367,15 +374,11 @@ 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];
ret = pci_epc_set_bar(epc, epf->func_no, bar,
epf_bar->phys_addr,
- epf_bar->size, flags);
+ epf_bar->size, bar_flags[bar]);
if (ret) {
pci_epf_free_space(epf, epf_test->reg[bar], bar);
dev_err(dev, "failed to set BAR%d\n", bar);
--
2.14.2
In order to properly handle 64-bit BARs, we need to know what BAR width
that was actually set-up by specific pci_epc_set_bar() implementations.
This is done so that we can know if we need to skip a BAR,
since a 64-bit BAR consists of a BAR pair.
It is important to know the BAR width that was actually set-up,
since some drivers, like the Cadence EP controller, does not
simply look at PCI_BASE_ADDRESS_MEM_TYPE_64, as it configures
all BARs larger than 2G as 64-bit.
Signed-off-by: Niklas Cassel <[email protected]>
---
drivers/pci/cadence/pcie-cadence-ep.c | 2 +-
drivers/pci/dwc/pcie-designware-ep.c | 4 ++--
drivers/pci/endpoint/functions/pci-epf-test.c | 7 ++++++-
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
index 3c3a97743453..0e4cc4cca56d 100644
--- a/drivers/pci/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/cadence/pcie-cadence-ep.c
@@ -135,7 +135,7 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, enum pci_barno bar,
CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_CTRL(b, ctrl));
cdns_pcie_writel(pcie, reg, cfg);
- return 0;
+ return is_64bits ? 1 : 0;
}
static void cdns_pcie_ep_clear_bar(struct pci_epc *epc, u8 fn,
diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index b20b2651caf9..f3c19f8ff8e5 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -139,7 +139,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
as_type = DW_PCIE_AS_IO;
ret = dw_pcie_ep_inbound_atu(ep, bar, bar_phys, as_type);
- if (ret)
+ if (ret < 0)
return ret;
dw_pcie_dbi_ro_wr_en(pci);
@@ -154,7 +154,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
}
dw_pcie_dbi_ro_wr_dis(pci);
- return 0;
+ return (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 1 : 0;
}
static int dw_pcie_find_index(struct dw_pcie_ep *ep, phys_addr_t addr,
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 7c70433b11a7..09878e011284 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -379,12 +379,17 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
ret = pci_epc_set_bar(epc, epf->func_no, bar,
epf_bar->phys_addr,
epf_bar->size, bar_flags[bar]);
- if (ret) {
+ if (ret < 0) {
pci_epf_free_space(epf, epf_test->reg[bar], bar);
dev_err(dev, "failed to set BAR%d\n", bar);
if (bar == test_reg_bar)
return ret;
}
+ /*
+ * pci_epc_set_bar() returns 1 if a 64-bit BAR was set-up,
+ * or 0 if a 32-bit BAR was set-up.
+ */
+ bar += ret;
}
return 0;
--
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 | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index 946bbdf53c4d..b20b2651caf9 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -22,11 +22,18 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
{
u32 reg;
+ u32 val;
reg = PCI_BASE_ADDRESS_0 + (4 * bar);
+ val = dw_pcie_readl_dbi(pci, reg);
dw_pcie_dbi_ro_wr_en(pci);
dw_pcie_writel_dbi2(pci, reg, 0x0);
dw_pcie_writel_dbi(pci, reg, 0x0);
+ if (!(val & PCI_BASE_ADDRESS_SPACE_IO) &&
+ (val & 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
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]>
---
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 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 9236b998327f..946bbdf53c4d 100644
--- 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 (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
On Thu, Mar 08, 2018 at 02:33:30PM +0100, Niklas Cassel wrote:
> 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]>
> ---
> drivers/misc/pci_endpoint_test.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
Where are the 4 other patches in this series? I can't just only apply
the last one, right?
Please be careful when sending patches to properly notify everyone...
this is now dropped from my patch queue.
greg k-h
On Thu, Mar 15, 2018 at 02:22:13PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 08, 2018 at 02:33:30PM +0100, Niklas Cassel wrote:
> > 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]>
> > ---
> > drivers/misc/pci_endpoint_test.c | 12 +++++++-----
> > 1 file changed, 7 insertions(+), 5 deletions(-)
>
> Where are the 4 other patches in this series? I can't just only apply
> the last one, right?
http://patchwork.ozlabs.org/project/linux-pci/list/?series=32658
I'm expecting all patches to go via Lorenzo's pci tree.
>
> Please be careful when sending patches to properly notify everyone...
I'm using scripts from Joe Perches that run get_maintainer.pl,
these scripts are then used as --to-cmd and --cc-cmd for git send-email:
https://lkml.org/lkml/2016/9/14/482
(I did have to modify them so they handle rerolled patch series.)
BTW. Joe, do you have updated versions of your scripts?
Greg, I'm assuming that you wouldn't want to be cc'd on all patches in the
series, since all except the last one are pci related.
However, perhaps it would be nice to add all those who are cc'd in the
individual patches, as cc to the cover letter, that way it might be easier
to figure out what tree will most likely merge a certain patch series.
Kind regards,
Niklas
On Thu, Mar 08, 2018 at 02:33:26PM +0100, 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).
>
> Since this driver is generic, default to always using BAR width
> of 32-bits. 64-bit BARs can easily be tested by replacing
> PCI_BASE_ADDRESS_MEM_TYPE_32 with PCI_BASE_ADDRESS_MEM_TYPE_64
> in bar_flags.
>
> Signed-off-by: Niklas Cassel <[email protected]>
> ---
> Note to Lorenzo/Bjorn:
> 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/endpoint/functions/pci-epf-test.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 800da09d9005..7c70433b11a7 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -71,6 +71,14 @@ struct pci_epf_test_data {
> };
>
> static int bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
> +static int bar_flags[] = {
> + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32
> +};
Niklas,
I think you are almost there, I have one question though to address
that can even simplify the patchset.
If, according to your own commit logs (and my reading of the code), the
Cadence driver makes a decision on the BAR size just by checking the
corresponding region size (I would be happy to hear the reason
underpinning that choice, BTW), why can't we do the same for DWC (ie to
let the DWC driver decides whether a BAR should be 64 or 32 bits ?)
This would mean that in this patch we would not bother about the BAR
32/64 size flag at all.
Thoughts ?
Lorenzo
>
> static int pci_epf_test_copy(struct pci_epf_test *epf_test)
> {
> @@ -358,7 +366,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;
> @@ -367,15 +374,11 @@ 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];
> ret = pci_epc_set_bar(epc, epf->func_no, bar,
> epf_bar->phys_addr,
> - epf_bar->size, flags);
> + epf_bar->size, bar_flags[bar]);
> if (ret) {
> pci_epf_free_space(epf, epf_test->reg[bar], bar);
> dev_err(dev, "failed to set BAR%d\n", bar);
> --
> 2.14.2
>
On Fri, Mar 16, 2018 at 06:02:20PM +0000, Lorenzo Pieralisi wrote:
> On Thu, Mar 08, 2018 at 02:33:26PM +0100, 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).
> >
> > Since this driver is generic, default to always using BAR width
> > of 32-bits. 64-bit BARs can easily be tested by replacing
> > PCI_BASE_ADDRESS_MEM_TYPE_32 with PCI_BASE_ADDRESS_MEM_TYPE_64
> > in bar_flags.
> >
> > Signed-off-by: Niklas Cassel <[email protected]>
> > ---
> > Note to Lorenzo/Bjorn:
> > 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/endpoint/functions/pci-epf-test.c | 15 +++++++++------
> > 1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index 800da09d9005..7c70433b11a7 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -71,6 +71,14 @@ struct pci_epf_test_data {
> > };
> >
> > static int bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
> > +static int bar_flags[] = {
> > + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32
> > +};
>
> Niklas,
>
> I think you are almost there, I have one question though to address
> that can even simplify the patchset.
>
> If, according to your own commit logs (and my reading of the code), the
> Cadence driver makes a decision on the BAR size just by checking the
> corresponding region size (I would be happy to hear the reason
> underpinning that choice, BTW), why can't we do the same for DWC (ie to
> let the DWC driver decides whether a BAR should be 64 or 32 bits ?)
We could, but I think that would be a mistake.
The API that the user/"endpoint function" has available to configure
the BARs:
pci_epc_set_bar()
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,
I think that the API should allow that.
This would not be possible if pci_epc_set_bar() would start to
ignore PCI_BASE_ADDRESS_MEM_TYPE_64 from the flags parameter
(and BAR width thus only being controlled by the size parameter).
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);
Best regards,
Niklas
On Sat, Mar 17, 2018 at 01:55:13AM +0100, Niklas Cassel wrote:
> On Fri, Mar 16, 2018 at 06:02:20PM +0000, Lorenzo Pieralisi wrote:
> > On Thu, Mar 08, 2018 at 02:33:26PM +0100, 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).
> > >
> > > Since this driver is generic, default to always using BAR width
> > > of 32-bits. 64-bit BARs can easily be tested by replacing
> > > PCI_BASE_ADDRESS_MEM_TYPE_32 with PCI_BASE_ADDRESS_MEM_TYPE_64
> > > in bar_flags.
> > >
> > > Signed-off-by: Niklas Cassel <[email protected]>
> > > ---
> > > Note to Lorenzo/Bjorn:
> > > 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/endpoint/functions/pci-epf-test.c | 15 +++++++++------
> > > 1 file changed, 9 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > index 800da09d9005..7c70433b11a7 100644
> > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > @@ -71,6 +71,14 @@ struct pci_epf_test_data {
> > > };
> > >
> > > static int bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
> > > +static int bar_flags[] = {
> > > + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > > + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > > + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > > + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > > + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> > > + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32
> > > +};
> >
> > Niklas,
> >
> > I think you are almost there, I have one question though to address
> > that can even simplify the patchset.
> >
> > If, according to your own commit logs (and my reading of the code), the
> > Cadence driver makes a decision on the BAR size just by checking the
> > corresponding region size (I would be happy to hear the reason
> > underpinning that choice, BTW), why can't we do the same for DWC (ie to
> > let the DWC driver decides whether a BAR should be 64 or 32 bits ?)
>
> We could, but I think that would be a mistake.
>
> The API that the user/"endpoint function" has available to configure
> the BARs:
> pci_epc_set_bar()
>
> 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,
> I think that the API should allow that.
Yes, that's a good point, thanks a lot for the explanation.
Lorenzo
> This would not be possible if pci_epc_set_bar() would start to
> ignore PCI_BASE_ADDRESS_MEM_TYPE_64 from the flags parameter
> (and BAR width thus only being controlled by the size parameter).
>
> 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);
>
>
>
> Best regards,
> Niklas
On Thu, Mar 08, 2018 at 02:33:25PM +0100, 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 (5):
> PCI: endpoint: BAR width should not depend on sizeof dma_addr_t
> PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs
> properly
> PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs
> properly
> PCI: endpoint: Make pci_epc_set_bar() return the BAR width that was
> set-up
> misc: pci_endpoint_test: Handle 64-bit BARs properly
>
> drivers/misc/pci_endpoint_test.c | 12 +++++++-----
> drivers/pci/cadence/pcie-cadence-ep.c | 2 +-
> drivers/pci/dwc/pcie-designware-ep.c | 22 ++++++++++++++++++----
> drivers/pci/endpoint/functions/pci-epf-test.c | 22 +++++++++++++++-------
> 4 files changed, 41 insertions(+), 17 deletions(-)
Niklas,
I am fine with the series but I'd need Kishon ACKs (and Greg's for the
last patch), I am ready to queue them then.
Thanks,
Lorenzo
Hello Greg,
Lorenzo is fine with this series
( https://marc.info/?l=linux-kernel&m=152147837619191&w=2 )
However, he wants your ack on this patch before merging.
Could you please have a look at this patch?
Kind regards,
Niklas
On Thu, Mar 08, 2018 at 02:33:30PM +0100, Niklas Cassel wrote:
> 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]>
> ---
> 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
>
On Mon, Mar 19, 2018 at 04:52:34PM +0000, Lorenzo Pieralisi wrote:
> On Thu, Mar 08, 2018 at 02:33:25PM +0100, 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 (5):
> > PCI: endpoint: BAR width should not depend on sizeof dma_addr_t
> > PCI: designware-ep: Make dw_pcie_ep_set_bar() handle 64-bit BARs
> > properly
> > PCI: designware-ep: Make dw_pcie_ep_reset_bar() handle 64-bit BARs
> > properly
> > PCI: endpoint: Make pci_epc_set_bar() return the BAR width that was
> > set-up
> > misc: pci_endpoint_test: Handle 64-bit BARs properly
> >
> > drivers/misc/pci_endpoint_test.c | 12 +++++++-----
> > drivers/pci/cadence/pcie-cadence-ep.c | 2 +-
> > drivers/pci/dwc/pcie-designware-ep.c | 22 ++++++++++++++++++----
> > drivers/pci/endpoint/functions/pci-epf-test.c | 22 +++++++++++++++-------
> > 4 files changed, 41 insertions(+), 17 deletions(-)
>
> Niklas,
>
> I am fine with the series but I'd need Kishon ACKs (and Greg's for the
> last patch), I am ready to queue them then.
Hello Kishon,
could you please have a look at this series?
Kind regards,
Niklas
On Thursday 08 March 2018 07:03 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).
>
> Since this driver is generic, default to always using BAR width
> of 32-bits. 64-bit BARs can easily be tested by replacing
> PCI_BASE_ADDRESS_MEM_TYPE_32 with PCI_BASE_ADDRESS_MEM_TYPE_64
> in bar_flags.
>
> Signed-off-by: Niklas Cassel <[email protected]>
Acked-by: Kishon Vijay Abraham I <[email protected]>
> ---
> Note to Lorenzo/Bjorn:
> 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/endpoint/functions/pci-epf-test.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 800da09d9005..7c70433b11a7 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -71,6 +71,14 @@ struct pci_epf_test_data {
> };
>
> static int bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
> +static int bar_flags[] = {
> + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32,
> + PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32
> +};
>
> static int pci_epf_test_copy(struct pci_epf_test *epf_test)
> {
> @@ -358,7 +366,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;
> @@ -367,15 +374,11 @@ 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];
> ret = pci_epc_set_bar(epc, epf->func_no, bar,
> epf_bar->phys_addr,
> - epf_bar->size, flags);
> + epf_bar->size, bar_flags[bar]);
> if (ret) {
> pci_epf_free_space(epf, epf_test->reg[bar], bar);
> dev_err(dev, "failed to set BAR%d\n", bar);
>
Hi Niklas,
On Thursday 08 March 2018 07:03 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 9236b998327f..946bbdf53c4d 100644
> --- 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 (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);
I think we should check in pci_epc_set_bar to make sure BAR_5 cannot have
PCI_BASE_ADDRESS_MEM_TYPE_64 flag set as this might lead undesired result.
Thanks
Kishon
Hi Niklas,
On Thursday 08 March 2018 07:03 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]>
> ---
> drivers/pci/dwc/pcie-designware-ep.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> index 946bbdf53c4d..b20b2651caf9 100644
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -22,11 +22,18 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
> {
> u32 reg;
> + u32 val;
>
> reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> + val = dw_pcie_readl_dbi(pci, reg);
> dw_pcie_dbi_ro_wr_en(pci);
> dw_pcie_writel_dbi2(pci, reg, 0x0);
> dw_pcie_writel_dbi(pci, reg, 0x0);
> + if (!(val & PCI_BASE_ADDRESS_SPACE_IO) &&
> + (val & PCI_BASE_ADDRESS_MEM_TYPE_64)) {
> + dw_pcie_writel_dbi2(pci, reg + 4, 0x0);
> + dw_pcie_writel_dbi(pci, reg + 4, 0x0);
> + }
same comment as previous patch apply here too.
Thanks
Kishon
On Thursday 08 March 2018 07:03 PM, Niklas Cassel wrote:
> 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];
>
Hi Niklas,
On Thursday 08 March 2018 07:03 PM, Niklas Cassel wrote:
> In order to properly handle 64-bit BARs, we need to know what BAR width
> that was actually set-up by specific pci_epc_set_bar() implementations.
>
> This is done so that we can know if we need to skip a BAR,
> since a 64-bit BAR consists of a BAR pair.
>
> It is important to know the BAR width that was actually set-up,
> since some drivers, like the Cadence EP controller, does not
> simply look at PCI_BASE_ADDRESS_MEM_TYPE_64, as it configures
> all BARs larger than 2G as 64-bit.
>
> Signed-off-by: Niklas Cassel <[email protected]>
> ---
> drivers/pci/cadence/pcie-cadence-ep.c | 2 +-
> drivers/pci/dwc/pcie-designware-ep.c | 4 ++--
> drivers/pci/endpoint/functions/pci-epf-test.c | 7 ++++++-
> 3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
> index 3c3a97743453..0e4cc4cca56d 100644
> --- a/drivers/pci/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/cadence/pcie-cadence-ep.c
> @@ -135,7 +135,7 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, enum pci_barno bar,
> CDNS_PCIE_LM_EP_FUNC_BAR_CFG_BAR_CTRL(b, ctrl));
> cdns_pcie_writel(pcie, reg, cfg);
>
> - return 0;
> + return is_64bits ? 1 : 0;
> }
>
> static void cdns_pcie_ep_clear_bar(struct pci_epc *epc, u8 fn,
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> index b20b2651caf9..f3c19f8ff8e5 100644
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -139,7 +139,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
> as_type = DW_PCIE_AS_IO;
>
> ret = dw_pcie_ep_inbound_atu(ep, bar, bar_phys, as_type);
> - if (ret)
> + if (ret < 0)
> return ret;
>
> dw_pcie_dbi_ro_wr_en(pci);
> @@ -154,7 +154,7 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
> }
> dw_pcie_dbi_ro_wr_dis(pci);
>
> - return 0;
> + return (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 1 : 0;
> }
>
> static int dw_pcie_find_index(struct dw_pcie_ep *ep, phys_addr_t addr,
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 7c70433b11a7..09878e011284 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -379,12 +379,17 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
> ret = pci_epc_set_bar(epc, epf->func_no, bar,
> epf_bar->phys_addr,
> epf_bar->size, bar_flags[bar]);
> - if (ret) {
> + if (ret < 0) {
> pci_epf_free_space(epf, epf_test->reg[bar], bar);
> dev_err(dev, "failed to set BAR%d\n", bar);
> if (bar == test_reg_bar)
> return ret;
> }
> + /*
> + * pci_epc_set_bar() returns 1 if a 64-bit BAR was set-up,
> + * or 0 if a 32-bit BAR was set-up.
> + */
> + bar += ret;
I'd prefer having a new argument bool *is_64bit in set_bar rather than having
to work with return value of set_bar.
Even otherwise, can't we set PCI_BASE_ADDRESS_MEM_TYPE_64 based on size in
pci-epf-test itself?
Thanks
Kishon
On Wed, Mar 21, 2018 at 04:59:31AM +0100, Niklas Cassel wrote:
> Hello Greg,
>
> Lorenzo is fine with this series
> ( https://marc.info/?l=linux-kernel&m=152147837619191&w=2 )
>
> However, he wants your ack on this patch before merging.
No need for my ack, if you all want to maintain this driver, it's fine
with me, go ahead and merge what you want :)
greg k-h