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

[Frank Li <Frank.li@nxp.com>]

On Thu, Dec 14, 2023 at 02:31:04PM +0000, Niklas Cassel wrote:
> 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.
> 

Please sent out your fixed patch. If want to me fix it, please tell me
reproduce steps.

Frank

> 
> 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