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

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

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

> 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