In subject, maybe "Write BAR_MASK before iATU registers"
I guess writing BAR_MASK is really configuring the *size* of the BAR? Maybe the size is the important semantic connection with iATU config?
On Wed, Nov 27, 2024 at 11:30:17AM +0100, Niklas Cassel wrote:
The DWC Databook description for the LWR_TARGET_RW and LWR_TARGET_HW fields in the IATU_LWR_TARGET_ADDR_OFF_INBOUND_i registers state that: "Field size depends on log2(BAR_MASK+1) in BAR match mode."
Can we include a databook revision and section here to help future maintainers?
I.e. only the upper bits are writable, and the number of writable bits is dependent on the configured BAR_MASK.
If we do not write the BAR_MASK before writing the iATU registers, we are relying the reset value of the BAR_MASK being larger than the requested size of the first set_bar() call. The reset value of the BAR_MASK is SoC dependent.
Thus, if the first set_bar() call requests a size that is larger than the reset value of the BAR_MASK, the iATU will try to write to read-only bits, which will cause the iATU to end up redirecting to a physical address that is different from the address that was intended.
Thus, we should always write the iATU registers after writing the BAR_MASK.
Apparently we write BAR_MASK and the iATU registers in the wrong order? I assume dw_pcie_ep_inbound_atu() writes the iATU registers.
I can't quite connect the commit log with the code change. I assume the dw_pcie_ep_writel_dbi2() and dw_pcie_ep_writel_dbi() writes update BAR_MASK?
And I guess the problem is that the previous code does:
dw_pcie_ep_inbound_atu # iATU dw_pcie_ep_writel_dbi2 # BAR_MASK (?) dw_pcie_ep_writel_dbi
and the new code basically does this:
if (ep->epf_bar[bar]) { dw_pcie_ep_writel_dbi2 # BAR_MASK (?) dw_pcie_ep_writel_dbi } dw_pcie_ep_inbound_atu # iATU ep->epf_bar[bar] = epf_bar
so the first time we call dw_pcie_ep_set_bar(), we write BAR_MASK before iATU, and if we call dw_pcie_ep_set_bar() again, we skip the BAR_MASK update?
Cc: stable@vger.kernel.org Fixes: f8aed6ec624f ("PCI: dwc: designware: Add EP mode support") Signed-off-by: Niklas Cassel cassel@kernel.org
.../pci/controller/dwc/pcie-designware-ep.c | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c index f3ac7d46a855..bad588ef69a4 100644 --- a/drivers/pci/controller/dwc/pcie-designware-ep.c +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c @@ -222,19 +222,10 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, if ((flags & PCI_BASE_ADDRESS_MEM_TYPE_64) && (bar & 1)) return -EINVAL;
- reg = PCI_BASE_ADDRESS_0 + (4 * bar);
- if (!(flags & PCI_BASE_ADDRESS_SPACE))
type = PCIE_ATU_TYPE_MEM;- else
type = PCIE_ATU_TYPE_IO;- ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
- if (ret)
return ret;- if (ep->epf_bar[bar])
return 0;
goto config_atu;- reg = PCI_BASE_ADDRESS_0 + (4 * bar);
dw_pcie_dbi_ro_wr_en(pci); @@ -246,9 +237,20 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, dw_pcie_ep_writel_dbi(ep, func_no, reg + 4, 0); }
- ep->epf_bar[bar] = epf_bar; dw_pcie_dbi_ro_wr_dis(pci);
+config_atu:
- if (!(flags & PCI_BASE_ADDRESS_SPACE))
type = PCIE_ATU_TYPE_MEM;- else
type = PCIE_ATU_TYPE_IO;- ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
- if (ret)
return ret;- ep->epf_bar[bar] = epf_bar;
- return 0;
} -- 2.47.0