On Thu, Dec 14, 2023 at 03:52:43PM -0500, Frank Li wrote:
> On Thu, Dec 14, 2023 at 08:23:14PM +0000, Niklas Cassel wrote:
> > On Thu, Dec 14, 2023 at 10:19:03AM -0500, Frank Li wrote:
> > > 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.
> >
> > Reproduce steps are simple:
> > 1) Add debug messages to dw_pcie_ep_inbound_atu() to see the iATU index for
> > each BAR.
> > 2) Boot an EP platform with a core_init_notifier.
> > 3) Boot the RC.
> > 4) Reboot the RC, which will assert + deassert PERST, and will call
> > pci_epc_init_notify(), which will call .core_init (pci_epf_test_core_init())
> > which will set the BARs.
> >
> >
> > In step 3) BAR0 will use iATU0.
> >
> > In step 4) BAR0 will use iATU6 instead of iATU0.
> > There is no reason for this, as it should really reuse the same
> > iATU index as before, just like all the other BARs do.
> > (This is because of find_first_zero_bit() misusage.)
> >
> >
> > I could send out my patch, but from inspecting the code, it looks like:
> >
> > > > > + if (ep->epf_bar[bar])
> > > > > + return 0;
>
> I checked current code.
>
> dw_pcie_ep_inbound_atu()
> {
> if (!ep->bar_to_atu[bar])
> free_win = find_first_zero_bit(ep->ib_window_map, pci->num_ib_windows);
> else
> free_win = ep->bar_to_atu[bar];
> }
>
> I missed conside '0' is validate windows number. I think we should init
> bar_to_atu to -1.
>
> if (ep->bar_to_atu[bar] < 0)
>
>
> Origial change want dw_pcie_ep_inbound_atu() can be call twice to update
> bar map address. vntb use "bar3" as memory map windows, so have not trigger
> this problem.
Does below change fix your problem?
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index f6207989fc6ad..2868d44649ef7 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -177,7 +177,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
if (!ep->bar_to_atu[bar])
free_win = find_first_zero_bit(ep->ib_window_map, pci->num_ib_windows);
else
- free_win = ep->bar_to_atu[bar];
+ free_win = ep->bar_to_atu[bar] - 1;
if (free_win >= pci->num_ib_windows) {
dev_err(pci->dev, "No free inbound window\n");
@@ -191,7 +191,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] = free_win + 1;
set_bit(free_win, ep->ib_window_map);
return 0;
@@ -228,7 +228,7 @@ static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
struct dw_pcie_ep *ep = epc_get_drvdata(epc);
struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
enum pci_barno bar = epf_bar->barno;
- u32 atu_index = ep->bar_to_atu[bar];
+ u32 atu_index = ep->bar_to_atu[bar] - 1;
__dw_pcie_ep_reset_bar(pci, func_no, bar, epf_bar->flags);
>
> Frank
>
> >
> > from dw_pcie_ep_set_bar(), also needs to be dropped, so that the iATU
> > settings will be re-written for platforms with core_init_notifiers.
> >
> > Right now, for a platform with a core_init_notifier, if you run
> > pci_endpoint_test + reboot the RC (so that PERST is asserted + deasserted),
> > and then run pci_endpoint_test again, then I'm quite sure that
> > pci_endpoint_test will not pass the second time (because iATU settings
> > were not rewritten after reset).
> >
> > It would be nice if Mani or Vidya could confirm this.
> >
> >
> > I guess that you added this statement for some reason, so I assume
> > that we can't just drop this line without breaking something else.
> >
> > I guess one option would be modify dw_pcie_ep_init_notify() to call
> > dw_pcie_ep_clear_bar() on all non-NULL BARs stored in ep->epf_bar[],
> > before calling pci_epc_init_notify(). That way the second .core_init
> > (pci_epf_test_core_init()) call will use write the settings, because
> > ep->epf_bar[] will be empty, so the "write the settings only the first
> > time" approach will then also work for core_init_notifier platforms.
> >
> >
> > Kind regards,
> > Niklas
