| Message ID | 20240304-pci-dbi-rework-v9-0-29d433d99cda@linaro.org |
|---|---|
| Headers | show |
| Series | PCI: dwc: ep: Fix DBI access failure for drivers requiring refclk from host | expand |
On Mon, Mar 04, 2024 at 02:52:17PM +0530, Manivannan Sadhasivam wrote: > The goal of the dw_pcie_ep_init_complete() API is to initialize the DWC > specific registers post registering the controller with the EP framework. > > But the naming doesn't reflect its functionality and causes confusion. So, > let's rename it to dw_pcie_ep_init_registers() to make it clear that it > initializes the DWC specific registers. > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- Reviewed-by: Niklas Cassel <cassel@kernel.org>
On Mon, Mar 04, 2024 at 02:52:19PM +0530, Manivannan Sadhasivam wrote: > "core_init_notifier" flag is set by the glue drivers requiring refclk from > the host to complete the DWC core initialization. Also, those drivers will > send a notification to the EPF drivers once the initialization is fully > completed using the pci_epc_init_notify() API. Only then, the EPF drivers > will start functioning. > > For the rest of the drivers generating refclk locally, EPF drivers will > start functioning post binding with them. EPF drivers rely on the > 'core_init_notifier' flag to differentiate between the drivers. > Unfortunately, this creates two different flows for the EPF drivers. > > So to avoid that, let's get rid of the "core_init_notifier" flag and follow > a single initialization flow for the EPF drivers. This is done by calling > the dw_pcie_ep_init_notify() from all glue drivers after the completion of > dw_pcie_ep_init_registers() API. This will allow all the glue drivers to > send the notification to the EPF drivers once the initialization is fully > completed. > > Only difference here is that, the drivers requiring refclk from host will > send the notification once refclk is received, while others will send it > during probe time itself. > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- You have removed the .core_init_notifier from EPC drivers, but the callback in EPF drivers is still called .core_init. Yes, this was a confusing name even before this patch, but after this patch, it is probably even worse :) The callback should be named from the perspective of EPF drivers IMO. .core_init sounds like a EPF driver should initialize the core. (But that is of course done by the EPC driver.) The .link_up() callback name is better, the EPF driver is informed that the link is up. Perhaps we could rename .core_init to .core_up ? It tells the EPF drivers that the core is now up. (And the EPF driver can configure the BARs.) Considering that you are not changing the name of the callback, and that it was already confusing before this patch: Reviewed-by: Niklas Cassel <cassel@kernel.org>
On Mon, Mar 04, 2024 at 02:52:21PM +0530, Manivannan Sadhasivam wrote: > Now that the API is available, let's make use of it. It also handles the > reinitialization of DWC non-sticky registers in addition to sending the > notification to EPF drivers. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/controller/dwc/pcie-qcom-ep.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c > index 2fb8c15e7a91..4e45bc4bca45 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c > @@ -640,7 +640,7 @@ static irqreturn_t qcom_pcie_ep_global_irq_thread(int irq, void *data) > if (FIELD_GET(PARF_INT_ALL_LINK_DOWN, status)) { > dev_dbg(dev, "Received Linkdown event\n"); > pcie_ep->link_status = QCOM_PCIE_EP_LINK_DOWN; > - pci_epc_linkdown(pci->ep.epc); > + dw_pcie_ep_linkdown(&pci->ep); > } else if (FIELD_GET(PARF_INT_ALL_BME, status)) { > dev_dbg(dev, "Received BME event. Link is enabled!\n"); > pcie_ep->link_status = QCOM_PCIE_EP_LINK_ENABLED; > > -- > 2.25.1 > Reviewed-by: Niklas Cassel <cassel@kernel.org>
On Thu, Mar 07, 2024 at 10:09:06PM +0100, Niklas Cassel wrote: > On Mon, Mar 04, 2024 at 02:52:19PM +0530, Manivannan Sadhasivam wrote: > > "core_init_notifier" flag is set by the glue drivers requiring refclk from > > the host to complete the DWC core initialization. Also, those drivers will > > send a notification to the EPF drivers once the initialization is fully > > completed using the pci_epc_init_notify() API. Only then, the EPF drivers > > will start functioning. > > > > For the rest of the drivers generating refclk locally, EPF drivers will > > start functioning post binding with them. EPF drivers rely on the > > 'core_init_notifier' flag to differentiate between the drivers. > > Unfortunately, this creates two different flows for the EPF drivers. > > > > So to avoid that, let's get rid of the "core_init_notifier" flag and follow > > a single initialization flow for the EPF drivers. This is done by calling > > the dw_pcie_ep_init_notify() from all glue drivers after the completion of > > dw_pcie_ep_init_registers() API. This will allow all the glue drivers to > > send the notification to the EPF drivers once the initialization is fully > > completed. > > > > Only difference here is that, the drivers requiring refclk from host will > > send the notification once refclk is received, while others will send it > > during probe time itself. > > > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > You have removed the .core_init_notifier from EPC drivers, > but the callback in EPF drivers is still called .core_init. > > Yes, this was a confusing name even before this patch, but > after this patch, it is probably even worse :) > > The callback should be named from the perspective of EPF drivers IMO. > .core_init sounds like a EPF driver should initialize the core. > (But that is of course done by the EPC driver.) > > The .link_up() callback name is better, the EPF driver is informed > that the link is up. > > Perhaps we could rename .core_init to .core_up ? > > It tells the EPF drivers that the core is now up. > (And the EPF driver can configure the BARs.) > I don't disagree :) I thought about it but then decided to not extend the scope of this series further. So saved that for next series. But yeah, it is good to clean it up here itself. > > Considering that you are not changing the name of the callback, > and that it was already confusing before this patch: > Reviewed-by: Niklas Cassel <cassel@kernel.org> Thanks! - Mani
On Fri, Mar 08, 2024 at 11:08:29AM +0530, Manivannan Sadhasivam wrote: > On Thu, Mar 07, 2024 at 10:09:06PM +0100, Niklas Cassel wrote: > > On Mon, Mar 04, 2024 at 02:52:19PM +0530, Manivannan Sadhasivam wrote: > > > "core_init_notifier" flag is set by the glue drivers requiring refclk from > > > the host to complete the DWC core initialization. Also, those drivers will > > > send a notification to the EPF drivers once the initialization is fully > > > completed using the pci_epc_init_notify() API. Only then, the EPF drivers > > > will start functioning. > > > > > > For the rest of the drivers generating refclk locally, EPF drivers will > > > start functioning post binding with them. EPF drivers rely on the > > > 'core_init_notifier' flag to differentiate between the drivers. > > > Unfortunately, this creates two different flows for the EPF drivers. > > > > > > So to avoid that, let's get rid of the "core_init_notifier" flag and follow > > > a single initialization flow for the EPF drivers. This is done by calling > > > the dw_pcie_ep_init_notify() from all glue drivers after the completion of > > > dw_pcie_ep_init_registers() API. This will allow all the glue drivers to > > > send the notification to the EPF drivers once the initialization is fully > > > completed. > > > > > > Only difference here is that, the drivers requiring refclk from host will > > > send the notification once refclk is received, while others will send it > > > during probe time itself. > > > > > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > --- > > > > You have removed the .core_init_notifier from EPC drivers, > > but the callback in EPF drivers is still called .core_init. > > > > Yes, this was a confusing name even before this patch, but > > after this patch, it is probably even worse :) > > > > The callback should be named from the perspective of EPF drivers IMO. > > .core_init sounds like a EPF driver should initialize the core. > > (But that is of course done by the EPC driver.) > > > > The .link_up() callback name is better, the EPF driver is informed > > that the link is up. > > > > Perhaps we could rename .core_init to .core_up ? > > > > It tells the EPF drivers that the core is now up. > > (And the EPF driver can configure the BARs.) > > > > I don't disagree :) I thought about it but then decided to not extend the scope > of this series further. So saved that for next series. > > But yeah, it is good to clean it up here itself. If you intend to create a .core_deinit or .core_down (or whatever name you decide on), perhaps it is better to leave this cleanup to be part of that same series? Kind regards, Niklas
On Fri, Mar 08, 2024 at 09:48:07AM +0100, Niklas Cassel wrote: > On Fri, Mar 08, 2024 at 11:08:29AM +0530, Manivannan Sadhasivam wrote: > > On Thu, Mar 07, 2024 at 10:09:06PM +0100, Niklas Cassel wrote: > > > On Mon, Mar 04, 2024 at 02:52:19PM +0530, Manivannan Sadhasivam wrote: > > > > "core_init_notifier" flag is set by the glue drivers requiring refclk from > > > > the host to complete the DWC core initialization. Also, those drivers will > > > > send a notification to the EPF drivers once the initialization is fully > > > > completed using the pci_epc_init_notify() API. Only then, the EPF drivers > > > > will start functioning. > > > > > > > > For the rest of the drivers generating refclk locally, EPF drivers will > > > > start functioning post binding with them. EPF drivers rely on the > > > > 'core_init_notifier' flag to differentiate between the drivers. > > > > Unfortunately, this creates two different flows for the EPF drivers. > > > > > > > > So to avoid that, let's get rid of the "core_init_notifier" flag and follow > > > > a single initialization flow for the EPF drivers. This is done by calling > > > > the dw_pcie_ep_init_notify() from all glue drivers after the completion of > > > > dw_pcie_ep_init_registers() API. This will allow all the glue drivers to > > > > send the notification to the EPF drivers once the initialization is fully > > > > completed. > > > > > > > > Only difference here is that, the drivers requiring refclk from host will > > > > send the notification once refclk is received, while others will send it > > > > during probe time itself. > > > > > > > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > --- > > > > > > You have removed the .core_init_notifier from EPC drivers, > > > but the callback in EPF drivers is still called .core_init. > > > > > > Yes, this was a confusing name even before this patch, but > > > after this patch, it is probably even worse :) > > > > > > The callback should be named from the perspective of EPF drivers IMO. > > > .core_init sounds like a EPF driver should initialize the core. > > > (But that is of course done by the EPC driver.) > > > > > > The .link_up() callback name is better, the EPF driver is informed > > > that the link is up. > > > > > > Perhaps we could rename .core_init to .core_up ? > > > > > > It tells the EPF drivers that the core is now up. > > > (And the EPF driver can configure the BARs.) > > > > > > > I don't disagree :) I thought about it but then decided to not extend the scope > > of this series further. So saved that for next series. > > > > But yeah, it is good to clean it up here itself. > > If you intend to create a .core_deinit or .core_down (or whatever name > you decide on), perhaps it is better to leave this cleanup to be part > of that same series? > I already added a patch. So let's do it here itself :) - Mani
Hello Manivannan, > From: Manivannan Sadhasivam, Sent: Monday, March 4, 2024 6:22 PM > > deinit() callback was solely introduced for the pcie-rcar-gen4 driver where > it is used to do platform specific resource deallocation. And this callback > is called right at the end of the dw_pcie_ep_exit() API. So it doesn't > matter whether it is called within or outside of dw_pcie_ep_exit() API. > > So let's remove this callback and directly call rcar_gen4_pcie_ep_deinit() > in pcie-rcar-gen4 driver to do resource deallocation after the completion > of dw_pcie_ep_exit() API in rcar_gen4_remove_dw_pcie_ep(). > > This simplifies the DWC layer. > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Thank you for the patch! Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Best regards, Yoshihiro Shimoda > --- > drivers/pci/controller/dwc/pcie-designware-ep.c | 9 +-------- > drivers/pci/controller/dwc/pcie-designware.h | 1 - > drivers/pci/controller/dwc/pcie-rcar-gen4.c | 14 ++++++++------ > 3 files changed, 9 insertions(+), 15 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > index 5befed2dc02b..d305f9b4cdfe 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > @@ -575,9 +575,6 @@ void dw_pcie_ep_exit(struct dw_pcie_ep *ep) > epc->mem->window.page_size); > > pci_epc_mem_exit(epc); > - > - if (ep->ops->deinit) > - ep->ops->deinit(ep); > } > EXPORT_SYMBOL_GPL(dw_pcie_ep_exit); > > @@ -738,7 +735,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > ep->page_size); > if (ret < 0) { > dev_err(dev, "Failed to initialize address space\n"); > - goto err_ep_deinit; > + return ret; > } > > ep->msi_mem = pci_epc_mem_alloc_addr(epc, &ep->msi_mem_phys, > @@ -775,10 +772,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > err_exit_epc_mem: > pci_epc_mem_exit(epc); > > -err_ep_deinit: > - if (ep->ops->deinit) > - ep->ops->deinit(ep); > - > return ret; > } > EXPORT_SYMBOL_GPL(dw_pcie_ep_init); > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 26dae4837462..ab7431a37209 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -333,7 +333,6 @@ struct dw_pcie_rp { > struct dw_pcie_ep_ops { > void (*pre_init)(struct dw_pcie_ep *ep); > void (*init)(struct dw_pcie_ep *ep); > - void (*deinit)(struct dw_pcie_ep *ep); > int (*raise_irq)(struct dw_pcie_ep *ep, u8 func_no, > unsigned int type, u16 interrupt_num); > const struct pci_epc_features* (*get_features)(struct dw_pcie_ep *ep); > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c > index e9166619b1f9..ac97d594ea47 100644 > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c > @@ -352,11 +352,8 @@ static void rcar_gen4_pcie_ep_init(struct dw_pcie_ep *ep) > dw_pcie_ep_reset_bar(pci, bar); > } > > -static void rcar_gen4_pcie_ep_deinit(struct dw_pcie_ep *ep) > +static void rcar_gen4_pcie_ep_deinit(struct rcar_gen4_pcie *rcar) > { > - struct dw_pcie *dw = to_dw_pcie_from_ep(ep); > - struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw); > - > writel(0, rcar->base + PCIEDMAINTSTSEN); > rcar_gen4_pcie_common_deinit(rcar); > } > @@ -408,7 +405,6 @@ static unsigned int rcar_gen4_pcie_ep_get_dbi2_offset(struct dw_pcie_ep *ep, > static const struct dw_pcie_ep_ops pcie_ep_ops = { > .pre_init = rcar_gen4_pcie_ep_pre_init, > .init = rcar_gen4_pcie_ep_init, > - .deinit = rcar_gen4_pcie_ep_deinit, > .raise_irq = rcar_gen4_pcie_ep_raise_irq, > .get_features = rcar_gen4_pcie_ep_get_features, > .get_dbi_offset = rcar_gen4_pcie_ep_get_dbi_offset, > @@ -418,18 +414,24 @@ static const struct dw_pcie_ep_ops pcie_ep_ops = { > static int rcar_gen4_add_dw_pcie_ep(struct rcar_gen4_pcie *rcar) > { > struct dw_pcie_ep *ep = &rcar->dw.ep; > + int ret; > > if (!IS_ENABLED(CONFIG_PCIE_RCAR_GEN4_EP)) > return -ENODEV; > > ep->ops = &pcie_ep_ops; > > - return dw_pcie_ep_init(ep); > + ret = dw_pcie_ep_init(ep); > + if (ret) > + rcar_gen4_pcie_ep_deinit(rcar); > + > + return ret; > } > > static void rcar_gen4_remove_dw_pcie_ep(struct rcar_gen4_pcie *rcar) > { > dw_pcie_ep_exit(&rcar->dw.ep); > + rcar_gen4_pcie_ep_deinit(rcar); > } > > /* Common */ > > -- > 2.25.1
On Mon, Mar 04, 2024 at 02:52:19PM +0530, Manivannan Sadhasivam wrote: > "core_init_notifier" flag is set by the glue drivers requiring refclk from > the host to complete the DWC core initialization. Also, those drivers will > send a notification to the EPF drivers once the initialization is fully > completed using the pci_epc_init_notify() API. Only then, the EPF drivers > will start functioning. > > For the rest of the drivers generating refclk locally, EPF drivers will > start functioning post binding with them. EPF drivers rely on the > 'core_init_notifier' flag to differentiate between the drivers. > Unfortunately, this creates two different flows for the EPF drivers. > > So to avoid that, let's get rid of the "core_init_notifier" flag and follow > a single initialization flow for the EPF drivers. This is done by calling > the dw_pcie_ep_init_notify() from all glue drivers after the completion of > dw_pcie_ep_init_registers() API. This will allow all the glue drivers to > send the notification to the EPF drivers once the initialization is fully > completed. > > Only difference here is that, the drivers requiring refclk from host will > send the notification once refclk is received, while others will send it > during probe time itself. > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > index 18c80002d3bd..fc0282b0d626 100644 > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > @@ -927,21 +928,12 @@ static int pci_epf_test_bind(struct pci_epf *epf) > if (ret) > return ret; > Hello Mani, Since you asked for testing, I gave your series a spin (with a driver without .core_init_notifier). There seems to be a problem that pci_epc_write_header() is never called. Debugging this, it seems that .core_init in pci-epf-test is never called. If I add debug prints in pci_epc_init_notify(), I see that it does not notify a single EPF driver. It appears that the patch in $subject will call pci_epc_init_notify() at EPC driver .probe() time, and at that point in time, there are no EPF drivers registered. They get registered later, when doing the configfs write. I would say that it is the following change that breaks things: > - if (!core_init_notifier) { > - ret = pci_epf_test_core_init(epf); > - if (ret) > - return ret; > - } > - Since without this code, pci_epf_test_core_init() will no longer be called, as there is currently no one that calls epf->core_init() for a EPF driver after it has been bound. (For drivers that call dw_pcie_ep_init_notify() in .probe()) I guess one way to solve this would be for the EPC core to keep track of the current EPC "core state" (up/down). If the core is "up" at EPF .bind() time, notify the EPF driver directly after .bind()? Kind regards, Niklas
On Fri, Mar 08, 2024 at 02:24:35PM +0100, Niklas Cassel wrote: > On Mon, Mar 04, 2024 at 02:52:19PM +0530, Manivannan Sadhasivam wrote: > > "core_init_notifier" flag is set by the glue drivers requiring refclk from > > the host to complete the DWC core initialization. Also, those drivers will > > send a notification to the EPF drivers once the initialization is fully > > completed using the pci_epc_init_notify() API. Only then, the EPF drivers > > will start functioning. > > > > For the rest of the drivers generating refclk locally, EPF drivers will > > start functioning post binding with them. EPF drivers rely on the > > 'core_init_notifier' flag to differentiate between the drivers. > > Unfortunately, this creates two different flows for the EPF drivers. > > > > So to avoid that, let's get rid of the "core_init_notifier" flag and follow > > a single initialization flow for the EPF drivers. This is done by calling > > the dw_pcie_ep_init_notify() from all glue drivers after the completion of > > dw_pcie_ep_init_registers() API. This will allow all the glue drivers to > > send the notification to the EPF drivers once the initialization is fully > > completed. > > > > Only difference here is that, the drivers requiring refclk from host will > > send the notification once refclk is received, while others will send it > > during probe time itself. > > > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > > index 18c80002d3bd..fc0282b0d626 100644 > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > > @@ -927,21 +928,12 @@ static int pci_epf_test_bind(struct pci_epf *epf) > > if (ret) > > return ret; > > > > Hello Mani, > > Since you asked for testing, I gave your series a spin > (with a driver without .core_init_notifier). > > > There seems to be a problem that pci_epc_write_header() is never called. > > Debugging this, it seems that .core_init in pci-epf-test is never called. > > If I add debug prints in pci_epc_init_notify(), I see that it does not > notify a single EPF driver. > > It appears that the patch in $subject will call pci_epc_init_notify() > at EPC driver .probe() time, and at that point in time, there are no > EPF drivers registered. > > They get registered later, when doing the configfs write. > > > I would say that it is the following change that breaks things: > > > - if (!core_init_notifier) { > > - ret = pci_epf_test_core_init(epf); > > - if (ret) > > - return ret; > > - } > > - > > Since without this code, pci_epf_test_core_init() will no longer be called, > as there is currently no one that calls epf->core_init() for a EPF driver > after it has been bound. (For drivers that call dw_pcie_ep_init_notify() in > .probe()) > Thanks a lot for testing, Niklas! > I guess one way to solve this would be for the EPC core to keep track of > the current EPC "core state" (up/down). If the core is "up" at EPF .bind() > time, notify the EPF driver directly after .bind()? > Yeah, that's a good solution. But I think it would be better if the EPC caches all events if the EPF drivers are not available and dispatch them once the bind happens for each EPF driver. Even though INIT_COMPLETE is the only event that is getting generated before bind() now, IMO it is better to add provision to catch other events also. Wdyt? - Mani
On Mon, Mar 11, 2024 at 08:15:59PM +0530, Manivannan Sadhasivam wrote: > > > > I would say that it is the following change that breaks things: > > > > > - if (!core_init_notifier) { > > > - ret = pci_epf_test_core_init(epf); > > > - if (ret) > > > - return ret; > > > - } > > > - > > > > Since without this code, pci_epf_test_core_init() will no longer be called, > > as there is currently no one that calls epf->core_init() for a EPF driver > > after it has been bound. (For drivers that call dw_pcie_ep_init_notify() in > > .probe()) > > > > Thanks a lot for testing, Niklas! > > > I guess one way to solve this would be for the EPC core to keep track of > > the current EPC "core state" (up/down). If the core is "up" at EPF .bind() > > time, notify the EPF driver directly after .bind()? > > > > Yeah, that's a good solution. But I think it would be better if the EPC caches > all events if the EPF drivers are not available and dispatch them once the bind > happens for each EPF driver. Even though INIT_COMPLETE is the only event that is > getting generated before bind() now, IMO it is better to add provision to catch > other events also. > > Wdyt? I'm not sure. What if the EPF goes up/down/up, it seems a bit silly to send all those events to the EPF driver that will alloc+free+alloc. Do we know for sure that we will want to store + replay events other than INIT_COMPLETE? And how many events should we store? Until we can think of a good reason which events other than UP/DOWN we can to store, I think that just storing the state as an integer in struct pci_epc seems simpler. Or I guess we could continue with a flag in struct pci_epc_features, like has_perst_notifier, which would then require the EPC driver to call both epc_notify_core_up() and epc_notify_core_down() when receiving the PERST deassert/assert. For a driver without the flag set, the EPC core would call .epc_notify_core_up() after bind. (And .epc_notify_core_down() would never be called, or it could call it before unbind().) That way an EPF driver itself would not need any different handling (all callbacks would always come, either triggered by an EPC driver that has PERST GPIO irq, or triggered by the EPC core for a driver that lacks a PERST GPIO). Kind regards, Niklas
On Mon, Mar 11, 2024 at 10:54:28PM +0100, Niklas Cassel wrote: > On Mon, Mar 11, 2024 at 08:15:59PM +0530, Manivannan Sadhasivam wrote: > > > > > > I would say that it is the following change that breaks things: > > > > > > > - if (!core_init_notifier) { > > > > - ret = pci_epf_test_core_init(epf); > > > > - if (ret) > > > > - return ret; > > > > - } > > > > - > > > > > > Since without this code, pci_epf_test_core_init() will no longer be called, > > > as there is currently no one that calls epf->core_init() for a EPF driver > > > after it has been bound. (For drivers that call dw_pcie_ep_init_notify() in > > > .probe()) > > > > > > > Thanks a lot for testing, Niklas! > > > > > I guess one way to solve this would be for the EPC core to keep track of > > > the current EPC "core state" (up/down). If the core is "up" at EPF .bind() > > > time, notify the EPF driver directly after .bind()? > > > > > > > Yeah, that's a good solution. But I think it would be better if the EPC caches > > all events if the EPF drivers are not available and dispatch them once the bind > > happens for each EPF driver. Even though INIT_COMPLETE is the only event that is > > getting generated before bind() now, IMO it is better to add provision to catch > > other events also. > > > > Wdyt? > > I'm not sure. > What if the EPF goes up/down/up, it seems a bit silly to send all those > events to the EPF driver that will alloc+free+alloc. > > Do we know for sure that we will want to store + replay events other than > INIT_COMPLETE? > > And how many events should we store? > > > Until we can think of a good reason which events other than UP/DOWN we > can to store, I think that just storing the state as an integer in > struct pci_epc seems simpler. > Hmm, makes sense. > > Or I guess we could continue with a flag in struct pci_epc_features, > like has_perst_notifier, which would then require the EPC driver to > call both epc_notify_core_up() and epc_notify_core_down() when receiving > the PERST deassert/assert. > For a driver without the flag set, the EPC core would call > .epc_notify_core_up() after bind. (And .epc_notify_core_down() would never > be called, or it could call it before unbind().) > That way an EPF driver itself would not need any different handling > (all callbacks would always come, either triggered by an EPC driver that > has PERST GPIO irq, or triggered by the EPC core for a driver that lacks > a PERST GPIO). > For simplicity, I've just used a flag in 'struct pci_epc' to track the core_init and call the callback during bind(). But the series has grown big, so I decided to split it into two. One to address the DBI access issue and also remove the 'core_init_notifier' flag and another one to make EPF drivers more robust to handle the host reboot scenario. - Mani
Hello, This series is the continuation of previous work by Vidya Sagar [1] to fix the issues related to accessing DBI register space before completing the core initialization in some EP platforms like Tegra194/234 and Qcom EP. Since Vidya is busy, I took over the series based on his consent (off-list discussion). NOTE ==== Based on the comments received in v7 [2], I've heavily modified the series to fix several other issues reported by Bjorn and Niklas. One noticeable change is getting rid of the 'core_init_notifer' flag added to differentiate between glue drivers requiring refclk from host and drivers getting refclk locally. By getting rid of this flag, now both the DWC EP driver and the EPF drivers can use a single flow and need not distinguish between the glue drivers. We can also get rid of the 'link_up_notifier' flag in the future by following the same convention. Testing ======= I've tested the series on Qcom SM8450 based dev board that depends on refclk from host with EPF_MHI driver. It'd be good to test this series on platforms that generate refclk locally and also with EPF_TEST driver. - Mani [1] https://lore.kernel.org/linux-pci/20221013175712.7539-1-vidyas@nvidia.com/ [2] https://lore.kernel.org/linux-pci/20231120084014.108274-1-manivannan.sadhasivam@linaro.org/ Changes in v9: - Incorporated changes for missing drivers (Niklas) - Reworded the dw_pcie_ep_cleanup() API kdoc (Niklas) - Reworded the description of patch 6/10 (Frank) - Collected reviews - Link to v8: https://lore.kernel.org/r/20240224-pci-dbi-rework-v8-0-64c7fd0cfe64@linaro.org Changes in v8: - Rebased on top of v6.8-rc1 - Removed the deinit callback from struct dw_pcie_ep_ops - Renamed dw_pcie_ep_exit() to dw_pcie_ep_deinit() - Introduced dw_pcie_ep_cleanup() API for drivers supporting PERST# - Renamed dw_pcie_ep_init_complete() to dw_pcie_ep_init_registers() - Called dw_pcie_ep_init_registers() API directly from all glue drivers - Removed "core_init_notifier" flag - Added a generic dw_pcie_ep_linkdown() API to handle LINK_DOWN event and used it in qcom driver - Added Kernel-doc comments for DWC EP APIs Changes in v7: - Rebased on top of v6.7-rc1 - Kept the current dw_pcie_ep_init_complete() API instead of renaming it to dw_pcie_ep_init_late(), since changing the name causes a slight ambiguity. - Splitted the change that moves pci_epc_init_notify() inside dw_pcie_ep_init_notify() to help bisecting and also to avoid build issue. - Added a new patch that moves pci_epc_init_notify() inside dw_pcie_ep_init_notify(). - Took over the authorship and dropped the previous Ack as the patches are heavily modified. Changes in v6: - Rebased on top of pci/next (6e2fca71e187) - removed ep_init_late() callback as it is no longer necessary For previous changelog, please refer [1]. Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- Manivannan Sadhasivam (10): PCI: dwc: ep: Remove deinit() callback from struct dw_pcie_ep_ops PCI: dwc: ep: Rename dw_pcie_ep_exit() to dw_pcie_ep_deinit() PCI: dwc: ep: Introduce dw_pcie_ep_cleanup() API for drivers supporting PERST# PCI: dwc: ep: Fix DBI access failure for drivers requiring refclk from host PCI: dwc: ep: Rename dw_pcie_ep_init_complete() to dw_pcie_ep_init_registers() PCI: dwc: ep: Call dw_pcie_ep_init_registers() API directly from all glue drivers PCI: dwc: ep: Remove "core_init_notifier" flag PCI: dwc: ep: Add a generic dw_pcie_ep_linkdown() API to handle LINK_DOWN event PCI: qcom-ep: Use the generic dw_pcie_ep_linkdown() API to handle LINK_DOWN event PCI: dwc: ep: Add Kernel-doc comments for APIs drivers/pci/controller/dwc/pci-dra7xx.c | 9 + drivers/pci/controller/dwc/pci-imx6.c | 10 + drivers/pci/controller/dwc/pci-keystone.c | 11 + drivers/pci/controller/dwc/pci-layerscape-ep.c | 9 + drivers/pci/controller/dwc/pcie-artpec6.c | 15 +- drivers/pci/controller/dwc/pcie-designware-ep.c | 309 +++++++++++++++------- drivers/pci/controller/dwc/pcie-designware-plat.c | 11 + drivers/pci/controller/dwc/pcie-designware.h | 19 +- drivers/pci/controller/dwc/pcie-keembay.c | 18 +- drivers/pci/controller/dwc/pcie-qcom-ep.c | 6 +- drivers/pci/controller/dwc/pcie-rcar-gen4.c | 28 +- drivers/pci/controller/dwc/pcie-tegra194.c | 5 +- drivers/pci/controller/dwc/pcie-uniphier-ep.c | 15 +- drivers/pci/endpoint/functions/pci-epf-test.c | 18 +- include/linux/pci-epc.h | 3 - 15 files changed, 354 insertions(+), 132 deletions(-) --- base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d change-id: 20240224-pci-dbi-rework-b2e99a62930c Best regards,