On Thu, Dec 14, 2023 at 04:28:44PM -0500, Frank Li wrote:
>
(snip)
> Does below change fix your problem?
It is basically the same fix as I sent out earlier in this thread,
but yes, it does solve 1 out of 2 problems introduced by the patch
in $subject, so:
Tested-by: Niklas Cassel <niklas.cassel@xxxxxxx>
> 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;
You probably want to add a:
if (!ep->bar_to_atu[bar])
return;
here, so that dw_pcie_ep_clear_bar() will never try to use -1 as index.
(E.g. if clear_bar() is called twice on the same BAR.)
>
> __dw_pcie_ep_reset_bar(pci, func_no, bar, epf_bar->flags);
>
(snip)
> > > 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.
So problem 2 out of 2 introduced by the patch in $subject is that
DWC drivers with a .core_init_notifier, will perform a reset_control_assert()
to reset the core (which will reset both sticky and non-sticky registers),
which means that the early return in dw_pcie_ep_set_bar():
https://github.com/torvalds/linux/blob/v6.7-rc5/drivers/pci/controller/dwc/pcie-designware-ep.c#L268-L269
while returning after the iATU settings have been written,
it will return before:
dw_pcie_writel_dbi2(pci, reg_dbi2, lower_32_bits(size - 1));
dw_pcie_writel_dbi(pci, reg, flags);
Which means that, for drivers with a .core_init_notifier, BARx_REG and
BARx_MASK registers will not be written. This means that they will have
reset values for these registers, which means that e.g. the BAR_SIZE
(which is defined by BARx_MASK) might be incorrect for these platforms
because this function returns early.
I will not send a fix for this problem, I will leave that to you, or Mani,
or Vidya, and hope that people are happy that I simply reported this issue.
Here is my suggested solution in case anyone wants to take a stab at it:
> > > 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
