This patchset is a second one in the series created in the framework of
my Baikal-T1 PCIe/eDMA-related work:
[1: In-progress v3] clk: Baikal-T1 DDR/PCIe resets and some xGMAC fixes
Link: https://lore.kernel.org/linux-pci/[email protected]/
[2: In-progress v2] PCI: dwc: Various fixes and cleanups
Link: https://lore.kernel.org/linux-pci/[email protected]/
[3: In-progress v1] PCI: dwc: Add dma-ranges/YAML-schema/Baikal-T1 support
Link: https://lore.kernel.org/linux-pci/[email protected]/
[4: In-progress v1] dmaengine: dw-edma: Add RP/EP local DMA controllers support
Link: https://lore.kernel.org/linux-pci/[email protected]/
Note it is very recommended to merge the patchsets in the same order as
they are placed in the list above in order to prevent possible merge/build
conflicts. Nothing prevents them from being reviewed synchronously though.
As it can be easily inferred from the patchset title, this series is about
the DW PCIe Root Port/End-point driver fixes and the code cleanups, where
fixes come before the cleanup patches. The patchset starts with adding the
stop_link() platform-specific method invocation in case of the PCIe host
probe procedure errors. It has been missing in the cleanup-on-error path
of the DW PCIe Host initialization method. After that there is a patch
which fixes the host own cfg-space accessors for the case of the
platform-specific DBI implementation. Third the unrolled CSRs layout is
added to the iATU disable procedure. Fourth the disable iATU procedure is
fixed to be called only for the internal ATU as being specific for the
internal ATU implementation. Last but no least the outbound iATU extended
region setup procedure is fixed to have the INCREASE_REGION_SIZE flag set
based on the limit-address - not the region size one.
Afterwards there is a series of cleanups. It concerns the changes like
adding braces to the multi-line if-else constructions, trailing new-lines
to the print format-string, dropping unnecessary version checking, and
various code simplifications and optimizations.
New features like adding two-level DT bindings abstraction, adding better
structured IP-core version interface, adding iATU regions size detection
and the PCIe regions verification procedure, adding dma-ranges support,
introducing a set of generic platform clocks and resets and finally adding
Baikal-T1 PCIe interface support will be submitted in the next part of the
series.
Link: https://lore.kernel.org/linux-pci/[email protected]/
Changelog v2:
- Fix the end address of the example in the patch log with
the INCREASE_REGION_SIZE flag usage fixup. It should be
0x1000FFFF and not 0x0000FFFF (@Manivannan).
- Add the cleanup-on-error path to the dw_pcie_ep_init() function.
(@Manivannan)
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Pavel Parkhomenko <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: "Krzysztof Wilczyński" <[email protected]>
Cc: Frank Li <[email protected]>
Cc: Manivannan Sadhasivam <[email protected]>
Cc: [email protected]
Cc: [email protected]
Serge Semin (13):
PCI: dwc: Stop link in the host init error and de-initialization
PCI: dwc: Don't use generic IO-ops for DBI-space access
PCI: dwc: Add unroll iATU space support to the regions disable method
PCI: dwc: Disable outbound windows for controllers with iATU
PCI: dwc: Set INCREASE_REGION_SIZE flag based on limit address
PCI: dwc: Add braces to the multi-line if-else statements
PCI: dwc: Add trailing new-line literals to the log messages
PCI: dwc: Discard IP-core version checking on unrolled iATU detection
PCI: dwc: Convert Link-up status method to using dw_pcie_readl_dbi()
PCI: dwc: Deallocate EPC memory on EP init error
PCI: dwc-plat: Simplify the probe method return value handling
PCI: dwc-plat: Discard unused regmap pointer
PCI: dwc-plat: Drop dw_plat_pcie_of_match forward declaration
.../pci/controller/dwc/pcie-designware-ep.c | 22 +++++--
.../pci/controller/dwc/pcie-designware-host.c | 66 +++++++++++++++----
.../pci/controller/dwc/pcie-designware-plat.c | 13 ++--
drivers/pci/controller/dwc/pcie-designware.c | 48 +++++++++-----
4 files changed, 109 insertions(+), 40 deletions(-)
--
2.35.1
In accordance with [1] if there is at least one multi-line if-else
clause in the statement, then each clause will need to be surrounded by
the braces. The driver code violates that coding style rule in a few
places. Let's fix it.
[1] Documentation/process/coding-style.rst
Signed-off-by: Serge Semin <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
---
drivers/pci/controller/dwc/pcie-designware-ep.c | 4 ++--
drivers/pci/controller/dwc/pcie-designware.c | 3 ++-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 0eda8236c125..7c9315fffe24 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -699,9 +699,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
if (!pci->dbi_base2) {
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
- if (!res)
+ if (!res) {
pci->dbi_base2 = pci->dbi_base + SZ_4K;
- else {
+ } else {
pci->dbi_base2 = devm_pci_remap_cfg_resource(dev, res);
if (IS_ERR(pci->dbi_base2))
return PTR_ERR(pci->dbi_base2);
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index d737af058903..9f4d2b44612b 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -699,8 +699,9 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
pci->atu_size = SZ_4K;
dw_pcie_iatu_detect_regions_unroll(pci);
- } else
+ } else {
dw_pcie_iatu_detect_regions(pci);
+ }
dev_info(pci->dev, "iATU unroll: %s\n", pci->iatu_unroll_enabled ?
"enabled" : "disabled");
--
2.35.1
While the rest of the generic DWC PCIe code is using the dedicated IO-mem
accessors, the dw_pcie_link_up() method for some unobvious reason directly
calls readl() to get PortLogic.DEBUG1 register content. Since the way the
dbi-bus is accessed can be platform-specific let's replace the direct dbi
memory space read procedure with the readl-wrapper invocation. Thus we'll
have a slightly more generic dw_pcie_link_up() method.
Signed-off-by: Serge Semin <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
---
drivers/pci/controller/dwc/pcie-designware.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index e3d2c11e6998..6e81264fdfb4 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -548,7 +548,7 @@ int dw_pcie_link_up(struct dw_pcie *pci)
if (pci->ops && pci->ops->link_up)
return pci->ops->link_up(pci);
- val = readl(pci->dbi_base + PCIE_PORT_DEBUG1);
+ val = dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1);
return ((val & PCIE_PORT_DEBUG1_LINK_UP) &&
(!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING)));
}
--
2.35.1
It was wrong to use the region size parameter in order to determine
whether the INCREASE_REGION_SIZE flag needs to be set for the outbound
iATU entry because in general there are cases when combining a region base
address and size together produces the out of bounds upper range limit
while upper_32_bits(size) still returns zero. So having a region size
within the permitted values doesn't mean the region limit address will fit
to the corresponding CSR. Here is the way iATU calculates the in- and
outbound untranslated regions if the INCREASE_REGION_SIZE flag is cleared
[1]:
Start address: End address:
63 31 0 63 31 0
+---------------+---------------+ +---------------+---------------+
| | | 0s | | | | Fs |
+---------------+---------------+ +---------------+---------------+
upper base | lower base !upper! base | limit address
address address address
So the region start address is determined by the iATU lower and upper base
address registers, while the region upper boundary is calculated based on
the 32-bits limit address register and the upper part of the base address.
In accordance with that logic for instance the range
0xf0000000 @ 0x20000000 does have the size smaller than 4GB, but the
actual limit address turns to be invalid forming the untranslated address
map as [0xf0000000; 0x1000FFFF], which isn't what the original range was.
In order to fix that we need to check whether the size after being added
to the lower part of the base address causes the 4GB range overflow. If it
does then we need to set the INCREASE_REGION_SIZE flag thus activating the
extended limit address by means of an additional iATU CSR (upper limit
address register) [2]:
Start address: End address:
63 31 0 63 x 31 0
+---------------+---------------+ +---------------+---------------+
| | | 0s | | | | | Fs |
+---------------+---------------+ +---------------+---------------+
upper base | lower base upper | upper | limit address
address address base | limit |
address|address|
Otherwise there is enough room in the 32-bits wide limit address register,
and the flag can be left unset.
Note the case when the size-based flag setting approach is correct implies
requiring to have the size-aligned base addresses only. But that
constraint isn't relevant to the PCIe ranges accepted by the kernel.
There is also no point in implementing it either seeing the problem can be
easily fixed by checking the whole limit address instead of the region
size.
[1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port,
v5.40a, March 2019, fig.3-36, p.175
[2] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port,
v5.40a, March 2019, fig.3-37, p.176
Fixes: 5b4cf0f65324 ("PCI: dwc: Add upper limit address for outbound iATU")
Signed-off-by: Serge Semin <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
---
Changelog v2:
- Fix the end address in the example of the patch log. It should be
0x1000FFFF and not 0x0000FFFF (@Manivannan).
---
drivers/pci/controller/dwc/pcie-designware.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 7dc8c360a0d4..d737af058903 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -287,8 +287,8 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET,
upper_32_bits(pci_addr));
val = type | PCIE_ATU_FUNC_NUM(func_no);
- val = upper_32_bits(size - 1) ?
- val | PCIE_ATU_INCREASE_REGION_SIZE : val;
+ if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr))
+ val |= PCIE_ATU_INCREASE_REGION_SIZE;
if (pci->version == 0x490A)
val = dw_pcie_enable_ecrc(val);
dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val);
@@ -315,6 +315,7 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
u64 pci_addr, u64 size)
{
u32 retries, val;
+ u64 limit_addr;
if (pci->ops && pci->ops->cpu_addr_fixup)
cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
@@ -325,6 +326,8 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
return;
}
+ limit_addr = cpu_addr + size - 1;
+
dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT,
PCIE_ATU_REGION_OUTBOUND | index);
dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_BASE,
@@ -332,17 +335,18 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_BASE,
upper_32_bits(cpu_addr));
dw_pcie_writel_dbi(pci, PCIE_ATU_LIMIT,
- lower_32_bits(cpu_addr + size - 1));
+ lower_32_bits(limit_addr));
if (pci->version >= 0x460A)
dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_LIMIT,
- upper_32_bits(cpu_addr + size - 1));
+ upper_32_bits(limit_addr));
dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_TARGET,
lower_32_bits(pci_addr));
dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_TARGET,
upper_32_bits(pci_addr));
val = type | PCIE_ATU_FUNC_NUM(func_no);
- val = ((upper_32_bits(size - 1)) && (pci->version >= 0x460A)) ?
- val | PCIE_ATU_INCREASE_REGION_SIZE : val;
+ if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
+ pci->version >= 0x460A)
+ val |= PCIE_ATU_INCREASE_REGION_SIZE;
if (pci->version == 0x490A)
val = dw_pcie_enable_ecrc(val);
dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, val);
--
2.35.1
The regmap pointer was added into the dw_plat_pcie structure in
commit 1d906b22076e ("PCI: dwc: Add support for EP mode"), but it hasn't
been utilized neither in the code submitted in the denoted so far nor in
the platform driver evolving afterwards. Drop it then for good.
Signed-off-by: Serge Semin <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
---
drivers/pci/controller/dwc/pcie-designware-plat.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-plat.c b/drivers/pci/controller/dwc/pcie-designware-plat.c
index fea785096261..99cf2ac5b0ba 100644
--- a/drivers/pci/controller/dwc/pcie-designware-plat.c
+++ b/drivers/pci/controller/dwc/pcie-designware-plat.c
@@ -17,13 +17,11 @@
#include <linux/platform_device.h>
#include <linux/resource.h>
#include <linux/types.h>
-#include <linux/regmap.h>
#include "pcie-designware.h"
struct dw_plat_pcie {
struct dw_pcie *pci;
- struct regmap *regmap;
enum dw_pcie_device_mode mode;
};
--
2.35.1
The dw_pcie_disable_atu() method was introduced in the commit f8aed6ec624f
("PCI: dwc: designware: Add EP mode support"). Since then it hasn't
changed at all. For all that time the method has supported the viewport
version of the iATU CSRs only. Basically it works for the DW PCIe IP-cores
older than v4.80a since the newer controllers are equipped with the
unrolled iATU/eDMA space. It means the methods using it like
pci_epc_ops.clear_bar and pci_epc_ops.unmap_addr callbacks just don't work
correctly for the DW PCIe controllers with unrolled iATU CSRs. The same
concerns the dw_pcie_setup_rc() method, which disables the outbound iATU
entries before re-initializing them.
So in order to fix the problems denoted above let's convert the
dw_pcie_disable_atu() method to disabling the iATU inbound and outbound
regions in the unrolled iATU CSRs in case the DW PCIe controller has been
synthesized with the ones support. The former semantics will be remained
for the controller having iATU mapped over the viewport.
Fixes: f8aed6ec624f ("PCI: dwc: designware: Add EP mode support")
Signed-off-by: Serge Semin <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
---
drivers/pci/controller/dwc/pcie-designware.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index d92c8a25094f..7dc8c360a0d4 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -504,8 +504,18 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, int index,
return;
}
- dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
- dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, ~(u32)PCIE_ATU_ENABLE);
+ if (pci->iatu_unroll_enabled) {
+ if (region == PCIE_ATU_REGION_INBOUND) {
+ dw_pcie_writel_ib_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
+ ~(u32)PCIE_ATU_ENABLE);
+ } else {
+ dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
+ ~(u32)PCIE_ATU_ENABLE);
+ }
+ } else {
+ dw_pcie_writel_dbi(pci, PCIE_ATU_VIEWPORT, region | index);
+ dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, ~(u32)PCIE_ATU_ENABLE);
+ }
}
int dw_pcie_wait_for_link(struct dw_pcie *pci)
--
2.35.1
The whole switch-case-logic implemented in the DWC PCIe RC/EP probe
procedure doesn't seem well thought through. First of all the ret variable
is unused in the EP-case and is only partly involved in the RC-case of the
switch-case statement, which unnecessary complicates the code. Secondly
the probe method will return zero if an unknown mode is detected. That is
improbable situation since the OF-device data is initialized only with
valid modes, but such code is still wrong at least from maintainability
point of view. So let's convert the switch-case part of the probe function
to being more coherent. We suggest to use the local ret variable to
preserve the status of the case-clauses and return its value from the
probe procedure after the work is done.
Signed-off-by: Serge Semin <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
---
drivers/pci/controller/dwc/pcie-designware-plat.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-plat.c b/drivers/pci/controller/dwc/pcie-designware-plat.c
index 0c5de87d3cc6..fea785096261 100644
--- a/drivers/pci/controller/dwc/pcie-designware-plat.c
+++ b/drivers/pci/controller/dwc/pcie-designware-plat.c
@@ -153,20 +153,21 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)
return -ENODEV;
ret = dw_plat_add_pcie_port(dw_plat_pcie, pdev);
- if (ret < 0)
- return ret;
break;
case DW_PCIE_EP_TYPE:
if (!IS_ENABLED(CONFIG_PCIE_DW_PLAT_EP))
return -ENODEV;
pci->ep.ops = &pcie_ep_ops;
- return dw_pcie_ep_init(&pci->ep);
+ ret = dw_pcie_ep_init(&pci->ep);
+ break;
default:
dev_err(dev, "INVALID device type %d\n", dw_plat_pcie->mode);
+ ret = -EINVAL;
+ break;
}
- return 0;
+ return ret;
}
static const struct dw_plat_pcie_of_data dw_plat_pcie_rc_of_data = {
--
2.35.1
The denoted forward declaration used to be required to get the OF-device
ID structure by calling the of_match_device() method. The later method
invocation has been replaced with the of_device_get_match_data() call in
the commit 5c204204cf24 ("PCI: designware-plat: Prefer
of_device_get_match_data()"). Thus the forward declaration of the
OF-compatible device strings no longer needed. Drop it for good.
Signed-off-by: Serge Semin <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
---
drivers/pci/controller/dwc/pcie-designware-plat.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-plat.c b/drivers/pci/controller/dwc/pcie-designware-plat.c
index 99cf2ac5b0ba..e606c5d5f06f 100644
--- a/drivers/pci/controller/dwc/pcie-designware-plat.c
+++ b/drivers/pci/controller/dwc/pcie-designware-plat.c
@@ -29,8 +29,6 @@ struct dw_plat_pcie_of_data {
enum dw_pcie_device_mode mode;
};
-static const struct of_device_id dw_plat_pcie_of_match[];
-
static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = {
};
--
2.35.1
It's logically correct to undo everything what was done in case of an
error is discovered or in the corresponding cleanup counterpart. Otherwise
the host controller will be left in an undetermined state. Seeing the link
is set up in the Host-initialization method it will be right to
de-activate it there in the cleanup-on-error block and stop the link in
the antagonistic routine - dw_pcie_host_deinit(). The link de-activation
is a platform-specific thing and is supposed to be implemented in the
framework of the dw_pcie_ops.stop_link() operation.
Fixes: 886a9c134755 ("PCI: dwc: Move link handling into common code")
Signed-off-by: Serge Semin <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
---
.../pci/controller/dwc/pcie-designware-host.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 2fa86f32d964..7403b1709726 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -420,8 +420,14 @@ int dw_pcie_host_init(struct pcie_port *pp)
bridge->sysdata = pp;
ret = pci_host_probe(bridge);
- if (!ret)
- return 0;
+ if (ret)
+ goto err_stop_link;
+
+ return 0;
+
+err_stop_link:
+ if (pci->ops && pci->ops->stop_link)
+ pci->ops->stop_link(pci);
err_free_msi:
if (pp->has_msi_ctrl)
@@ -432,8 +438,14 @@ EXPORT_SYMBOL_GPL(dw_pcie_host_init);
void dw_pcie_host_deinit(struct pcie_port *pp)
{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
pci_stop_root_bus(pp->bridge->bus);
pci_remove_root_bus(pp->bridge->bus);
+
+ if (pci->ops && pci->ops->stop_link)
+ pci->ops->stop_link(pci);
+
if (pp->has_msi_ctrl)
dw_pcie_free_msi(pp);
}
--
2.35.1
It's pretty much pointless. Even though unrolled version of the internal
ATU has been indeed available since DWC PCIe v4.80a IP-core, there is no
guarantee it was enabled during the IP-core configuration (Synopsys
suggests to contact the Solvnet support for guidance of how to do that for
the newer IP-cores). So the only reliable way to find out the unrolled
iATU feature availability is indeed to check the iATU viewport register
content. In accordance with the reference manual [1] if the register
doesn't exist (unrolled iATU is enabled) it's content is fixed with
0xff-s, otherwise it will contain some zeros. So we can freely drop the
IP-core version checking in this matter then and use the
dw_pcie_iatu_unroll_enabled() method only to detect whether iATU/eDMA
space is unrolled.
[1] DesignWare Cores, PCI Express Controller, Register Desciptions,
v.4.90a, December 2016, p.855
Signed-off-by: Serge Semin <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
---
drivers/pci/controller/dwc/pcie-designware.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 3bd1cfd12148..e3d2c11e6998 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -600,15 +600,15 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
}
-static u8 dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
+static bool dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
{
u32 val;
val = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT);
if (val == 0xffffffff)
- return 1;
+ return true;
- return 0;
+ return false;
}
static void dw_pcie_iatu_detect_regions_unroll(struct dw_pcie *pci)
@@ -680,9 +680,8 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
struct device *dev = pci->dev;
struct platform_device *pdev = to_platform_device(dev);
- if (pci->version >= 0x480A || (!pci->version &&
- dw_pcie_iatu_unroll_enabled(pci))) {
- pci->iatu_unroll_enabled = true;
+ pci->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pci);
+ if (pci->iatu_unroll_enabled) {
if (!pci->atu_base) {
struct resource *res =
platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
--
2.35.1
On Fri, May 13, 2022 at 09:49:58AM +0100, Lorenzo Pieralisi wrote:
> On Fri, May 13, 2022 at 02:20:33AM +0300, Serge Semin wrote:
> > On Thu, May 12, 2022 at 10:41:45PM +0100, Lorenzo Pieralisi wrote:
> > > On Wed, May 04, 2022 at 12:22:47AM +0300, Serge Semin wrote:
> > > > This patchset is a second one in the series created in the framework of
> > > > my Baikal-T1 PCIe/eDMA-related work:
> > > >
> > > > [1: In-progress v3] clk: Baikal-T1 DDR/PCIe resets and some xGMAC fixes
> > > > Link: https://lore.kernel.org/linux-pci/[email protected]/
> > > > [2: In-progress v2] PCI: dwc: Various fixes and cleanups
> > > > Link: https://lore.kernel.org/linux-pci/[email protected]/
> > > > [3: In-progress v1] PCI: dwc: Add dma-ranges/YAML-schema/Baikal-T1 support
> > > > Link: https://lore.kernel.org/linux-pci/[email protected]/
> > > > [4: In-progress v1] dmaengine: dw-edma: Add RP/EP local DMA controllers support
> > > > Link: https://lore.kernel.org/linux-pci/[email protected]/
> > > >
> > > > Note it is very recommended to merge the patchsets in the same order as
> > > > they are placed in the list above in order to prevent possible merge/build
> > > > conflicts. Nothing prevents them from being reviewed synchronously though.
> > > >
> > > > As it can be easily inferred from the patchset title, this series is about
> > > > the DW PCIe Root Port/End-point driver fixes and the code cleanups, where
> > > > fixes come before the cleanup patches. The patchset starts with adding the
> > > > stop_link() platform-specific method invocation in case of the PCIe host
> > > > probe procedure errors. It has been missing in the cleanup-on-error path
> > > > of the DW PCIe Host initialization method. After that there is a patch
> > > > which fixes the host own cfg-space accessors for the case of the
> > > > platform-specific DBI implementation. Third the unrolled CSRs layout is
> > > > added to the iATU disable procedure. Fourth the disable iATU procedure is
> > > > fixed to be called only for the internal ATU as being specific for the
> > > > internal ATU implementation. Last but no least the outbound iATU extended
> > > > region setup procedure is fixed to have the INCREASE_REGION_SIZE flag set
> > > > based on the limit-address - not the region size one.
> > > >
> > > > Afterwards there is a series of cleanups. It concerns the changes like
> > > > adding braces to the multi-line if-else constructions, trailing new-lines
> > > > to the print format-string, dropping unnecessary version checking, and
> > > > various code simplifications and optimizations.
> > >
> > > Hi,
> > >
> > > I went through the series and I don't have any specific objections.
> > >
> >
> > > We can try to queue it for v5.19, with the caveat that the fixes
> > > _need_ testing on several DWC platforms
> >
> > Alas I can't deliver that. But Manivannan has done a testing on his
> > Qualcomm devices.
>
> Fixes need testing. I don't want to merge fixes that break platforms
> on which you can't test.
Every submitted change needs testing, but in most of the cases a
full comprehensive evaluation just can't be delivered. It doesn't mean
the changes can't be accepted especially if they are logically correct,
well justified, tested on several platforms and already reviewed. All
of that has been already done for this and following up patchsets.
>
> Also, fixes need bug reports and as far as I can see I have not
> seen any reported that point at current mainline failures.
I reported that there are issues in the current code implementation.
They are thoroughly described in the patch logs and fixed by the
corresponding patches. But anyway I couldn't find in any kernel doc
that having a bug report is required to post fixes patch. It's like
waiting for kernel to crash/hangup/etc before getting permission to fix
its bugs.
Note, having Fixes tag doesn't mean the patch always need to be merged
in into the stable trees. It just indicates the commit originating the
issue, helps to review the change and if necessary assists the stable
kernel team in determining which stable kernel versions should receive
your fix (Documentation/process/submitting-patches.rst:555).
Also note I haven't Cc'ed the stable team reviewers so basically the
patches aren't strictly required to be merged into the stable kernels.
>
> > > (and I _strongly_ encourage
> > > DWC maintainers to chime in). To sum it up:
> > >
> >
> > > - It is a mixture of clean-ups and fixes. I would prefer having the
> > > cleanups earlier in the series and rebase (if there is a need, I
> > > can try to reshuffle the patches myself) the fixes on top. That
> > > because we may have to drop some of the fixes (and if we merge them
> > > we may have to revert them as cleanly as we can),
> >
> > Normally fixes patches go before ahead of the rest of the series to
> > simplify the backporting procedure (makes Greg's life much easier).
>
> See above. We should not have fixes and cleanups in the
> same series
What kernel document describes that requirement?
> and I am not taking fixes that can affect other
> platforms unless they are tested.
>
> > Revertability has much less priority. In the worst case an additional
> > fixes patch is more preferable especially if the original patch has
> > been reviewed, accepted and most likely backported. Only if the patch
> > author didn't provide a fix then the reversion is proceeded. So I
> > wouldn't recommend to reshuffle the patches.
>
> Feel free to ignore what I say, my comments still stand.
>
> > > my concern is that
> > > they require testing on a number of platforms you have not been
> > > exposed to
> >
> > This series and another patchsets have been available for testing for
> > about two months now. There have been more than enough time to give it
> > a try and review.
>
> Sure. To merge fixes they need to be tested on platforms before
> we can accept them and we need bug reports to show they are fixing
> something in the first place.
see my comment above regarding the requirement you are stating. There
must be some confusion regarding stable kernel patches, having bug
reports and full cross-platform testing. If you suggest to wait while
the series are tested on all the supported platforms we most likely will
never get then merged in.
>
> > > - Kbot complained about patch (3)
> >
> > Could you be more specific what it was complaining about? I haven't
> > got any relevant message in none of my emails.
>
> https://lore.kernel.org/linux-pci/[email protected]
Thanks. For some reason it was missing in both of my inboxes. Most
likely it was dropped by the corporate spam bot. But I am wondering
how come Kbot just removed my gmail address from Cc...
Anyway the report is reasonable. I'll fix it in v3.
>
> > > - I will have comments about the commit logs but I can try to fix them
> > > myself
> >
> > Feel free to submit your comments. I'll take them into account in v3.
> >
> > >
> > > I have concerns especially about patches (2, 3, 4, 5, 8, 9), because
> > > they can affect DWC platforms other than the ones you are testing on.
> >
> > Basically any generic code change affects the dependent platforms. If
> > you don't see any exact issue in mind then I don't see any reason to
> > sustain the series any longer especially seeing it has been already
> > tested on an alternative platform.
>
> See above.
>
> > > The cleanups we can definitely queue them up.
> >
> > > As I said, and there
> > > is nothing I can do about it, I will be off the radar for two months
> > > from wednesday,
> >
> > As I noted this isn't good since I haven't noticed much activity for
> > the last two months. Alas Bjorn hasn't participated in the review
> > process either. I can offer a help with reviewing the patches
> > concerning the DW PCIe drivers since during the patchsets development
> > I've got a good notion regarding the DWC drivers and HW internals. But
> > it only concerns the DW PCIe Host/End-point code.
>
> We(I) don't have DWC HW
So basically you are maintaining the part for which you don't have
neither hw reference manual nor hw instance, right? This isn't easy
task I'd say. I offered my help twice, but you tend to ignore it. Note
I don't expect that accepting the help means immediately merging my
patches in. They still need to go through the review process.
> and DWC maintainers are supposed to review DWC
> code. We maintain the generic bits in host controller drivers to
> make sure they guarantee a uniform behaviour across hosts.
I would have agreed with you if we didn't have the series tested on
other than mine hardware and haven't had any review comment posted so
far. But the patchset has been available for review for quiet a while.
During that time it was tested on another platform and even got RB
tags from a person having HW instances and reference manuals.
>
> That's what I have to say. If you want your code merged we need
> to find a way to get DWC maintainers to test it and provide
> review,
Isn't Manivanna review and tests enough? Isn't me having a bunch of
DWC PCIe hw manuals and testing on at least one DWC PCIe IP-core
instance enough?
If not what do you suggest then? There is no dedicated DWC maintainer
for the generic DWC PCIe bits, the patchset has been hanging out on
review for about two months, and you are going to be away for the next
two months. Waiting for two more months until someone gets to appear
and finds a time to test it out? Really, four months for just 13
patches seems too much.
What I can suggest in this matter is to resend the series today with
all the depended platforms mailing lists in Cc (shall I explicitly add
the maintainers too?). If no serious problem reported until Wednesday
and if I fixed all the reported notes/comments until then, you get to
merge the patchset(s) in. What do think about this?
> I agree with you that feedback is lacking and that's
> something that has to be solved.
As I see it this is the main reason of all the arguing above. I think
that there was enough feedback for all the patchsets during the last
two months. You say there wasn't.
-Sergey
>
> Thanks for your patience.
>
> Lorenzo
>
> > > please try to repost with the Kbot issue fixed and
> >
> > See my comment above regarding kbot.
> >
> > -Sergey
> >
> > > with the comments above in mind and I will do my best to queue as
> > > many patches from this series as possible for v5.19.
> > >
> > > Thanks,
> > > Lorenzo
> > >
> > > > New features like adding two-level DT bindings abstraction, adding better
> > > > structured IP-core version interface, adding iATU regions size detection
> > > > and the PCIe regions verification procedure, adding dma-ranges support,
> > > > introducing a set of generic platform clocks and resets and finally adding
> > > > Baikal-T1 PCIe interface support will be submitted in the next part of the
> > > > series.
> > > >
> > > > Link: https://lore.kernel.org/linux-pci/[email protected]/
> > > > Changelog v2:
> > > > - Fix the end address of the example in the patch log with
> > > > the INCREASE_REGION_SIZE flag usage fixup. It should be
> > > > 0x1000FFFF and not 0x0000FFFF (@Manivannan).
> > > > - Add the cleanup-on-error path to the dw_pcie_ep_init() function.
> > > > (@Manivannan)
> > > >
> > > > Signed-off-by: Serge Semin <[email protected]>
> > > > Cc: Alexey Malahov <[email protected]>
> > > > Cc: Pavel Parkhomenko <[email protected]>
> > > > Cc: Rob Herring <[email protected]>
> > > > Cc: "Krzysztof Wilczyński" <[email protected]>
> > > > Cc: Frank Li <[email protected]>
> > > > Cc: Manivannan Sadhasivam <[email protected]>
> > > > Cc: [email protected]
> > > > Cc: [email protected]
> > > >
> > > > Serge Semin (13):
> > > > PCI: dwc: Stop link in the host init error and de-initialization
> > > > PCI: dwc: Don't use generic IO-ops for DBI-space access
> > > > PCI: dwc: Add unroll iATU space support to the regions disable method
> > > > PCI: dwc: Disable outbound windows for controllers with iATU
> > > > PCI: dwc: Set INCREASE_REGION_SIZE flag based on limit address
> > > > PCI: dwc: Add braces to the multi-line if-else statements
> > > > PCI: dwc: Add trailing new-line literals to the log messages
> > > > PCI: dwc: Discard IP-core version checking on unrolled iATU detection
> > > > PCI: dwc: Convert Link-up status method to using dw_pcie_readl_dbi()
> > > > PCI: dwc: Deallocate EPC memory on EP init error
> > > > PCI: dwc-plat: Simplify the probe method return value handling
> > > > PCI: dwc-plat: Discard unused regmap pointer
> > > > PCI: dwc-plat: Drop dw_plat_pcie_of_match forward declaration
> > > >
> > > > .../pci/controller/dwc/pcie-designware-ep.c | 22 +++++--
> > > > .../pci/controller/dwc/pcie-designware-host.c | 66 +++++++++++++++----
> > > > .../pci/controller/dwc/pcie-designware-plat.c | 13 ++--
> > > > drivers/pci/controller/dwc/pcie-designware.c | 48 +++++++++-----
> > > > 4 files changed, 109 insertions(+), 40 deletions(-)
> > > >
> > > > --
> > > > 2.35.1
> > > >
On Wed, May 04, 2022 at 12:22:47AM +0300, Serge Semin wrote:
> This patchset is a second one in the series created in the framework of
> my Baikal-T1 PCIe/eDMA-related work:
>
> [1: In-progress v3] clk: Baikal-T1 DDR/PCIe resets and some xGMAC fixes
> Link: https://lore.kernel.org/linux-pci/[email protected]/
> [2: In-progress v2] PCI: dwc: Various fixes and cleanups
> Link: https://lore.kernel.org/linux-pci/[email protected]/
> [3: In-progress v1] PCI: dwc: Add dma-ranges/YAML-schema/Baikal-T1 support
> Link: https://lore.kernel.org/linux-pci/[email protected]/
> [4: In-progress v1] dmaengine: dw-edma: Add RP/EP local DMA controllers support
> Link: https://lore.kernel.org/linux-pci/[email protected]/
>
> Note it is very recommended to merge the patchsets in the same order as
> they are placed in the list above in order to prevent possible merge/build
> conflicts. Nothing prevents them from being reviewed synchronously though.
>
> As it can be easily inferred from the patchset title, this series is about
> the DW PCIe Root Port/End-point driver fixes and the code cleanups, where
> fixes come before the cleanup patches. The patchset starts with adding the
> stop_link() platform-specific method invocation in case of the PCIe host
> probe procedure errors. It has been missing in the cleanup-on-error path
> of the DW PCIe Host initialization method. After that there is a patch
> which fixes the host own cfg-space accessors for the case of the
> platform-specific DBI implementation. Third the unrolled CSRs layout is
> added to the iATU disable procedure. Fourth the disable iATU procedure is
> fixed to be called only for the internal ATU as being specific for the
> internal ATU implementation. Last but no least the outbound iATU extended
> region setup procedure is fixed to have the INCREASE_REGION_SIZE flag set
> based on the limit-address - not the region size one.
>
> Afterwards there is a series of cleanups. It concerns the changes like
> adding braces to the multi-line if-else constructions, trailing new-lines
> to the print format-string, dropping unnecessary version checking, and
> various code simplifications and optimizations.
Hi,
I went through the series and I don't have any specific objections.
We can try to queue it for v5.19, with the caveat that the fixes
_need_ testing on several DWC platforms (and I _strongly_ encourage
DWC maintainers to chime in). To sum it up:
- It is a mixture of clean-ups and fixes. I would prefer having the
cleanups earlier in the series and rebase (if there is a need, I
can try to reshuffle the patches myself) the fixes on top. That
because we may have to drop some of the fixes (and if we merge them
we may have to revert them as cleanly as we can), my concern is that
they require testing on a number of platforms you have not been
exposed to
- Kbot complained about patch (3)
- I will have comments about the commit logs but I can try to fix them
myself
I have concerns especially about patches (2, 3, 4, 5, 8, 9), because
they can affect DWC platforms other than the ones you are testing on.
The cleanups we can definitely queue them up. As I said, and there
is nothing I can do about it, I will be off the radar for two months
from wednesday, please try to repost with the Kbot issue fixed and
with the comments above in mind and I will do my best to queue as
many patches from this series as possible for v5.19.
Thanks,
Lorenzo
> New features like adding two-level DT bindings abstraction, adding better
> structured IP-core version interface, adding iATU regions size detection
> and the PCIe regions verification procedure, adding dma-ranges support,
> introducing a set of generic platform clocks and resets and finally adding
> Baikal-T1 PCIe interface support will be submitted in the next part of the
> series.
>
> Link: https://lore.kernel.org/linux-pci/[email protected]/
> Changelog v2:
> - Fix the end address of the example in the patch log with
> the INCREASE_REGION_SIZE flag usage fixup. It should be
> 0x1000FFFF and not 0x0000FFFF (@Manivannan).
> - Add the cleanup-on-error path to the dw_pcie_ep_init() function.
> (@Manivannan)
>
> Signed-off-by: Serge Semin <[email protected]>
> Cc: Alexey Malahov <[email protected]>
> Cc: Pavel Parkhomenko <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: "Krzysztof Wilczyński" <[email protected]>
> Cc: Frank Li <[email protected]>
> Cc: Manivannan Sadhasivam <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
>
> Serge Semin (13):
> PCI: dwc: Stop link in the host init error and de-initialization
> PCI: dwc: Don't use generic IO-ops for DBI-space access
> PCI: dwc: Add unroll iATU space support to the regions disable method
> PCI: dwc: Disable outbound windows for controllers with iATU
> PCI: dwc: Set INCREASE_REGION_SIZE flag based on limit address
> PCI: dwc: Add braces to the multi-line if-else statements
> PCI: dwc: Add trailing new-line literals to the log messages
> PCI: dwc: Discard IP-core version checking on unrolled iATU detection
> PCI: dwc: Convert Link-up status method to using dw_pcie_readl_dbi()
> PCI: dwc: Deallocate EPC memory on EP init error
> PCI: dwc-plat: Simplify the probe method return value handling
> PCI: dwc-plat: Discard unused regmap pointer
> PCI: dwc-plat: Drop dw_plat_pcie_of_match forward declaration
>
> .../pci/controller/dwc/pcie-designware-ep.c | 22 +++++--
> .../pci/controller/dwc/pcie-designware-host.c | 66 +++++++++++++++----
> .../pci/controller/dwc/pcie-designware-plat.c | 13 ++--
> drivers/pci/controller/dwc/pcie-designware.c | 48 +++++++++-----
> 4 files changed, 109 insertions(+), 40 deletions(-)
>
> --
> 2.35.1
>
On Thu, May 12, 2022 at 10:41:45PM +0100, Lorenzo Pieralisi wrote:
> On Wed, May 04, 2022 at 12:22:47AM +0300, Serge Semin wrote:
> > This patchset is a second one in the series created in the framework of
> > my Baikal-T1 PCIe/eDMA-related work:
> >
> > [1: In-progress v3] clk: Baikal-T1 DDR/PCIe resets and some xGMAC fixes
> > Link: https://lore.kernel.org/linux-pci/[email protected]/
> > [2: In-progress v2] PCI: dwc: Various fixes and cleanups
> > Link: https://lore.kernel.org/linux-pci/[email protected]/
> > [3: In-progress v1] PCI: dwc: Add dma-ranges/YAML-schema/Baikal-T1 support
> > Link: https://lore.kernel.org/linux-pci/[email protected]/
> > [4: In-progress v1] dmaengine: dw-edma: Add RP/EP local DMA controllers support
> > Link: https://lore.kernel.org/linux-pci/[email protected]/
> >
> > Note it is very recommended to merge the patchsets in the same order as
> > they are placed in the list above in order to prevent possible merge/build
> > conflicts. Nothing prevents them from being reviewed synchronously though.
> >
> > As it can be easily inferred from the patchset title, this series is about
> > the DW PCIe Root Port/End-point driver fixes and the code cleanups, where
> > fixes come before the cleanup patches. The patchset starts with adding the
> > stop_link() platform-specific method invocation in case of the PCIe host
> > probe procedure errors. It has been missing in the cleanup-on-error path
> > of the DW PCIe Host initialization method. After that there is a patch
> > which fixes the host own cfg-space accessors for the case of the
> > platform-specific DBI implementation. Third the unrolled CSRs layout is
> > added to the iATU disable procedure. Fourth the disable iATU procedure is
> > fixed to be called only for the internal ATU as being specific for the
> > internal ATU implementation. Last but no least the outbound iATU extended
> > region setup procedure is fixed to have the INCREASE_REGION_SIZE flag set
> > based on the limit-address - not the region size one.
> >
> > Afterwards there is a series of cleanups. It concerns the changes like
> > adding braces to the multi-line if-else constructions, trailing new-lines
> > to the print format-string, dropping unnecessary version checking, and
> > various code simplifications and optimizations.
>
> Hi,
>
> I went through the series and I don't have any specific objections.
>
> We can try to queue it for v5.19, with the caveat that the fixes
> _need_ testing on several DWC platforms
Alas I can't deliver that. But Manivannan has done a testing on his
Qualcomm devices.
> (and I _strongly_ encourage
> DWC maintainers to chime in). To sum it up:
>
> - It is a mixture of clean-ups and fixes. I would prefer having the
> cleanups earlier in the series and rebase (if there is a need, I
> can try to reshuffle the patches myself) the fixes on top. That
> because we may have to drop some of the fixes (and if we merge them
> we may have to revert them as cleanly as we can),
Normally fixes patches go before ahead of the rest of the series to
simplify the backporting procedure (makes Greg's life much easier).
Revertability has much less priority. In the worst case an additional
fixes patch is more preferable especially if the original patch has
been reviewed, accepted and most likely backported. Only if the patch
author didn't provide a fix then the reversion is proceeded. So I
wouldn't recommend to reshuffle the patches.
> my concern is that
> they require testing on a number of platforms you have not been
> exposed to
This series and another patchsets have been available for testing for
about two months now. There have been more than enough time to give it
a try and review.
> - Kbot complained about patch (3)
Could you be more specific what it was complaining about? I haven't
got any relevant message in none of my emails.
> - I will have comments about the commit logs but I can try to fix them
> myself
Feel free to submit your comments. I'll take them into account in v3.
>
> I have concerns especially about patches (2, 3, 4, 5, 8, 9), because
> they can affect DWC platforms other than the ones you are testing on.
Basically any generic code change affects the dependent platforms. If
you don't see any exact issue in mind then I don't see any reason to
sustain the series any longer especially seeing it has been already
tested on an alternative platform.
>
> The cleanups we can definitely queue them up.
> As I said, and there
> is nothing I can do about it, I will be off the radar for two months
> from wednesday,
As I noted this isn't good since I haven't noticed much activity for
the last two months. Alas Bjorn hasn't participated in the review
process either. I can offer a help with reviewing the patches
concerning the DW PCIe drivers since during the patchsets development
I've got a good notion regarding the DWC drivers and HW internals. But
it only concerns the DW PCIe Host/End-point code.
> please try to repost with the Kbot issue fixed and
See my comment above regarding kbot.
-Sergey
> with the comments above in mind and I will do my best to queue as
> many patches from this series as possible for v5.19.
>
> Thanks,
> Lorenzo
>
> > New features like adding two-level DT bindings abstraction, adding better
> > structured IP-core version interface, adding iATU regions size detection
> > and the PCIe regions verification procedure, adding dma-ranges support,
> > introducing a set of generic platform clocks and resets and finally adding
> > Baikal-T1 PCIe interface support will be submitted in the next part of the
> > series.
> >
> > Link: https://lore.kernel.org/linux-pci/[email protected]/
> > Changelog v2:
> > - Fix the end address of the example in the patch log with
> > the INCREASE_REGION_SIZE flag usage fixup. It should be
> > 0x1000FFFF and not 0x0000FFFF (@Manivannan).
> > - Add the cleanup-on-error path to the dw_pcie_ep_init() function.
> > (@Manivannan)
> >
> > Signed-off-by: Serge Semin <[email protected]>
> > Cc: Alexey Malahov <[email protected]>
> > Cc: Pavel Parkhomenko <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: "Krzysztof Wilczyński" <[email protected]>
> > Cc: Frank Li <[email protected]>
> > Cc: Manivannan Sadhasivam <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> >
> > Serge Semin (13):
> > PCI: dwc: Stop link in the host init error and de-initialization
> > PCI: dwc: Don't use generic IO-ops for DBI-space access
> > PCI: dwc: Add unroll iATU space support to the regions disable method
> > PCI: dwc: Disable outbound windows for controllers with iATU
> > PCI: dwc: Set INCREASE_REGION_SIZE flag based on limit address
> > PCI: dwc: Add braces to the multi-line if-else statements
> > PCI: dwc: Add trailing new-line literals to the log messages
> > PCI: dwc: Discard IP-core version checking on unrolled iATU detection
> > PCI: dwc: Convert Link-up status method to using dw_pcie_readl_dbi()
> > PCI: dwc: Deallocate EPC memory on EP init error
> > PCI: dwc-plat: Simplify the probe method return value handling
> > PCI: dwc-plat: Discard unused regmap pointer
> > PCI: dwc-plat: Drop dw_plat_pcie_of_match forward declaration
> >
> > .../pci/controller/dwc/pcie-designware-ep.c | 22 +++++--
> > .../pci/controller/dwc/pcie-designware-host.c | 66 +++++++++++++++----
> > .../pci/controller/dwc/pcie-designware-plat.c | 13 ++--
> > drivers/pci/controller/dwc/pcie-designware.c | 48 +++++++++-----
> > 4 files changed, 109 insertions(+), 40 deletions(-)
> >
> > --
> > 2.35.1
> >
On Fri, May 13, 2022 at 02:20:33AM +0300, Serge Semin wrote:
> On Thu, May 12, 2022 at 10:41:45PM +0100, Lorenzo Pieralisi wrote:
> > On Wed, May 04, 2022 at 12:22:47AM +0300, Serge Semin wrote:
> > > This patchset is a second one in the series created in the framework of
> > > my Baikal-T1 PCIe/eDMA-related work:
> > >
> > > [1: In-progress v3] clk: Baikal-T1 DDR/PCIe resets and some xGMAC fixes
> > > Link: https://lore.kernel.org/linux-pci/[email protected]/
> > > [2: In-progress v2] PCI: dwc: Various fixes and cleanups
> > > Link: https://lore.kernel.org/linux-pci/[email protected]/
> > > [3: In-progress v1] PCI: dwc: Add dma-ranges/YAML-schema/Baikal-T1 support
> > > Link: https://lore.kernel.org/linux-pci/[email protected]/
> > > [4: In-progress v1] dmaengine: dw-edma: Add RP/EP local DMA controllers support
> > > Link: https://lore.kernel.org/linux-pci/[email protected]/
> > >
> > > Note it is very recommended to merge the patchsets in the same order as
> > > they are placed in the list above in order to prevent possible merge/build
> > > conflicts. Nothing prevents them from being reviewed synchronously though.
> > >
> > > As it can be easily inferred from the patchset title, this series is about
> > > the DW PCIe Root Port/End-point driver fixes and the code cleanups, where
> > > fixes come before the cleanup patches. The patchset starts with adding the
> > > stop_link() platform-specific method invocation in case of the PCIe host
> > > probe procedure errors. It has been missing in the cleanup-on-error path
> > > of the DW PCIe Host initialization method. After that there is a patch
> > > which fixes the host own cfg-space accessors for the case of the
> > > platform-specific DBI implementation. Third the unrolled CSRs layout is
> > > added to the iATU disable procedure. Fourth the disable iATU procedure is
> > > fixed to be called only for the internal ATU as being specific for the
> > > internal ATU implementation. Last but no least the outbound iATU extended
> > > region setup procedure is fixed to have the INCREASE_REGION_SIZE flag set
> > > based on the limit-address - not the region size one.
> > >
> > > Afterwards there is a series of cleanups. It concerns the changes like
> > > adding braces to the multi-line if-else constructions, trailing new-lines
> > > to the print format-string, dropping unnecessary version checking, and
> > > various code simplifications and optimizations.
> >
> > Hi,
> >
> > I went through the series and I don't have any specific objections.
> >
>
> > We can try to queue it for v5.19, with the caveat that the fixes
> > _need_ testing on several DWC platforms
>
> Alas I can't deliver that. But Manivannan has done a testing on his
> Qualcomm devices.
Fixes need testing. I don't want to merge fixes that break platforms
on which you can't test.
Also, fixes need bug reports and as far as I can see I have not
seen any reported that point at current mainline failures.
> > (and I _strongly_ encourage
> > DWC maintainers to chime in). To sum it up:
> >
>
> > - It is a mixture of clean-ups and fixes. I would prefer having the
> > cleanups earlier in the series and rebase (if there is a need, I
> > can try to reshuffle the patches myself) the fixes on top. That
> > because we may have to drop some of the fixes (and if we merge them
> > we may have to revert them as cleanly as we can),
>
> Normally fixes patches go before ahead of the rest of the series to
> simplify the backporting procedure (makes Greg's life much easier).
See above. We should not have fixes and cleanups in the
same series and I am not taking fixes that can affect other
platforms unless they are tested.
> Revertability has much less priority. In the worst case an additional
> fixes patch is more preferable especially if the original patch has
> been reviewed, accepted and most likely backported. Only if the patch
> author didn't provide a fix then the reversion is proceeded. So I
> wouldn't recommend to reshuffle the patches.
Feel free to ignore what I say, my comments still stand.
> > my concern is that
> > they require testing on a number of platforms you have not been
> > exposed to
>
> This series and another patchsets have been available for testing for
> about two months now. There have been more than enough time to give it
> a try and review.
Sure. To merge fixes they need to be tested on platforms before
we can accept them and we need bug reports to show they are fixing
something in the first place.
> > - Kbot complained about patch (3)
>
> Could you be more specific what it was complaining about? I haven't
> got any relevant message in none of my emails.
https://lore.kernel.org/linux-pci/[email protected]
> > - I will have comments about the commit logs but I can try to fix them
> > myself
>
> Feel free to submit your comments. I'll take them into account in v3.
>
> >
> > I have concerns especially about patches (2, 3, 4, 5, 8, 9), because
> > they can affect DWC platforms other than the ones you are testing on.
>
> Basically any generic code change affects the dependent platforms. If
> you don't see any exact issue in mind then I don't see any reason to
> sustain the series any longer especially seeing it has been already
> tested on an alternative platform.
See above.
> > The cleanups we can definitely queue them up.
>
> > As I said, and there
> > is nothing I can do about it, I will be off the radar for two months
> > from wednesday,
>
> As I noted this isn't good since I haven't noticed much activity for
> the last two months. Alas Bjorn hasn't participated in the review
> process either. I can offer a help with reviewing the patches
> concerning the DW PCIe drivers since during the patchsets development
> I've got a good notion regarding the DWC drivers and HW internals. But
> it only concerns the DW PCIe Host/End-point code.
We(I) don't have DWC HW and DWC maintainers are supposed to review DWC
code. We maintain the generic bits in host controller drivers to
make sure they guarantee a uniform behaviour across hosts.
That's what I have to say. If you want your code merged we need
to find a way to get DWC maintainers to test it and provide
review, I agree with you that feedback is lacking and that's
something that has to be solved.
Thanks for your patience.
Lorenzo
> > please try to repost with the Kbot issue fixed and
>
> See my comment above regarding kbot.
>
> -Sergey
>
> > with the comments above in mind and I will do my best to queue as
> > many patches from this series as possible for v5.19.
> >
> > Thanks,
> > Lorenzo
> >
> > > New features like adding two-level DT bindings abstraction, adding better
> > > structured IP-core version interface, adding iATU regions size detection
> > > and the PCIe regions verification procedure, adding dma-ranges support,
> > > introducing a set of generic platform clocks and resets and finally adding
> > > Baikal-T1 PCIe interface support will be submitted in the next part of the
> > > series.
> > >
> > > Link: https://lore.kernel.org/linux-pci/[email protected]/
> > > Changelog v2:
> > > - Fix the end address of the example in the patch log with
> > > the INCREASE_REGION_SIZE flag usage fixup. It should be
> > > 0x1000FFFF and not 0x0000FFFF (@Manivannan).
> > > - Add the cleanup-on-error path to the dw_pcie_ep_init() function.
> > > (@Manivannan)
> > >
> > > Signed-off-by: Serge Semin <[email protected]>
> > > Cc: Alexey Malahov <[email protected]>
> > > Cc: Pavel Parkhomenko <[email protected]>
> > > Cc: Rob Herring <[email protected]>
> > > Cc: "Krzysztof Wilczyński" <[email protected]>
> > > Cc: Frank Li <[email protected]>
> > > Cc: Manivannan Sadhasivam <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > >
> > > Serge Semin (13):
> > > PCI: dwc: Stop link in the host init error and de-initialization
> > > PCI: dwc: Don't use generic IO-ops for DBI-space access
> > > PCI: dwc: Add unroll iATU space support to the regions disable method
> > > PCI: dwc: Disable outbound windows for controllers with iATU
> > > PCI: dwc: Set INCREASE_REGION_SIZE flag based on limit address
> > > PCI: dwc: Add braces to the multi-line if-else statements
> > > PCI: dwc: Add trailing new-line literals to the log messages
> > > PCI: dwc: Discard IP-core version checking on unrolled iATU detection
> > > PCI: dwc: Convert Link-up status method to using dw_pcie_readl_dbi()
> > > PCI: dwc: Deallocate EPC memory on EP init error
> > > PCI: dwc-plat: Simplify the probe method return value handling
> > > PCI: dwc-plat: Discard unused regmap pointer
> > > PCI: dwc-plat: Drop dw_plat_pcie_of_match forward declaration
> > >
> > > .../pci/controller/dwc/pcie-designware-ep.c | 22 +++++--
> > > .../pci/controller/dwc/pcie-designware-host.c | 66 +++++++++++++++----
> > > .../pci/controller/dwc/pcie-designware-plat.c | 13 ++--
> > > drivers/pci/controller/dwc/pcie-designware.c | 48 +++++++++-----
> > > 4 files changed, 109 insertions(+), 40 deletions(-)
> > >
> > > --
> > > 2.35.1
> > >