Re: [PATCH v2 1/4] PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address

[Niklas Cassel <Niklas.Cassel@wdc.com>]

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@nxp.com>
> ---
> 
> 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