| Message ID | 20220222162355.32369-2-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4284c88fff0efc4e418abb53d78e02dc4f099d6c |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | NTB function for PCIe RC to EP connection | expand |
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. 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
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
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@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. 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; 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
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@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. > > 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. 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
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@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. > > > > 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
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@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. > > > > 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. Feel free to send out a patch, I just wanted to inform people about the problem. I can understand that the point of the patch in $subject was to re-use the exact same iATU for a specific BAR, however, does the change: if (ep->epf_bar[bar]) return 0; to dw_pcie_ep_set_bar(), really provide any value? I guess it will avoid clearing the BAR_REG, if the RC has assigned an PCI address to that BAR, which I guess was important for vntb? If so, I don't see any alternative but to let DWC drivers with a core_init_notifier to call dw_pcie_ep_clear_bar() when asserting perst, or to modify dw_pcie_ep_init_notify() to call dw_pcie_ep_clear_bar(). Kind regards, Niklas
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
On Sat, Dec 16, 2023 at 10:11:19AM +0000, Niklas Cassel wrote: > 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. > The fix for this issue shouldn't be in the DWC driver but rather in the EPF drivers. Because, EPF drivers are the ones calling pci_epc_set_bar() during bind(). So they have to properly cleanup the resources once the perst assertion happens. This issue also applies to other resources such as DMA channels. The problem here is, there is a disconnect between DWC (EPC) and EPF drivers. When the perst assertion happens, the event is not passed to EPF drivers resulting in the EPF drivers having no knowledge that they need to give up the resources. We need to fix this in a sane way and I'm looking into it. But I really appreciate your finding here and in other thread where we discussed similar issue. - Mani > > 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
On 12/20/23 02:59, Manivannan Sadhasivam wrote: >>>>> 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. >> > > The fix for this issue shouldn't be in the DWC driver but rather in the EPF > drivers. Because, EPF drivers are the ones calling pci_epc_set_bar() during > bind(). So they have to properly cleanup the resources once the perst assertion > happens. This issue also applies to other resources such as DMA channels. > > The problem here is, there is a disconnect between DWC (EPC) and EPF drivers. > When the perst assertion happens, the event is not passed to EPF drivers > resulting in the EPF drivers having no knowledge that they need to give up the > resources. > > We need to fix this in a sane way and I'm looking into it. > > But I really appreciate your finding here and in other thread where we discussed > similar issue. We have core_init and link_up notifiers for EPF drivers. Adding link_down and core_exit notifiers would make a lot of sense :) A core_reset notifier could also be a solution. Adding that would also help EPF drivers to handle links going down temporarily (which can happen). Right now, an EPF driver can only deal with such event by tracking if it gets multiple link_up events.
On Wed, Dec 20, 2023 at 02:14:53PM +0900, Damien Le Moal wrote: > On 12/20/23 02:59, Manivannan Sadhasivam wrote: > >>>>> 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. > >> > > > > The fix for this issue shouldn't be in the DWC driver but rather in the EPF > > drivers. Because, EPF drivers are the ones calling pci_epc_set_bar() during > > bind(). So they have to properly cleanup the resources once the perst assertion > > happens. This issue also applies to other resources such as DMA channels. > > > > The problem here is, there is a disconnect between DWC (EPC) and EPF drivers. > > When the perst assertion happens, the event is not passed to EPF drivers > > resulting in the EPF drivers having no knowledge that they need to give up the > > resources. > > > > We need to fix this in a sane way and I'm looking into it. > > > > But I really appreciate your finding here and in other thread where we discussed > > similar issue. > > We have core_init and link_up notifiers for EPF drivers. Adding link_down and > core_exit notifiers would make a lot of sense :) A core_reset notifier could > also be a solution. > Yeah, I'm thinking along those lines. > Adding that would also help EPF drivers to handle links going down temporarily > (which can happen). Right now, an EPF driver can only deal with such event by > tracking if it gets multiple link_up events. > There is already link_down notifier that I introduced a while back. But it is only used by MHI_EPF driver. - Mani > > -- > Damien Le Moal > Western Digital Research > >
On 12/20/23 15:03, Manivannan Sadhasivam wrote: >> We have core_init and link_up notifiers for EPF drivers. Adding link_down and >> core_exit notifiers would make a lot of sense :) A core_reset notifier could >> also be a solution. >> > > Yeah, I'm thinking along those lines. > >> Adding that would also help EPF drivers to handle links going down temporarily >> (which can happen). Right now, an EPF driver can only deal with such event by >> tracking if it gets multiple link_up events. >> > > There is already link_down notifier that I introduced a while back. But it is > only used by MHI_EPF driver. I missed that. I need that notifier :)
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); + 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]; + if (free_win >= pci->num_ib_windows) { dev_err(pci->dev, "No free inbound window\n"); return -EINVAL; @@ -215,6 +219,7 @@ static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, dw_pcie_disable_atu(pci, atu_index, DW_PCIE_REGION_INBOUND); clear_bit(atu_index, ep->ib_window_map); ep->epf_bar[bar] = NULL; + ep->bar_to_atu[bar] = 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; + dw_pcie_dbi_ro_wr_en(pci); dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
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(-)