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