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