Re: [PATCH v9 07/10] PCI: dwc: ep: Remove "core_init_notifier" flag
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
--
மணிவண்ணன் சதாசிவம்
Re: [PATCH v9 07/10] PCI: dwc: ep: Remove "core_init_notifier" flag
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
Re: [PATCH v9 07/10] PCI: dwc: ep: Remove "core_init_notifier" flag
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
> > Signed-off-by: Manivannan Sadhasivam
> > ---
> > 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
--
மணிவண்ணன் சதாசிவம்
Re: [PATCH v9 07/10] PCI: dwc: ep: Remove "core_init_notifier" flag
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
> Signed-off-by: Manivannan Sadhasivam
> ---
> 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
Re: [PATCH v9 07/10] PCI: dwc: ep: Remove "core_init_notifier" flag
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 > > > > Signed-off-by: Manivannan Sadhasivam > > > > --- > > > > > > 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 -- மணிவண்ணன் சதாசிவம்
Re: [PATCH v9 07/10] PCI: dwc: ep: Remove "core_init_notifier" flag
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 > > > Signed-off-by: Manivannan Sadhasivam > > > --- > > > > 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
Re: [PATCH v9 07/10] PCI: dwc: ep: Remove "core_init_notifier" flag
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 > > Signed-off-by: Manivannan Sadhasivam > > --- > > 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 Thanks! - Mani -- மணிவண்ணன் சதாசிவம்
Re: [PATCH v9 07/10] PCI: dwc: ep: Remove "core_init_notifier" flag
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 > Signed-off-by: Manivannan Sadhasivam > --- 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
