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@xxxxxxxxxxxxxxx
> Fixes: f8aed6ec624f ("PCI: dwc: designware: Add EP mode support")
> Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
> ---
> .../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
>
