Hello Frank,
On Tue, Feb 22, 2022 at 10:23:52AM -0600, Frank Li wrote:
> ntb_mw_set_trans() will set memory map window after endpoint function
> driver bind. The inbound map address need be updated dynamically when
> using NTB by PCIe Root Port and PCIe Endpoint connection.
>
> Checking if iatu already assigned to the BAR, if yes, using assigned iatu
> number to update inbound address map and skip set BAR's register.
>
> Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> ---
>
> Change from V1:
> - improve commit message
>
> drivers/pci/controller/dwc/pcie-designware-ep.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 998b698f40858..cad5d9ea7cc6c 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -161,7 +161,11 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no,
> u32 free_win;
> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>
> - free_win = find_first_zero_bit(ep->ib_window_map, pci->num_ib_windows);
find_first_zero_bit() can return 0, representing bit 0,
which is a perfectly valid return value.
> + if (!ep->bar_to_atu[bar])
so this check is not correct.
For platforms that has a core_init_notifier, e.g. qcom and tegra,
what will happen is that, on first boot:
BAR0: iATU0
BAR1: iATU1
BAR2: iATU2
BAR3: iATU3
BAR4: iATU4
BAR5: iATU5
Rebooting the RC, will cause a perst assertion + deassertion,
after which:
BAR0: iATU6
BAR1: iATU1
BAR2: iATU2
BAR3: iATU3
BAR4: iATU4
BAR5: iATU5
This is obviously a bug, because you cannot use:
if (!ep->bar_to_atu[bar])
when 0 is a valid return value.
My proposed fix is:
@@ -172,16 +176,26 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
{
int ret;
u32 free_win;
+ u32 saved_atu;
struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
- if (!ep->bar_to_atu[bar])
+ saved_atu = ep->bar_to_atu[bar];
+ if (!saved_atu) {
free_win = find_first_zero_bit(ep->ib_window_map, pci->num_ib_windows);
- else
- free_win = ep->bar_to_atu[bar];
-
- if (free_win >= pci->num_ib_windows) {
- dev_err(pci->dev, "No free inbound window\n");
- return -EINVAL;
+ //pr_err("%s BAR: %d, found no ATU, using first free, index: %d\n", __func__, bar, free_win);
+ if (free_win >= pci->num_ib_windows) {
+ dev_err(pci->dev, "No free inbound window\n");
+ return -EINVAL;
+ }
+
+ /*
+ * In order for bar_to_atu[bar] == 0 to equal no iATU, offset
+ * the saved value with 1.
+ */
+ saved_atu = free_win + 1;
+ } else {
+ free_win = saved_atu - 1;
+ //pr_err("%s BAR: %d, already has ATU, index: %d\n", __func__, bar, free_win);
}
ret = dw_pcie_prog_ep_inbound_atu(pci, func_no, free_win, type,
@@ -191,7 +205,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
return ret;
}
- ep->bar_to_atu[bar] = free_win;
+ ep->bar_to_atu[bar] = saved_atu;
set_bit(free_win, ep->ib_window_map);
return 0;
> static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> @@ -244,6 +249,9 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> if (ret)
> return ret;
>
> + if (ep->epf_bar[bar])
> + return 0;
> +
However, here you ignore writing the settings if (ep->epf_bar[bar]),
again, this will not work for a platform with a core_init_notifier,
as they perform a full core reset using reset_control_assert(),
when perst is asserted + deasserted, so AFAICT, these settings will
get cleared for those platforms, so they will need to be re-written.
> dw_pcie_dbi_ro_wr_en(pci);
>
> dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
Kind regards,
Niklas
