PCI: dwc: Clean up iATU index mess
Subsystems (4)
v4 cover PCI: dwc: Clean up iATU index mess
This series tries to clean up the iATU index mess, by treating iATU indexing for outbound and inbound address translation in a consistent way (increment the index after assignment, and only if the function call succeeded).
Changes since v3:
-The ECAM config space accessors do not support cfg0_io_shared, thus if
running in ECAM mode, and we are out of outbound iATUs, return an error instead of enabling the cfg0_io_shared feature.
Link to v3:
https://lore.kernel.org/linux-pci/[email protected]/
Link to v2:
https://lore.kernel.org/linux-pci/[email protected]/
Link to v1:
https://lore.kernel.org/linux-pci/[email protected]/
Krishna Chaitanya Chundru (1):
PCI: dwc: Fix missing iATU setup when ECAM is enabled
Niklas Cassel (3):
PCI: dwc: Fix msg_atu_index assignment
PCI: dwc: Improve msg_atu_index error handling
PCI: dwc: Clean up iATU index usage in dw_pcie_iatu_setup()
.../pci/controller/dwc/pcie-designware-host.c | 91 ++++++++++++------- drivers/pci/controller/dwc/pcie-designware.c | 6 ++ 2 files changed, 62 insertions(+), 35 deletions(-)
base-commit: 0ecd890e3cf54a0586247b9a384702703277e4fd
Signature · 1 line
2.52.0
PATCH v4 1/4 PCI: dwc: Fix msg_atu_index assignment
When dw_pcie_iatu_setup() configures outbound address translation for both type PCIE_ATU_TYPE_MEM and PCIE_ATU_TYPE_IO, the iATU index to use is incremented before calling dw_pcie_prog_outbound_atu().
However, for msg_atu_index the index is not incremented before use, causing the iATU index to be the same as the last configured iATU index, which means that it will incorrectly use the same iATU index that is already in use, breaking outbound address translation.
--- drivers/pci/controller/dwc/pcie-designware-host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/dwc/pcie-designware-host.c +1 -1
@@ -982,7 +982,7 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)dev_warn (pci->dev,"Ranges exceed outbound iATU size (%d)\n" , pci->num_ob_windows); - pp->msg_atu_index = i; + pp->msg_atu_index = ++i; i =0 ;resource_list_for_each_entry (entry, &pp->bridge->dma_ranges) {
Signature · 1 line
2.52.0
Re: [PATCH v4 1/4] PCI: dwc: Fix msg_atu_index assignment
Quoted 24 lines · Niklas Cassel
When
dw_pcie_iatu_setup()configures outbound address translation for both typePCIE_ATU_TYPE_MEMandPCIE_ATU_TYPE_IO, the iATU index to use is incremented before callingdw_pcie_prog_outbound_atu().However, for
msg_atu_indexthe index is not incremented before use, causing the iATU index to be the same as the last configured iATU index, which means that it will incorrectly use the same iATU index that is already in use, breaking outbound address translation.Fixes e1a4ec1a9520 ("PCI: dwc: Add generic MSG TLP support for sending PME_Turn_Off when system suspend")Reviewed-by Frank Li <[email protected]>Reviewed-by Shawn Lin <[email protected]>Reviewed-by Hans Zhang <[email protected]>Signed-off-by Niklas Cassel <[email protected]>--- drivers/pci/controller/dwc/pcie-designware-host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pci/controller/dwc/pcie-designware-host.c +0
-0
drivers/pci/controller/dwc/pcie-designware-host.c static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp) · line 985
Quoted 5 lines
drivers/pci/controller/dwc/pcie-designware-host.c +1
-1@@ -982,7 +982,7 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)dev_warn (pci->dev,"Ranges exceed outbound iATU size (%d)\n" , pci->num_ob_windows); - pp->msg_atu_index = i; + pp->msg_atu_index = ++i;
pp->msg_atu_index = i + 1;
is a lot more readable in my opinion. Changing i itself is useless since it is reset to 0 below.
Quoted 3 lines
drivers/pci/controller/dwc/pcie-designware-host.c +0
-0i =0 ;resource_list_for_each_entry (entry, &pp->bridge->dma_ranges) {
Signature · 2 lines
Damien Le Moal Western Digital Research
PATCH v4 2/4 PCI: dwc: Improve msg_atu_index error handling
In reply to Niklas CasselOnly try to dedicate an outbound iATU to MSG TLP if use_atu_msg is set. If use_atu_msg is not set, it is completely useless to bump the index.
Additionally, if use_atu_msg is set, and there is no outbound iATU available, return an error.
--- drivers/pci/controller/dwc/pcie-designware-host.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/pci/controller/dwc/pcie-designware-host.c +8 -1
@@ -982,7 +982,14 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)dev_warn (pci->dev,"Ranges exceed outbound iATU size (%d)\n" , pci->num_ob_windows); - pp->msg_atu_index = ++i; +if (pp->use_atu_msg) { +if (pci->num_ob_windows > ++i) { + pp->msg_atu_index = i; + }else { +dev_err (pci->dev,"Cannot add outbound window for MSG TLP\n" ); +return -ENOMEM ; + } + } i =0 ;resource_list_for_each_entry (entry, &pp->bridge->dma_ranges) {
Signature · 1 line
2.52.0
Re: [PATCH v4 2/4] PCI: dwc: Improve msg_atu_index error handling
Quoted 19 lines · Niklas Cassel
Only try to dedicate an outbound iATU to MSG TLP if
use_atu_msgis set. Ifuse_atu_msgis not set, it is completely useless to bump the index.Additionally, if
use_atu_msgis set, and there is no outbound iATU available, return an error.Fixes e1a4ec1a9520 ("PCI: dwc: Add generic MSG TLP support for sending PME_Turn_Off when system suspend")Reviewed-by Frank Li <[email protected]>Reviewed-by Shawn Lin <[email protected]>Signed-off-by Niklas Cassel <[email protected]>--- drivers/pci/controller/dwc/pcie-designware-host.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)drivers/pci/controller/dwc/pcie-designware-host.c +0
-0
drivers/pci/controller/dwc/pcie-designware-host.c static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp) · line 986
Quoted 6 lines
drivers/pci/controller/dwc/pcie-designware-host.c +2
-1@@ -982,7 +982,14 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)dev_warn (pci->dev,"Ranges exceed outbound iATU size (%d)\n" , pci->num_ob_windows); - pp->msg_atu_index = ++i; +if (pp->use_atu_msg) { +if (pci->num_ob_windows > ++i) {
if (pp->use_atu_msg) {
i++;
if (pci->num_ob_windows > i) {is far more readable in my opinion.
drivers/pci/controller/dwc/pcie-designware-host.c static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp) · line 992
Quoted 6 lines
drivers/pci/controller/dwc/pcie-designware-host.c +6
-0+ pp->msg_atu_index = i; + }else { +dev_err (pci->dev,"Cannot add outbound window for MSG TLP\n" ); +return -ENOMEM ; + } + }
Seeing this, I do not really see the point of the previous patch since you completely nuke that change here. So maybe drop patch 1 ?
Quoted 3 lines
drivers/pci/controller/dwc/pcie-designware-host.c +0
-0i =0 ;resource_list_for_each_entry (entry, &pp->bridge->dma_ranges) {
Signature · 2 lines
Damien Le Moal Western Digital Research
Re: [PATCH v4 2/4] PCI: dwc: Improve msg_atu_index error handling
Quoted 31 lines · Damien Le Moal
Quoted 19 lines · Niklas Cassel
Only try to dedicate an outbound iATU to MSG TLP if
use_atu_msgis set. Ifuse_atu_msgis not set, it is completely useless to bump the index.Additionally, if
use_atu_msgis set, and there is no outbound iATU available, return an error.Fixes e1a4ec1a9520 ("PCI: dwc: Add generic MSG TLP support for sending PME_Turn_Off when system suspend")Reviewed-by Frank Li <[email protected]>Reviewed-by Shawn Lin <[email protected]>Signed-off-by Niklas Cassel <[email protected]>--- drivers/pci/controller/dwc/pcie-designware-host.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)drivers/pci/controller/dwc/pcie-designware-host.c +0
-0drivers/pci/controller/dwc/pcie-designware-host.c static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp) · line 986
Quoted 6 lines
drivers/pci/controller/dwc/pcie-designware-host.c +2
-1@@ -982,7 +982,14 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)dev_warn (pci->dev,"Ranges exceed outbound iATU size (%d)\n" , pci->num_ob_windows); - pp->msg_atu_index = ++i; +if (pp->use_atu_msg) { +if (pci->num_ob_windows > ++i) {if (pp->use_atu_msg) { i++; if (pci->num_ob_windows > i) {
Quoted 1 line
is far more readable in my opinion.
Yes, I agree that it is more readable.
However, in patch 3/4 in series, I'm do a cleanup of this whole function, not just this statement.
This patch changes the code in a way that is consistent with the existing coding style of this function, see e.g.:
https://github.com/torvalds/linux/blob/v6.19-rc7/drivers/pci/controller/dwc/pcie-designware-host.c#L932-L934
Then patch 3/4 modifies the whole function to be more readable.
So I prefer to still keep this patch as is, such the code is always in a "consistent coding style".
And then do all cleanups in the cleanup patch.
Quoted 10 lines
drivers/pci/controller/dwc/pcie-designware-host.c static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp) · line 992
Quoted 7 lines
drivers/pci/controller/dwc/pcie-designware-host.c +6
-0+ pp->msg_atu_index = i; + }else { +dev_err (pci->dev,"Cannot add outbound window for MSG TLP\n" ); +return -ENOMEM ; + } + }Seeing this, I do not really see the point of the previous patch since you completely nuke that change here. So maybe drop patch 1 ?
My thinking was that the previous patch has
CC: stable, so I wanted the smallest viable backportable fix.
But, I guess that the "if (pp->use_atu_msg) {" guard makes sense even for a backported fix, as it is pointless to increment the counter if use_atu_msg is not set, so sure, I can squash patch 1 and 2.
Kind regards,
Niklas
PATCH v4 3/4 PCI: dwc: Clean up iATU index usage in dw_pcie_iatu_setup()
In reply to Niklas CasselThe current iATU index usage in dw_pcie_iatu_setup() is a mess.
For outbound address translation the index is incremented before usage. For inbound address translation the index is incremented after usage.
Incrementing the index after usage make much more sense, and make the index usage consistent for both outbound and inbound address translation.
Most likely, the overly complicated logic for the outbound address translation is because the iATU at index 0 is reserved for CFG IOs (dw_pcie_other_conf_map_bus()), however, we should be able to use the exact same logic for the indexing of the outbound and inbound iATUs. (Only the starting index should be different.)
Create two new variables ob_iatu_index and ib_iatu_index, which makes it more clear from the name itself that it is a zeroes based index, and only increment the index if the iATU configuration call succeeded.
Since we always check if there is an index available immediately before programming the iATU, we can remove the useless "ranges exceed outbound iATU size" warnings, as the code is already unreachable. For the same reason, we can also remove the useless breaks outside of the while loops.
No functional changes intended.
--- .../pci/controller/dwc/pcie-designware-host.c | 59 ++++++++++--------- 1 file changed, 31 insertions(+), 28 deletions(-)
drivers/pci/controller/dwc/pcie-designware-host.c +31 -28
@@ -892,9 +892,10 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)struct dw_pcie *pci =to_dw_pcie_from_pp (pp);struct dw_pcie_ob_atu_cfg atu = {0 };struct resource_entry *entry; +int ob_iatu_index; +int ib_iatu_index;int i, ret; -/* Note the very first outbound ATU is used for CFG IOs */ if (!pci->num_ob_windows) {dev_err (pci->dev,"No outbound iATU found\n" );return -EINVAL ;
@@ -910,16 +911,18 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)for (i =0 ; i < pci->num_ib_windows; i++)dw_pcie_disable_atu (pci,PCIE_ATU_REGION_DIR_IB , i); - i =0 ; +/* +* NOTE: For outbound address translation, outbound iATU at index 0 is +* reserved for CFG IOs (dw_pcie_other_conf_map_bus()), thus start at +* index 1. +*/ + ob_iatu_index =1 ;resource_list_for_each_entry (entry, &pp->bridge->windows) {resource_size_t res_size;if (resource_type (entry->res) !=IORESOURCE_MEM )continue ; -if (pci->num_ob_windows <= i +1 ) -break ; - atu.type =PCIE_ATU_TYPE_MEM ; atu.parent_bus_addr = entry->res->start - pci->parent_bus_offset; atu.pci_addr = entry->res->start - entry->offset;
@@ -937,13 +940,13 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)* middle. Otherwise, we would end up only partially * mapping a single resource. */ -if (pci->num_ob_windows <= ++i) { -dev_err (pci->dev,"Exhausted outbound windows for region: %pr\n" , +if (ob_iatu_index >= pci->num_ob_windows) { +dev_err (pci->dev,"Cannot add outbound window for region: %pr\n" , entry->res);return -ENOMEM ; } - atu.index = i; + atu.index = ob_iatu_index; atu.size =MIN (pci->region_limit +1 , res_size); ret =dw_pcie_prog_outbound_atu (pci, &atu);
@@ -953,6 +956,7 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)return ret; } + ob_iatu_index++; atu.parent_bus_addr += atu.size; atu.pci_addr += atu.size; res_size -= atu.size;
@@ -960,8 +964,8 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp) }if (pp->io_size) { -if (pci->num_ob_windows > ++i) { - atu.index = i; +if (ob_iatu_index < pci->num_ob_windows) { + atu.index = ob_iatu_index; atu.type =PCIE_ATU_TYPE_IO ; atu.parent_bus_addr = pp->io_base - pci->parent_bus_offset; atu.pci_addr = pp->io_bus_addr;
@@ -973,34 +977,35 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp) entry->res);return ret; } + ob_iatu_index++; }else { +/* +* If there are not enough outbound windows to give I/O +* space its own iATU, the outbound iATU at index 0 will +* be shared between I/O space and CFG IOs, by +* temporarily reconfiguring the iATU to CFG space, in +* order to do a CFG IO, and then immediately restoring +* it to I/O space. +*/ pp->cfg0_io_shared = true; } } -if (pci->num_ob_windows <= i) -dev_warn (pci->dev,"Ranges exceed outbound iATU size (%d)\n" , - pci->num_ob_windows); -if (pp->use_atu_msg) { -if (pci->num_ob_windows > ++i) { - pp->msg_atu_index = i; - }else { +if (ob_iatu_index >= pci->num_ob_windows) {dev_err (pci->dev,"Cannot add outbound window for MSG TLP\n" );return -ENOMEM ; } + pp->msg_atu_index = ob_iatu_index++; } - i =0 ; + ib_iatu_index =0 ;resource_list_for_each_entry (entry, &pp->bridge->dma_ranges) {resource_size_t res_start, res_size, window_size;if (resource_type (entry->res) !=IORESOURCE_MEM )continue ; -if (pci->num_ib_windows <= i) -break ; - res_size =resource_size (entry->res); res_start = entry->res->start;while (res_size >0 ) {
@@ -1009,14 +1014,15 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)* middle. Otherwise, we would end up only partially * mapping a single resource. */ -if (pci->num_ib_windows <= i) { -dev_err (pci->dev,"Exhausted inbound windows for region: %pr\n" , +if (ib_iatu_index >= pci->num_ib_windows) { +dev_err (pci->dev,"Cannot add inbound window for region: %pr\n" , entry->res);return -ENOMEM ; } window_size =MIN (pci->region_limit +1 , res_size); - ret =dw_pcie_prog_inbound_atu (pci, i++,PCIE_ATU_TYPE_MEM , res_start, + ret =dw_pcie_prog_inbound_atu (pci, ib_iatu_index, +PCIE_ATU_TYPE_MEM , res_start, res_start - entry->offset, window_size);if (ret) { dev_err(pci->dev, "Failed to set DMA range %pr\n",
@@ -1024,15 +1030,12 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)return ret; } + ib_iatu_index++; res_start += window_size; res_size -= window_size; } } -if (pci->num_ib_windows <= i) -dev_warn (pci->dev,"Dma-ranges exceed inbound iATU size (%u)\n" , - pci->num_ib_windows); -return 0 ; }
Signature · 1 line
2.52.0
PATCH v4 4/4 PCI: dwc: Fix missing iATU setup when ECAM is enabled
In reply to Niklas CasselFrom: Krishna Chaitanya Chundru <[email protected]>
When ECAM is enabled, the driver skipped calling dw_pcie_iatu_setup() before configuring ECAM iATU entries. This left IO and MEM outbound windows unprogrammed, resulting in broken IO transactions. Additionally, dw_pcie_config_ecam_iatu() was only called during host initialization, so ECAM-related iATU entries were not restored after suspend/resume, leading to failures in configuration space access
To resolve these issues, the ECAM iATU configuration is moved into dw_pcie_iatu_setup(), and dw_pcie_iatu_setup() is invoked when ECAM is enabled.
Furthermore, error checks are added in dw_pcie_prog_outbound_atu() and dw_pcie_prog_inbound_atu() such that an error is returned if trying to program an iATU that is outside the number of iATUs provided by the controller.
--- .../pci/controller/dwc/pcie-designware-host.c | 33 ++++++++++++------- drivers/pci/controller/dwc/pcie-designware.c | 6 ++++ 2 files changed, 28 insertions(+), 11 deletions(-)
drivers/pci/controller/dwc/pcie-designware-host.c +22 -11
@@ -641,14 +641,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)if (ret) goto err_free_msi; -if (pp->ecam_enabled) { - ret =dw_pcie_config_ecam_iatu (pp); -if (ret) { -dev_err (dev,"Failed to configure iATU in ECAM mode\n" ); - goto err_free_msi; - } - } -/* * Allocate the resource for MSG TLP before programming the iATU * outbound window in dw_pcie_setup_rc(). Since the allocation depends
@@ -915,8 +907,21 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)* NOTE: For outbound address translation, outbound iATU at index 0 is * reserved for CFG IOs (dw_pcie_other_conf_map_bus()), thus start at * index 1. +* +* If using ECAM, outbound iATU at index 0 and index 1 is reserved for +* CFG IOs. */ - ob_iatu_index =1 ; +if (pp->ecam_enabled) { + ob_iatu_index =2 ; + ret =dw_pcie_config_ecam_iatu (pp); +if (ret) { +dev_err (pci->dev,"Failed to configure iATU in ECAM mode\n" ); +return ret; + } + }else { + ob_iatu_index =1 ; + } +resource_list_for_each_entry (entry, &pp->bridge->windows) {resource_size_t res_size;
@@ -985,8 +990,14 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)* be shared between I/O space and CFG IOs, by * temporarily reconfiguring the iATU to CFG space, in * order to do a CFG IO, and then immediately restoring -* it to I/O space. +* it to I/O space. This is only implemented when using +* dw_pcie_other_conf_map_bus(), which is not the case +* when using ECAM. */ +if (pp->ecam_enabled) { +dev_err (pci->dev,"Cannot add outbound window for I/O\n" ); +return -ENOMEM ; + } pp->cfg0_io_shared = true; } }
@@ -1157,7 +1168,7 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)* the platform uses its own address translation component rather than * ATU, so we should not program the ATU here. */ -if (pp->bridge->child_ops == &dw_child_pcie_ops) { +if (pp->bridge->child_ops == &dw_child_pcie_ops || pp->ecam_enabled) { ret =dw_pcie_iatu_setup (pp);if (ret)return ret;
drivers/pci/controller/dwc/pcie-designware.c +6 -0
@@ -532,6 +532,9 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,u32 retries, val;u64 limit_addr; +if (atu->index >= pci->num_ob_windows) +return -ENOSPC ; + limit_addr = parent_bus_addr + atu->size -1 ; if ((limit_addr & ~pci->region_limit) != (parent_bus_addr & ~pci->region_limit) ||
@@ -605,6 +608,9 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,u64 limit_addr = pci_addr + size -1 ;u32 retries, val; +if (index >= pci->num_ib_windows) +return -ENOSPC ; +if ((limit_addr & ~pci->region_limit) != (pci_addr & ~pci->region_limit) || !IS_ALIGNED (parent_bus_addr, pci->region_align) || !IS_ALIGNED (pci_addr, pci->region_align) || !size) {
Signature · 1 line
2.52.0
Re: [PATCH v4 4/4] PCI: dwc: Fix missing iATU setup when ECAM is enabled
Quoted 18 lines · Niklas Cassel
From: Krishna Chaitanya Chundru <[email protected]>
When ECAM is enabled, the driver skipped calling
dw_pcie_iatu_setup()before configuring ECAM iATU entries. This left IO and MEM outbound windows unprogrammed, resulting in broken IO transactions. Additionally,dw_pcie_config_ecam_iatu()was only called during host initialization, so ECAM-related iATU entries were not restored after suspend/resume, leading to failures in configuration space accessTo resolve these issues, the ECAM iATU configuration is moved into
dw_pcie_iatu_setup(), anddw_pcie_iatu_setup()is invoked when ECAM is enabled.Furthermore, error checks are added in
dw_pcie_prog_outbound_atu()anddw_pcie_prog_inbound_atu()such that an error is returned if trying to program an iATU that is outside the number of iATUs provided by the controller.
Quoted 6 lines
Fixes f6fd357f7afb ("PCI: dwc: Prepare the driver for enabling ECAM mechanism using iATU 'CFG Shift Feature'")Reported-by Maciej W. Rozycki <[email protected]>Signed-off-by Krishna Chaitanya Chundru <[email protected]>Co-developed-by Niklas Cassel <[email protected]>Signed-off-by Niklas Cassel <[email protected]>
It looks like this patch depends on patch 3, which does not have a Fixes tag. So why not squash together patches 3 and 4 ?
Quoted 96 lines
--- .../pci/controller/dwc/pcie-designware-host.c | 33 ++++++++++++------- drivers/pci/controller/dwc/pcie-designware.c | 6 ++++ 2 files changed, 28 insertions(+), 11 deletions(-)drivers/pci/controller/dwc/pcie-designware-host.c +22
-11@@ -641,14 +641,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)if (ret) goto err_free_msi; -if (pp->ecam_enabled) { - ret =dw_pcie_config_ecam_iatu (pp); -if (ret) { -dev_err (dev,"Failed to configure iATU in ECAM mode\n" ); - goto err_free_msi; - } - } -/* * Allocate the resource for MSG TLP before programming the iATU * outbound window in dw_pcie_setup_rc(). Since the allocation depends @@ -915,8 +907,21 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)* NOTE: For outbound address translation, outbound iATU at index 0 is * reserved for CFG IOs (dw_pcie_other_conf_map_bus()), thus start at * index 1. +* +* If using ECAM, outbound iATU at index 0 and index 1 is reserved for +* CFG IOs. */ - ob_iatu_index =1 ; +if (pp->ecam_enabled) { + ob_iatu_index =2 ; + ret =dw_pcie_config_ecam_iatu (pp); +if (ret) { +dev_err (pci->dev,"Failed to configure iATU in ECAM mode\n" ); +return ret; + } + }else { + ob_iatu_index =1 ; + } +resource_list_for_each_entry (entry, &pp->bridge->windows) {resource_size_t res_size;@@ -985,8 +990,14 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)* be shared between I/O space and CFG IOs, by * temporarily reconfiguring the iATU to CFG space, in * order to do a CFG IO, and then immediately restoring -* it to I/O space. +* it to I/O space. This is only implemented when using +* dw_pcie_other_conf_map_bus(), which is not the case +* when using ECAM. */ +if (pp->ecam_enabled) { +dev_err (pci->dev,"Cannot add outbound window for I/O\n" ); +return -ENOMEM ; + } pp->cfg0_io_shared = true; } }@@ -1157,7 +1168,7 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)* the platform uses its own address translation component rather than * ATU, so we should not program the ATU here. */ -if (pp->bridge->child_ops == &dw_child_pcie_ops) { +if (pp->bridge->child_ops == &dw_child_pcie_ops || pp->ecam_enabled) { ret =dw_pcie_iatu_setup (pp);if (ret)return ret;drivers/pci/controller/dwc/pcie-designware.c +6
-0@@ -532,6 +532,9 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,u32 retries, val;u64 limit_addr; +if (atu->index >= pci->num_ob_windows) +return -ENOSPC ; + limit_addr = parent_bus_addr + atu->size -1 ; if ((limit_addr & ~pci->region_limit) != (parent_bus_addr & ~pci->region_limit) ||@@ -605,6 +608,9 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,u64 limit_addr = pci_addr + size -1 ;u32 retries, val; +if (index >= pci->num_ib_windows) +return -ENOSPC ; +if ((limit_addr & ~pci->region_limit) != (pci_addr & ~pci->region_limit) || !IS_ALIGNED (parent_bus_addr, pci->region_align) || !IS_ALIGNED (pci_addr, pci->region_align) || !size) {
Signature · 2 lines
Damien Le Moal Western Digital Research
Re: [PATCH v4 4/4] PCI: dwc: Fix missing iATU setup when ECAM is enabled
Quoted 26 lines · Damien Le Moal
Quoted 18 lines · Niklas Cassel
From: Krishna Chaitanya Chundru <[email protected]>
When ECAM is enabled, the driver skipped calling
dw_pcie_iatu_setup()before configuring ECAM iATU entries. This left IO and MEM outbound windows unprogrammed, resulting in broken IO transactions. Additionally,dw_pcie_config_ecam_iatu()was only called during host initialization, so ECAM-related iATU entries were not restored after suspend/resume, leading to failures in configuration space accessTo resolve these issues, the ECAM iATU configuration is moved into
dw_pcie_iatu_setup(), anddw_pcie_iatu_setup()is invoked when ECAM is enabled.Furthermore, error checks are added in
dw_pcie_prog_outbound_atu()anddw_pcie_prog_inbound_atu()such that an error is returned if trying to program an iATU that is outside the number of iATUs provided by the controller.Quoted 6 lines
Fixes f6fd357f7afb ("PCI: dwc: Prepare the driver for enabling ECAM mechanism using iATU 'CFG Shift Feature'")Reported-by Maciej W. Rozycki <[email protected]>Signed-off-by Krishna Chaitanya Chundru <[email protected]>Co-developed-by Niklas Cassel <[email protected]>Signed-off-by Niklas Cassel <[email protected]>
Quoted 2 lines
It looks like this patch depends on patch 3, which does not have a Fixes tag. So why not squash together patches 3 and 4 ?
I think it would be wrong to squash the patches.
This patch is mostly written by Krishna, and I think it is wrong to squash a cleanup patch with a fix patch.
It is okay to have a fix patch that is done on top of a cleanup patch. (It would require a lot more effort to write a fix without the cleanup having been done first.)
Therefore, I will simply add:
Cc: [email protected] # depends on Clean up iATU index usage in dw_pcie_iatu_setup()
Such that stable will see that this patch requires a small dependency.
Kind regards,
Niklas