[PATCH 00/11] PCI: endpoint: Make host reboot handling more robust

[PATCH 00/11] PCI: endpoint: Make host reboot handling more robust

[Manivannan Sadhasivam]

Hello,

This is the follow up series of [1], to improve the handling of host reboot in
the endpoint subsystem. This involves refining the PERST# and Link Down event
handling in both the controller and function drivers.

Testing
=======

This series is tested on Qcom SM8450 based development board with both MHI_EPF
and EPF_TEST function drivers.

Dependency
==========

This series depends on [1].

- Mani

[1] https://lore.kernel.org/linux-pci/20240314-pci-dbi-rework-v10-0-14a45c5a938e@linaro.org/

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
Manivannan Sadhasivam (11):
      PCI: qcom-ep: Disable resources unconditionally during PERST# assert
      PCI: endpoint: Decouple EPC and PCIe bus specific events
      PCI: endpoint: Rename core_init() callback in 'struct pci_epc_event_ops' to init()
      PCI: epf-test: Refactor pci_epf_test_unbind() function
      PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback
      PCI: endpoint: Introduce EPC 'deinit' event and notify the EPF drivers
      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: epf-test: Handle Link Down event
      PCI: qcom-ep: Rework {start/stop}_link() callbacks implementation
      PCI: tegra194: Rework {start/stop}_link() callbacks implementation

 drivers/pci/controller/dwc/pcie-designware-ep.c |  93 +++++++++++++-------
 drivers/pci/controller/dwc/pcie-designware.h    |   5 ++
 drivers/pci/controller/dwc/pcie-qcom-ep.c       |  30 +++----
 drivers/pci/controller/dwc/pcie-tegra194.c      |  25 +++---
 drivers/pci/endpoint/functions/pci-epf-mhi.c    |  47 ++++++++---
 drivers/pci/endpoint/functions/pci-epf-test.c   | 108 +++++++++++++++++-------
 drivers/pci/endpoint/pci-epc-core.c             |  53 +++++++++---
 include/linux/pci-epc.h                         |   1 +
 include/linux/pci-epf.h                         |  27 ++++--
 9 files changed, 265 insertions(+), 124 deletions(-)
---
base-commit: d65c75fc384ada26392a975c351689ec4e069c50
change-id: 20240314-pci-epf-rework-a6e65b103a79

Best regards,
-- 
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

[PATCH 01/11] PCI: qcom-ep: Disable resources unconditionally during PERST# assert

[Manivannan Sadhasivam]

All EP specific resources are enabled during PERST# deassert. As a counter
operation, all resources should be disabled during PERST# assert. There is
no point in skipping that if the link was not enabled.

This will also result in enablement of the resources twice if PERST# got
deasserted again. So remove the check from qcom_pcie_perst_assert() and
disable all the resources unconditionally.

Fixes: f55fee56a631 ("PCI: qcom-ep: Add Qualcomm PCIe Endpoint controller driver")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom-ep.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index 2fb8c15e7a91..50b1635e3cbb 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -500,12 +500,6 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
 static void qcom_pcie_perst_assert(struct dw_pcie *pci)
 {
 	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
-	struct device *dev = pci->dev;
-
-	if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_DISABLED) {
-		dev_dbg(dev, "Link is already disabled\n");
-		return;
-	}
 
 	dw_pcie_ep_cleanup(&pci->ep);
 	qcom_pcie_disable_resources(pcie_ep);

-- 
2.25.1

Re: [PATCH 01/11] PCI: qcom-ep: Disable resources unconditionally during PERST# assert

[Niklas Cassel]

On Thu, Mar 14, 2024 at 08:53:40PM +0530, Manivannan Sadhasivam wrote:
> All EP specific resources are enabled during PERST# deassert. As a counter
> operation, all resources should be disabled during PERST# assert. There is
> no point in skipping that if the link was not enabled.
> 
> This will also result in enablement of the resources twice if PERST# got
> deasserted again. So remove the check from qcom_pcie_perst_assert() and
> disable all the resources unconditionally.
> 
> Fixes: f55fee56a631 ("PCI: qcom-ep: Add Qualcomm PCIe Endpoint controller driver")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom-ep.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index 2fb8c15e7a91..50b1635e3cbb 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -500,12 +500,6 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
>  static void qcom_pcie_perst_assert(struct dw_pcie *pci)
>  {
>  	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> -	struct device *dev = pci->dev;
> -
> -	if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_DISABLED) {
> -		dev_dbg(dev, "Link is already disabled\n");
> -		return;
> -	}
>  
>  	dw_pcie_ep_cleanup(&pci->ep);
>  	qcom_pcie_disable_resources(pcie_ep);

Are you really sure that this is safe?

I think I remember seeing some splat in dmesg if some clks, or maybe it
was regulators, got disabled while already being disabled.

Perhaps you could test it by simply calling:
qcom_pcie_disable_resources();
twice here, and see if you see and splat in dmesg.


Kind regards,
Niklas

Re: [PATCH 01/11] PCI: qcom-ep: Disable resources unconditionally during PERST# assert

[Manivannan Sadhasivam]

On Fri, Mar 22, 2024 at 05:08:22PM +0100, Niklas Cassel wrote:
> On Thu, Mar 14, 2024 at 08:53:40PM +0530, Manivannan Sadhasivam wrote:
> > All EP specific resources are enabled during PERST# deassert. As a counter
> > operation, all resources should be disabled during PERST# assert. There is
> > no point in skipping that if the link was not enabled.
> > 
> > This will also result in enablement of the resources twice if PERST# got
> > deasserted again. So remove the check from qcom_pcie_perst_assert() and
> > disable all the resources unconditionally.
> > 
> > Fixes: f55fee56a631 ("PCI: qcom-ep: Add Qualcomm PCIe Endpoint controller driver")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom-ep.c | 6 ------
> >  1 file changed, 6 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > index 2fb8c15e7a91..50b1635e3cbb 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > @@ -500,12 +500,6 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
> >  static void qcom_pcie_perst_assert(struct dw_pcie *pci)
> >  {
> >  	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> > -	struct device *dev = pci->dev;
> > -
> > -	if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_DISABLED) {
> > -		dev_dbg(dev, "Link is already disabled\n");
> > -		return;
> > -	}
> >  
> >  	dw_pcie_ep_cleanup(&pci->ep);
> >  	qcom_pcie_disable_resources(pcie_ep);
> 
> Are you really sure that this is safe?
> 
> I think I remember seeing some splat in dmesg if some clks, or maybe it
> was regulators, got disabled while already being disabled.
> 
> Perhaps you could test it by simply calling:
> qcom_pcie_disable_resources();
> twice here, and see if you see and splat in dmesg.
> 

Calling the disable_resources() function twice will definitely result in the
splat. But here PERST# is level triggered, so I don't see how the EP can see
assert twice.

Am I missing something?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 01/11] PCI: qcom-ep: Disable resources unconditionally during PERST# assert

[Niklas Cassel]

On Tue, Mar 26, 2024 at 01:14:29PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Mar 22, 2024 at 05:08:22PM +0100, Niklas Cassel wrote:
> > On Thu, Mar 14, 2024 at 08:53:40PM +0530, Manivannan Sadhasivam wrote:
> > > All EP specific resources are enabled during PERST# deassert. As a counter
> > > operation, all resources should be disabled during PERST# assert. There is
> > > no point in skipping that if the link was not enabled.
> > > 
> > > This will also result in enablement of the resources twice if PERST# got
> > > deasserted again. So remove the check from qcom_pcie_perst_assert() and
> > > disable all the resources unconditionally.
> > > 
> > > Fixes: f55fee56a631 ("PCI: qcom-ep: Add Qualcomm PCIe Endpoint controller driver")
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-qcom-ep.c | 6 ------
> > >  1 file changed, 6 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > index 2fb8c15e7a91..50b1635e3cbb 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > @@ -500,12 +500,6 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
> > >  static void qcom_pcie_perst_assert(struct dw_pcie *pci)
> > >  {
> > >  	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> > > -	struct device *dev = pci->dev;
> > > -
> > > -	if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_DISABLED) {
> > > -		dev_dbg(dev, "Link is already disabled\n");
> > > -		return;
> > > -	}
> > >  
> > >  	dw_pcie_ep_cleanup(&pci->ep);
> > >  	qcom_pcie_disable_resources(pcie_ep);
> > 
> > Are you really sure that this is safe?
> > 
> > I think I remember seeing some splat in dmesg if some clks, or maybe it
> > was regulators, got disabled while already being disabled.
> > 
> > Perhaps you could test it by simply calling:
> > qcom_pcie_disable_resources();
> > twice here, and see if you see and splat in dmesg.
> > 
> 
> Calling the disable_resources() function twice will definitely result in the
> splat. But here PERST# is level triggered, so I don't see how the EP can see
> assert twice.
> 
> Am I missing something?

I think I remember now, I was developing a driver using a .core_init_notifier,
but I followed the pcie-tegra model, which does not enable any resources in
probe() (it only gets them), so I got the splat because when PERST got
asserted, resources would get disabled even though they were already disabled.

pcie-qcom:
-gets resources in .probe()
-enables resources in .probe()
-sets no default state in .probe()

pcie-tegra:
-gets resources in .probe()
-enables resources in perst_deassert()
-sets default state to EP_STATE_DISABLED in probe()

So pcie-qcom does not seem to be following the same pattern like pcie-tegra,
because pcie-qcom actually does enable resources for the first time in
probe(), while tegra will enable resources for the first time in
perst_deassert().

Sorry for the noise.


Kind regards,
Niklas

Re: [PATCH 01/11] PCI: qcom-ep: Disable resources unconditionally during PERST# assert

[Manivannan Sadhasivam]

On Tue, Mar 26, 2024 at 11:24:18AM +0100, Niklas Cassel wrote:
> On Tue, Mar 26, 2024 at 01:14:29PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Mar 22, 2024 at 05:08:22PM +0100, Niklas Cassel wrote:
> > > On Thu, Mar 14, 2024 at 08:53:40PM +0530, Manivannan Sadhasivam wrote:
> > > > All EP specific resources are enabled during PERST# deassert. As a counter
> > > > operation, all resources should be disabled during PERST# assert. There is
> > > > no point in skipping that if the link was not enabled.
> > > > 
> > > > This will also result in enablement of the resources twice if PERST# got
> > > > deasserted again. So remove the check from qcom_pcie_perst_assert() and
> > > > disable all the resources unconditionally.
> > > > 
> > > > Fixes: f55fee56a631 ("PCI: qcom-ep: Add Qualcomm PCIe Endpoint controller driver")
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > >  drivers/pci/controller/dwc/pcie-qcom-ep.c | 6 ------
> > > >  1 file changed, 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > > index 2fb8c15e7a91..50b1635e3cbb 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > > @@ -500,12 +500,6 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
> > > >  static void qcom_pcie_perst_assert(struct dw_pcie *pci)
> > > >  {
> > > >  	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> > > > -	struct device *dev = pci->dev;
> > > > -
> > > > -	if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_DISABLED) {
> > > > -		dev_dbg(dev, "Link is already disabled\n");
> > > > -		return;
> > > > -	}
> > > >  
> > > >  	dw_pcie_ep_cleanup(&pci->ep);
> > > >  	qcom_pcie_disable_resources(pcie_ep);
> > > 
> > > Are you really sure that this is safe?
> > > 
> > > I think I remember seeing some splat in dmesg if some clks, or maybe it
> > > was regulators, got disabled while already being disabled.
> > > 
> > > Perhaps you could test it by simply calling:
> > > qcom_pcie_disable_resources();
> > > twice here, and see if you see and splat in dmesg.
> > > 
> > 
> > Calling the disable_resources() function twice will definitely result in the
> > splat. But here PERST# is level triggered, so I don't see how the EP can see
> > assert twice.
> > 
> > Am I missing something?
> 
> I think I remember now, I was developing a driver using a .core_init_notifier,
> but I followed the pcie-tegra model, which does not enable any resources in
> probe() (it only gets them), so I got the splat because when PERST got
> asserted, resources would get disabled even though they were already disabled.
> 
> pcie-qcom:
> -gets resources in .probe()
> -enables resources in .probe()
> -sets no default state in .probe()
> 
> pcie-tegra:
> -gets resources in .probe()
> -enables resources in perst_deassert()
> -sets default state to EP_STATE_DISABLED in probe()
> 
> So pcie-qcom does not seem to be following the same pattern like pcie-tegra,
> because pcie-qcom actually does enable resources for the first time in
> probe(), while tegra will enable resources for the first time in
> perst_deassert().
> 
> Sorry for the noise.
> 

I was planning to drop enable_resources() from Qcom driver once the DBI rework
series gets merged. Because, the resource enablement during probe is currently
done to avoid the crash that is bound to happen if registers are accessed during
probe.

But what your observation reveals is that it is possible to get PERST# assert
during the EP boot up itself which I was not accounting for. I always assumed
that the EP will receive PERST# deassert first. If that is not the case, then
this patch needs to be dropped.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 01/11] PCI: qcom-ep: Disable resources unconditionally during PERST# assert

[Niklas Cassel]

On Tue, Mar 26, 2024 at 04:40:21PM +0530, Manivannan Sadhasivam wrote:
> 
> I was planning to drop enable_resources() from Qcom driver once the DBI rework
> series gets merged. Because, the resource enablement during probe is currently
> done to avoid the crash that is bound to happen if registers are accessed during
> probe.
> 
> But what your observation reveals is that it is possible to get PERST# assert
> during the EP boot up itself which I was not accounting for. I always assumed
> that the EP will receive PERST# deassert first. If that is not the case, then
> this patch needs to be dropped.

From what I saw when having debug prints from my old email to you:
https://lore.kernel.org/linux-pci/Zalu%2F%2FdNi5BhZlBU@x1-carbon/


## RC side:
# reboot

## EP side
[  845.606810] pci: PERST asserted by host!
[  852.483985] pci: PERST de-asserted by host!
[  852.503041] pci: PERST asserted by host!
[  852.522375] pci: link up! (LTSSM_STATUS: 0x230011)
[  852.610318] pci: PERST de-asserted by host!



So in my case, I assume that the RC asserts PERST during a SoC reset.

This is obviously from the RC driver asserting PERST + sleep 100 ms +
PERST deassert:
[  852.503041] pci: PERST asserted by host!
[  852.610318] pci: PERST de-asserted by host!

The two before that:
[  852.483985] pci: PERST de-asserted by host!
[  852.503041] pci: PERST asserted by host!

appears to be because the RC I am using, incorrectly sets the PERST gpio as
ACTIVE HIGH:
https://github.com/torvalds/linux/blob/v6.9-rc1/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts#L300

Well, at least they are bug compatible and sets the output to:
https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L170-L184

0 and the 1, which, since the DT binding is incorrect, will actually
do the right thing and assert and the deassert PERST.

The problem seems to be that the initial flags:
https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L242-L243
is: GPIOD_OUT_HIGH

which explains why I get the extra:
[  852.483985] pci: PERST de-asserted by host!
before
[  852.503041] pci: PERST asserted by host!

with basically no time between them..


I guess I should send a patch to set the initial value to
GPIOD_OUT_LOW, so that the RC driver does not trigger a
"spurious" PERST deassertion when requesting the IRQ.


So I think this patch should be fine if the RC is not buggy,
but as we can see, in reality there are at least one platform
in mainline that does manage to get this wrong.


Kind regards,
Niklas

Re: [PATCH 01/11] PCI: qcom-ep: Disable resources unconditionally during PERST# assert

[Manivannan Sadhasivam]

On Tue, Mar 26, 2024 at 02:47:30PM +0100, Niklas Cassel wrote:
> On Tue, Mar 26, 2024 at 04:40:21PM +0530, Manivannan Sadhasivam wrote:
> > 
> > I was planning to drop enable_resources() from Qcom driver once the DBI rework
> > series gets merged. Because, the resource enablement during probe is currently
> > done to avoid the crash that is bound to happen if registers are accessed during
> > probe.
> > 
> > But what your observation reveals is that it is possible to get PERST# assert
> > during the EP boot up itself which I was not accounting for. I always assumed
> > that the EP will receive PERST# deassert first. If that is not the case, then
> > this patch needs to be dropped.
> 
> From what I saw when having debug prints from my old email to you:
> https://lore.kernel.org/linux-pci/Zalu%2F%2FdNi5BhZlBU@x1-carbon/
> 
> 
> ## RC side:
> # reboot
> 
> ## EP side
> [  845.606810] pci: PERST asserted by host!
> [  852.483985] pci: PERST de-asserted by host!
> [  852.503041] pci: PERST asserted by host!
> [  852.522375] pci: link up! (LTSSM_STATUS: 0x230011)
> [  852.610318] pci: PERST de-asserted by host!
> 
> 
> 
> So in my case, I assume that the RC asserts PERST during a SoC reset.
> 
> This is obviously from the RC driver asserting PERST + sleep 100 ms +
> PERST deassert:
> [  852.503041] pci: PERST asserted by host!
> [  852.610318] pci: PERST de-asserted by host!
> 
> The two before that:
> [  852.483985] pci: PERST de-asserted by host!
> [  852.503041] pci: PERST asserted by host!
> 
> appears to be because the RC I am using, incorrectly sets the PERST gpio as
> ACTIVE HIGH:
> https://github.com/torvalds/linux/blob/v6.9-rc1/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts#L300
> 
> Well, at least they are bug compatible and sets the output to:
> https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L170-L184
> 
> 0 and the 1, which, since the DT binding is incorrect, will actually
> do the right thing and assert and the deassert PERST.
> 
> The problem seems to be that the initial flags:
> https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L242-L243
> is: GPIOD_OUT_HIGH
> 
> which explains why I get the extra:
> [  852.483985] pci: PERST de-asserted by host!
> before
> [  852.503041] pci: PERST asserted by host!
> 
> with basically no time between them..
> 
> 
> I guess I should send a patch to set the initial value to
> GPIOD_OUT_LOW, so that the RC driver does not trigger a
> "spurious" PERST deassertion when requesting the IRQ.
> 
> 
> So I think this patch should be fine if the RC is not buggy,
> but as we can see, in reality there are at least one platform
> in mainline that does manage to get this wrong.
> 

I've validated with x86 and Qcom RCs so far and didn't see this behavior. So I
guess I'll just keep the patch for now.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 01/11] PCI: qcom-ep: Disable resources unconditionally during PERST# assert

[Niklas Cassel]

On Thu, Mar 14, 2024 at 08:53:40PM +0530, Manivannan Sadhasivam wrote:
> All EP specific resources are enabled during PERST# deassert. As a counter
> operation, all resources should be disabled during PERST# assert. There is
> no point in skipping that if the link was not enabled.
> 
> This will also result in enablement of the resources twice if PERST# got
> deasserted again. So remove the check from qcom_pcie_perst_assert() and
> disable all the resources unconditionally.
> 
> Fixes: f55fee56a631 ("PCI: qcom-ep: Add Qualcomm PCIe Endpoint controller driver")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom-ep.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index 2fb8c15e7a91..50b1635e3cbb 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -500,12 +500,6 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
>  static void qcom_pcie_perst_assert(struct dw_pcie *pci)
>  {
>  	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> -	struct device *dev = pci->dev;
> -
> -	if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_DISABLED) {
> -		dev_dbg(dev, "Link is already disabled\n");
> -		return;
> -	}
>  
>  	dw_pcie_ep_cleanup(&pci->ep);
>  	qcom_pcie_disable_resources(pcie_ep);
> 
> -- 
> 2.25.1
> 

It would be nice if you could do a similar change to pcie-tegra,
so that the drivers are in sync, as that is not strictly related:

Reviewed-by: Niklas Cassel <cassel@kernel.org>

[PATCH 02/11] PCI: endpoint: Decouple EPC and PCIe bus specific events

[Manivannan Sadhasivam]

Currently, 'struct pci_epc_event_ops' has a bunch of events that are sent
from the EPC driver to EPF driver. But those events are a mix of EPC
specific events like core_init and PCIe bus specific events like LINK_UP,
LINK_DOWN, BME etc...

Let's decouple them to respective structs (pci_epc_event_ops,
pci_epc_bus_event_ops) to make the separation clear.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/endpoint/functions/pci-epf-mhi.c  |  8 ++++++--
 drivers/pci/endpoint/functions/pci-epf-test.c |  8 ++++++--
 drivers/pci/endpoint/pci-epc-core.c           | 20 ++++++++++----------
 include/linux/pci-epf.h                       | 23 ++++++++++++++++-------
 4 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
index 1c3e4ea76bd2..e5d67aec7574 100644
--- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
+++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
@@ -880,8 +880,11 @@ static void pci_epf_mhi_unbind(struct pci_epf *epf)
 	pci_epc_clear_bar(epc, epf->func_no, epf->vfunc_no, epf_bar);
 }
 
-static const struct pci_epc_event_ops pci_epf_mhi_event_ops = {
+static const struct pci_epc_event_ops pci_epf_mhi_epc_event_ops = {
 	.core_init = pci_epf_mhi_core_init,
+};
+
+static const struct pci_epc_bus_event_ops pci_epf_mhi_bus_event_ops = {
 	.link_up = pci_epf_mhi_link_up,
 	.link_down = pci_epf_mhi_link_down,
 	.bme = pci_epf_mhi_bme,
@@ -903,7 +906,8 @@ static int pci_epf_mhi_probe(struct pci_epf *epf,
 	epf_mhi->info = info;
 	epf_mhi->epf = epf;
 
-	epf->event_ops = &pci_epf_mhi_event_ops;
+	epf->epc_event_ops = &pci_epf_mhi_epc_event_ops;
+	epf->bus_event_ops = &pci_epf_mhi_bus_event_ops;
 
 	mutex_init(&epf_mhi->lock);
 
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index fc0282b0d626..751dab5799d5 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -813,8 +813,11 @@ static int pci_epf_test_link_up(struct pci_epf *epf)
 	return 0;
 }
 
-static const struct pci_epc_event_ops pci_epf_test_event_ops = {
+static const struct pci_epc_event_ops pci_epf_test_epc_event_ops = {
 	.core_init = pci_epf_test_core_init,
+};
+
+static const struct pci_epc_bus_event_ops pci_epf_test_bus_event_ops = {
 	.link_up = pci_epf_test_link_up,
 };
 
@@ -959,7 +962,8 @@ static int pci_epf_test_probe(struct pci_epf *epf,
 
 	INIT_DELAYED_WORK(&epf_test->cmd_handler, pci_epf_test_cmd_handler);
 
-	epf->event_ops = &pci_epf_test_event_ops;
+	epf->epc_event_ops = &pci_epf_test_epc_event_ops;
+	epf->bus_event_ops = &pci_epf_test_bus_event_ops;
 
 	epf_set_drvdata(epf, epf_test);
 	return 0;
diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index ba2ff037dfa6..f602f08a11a2 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -697,8 +697,8 @@ void pci_epc_linkup(struct pci_epc *epc)
 	mutex_lock(&epc->list_lock);
 	list_for_each_entry(epf, &epc->pci_epf, list) {
 		mutex_lock(&epf->lock);
-		if (epf->event_ops && epf->event_ops->link_up)
-			epf->event_ops->link_up(epf);
+		if (epf->bus_event_ops && epf->bus_event_ops->link_up)
+			epf->bus_event_ops->link_up(epf);
 		mutex_unlock(&epf->lock);
 	}
 	mutex_unlock(&epc->list_lock);
@@ -723,8 +723,8 @@ void pci_epc_linkdown(struct pci_epc *epc)
 	mutex_lock(&epc->list_lock);
 	list_for_each_entry(epf, &epc->pci_epf, list) {
 		mutex_lock(&epf->lock);
-		if (epf->event_ops && epf->event_ops->link_down)
-			epf->event_ops->link_down(epf);
+		if (epf->bus_event_ops && epf->bus_event_ops->link_down)
+			epf->bus_event_ops->link_down(epf);
 		mutex_unlock(&epf->lock);
 	}
 	mutex_unlock(&epc->list_lock);
@@ -749,8 +749,8 @@ void pci_epc_init_notify(struct pci_epc *epc)
 	mutex_lock(&epc->list_lock);
 	list_for_each_entry(epf, &epc->pci_epf, list) {
 		mutex_lock(&epf->lock);
-		if (epf->event_ops && epf->event_ops->core_init)
-			epf->event_ops->core_init(epf);
+		if (epf->epc_event_ops && epf->epc_event_ops->core_init)
+			epf->epc_event_ops->core_init(epf);
 		mutex_unlock(&epf->lock);
 	}
 	epc->init_complete = true;
@@ -772,8 +772,8 @@ void pci_epc_notify_pending_init(struct pci_epc *epc, struct pci_epf *epf)
 {
 	if (epc->init_complete) {
 		mutex_lock(&epf->lock);
-		if (epf->event_ops && epf->event_ops->core_init)
-			epf->event_ops->core_init(epf);
+		if (epf->epc_event_ops && epf->epc_event_ops->core_init)
+			epf->epc_event_ops->core_init(epf);
 		mutex_unlock(&epf->lock);
 	}
 }
@@ -797,8 +797,8 @@ void pci_epc_bme_notify(struct pci_epc *epc)
 	mutex_lock(&epc->list_lock);
 	list_for_each_entry(epf, &epc->pci_epf, list) {
 		mutex_lock(&epf->lock);
-		if (epf->event_ops && epf->event_ops->bme)
-			epf->event_ops->bme(epf);
+		if (epf->bus_event_ops && epf->bus_event_ops->bme)
+			epf->bus_event_ops->bme(epf);
 		mutex_unlock(&epf->lock);
 	}
 	mutex_unlock(&epc->list_lock);
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index 77b146e0f672..1271e1e00bbd 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -68,14 +68,21 @@ struct pci_epf_ops {
 };
 
 /**
- * struct pci_epc_event_ops - Callbacks for capturing the EPC events
- * @core_init: Callback for the EPC initialization complete event
- * @link_up: Callback for the EPC link up event
- * @link_down: Callback for the EPC link down event
- * @bme: Callback for the EPC BME (Bus Master Enable) event
+ * struct pci_epc_event_ops - Callbacks for capturing the EPC specific events
+ * @core_init: Callback for the EPC initialization event
  */
 struct pci_epc_event_ops {
 	int (*core_init)(struct pci_epf *epf);
+};
+
+/**
+ * struct pci_epc_bus_event_ops - Callbacks for capturing the PCIe bus specific
+ *                               events
+ * @link_up: Callback for the PCIe bus link up event
+ * @link_down: Callback for the PCIe bus link down event
+ * @bme: Callback for the PCIe bus BME (Bus Master Enable) event
+ */
+struct pci_epc_bus_event_ops {
 	int (*link_up)(struct pci_epf *epf);
 	int (*link_down)(struct pci_epf *epf);
 	int (*bme)(struct pci_epf *epf);
@@ -149,7 +156,8 @@ struct pci_epf_bar {
  * @is_vf: true - virtual function, false - physical function
  * @vfunction_num_map: bitmap to manage virtual function number
  * @pci_vepf: list of virtual endpoint functions associated with this function
- * @event_ops: Callbacks for capturing the EPC events
+ * @epc_event_ops: Callbacks for capturing the EPC events
+ * @bus_event_ops: Callbacks for capturing the PCIe bus events
  */
 struct pci_epf {
 	struct device		dev;
@@ -179,7 +187,8 @@ struct pci_epf {
 	unsigned int		is_vf;
 	unsigned long		vfunction_num_map;
 	struct list_head	pci_vepf;
-	const struct pci_epc_event_ops *event_ops;
+	const struct pci_epc_event_ops *epc_event_ops;
+	const struct pci_epc_bus_event_ops *bus_event_ops;
 };
 
 /**

-- 
2.25.1

Re: [PATCH 02/11] PCI: endpoint: Decouple EPC and PCIe bus specific events

[Niklas Cassel]

On Thu, Mar 14, 2024 at 08:53:41PM +0530, Manivannan Sadhasivam wrote:
> Currently, 'struct pci_epc_event_ops' has a bunch of events that are sent
> from the EPC driver to EPF driver. But those events are a mix of EPC
> specific events like core_init and PCIe bus specific events like LINK_UP,
> LINK_DOWN, BME etc...
> 
> Let's decouple them to respective structs (pci_epc_event_ops,
> pci_epc_bus_event_ops) to make the separation clear.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/endpoint/functions/pci-epf-mhi.c  |  8 ++++++--
>  drivers/pci/endpoint/functions/pci-epf-test.c |  8 ++++++--
>  drivers/pci/endpoint/pci-epc-core.c           | 20 ++++++++++----------
>  include/linux/pci-epf.h                       | 23 ++++++++++++++++-------
>  4 files changed, 38 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> index 1c3e4ea76bd2..e5d67aec7574 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> @@ -880,8 +880,11 @@ static void pci_epf_mhi_unbind(struct pci_epf *epf)
>  	pci_epc_clear_bar(epc, epf->func_no, epf->vfunc_no, epf_bar);
>  }
>  
> -static const struct pci_epc_event_ops pci_epf_mhi_event_ops = {
> +static const struct pci_epc_event_ops pci_epf_mhi_epc_event_ops = {
>  	.core_init = pci_epf_mhi_core_init,
> +};
> +
> +static const struct pci_epc_bus_event_ops pci_epf_mhi_bus_event_ops = {
>  	.link_up = pci_epf_mhi_link_up,
>  	.link_down = pci_epf_mhi_link_down,
>  	.bme = pci_epf_mhi_bme,
> @@ -903,7 +906,8 @@ static int pci_epf_mhi_probe(struct pci_epf *epf,
>  	epf_mhi->info = info;
>  	epf_mhi->epf = epf;
>  
> -	epf->event_ops = &pci_epf_mhi_event_ops;
> +	epf->epc_event_ops = &pci_epf_mhi_epc_event_ops;
> +	epf->bus_event_ops = &pci_epf_mhi_bus_event_ops;
>  
>  	mutex_init(&epf_mhi->lock);
>  
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index fc0282b0d626..751dab5799d5 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -813,8 +813,11 @@ static int pci_epf_test_link_up(struct pci_epf *epf)
>  	return 0;
>  }
>  
> -static const struct pci_epc_event_ops pci_epf_test_event_ops = {
> +static const struct pci_epc_event_ops pci_epf_test_epc_event_ops = {
>  	.core_init = pci_epf_test_core_init,
> +};
> +
> +static const struct pci_epc_bus_event_ops pci_epf_test_bus_event_ops = {
>  	.link_up = pci_epf_test_link_up,
>  };

I'm not a big fan of every EPF driver now needing two different
static const struct pci_*_event_ops.

Is really:
static const struct pci_epc_event_ops pci_epf_test_epc_event_ops = {
	.core_init = pci_epf_test_core_init,
};

static const struct pci_epc_bus_event_ops pci_epf_test_bus_event_ops = {
	.link_up = pci_epf_test_link_up,
};


Better than:
static const struct pci_epc_event_ops pci_epf_test_event_ops = {
	.core_init = pci_epf_test_core_init,
	.link_up = pci_epf_test_link_up,
}

The callbacks should have sufficiently distinct names that it is obvious
what it is happening?

Link up is that the EPC driver tells me that it is link up.
Init is that the EPF function should initialize the BARs etc.

I'm not saying that I'm totally against this, but I'm not sure that there
are so many EPC callbacks that this is needed?

How many will there be after this series?
Four? .init, .deinit, .link_up, .link_down ?

I would vote to keep all callbacks in the same struct for now,
but you are the maintainer.


>  
> @@ -959,7 +962,8 @@ static int pci_epf_test_probe(struct pci_epf *epf,
>  
>  	INIT_DELAYED_WORK(&epf_test->cmd_handler, pci_epf_test_cmd_handler);
>  
> -	epf->event_ops = &pci_epf_test_event_ops;
> +	epf->epc_event_ops = &pci_epf_test_epc_event_ops;
> +	epf->bus_event_ops = &pci_epf_test_bus_event_ops;
>  
>  	epf_set_drvdata(epf, epf_test);
>  	return 0;
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index ba2ff037dfa6..f602f08a11a2 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -697,8 +697,8 @@ void pci_epc_linkup(struct pci_epc *epc)
>  	mutex_lock(&epc->list_lock);
>  	list_for_each_entry(epf, &epc->pci_epf, list) {
>  		mutex_lock(&epf->lock);
> -		if (epf->event_ops && epf->event_ops->link_up)
> -			epf->event_ops->link_up(epf);
> +		if (epf->bus_event_ops && epf->bus_event_ops->link_up)
> +			epf->bus_event_ops->link_up(epf);
>  		mutex_unlock(&epf->lock);
>  	}
>  	mutex_unlock(&epc->list_lock);
> @@ -723,8 +723,8 @@ void pci_epc_linkdown(struct pci_epc *epc)
>  	mutex_lock(&epc->list_lock);
>  	list_for_each_entry(epf, &epc->pci_epf, list) {
>  		mutex_lock(&epf->lock);
> -		if (epf->event_ops && epf->event_ops->link_down)
> -			epf->event_ops->link_down(epf);
> +		if (epf->bus_event_ops && epf->bus_event_ops->link_down)
> +			epf->bus_event_ops->link_down(epf);
>  		mutex_unlock(&epf->lock);
>  	}
>  	mutex_unlock(&epc->list_lock);
> @@ -749,8 +749,8 @@ void pci_epc_init_notify(struct pci_epc *epc)
>  	mutex_lock(&epc->list_lock);
>  	list_for_each_entry(epf, &epc->pci_epf, list) {
>  		mutex_lock(&epf->lock);
> -		if (epf->event_ops && epf->event_ops->core_init)
> -			epf->event_ops->core_init(epf);
> +		if (epf->epc_event_ops && epf->epc_event_ops->core_init)
> +			epf->epc_event_ops->core_init(epf);
>  		mutex_unlock(&epf->lock);
>  	}
>  	epc->init_complete = true;
> @@ -772,8 +772,8 @@ void pci_epc_notify_pending_init(struct pci_epc *epc, struct pci_epf *epf)
>  {
>  	if (epc->init_complete) {
>  		mutex_lock(&epf->lock);
> -		if (epf->event_ops && epf->event_ops->core_init)
> -			epf->event_ops->core_init(epf);
> +		if (epf->epc_event_ops && epf->epc_event_ops->core_init)
> +			epf->epc_event_ops->core_init(epf);
>  		mutex_unlock(&epf->lock);
>  	}
>  }
> @@ -797,8 +797,8 @@ void pci_epc_bme_notify(struct pci_epc *epc)
>  	mutex_lock(&epc->list_lock);
>  	list_for_each_entry(epf, &epc->pci_epf, list) {
>  		mutex_lock(&epf->lock);
> -		if (epf->event_ops && epf->event_ops->bme)
> -			epf->event_ops->bme(epf);
> +		if (epf->bus_event_ops && epf->bus_event_ops->bme)
> +			epf->bus_event_ops->bme(epf);
>  		mutex_unlock(&epf->lock);
>  	}
>  	mutex_unlock(&epc->list_lock);
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index 77b146e0f672..1271e1e00bbd 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -68,14 +68,21 @@ struct pci_epf_ops {
>  };
>  
>  /**
> - * struct pci_epc_event_ops - Callbacks for capturing the EPC events
> - * @core_init: Callback for the EPC initialization complete event
> - * @link_up: Callback for the EPC link up event
> - * @link_down: Callback for the EPC link down event
> - * @bme: Callback for the EPC BME (Bus Master Enable) event
> + * struct pci_epc_event_ops - Callbacks for capturing the EPC specific events
> + * @core_init: Callback for the EPC initialization event
>   */
>  struct pci_epc_event_ops {
>  	int (*core_init)(struct pci_epf *epf);
> +};
> +
> +/**
> + * struct pci_epc_bus_event_ops - Callbacks for capturing the PCIe bus specific
> + *                               events
> + * @link_up: Callback for the PCIe bus link up event
> + * @link_down: Callback for the PCIe bus link down event
> + * @bme: Callback for the PCIe bus BME (Bus Master Enable) event
> + */
> +struct pci_epc_bus_event_ops {
>  	int (*link_up)(struct pci_epf *epf);
>  	int (*link_down)(struct pci_epf *epf);
>  	int (*bme)(struct pci_epf *epf);
> @@ -149,7 +156,8 @@ struct pci_epf_bar {
>   * @is_vf: true - virtual function, false - physical function
>   * @vfunction_num_map: bitmap to manage virtual function number
>   * @pci_vepf: list of virtual endpoint functions associated with this function
> - * @event_ops: Callbacks for capturing the EPC events
> + * @epc_event_ops: Callbacks for capturing the EPC events
> + * @bus_event_ops: Callbacks for capturing the PCIe bus events
>   */
>  struct pci_epf {
>  	struct device		dev;
> @@ -179,7 +187,8 @@ struct pci_epf {
>  	unsigned int		is_vf;
>  	unsigned long		vfunction_num_map;
>  	struct list_head	pci_vepf;
> -	const struct pci_epc_event_ops *event_ops;
> +	const struct pci_epc_event_ops *epc_event_ops;
> +	const struct pci_epc_bus_event_ops *bus_event_ops;
>  };
>  
>  /**
> 
> -- 
> 2.25.1
> 

Re: [PATCH 02/11] PCI: endpoint: Decouple EPC and PCIe bus specific events

[Manivannan Sadhasivam]

On Fri, Mar 22, 2024 at 05:08:36PM +0100, Niklas Cassel wrote:
> On Thu, Mar 14, 2024 at 08:53:41PM +0530, Manivannan Sadhasivam wrote:
> > Currently, 'struct pci_epc_event_ops' has a bunch of events that are sent
> > from the EPC driver to EPF driver. But those events are a mix of EPC
> > specific events like core_init and PCIe bus specific events like LINK_UP,
> > LINK_DOWN, BME etc...
> > 
> > Let's decouple them to respective structs (pci_epc_event_ops,
> > pci_epc_bus_event_ops) to make the separation clear.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-mhi.c  |  8 ++++++--
> >  drivers/pci/endpoint/functions/pci-epf-test.c |  8 ++++++--
> >  drivers/pci/endpoint/pci-epc-core.c           | 20 ++++++++++----------
> >  include/linux/pci-epf.h                       | 23 ++++++++++++++++-------
> >  4 files changed, 38 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > index 1c3e4ea76bd2..e5d67aec7574 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > @@ -880,8 +880,11 @@ static void pci_epf_mhi_unbind(struct pci_epf *epf)
> >  	pci_epc_clear_bar(epc, epf->func_no, epf->vfunc_no, epf_bar);
> >  }
> >  
> > -static const struct pci_epc_event_ops pci_epf_mhi_event_ops = {
> > +static const struct pci_epc_event_ops pci_epf_mhi_epc_event_ops = {
> >  	.core_init = pci_epf_mhi_core_init,
> > +};
> > +
> > +static const struct pci_epc_bus_event_ops pci_epf_mhi_bus_event_ops = {
> >  	.link_up = pci_epf_mhi_link_up,
> >  	.link_down = pci_epf_mhi_link_down,
> >  	.bme = pci_epf_mhi_bme,
> > @@ -903,7 +906,8 @@ static int pci_epf_mhi_probe(struct pci_epf *epf,
> >  	epf_mhi->info = info;
> >  	epf_mhi->epf = epf;
> >  
> > -	epf->event_ops = &pci_epf_mhi_event_ops;
> > +	epf->epc_event_ops = &pci_epf_mhi_epc_event_ops;
> > +	epf->bus_event_ops = &pci_epf_mhi_bus_event_ops;
> >  
> >  	mutex_init(&epf_mhi->lock);
> >  
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index fc0282b0d626..751dab5799d5 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -813,8 +813,11 @@ static int pci_epf_test_link_up(struct pci_epf *epf)
> >  	return 0;
> >  }
> >  
> > -static const struct pci_epc_event_ops pci_epf_test_event_ops = {
> > +static const struct pci_epc_event_ops pci_epf_test_epc_event_ops = {
> >  	.core_init = pci_epf_test_core_init,
> > +};
> > +
> > +static const struct pci_epc_bus_event_ops pci_epf_test_bus_event_ops = {
> >  	.link_up = pci_epf_test_link_up,
> >  };
> 
> I'm not a big fan of every EPF driver now needing two different
> static const struct pci_*_event_ops.
> 
> Is really:
> static const struct pci_epc_event_ops pci_epf_test_epc_event_ops = {
> 	.core_init = pci_epf_test_core_init,
> };
> 
> static const struct pci_epc_bus_event_ops pci_epf_test_bus_event_ops = {
> 	.link_up = pci_epf_test_link_up,
> };
> 
> 
> Better than:
> static const struct pci_epc_event_ops pci_epf_test_event_ops = {
> 	.core_init = pci_epf_test_core_init,
> 	.link_up = pci_epf_test_link_up,
> }
> 
> The callbacks should have sufficiently distinct names that it is obvious
> what it is happening?
> 
> Link up is that the EPC driver tells me that it is link up.
> Init is that the EPF function should initialize the BARs etc.
> 
> I'm not saying that I'm totally against this, but I'm not sure that there
> are so many EPC callbacks that this is needed?
> 

The issue I'm seeing is that these callbacks are serving different purposes. One
is purely EPC specific and another is PCIe Link specific. So mixing them in a
single struct doesn't look good IMO.

And I agree that we will be left with 2 structs, but at least I can see that it
gives a clear representation of the purposes of the callbacks.

- Mani

> How many will there be after this series?
> Four? .init, .deinit, .link_up, .link_down ?
> 
> I would vote to keep all callbacks in the same struct for now,
> but you are the maintainer.
> 
> 
> >  
> > @@ -959,7 +962,8 @@ static int pci_epf_test_probe(struct pci_epf *epf,
> >  
> >  	INIT_DELAYED_WORK(&epf_test->cmd_handler, pci_epf_test_cmd_handler);
> >  
> > -	epf->event_ops = &pci_epf_test_event_ops;
> > +	epf->epc_event_ops = &pci_epf_test_epc_event_ops;
> > +	epf->bus_event_ops = &pci_epf_test_bus_event_ops;
> >  
> >  	epf_set_drvdata(epf, epf_test);
> >  	return 0;
> > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> > index ba2ff037dfa6..f602f08a11a2 100644
> > --- a/drivers/pci/endpoint/pci-epc-core.c
> > +++ b/drivers/pci/endpoint/pci-epc-core.c
> > @@ -697,8 +697,8 @@ void pci_epc_linkup(struct pci_epc *epc)
> >  	mutex_lock(&epc->list_lock);
> >  	list_for_each_entry(epf, &epc->pci_epf, list) {
> >  		mutex_lock(&epf->lock);
> > -		if (epf->event_ops && epf->event_ops->link_up)
> > -			epf->event_ops->link_up(epf);
> > +		if (epf->bus_event_ops && epf->bus_event_ops->link_up)
> > +			epf->bus_event_ops->link_up(epf);
> >  		mutex_unlock(&epf->lock);
> >  	}
> >  	mutex_unlock(&epc->list_lock);
> > @@ -723,8 +723,8 @@ void pci_epc_linkdown(struct pci_epc *epc)
> >  	mutex_lock(&epc->list_lock);
> >  	list_for_each_entry(epf, &epc->pci_epf, list) {
> >  		mutex_lock(&epf->lock);
> > -		if (epf->event_ops && epf->event_ops->link_down)
> > -			epf->event_ops->link_down(epf);
> > +		if (epf->bus_event_ops && epf->bus_event_ops->link_down)
> > +			epf->bus_event_ops->link_down(epf);
> >  		mutex_unlock(&epf->lock);
> >  	}
> >  	mutex_unlock(&epc->list_lock);
> > @@ -749,8 +749,8 @@ void pci_epc_init_notify(struct pci_epc *epc)
> >  	mutex_lock(&epc->list_lock);
> >  	list_for_each_entry(epf, &epc->pci_epf, list) {
> >  		mutex_lock(&epf->lock);
> > -		if (epf->event_ops && epf->event_ops->core_init)
> > -			epf->event_ops->core_init(epf);
> > +		if (epf->epc_event_ops && epf->epc_event_ops->core_init)
> > +			epf->epc_event_ops->core_init(epf);
> >  		mutex_unlock(&epf->lock);
> >  	}
> >  	epc->init_complete = true;
> > @@ -772,8 +772,8 @@ void pci_epc_notify_pending_init(struct pci_epc *epc, struct pci_epf *epf)
> >  {
> >  	if (epc->init_complete) {
> >  		mutex_lock(&epf->lock);
> > -		if (epf->event_ops && epf->event_ops->core_init)
> > -			epf->event_ops->core_init(epf);
> > +		if (epf->epc_event_ops && epf->epc_event_ops->core_init)
> > +			epf->epc_event_ops->core_init(epf);
> >  		mutex_unlock(&epf->lock);
> >  	}
> >  }
> > @@ -797,8 +797,8 @@ void pci_epc_bme_notify(struct pci_epc *epc)
> >  	mutex_lock(&epc->list_lock);
> >  	list_for_each_entry(epf, &epc->pci_epf, list) {
> >  		mutex_lock(&epf->lock);
> > -		if (epf->event_ops && epf->event_ops->bme)
> > -			epf->event_ops->bme(epf);
> > +		if (epf->bus_event_ops && epf->bus_event_ops->bme)
> > +			epf->bus_event_ops->bme(epf);
> >  		mutex_unlock(&epf->lock);
> >  	}
> >  	mutex_unlock(&epc->list_lock);
> > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > index 77b146e0f672..1271e1e00bbd 100644
> > --- a/include/linux/pci-epf.h
> > +++ b/include/linux/pci-epf.h
> > @@ -68,14 +68,21 @@ struct pci_epf_ops {
> >  };
> >  
> >  /**
> > - * struct pci_epc_event_ops - Callbacks for capturing the EPC events
> > - * @core_init: Callback for the EPC initialization complete event
> > - * @link_up: Callback for the EPC link up event
> > - * @link_down: Callback for the EPC link down event
> > - * @bme: Callback for the EPC BME (Bus Master Enable) event
> > + * struct pci_epc_event_ops - Callbacks for capturing the EPC specific events
> > + * @core_init: Callback for the EPC initialization event
> >   */
> >  struct pci_epc_event_ops {
> >  	int (*core_init)(struct pci_epf *epf);
> > +};
> > +
> > +/**
> > + * struct pci_epc_bus_event_ops - Callbacks for capturing the PCIe bus specific
> > + *                               events
> > + * @link_up: Callback for the PCIe bus link up event
> > + * @link_down: Callback for the PCIe bus link down event
> > + * @bme: Callback for the PCIe bus BME (Bus Master Enable) event
> > + */
> > +struct pci_epc_bus_event_ops {
> >  	int (*link_up)(struct pci_epf *epf);
> >  	int (*link_down)(struct pci_epf *epf);
> >  	int (*bme)(struct pci_epf *epf);
> > @@ -149,7 +156,8 @@ struct pci_epf_bar {
> >   * @is_vf: true - virtual function, false - physical function
> >   * @vfunction_num_map: bitmap to manage virtual function number
> >   * @pci_vepf: list of virtual endpoint functions associated with this function
> > - * @event_ops: Callbacks for capturing the EPC events
> > + * @epc_event_ops: Callbacks for capturing the EPC events
> > + * @bus_event_ops: Callbacks for capturing the PCIe bus events
> >   */
> >  struct pci_epf {
> >  	struct device		dev;
> > @@ -179,7 +187,8 @@ struct pci_epf {
> >  	unsigned int		is_vf;
> >  	unsigned long		vfunction_num_map;
> >  	struct list_head	pci_vepf;
> > -	const struct pci_epc_event_ops *event_ops;
> > +	const struct pci_epc_event_ops *epc_event_ops;
> > +	const struct pci_epc_bus_event_ops *bus_event_ops;
> >  };
> >  
> >  /**
> > 
> > -- 
> > 2.25.1
> > 
> 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 03/11] PCI: endpoint: Rename core_init() callback in 'struct pci_epc_event_ops' to init()

[Manivannan Sadhasivam]

core_init() callback is used to notify the EPC initialization event to the
EPF drivers. The 'core' prefix was used indicate that the controller IP
core has completed initialization. But it serves no purpose as the EPF
driver will only care about the EPC initialization as a whole and there is
no real benefit to distinguish the IP core part.

So let's rename the core_init() callback in 'struct pci_epc_event_ops' to
just init() to make it more clear.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/endpoint/functions/pci-epf-mhi.c  |  4 ++--
 drivers/pci/endpoint/functions/pci-epf-test.c |  4 ++--
 drivers/pci/endpoint/pci-epc-core.c           | 16 ++++++++--------
 include/linux/pci-epf.h                       |  4 ++--
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
index e5d67aec7574..da894a9a447e 100644
--- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
+++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
@@ -700,7 +700,7 @@ static void pci_epf_mhi_dma_deinit(struct pci_epf_mhi *epf_mhi)
 	epf_mhi->dma_chan_rx = NULL;
 }
 
-static int pci_epf_mhi_core_init(struct pci_epf *epf)
+static int pci_epf_mhi_epc_init(struct pci_epf *epf)
 {
 	struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
 	const struct pci_epf_mhi_ep_info *info = epf_mhi->info;
@@ -881,7 +881,7 @@ static void pci_epf_mhi_unbind(struct pci_epf *epf)
 }
 
 static const struct pci_epc_event_ops pci_epf_mhi_epc_event_ops = {
-	.core_init = pci_epf_mhi_core_init,
+	.init = pci_epf_mhi_epc_init,
 };
 
 static const struct pci_epc_bus_event_ops pci_epf_mhi_bus_event_ops = {
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 751dab5799d5..1dae0fce8fc4 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -746,7 +746,7 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
 	return 0;
 }
 
-static int pci_epf_test_core_init(struct pci_epf *epf)
+static int pci_epf_test_epc_init(struct pci_epf *epf)
 {
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
 	struct pci_epf_header *header = epf->header;
@@ -814,7 +814,7 @@ static int pci_epf_test_link_up(struct pci_epf *epf)
 }
 
 static const struct pci_epc_event_ops pci_epf_test_epc_event_ops = {
-	.core_init = pci_epf_test_core_init,
+	.init = pci_epf_test_epc_init,
 };
 
 static const struct pci_epc_bus_event_ops pci_epf_test_bus_event_ops = {
diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index f602f08a11a2..5a522b2842e2 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -732,9 +732,9 @@ void pci_epc_linkdown(struct pci_epc *epc)
 EXPORT_SYMBOL_GPL(pci_epc_linkdown);
 
 /**
- * pci_epc_init_notify() - Notify the EPF device that EPC device's core
- *			   initialization is completed.
- * @epc: the EPC device whose core initialization is completed
+ * pci_epc_init_notify() - Notify the EPF device that EPC device initialization
+ *                         is completed.
+ * @epc: the EPC device whose initialization is completed
  *
  * Invoke to Notify the EPF device that the EPC device's initialization
  * is completed.
@@ -749,8 +749,8 @@ void pci_epc_init_notify(struct pci_epc *epc)
 	mutex_lock(&epc->list_lock);
 	list_for_each_entry(epf, &epc->pci_epf, list) {
 		mutex_lock(&epf->lock);
-		if (epf->epc_event_ops && epf->epc_event_ops->core_init)
-			epf->epc_event_ops->core_init(epf);
+		if (epf->epc_event_ops && epf->epc_event_ops->init)
+			epf->epc_event_ops->init(epf);
 		mutex_unlock(&epf->lock);
 	}
 	epc->init_complete = true;
@@ -761,7 +761,7 @@ EXPORT_SYMBOL_GPL(pci_epc_init_notify);
 /**
  * pci_epc_notify_pending_init() - Notify the pending EPC device initialization
  *                                 complete to the EPF device
- * @epc: the EPC device whose core initialization is pending to be notified
+ * @epc: the EPC device whose initialization is pending to be notified
  * @epf: the EPF device to be notified
  *
  * Invoke to notify the pending EPC device initialization complete to the EPF
@@ -772,8 +772,8 @@ void pci_epc_notify_pending_init(struct pci_epc *epc, struct pci_epf *epf)
 {
 	if (epc->init_complete) {
 		mutex_lock(&epf->lock);
-		if (epf->epc_event_ops && epf->epc_event_ops->core_init)
-			epf->epc_event_ops->core_init(epf);
+		if (epf->epc_event_ops && epf->epc_event_ops->init)
+			epf->epc_event_ops->init(epf);
 		mutex_unlock(&epf->lock);
 	}
 }
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index 1271e1e00bbd..ff8304e72f8e 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -69,10 +69,10 @@ struct pci_epf_ops {
 
 /**
  * struct pci_epc_event_ops - Callbacks for capturing the EPC specific events
- * @core_init: Callback for the EPC initialization event
+ * @init: Callback for the EPC initialization event
  */
 struct pci_epc_event_ops {
-	int (*core_init)(struct pci_epf *epf);
+	int (*init)(struct pci_epf *epf);
 };
 
 /**

-- 
2.25.1

Re: [PATCH 03/11] PCI: endpoint: Rename core_init() callback in 'struct pci_epc_event_ops' to init()

[Niklas Cassel]

On Thu, Mar 14, 2024 at 08:53:42PM +0530, Manivannan Sadhasivam wrote:
> core_init() callback is used to notify the EPC initialization event to the
> EPF drivers. The 'core' prefix was used indicate that the controller IP
> core has completed initialization. But it serves no purpose as the EPF
> driver will only care about the EPC initialization as a whole and there is
> no real benefit to distinguish the IP core part.
> 
> So let's rename the core_init() callback in 'struct pci_epc_event_ops' to
> just init() to make it more clear.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/endpoint/functions/pci-epf-mhi.c  |  4 ++--
>  drivers/pci/endpoint/functions/pci-epf-test.c |  4 ++--
>  drivers/pci/endpoint/pci-epc-core.c           | 16 ++++++++--------
>  include/linux/pci-epf.h                       |  4 ++--
>  4 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> index e5d67aec7574..da894a9a447e 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> @@ -700,7 +700,7 @@ static void pci_epf_mhi_dma_deinit(struct pci_epf_mhi *epf_mhi)
>  	epf_mhi->dma_chan_rx = NULL;
>  }
>  
> -static int pci_epf_mhi_core_init(struct pci_epf *epf)
> +static int pci_epf_mhi_epc_init(struct pci_epf *epf)
>  {
>  	struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
>  	const struct pci_epf_mhi_ep_info *info = epf_mhi->info;
> @@ -881,7 +881,7 @@ static void pci_epf_mhi_unbind(struct pci_epf *epf)
>  }
>  
>  static const struct pci_epc_event_ops pci_epf_mhi_epc_event_ops = {
> -	.core_init = pci_epf_mhi_core_init,
> +	.init = pci_epf_mhi_epc_init,
>  };
>  
>  static const struct pci_epc_bus_event_ops pci_epf_mhi_bus_event_ops = {
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 751dab5799d5..1dae0fce8fc4 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -746,7 +746,7 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
>  	return 0;
>  }
>  
> -static int pci_epf_test_core_init(struct pci_epf *epf)
> +static int pci_epf_test_epc_init(struct pci_epf *epf)

Why have _epc_ init in the name at all?

Isn't
static int pci_epf_test_init(struct pci_epf *epf)

Enough?

From my perspective, it is the EPF that is initializing
(by configuring the BARS according to it's liking),
not the EPC initializing.


>  {
>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>  	struct pci_epf_header *header = epf->header;
> @@ -814,7 +814,7 @@ static int pci_epf_test_link_up(struct pci_epf *epf)
>  }
>  
>  static const struct pci_epc_event_ops pci_epf_test_epc_event_ops = {
> -	.core_init = pci_epf_test_core_init,
> +	.init = pci_epf_test_epc_init,
>  };
>  
>  static const struct pci_epc_bus_event_ops pci_epf_test_bus_event_ops = {
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index f602f08a11a2..5a522b2842e2 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -732,9 +732,9 @@ void pci_epc_linkdown(struct pci_epc *epc)
>  EXPORT_SYMBOL_GPL(pci_epc_linkdown);
>  
>  /**
> - * pci_epc_init_notify() - Notify the EPF device that EPC device's core
> - *			   initialization is completed.
> - * @epc: the EPC device whose core initialization is completed
> + * pci_epc_init_notify() - Notify the EPF device that EPC device initialization
> + *                         is completed.
> + * @epc: the EPC device whose initialization is completed
>   *
>   * Invoke to Notify the EPF device that the EPC device's initialization
>   * is completed.
> @@ -749,8 +749,8 @@ void pci_epc_init_notify(struct pci_epc *epc)
>  	mutex_lock(&epc->list_lock);
>  	list_for_each_entry(epf, &epc->pci_epf, list) {
>  		mutex_lock(&epf->lock);
> -		if (epf->epc_event_ops && epf->epc_event_ops->core_init)
> -			epf->epc_event_ops->core_init(epf);
> +		if (epf->epc_event_ops && epf->epc_event_ops->init)
> +			epf->epc_event_ops->init(epf);
>  		mutex_unlock(&epf->lock);
>  	}
>  	epc->init_complete = true;
> @@ -761,7 +761,7 @@ EXPORT_SYMBOL_GPL(pci_epc_init_notify);
>  /**
>   * pci_epc_notify_pending_init() - Notify the pending EPC device initialization
>   *                                 complete to the EPF device
> - * @epc: the EPC device whose core initialization is pending to be notified
> + * @epc: the EPC device whose initialization is pending to be notified
>   * @epf: the EPF device to be notified
>   *
>   * Invoke to notify the pending EPC device initialization complete to the EPF
> @@ -772,8 +772,8 @@ void pci_epc_notify_pending_init(struct pci_epc *epc, struct pci_epf *epf)
>  {
>  	if (epc->init_complete) {
>  		mutex_lock(&epf->lock);
> -		if (epf->epc_event_ops && epf->epc_event_ops->core_init)
> -			epf->epc_event_ops->core_init(epf);
> +		if (epf->epc_event_ops && epf->epc_event_ops->init)
> +			epf->epc_event_ops->init(epf);
>  		mutex_unlock(&epf->lock);
>  	}
>  }
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index 1271e1e00bbd..ff8304e72f8e 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -69,10 +69,10 @@ struct pci_epf_ops {
>  
>  /**
>   * struct pci_epc_event_ops - Callbacks for capturing the EPC specific events
> - * @core_init: Callback for the EPC initialization event
> + * @init: Callback for the EPC initialization event
>   */
>  struct pci_epc_event_ops {
> -	int (*core_init)(struct pci_epf *epf);
> +	int (*init)(struct pci_epf *epf);
>  };
>  
>  /**
> 
> -- 
> 2.25.1
> 

Re: [PATCH 03/11] PCI: endpoint: Rename core_init() callback in 'struct pci_epc_event_ops' to init()

[Manivannan Sadhasivam]

On Fri, Mar 22, 2024 at 05:08:48PM +0100, Niklas Cassel wrote:
> On Thu, Mar 14, 2024 at 08:53:42PM +0530, Manivannan Sadhasivam wrote:
> > core_init() callback is used to notify the EPC initialization event to the
> > EPF drivers. The 'core' prefix was used indicate that the controller IP
> > core has completed initialization. But it serves no purpose as the EPF
> > driver will only care about the EPC initialization as a whole and there is
> > no real benefit to distinguish the IP core part.
> > 
> > So let's rename the core_init() callback in 'struct pci_epc_event_ops' to
> > just init() to make it more clear.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-mhi.c  |  4 ++--
> >  drivers/pci/endpoint/functions/pci-epf-test.c |  4 ++--
> >  drivers/pci/endpoint/pci-epc-core.c           | 16 ++++++++--------
> >  include/linux/pci-epf.h                       |  4 ++--
> >  4 files changed, 14 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > index e5d67aec7574..da894a9a447e 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > @@ -700,7 +700,7 @@ static void pci_epf_mhi_dma_deinit(struct pci_epf_mhi *epf_mhi)
> >  	epf_mhi->dma_chan_rx = NULL;
> >  }
> >  
> > -static int pci_epf_mhi_core_init(struct pci_epf *epf)
> > +static int pci_epf_mhi_epc_init(struct pci_epf *epf)
> >  {
> >  	struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
> >  	const struct pci_epf_mhi_ep_info *info = epf_mhi->info;
> > @@ -881,7 +881,7 @@ static void pci_epf_mhi_unbind(struct pci_epf *epf)
> >  }
> >  
> >  static const struct pci_epc_event_ops pci_epf_mhi_epc_event_ops = {
> > -	.core_init = pci_epf_mhi_core_init,
> > +	.init = pci_epf_mhi_epc_init,
> >  };
> >  
> >  static const struct pci_epc_bus_event_ops pci_epf_mhi_bus_event_ops = {
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index 751dab5799d5..1dae0fce8fc4 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -746,7 +746,7 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
> >  	return 0;
> >  }
> >  
> > -static int pci_epf_test_core_init(struct pci_epf *epf)
> > +static int pci_epf_test_epc_init(struct pci_epf *epf)
> 
> Why have _epc_ init in the name at all?
> 
> Isn't
> static int pci_epf_test_init(struct pci_epf *epf)
> 
> Enough?
> 
> From my perspective, it is the EPF that is initializing
> (by configuring the BARS according to it's liking),
> not the EPC initializing.
> 

Hmm, you are right. It makes sense to remove 'epc' from the naming.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 04/11] PCI: epf-test: Refactor pci_epf_test_unbind() function

[Manivannan Sadhasivam]

Move the pci_epc_clear_bar() and pci_epf_free_space() code to respective
helper functions. This allows reusing the helpers in future commits.

This also requires moving the pci_epf_test_unbind() definition below
pci_epf_test_bind() to avoid forward declaration of the above helpers.

No functional change.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 63 ++++++++++++++++++---------
 1 file changed, 42 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 1dae0fce8fc4..2fac36553633 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -686,27 +686,6 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 			   msecs_to_jiffies(1));
 }
 
-static void pci_epf_test_unbind(struct pci_epf *epf)
-{
-	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
-	struct pci_epc *epc = epf->epc;
-	struct pci_epf_bar *epf_bar;
-	int bar;
-
-	cancel_delayed_work(&epf_test->cmd_handler);
-	pci_epf_test_clean_dma_chan(epf_test);
-	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
-		epf_bar = &epf->bar[bar];
-
-		if (epf_test->reg[bar]) {
-			pci_epc_clear_bar(epc, epf->func_no, epf->vfunc_no,
-					  epf_bar);
-			pci_epf_free_space(epf, epf_test->reg[bar], bar,
-					   PRIMARY_INTERFACE);
-		}
-	}
-}
-
 static int pci_epf_test_set_bar(struct pci_epf *epf)
 {
 	int bar, add;
@@ -746,6 +725,22 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
 	return 0;
 }
 
+static void pci_epf_test_clear_bar(struct pci_epf *epf)
+{
+	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
+	struct pci_epc *epc = epf->epc;
+	struct pci_epf_bar *epf_bar;
+	int bar;
+
+	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
+		epf_bar = &epf->bar[bar];
+
+		if (epf_test->reg[bar])
+			pci_epc_clear_bar(epc, epf->func_no, epf->vfunc_no,
+					  epf_bar);
+	}
+}
+
 static int pci_epf_test_epc_init(struct pci_epf *epf)
 {
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
@@ -885,6 +880,22 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
 	return 0;
 }
 
+static void pci_epf_test_free_space(struct pci_epf *epf)
+{
+	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
+	struct pci_epf_bar *epf_bar;
+	int bar;
+
+	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
+		epf_bar = &epf->bar[bar];
+
+		if (epf_test->reg[bar]) {
+			pci_epf_free_space(epf, epf_test->reg[bar], bar,
+					   PRIMARY_INTERFACE);
+		}
+	}
+}
+
 static void pci_epf_configure_bar(struct pci_epf *epf,
 				  const struct pci_epc_features *epc_features)
 {
@@ -940,6 +951,16 @@ static int pci_epf_test_bind(struct pci_epf *epf)
 	return 0;
 }
 
+static void pci_epf_test_unbind(struct pci_epf *epf)
+{
+	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
+
+	cancel_delayed_work(&epf_test->cmd_handler);
+	pci_epf_test_clean_dma_chan(epf_test);
+	pci_epf_test_clear_bar(epf);
+	pci_epf_test_free_space(epf);
+}
+
 static const struct pci_epf_device_id pci_epf_test_ids[] = {
 	{
 		.name = "pci_epf_test",

-- 
2.25.1

Re: [PATCH 04/11] PCI: epf-test: Refactor pci_epf_test_unbind() function

[Niklas Cassel]

On Thu, Mar 14, 2024 at 08:53:43PM +0530, Manivannan Sadhasivam wrote:
> Move the pci_epc_clear_bar() and pci_epf_free_space() code to respective
> helper functions. This allows reusing the helpers in future commits.
> 
> This also requires moving the pci_epf_test_unbind() definition below
> pci_epf_test_bind() to avoid forward declaration of the above helpers.
> 
> No functional change.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 63 ++++++++++++++++++---------
>  1 file changed, 42 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 1dae0fce8fc4..2fac36553633 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -686,27 +686,6 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  			   msecs_to_jiffies(1));
>  }
>  
> -static void pci_epf_test_unbind(struct pci_epf *epf)
> -{
> -	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> -	struct pci_epc *epc = epf->epc;
> -	struct pci_epf_bar *epf_bar;
> -	int bar;
> -
> -	cancel_delayed_work(&epf_test->cmd_handler);
> -	pci_epf_test_clean_dma_chan(epf_test);
> -	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> -		epf_bar = &epf->bar[bar];
> -
> -		if (epf_test->reg[bar]) {
> -			pci_epc_clear_bar(epc, epf->func_no, epf->vfunc_no,
> -					  epf_bar);
> -			pci_epf_free_space(epf, epf_test->reg[bar], bar,
> -					   PRIMARY_INTERFACE);
> -		}
> -	}
> -}
> -
>  static int pci_epf_test_set_bar(struct pci_epf *epf)
>  {
>  	int bar, add;
> @@ -746,6 +725,22 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
>  	return 0;
>  }
>  
> +static void pci_epf_test_clear_bar(struct pci_epf *epf)
> +{
> +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> +	struct pci_epc *epc = epf->epc;
> +	struct pci_epf_bar *epf_bar;
> +	int bar;
> +
> +	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> +		epf_bar = &epf->bar[bar];
> +
> +		if (epf_test->reg[bar])
> +			pci_epc_clear_bar(epc, epf->func_no, epf->vfunc_no,
> +					  epf_bar);
> +	}
> +}
> +
>  static int pci_epf_test_epc_init(struct pci_epf *epf)
>  {
>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> @@ -885,6 +880,22 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
>  	return 0;
>  }
>  
> +static void pci_epf_test_free_space(struct pci_epf *epf)
> +{
> +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> +	struct pci_epf_bar *epf_bar;
> +	int bar;
> +
> +	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> +		epf_bar = &epf->bar[bar];
> +
> +		if (epf_test->reg[bar]) {
> +			pci_epf_free_space(epf, epf_test->reg[bar], bar,
> +					   PRIMARY_INTERFACE);
> +		}

Nit: No need for braces here. (Just like you don't have braces in
pci_epf_test_clear_bar()).

Like you said in the other thread, this commit clashes with changes done
in my series.

However, except for the small nit, the commit looks good:
Reviewed-by: Niklas Cassel <cassel@kernel.org>


> +	}
> +}
> +
>  static void pci_epf_configure_bar(struct pci_epf *epf,
>  				  const struct pci_epc_features *epc_features)
>  {
> @@ -940,6 +951,16 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  	return 0;
>  }
>  
> +static void pci_epf_test_unbind(struct pci_epf *epf)
> +{
> +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> +
> +	cancel_delayed_work(&epf_test->cmd_handler);
> +	pci_epf_test_clean_dma_chan(epf_test);
> +	pci_epf_test_clear_bar(epf);
> +	pci_epf_test_free_space(epf);
> +}
> +
>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
>  	{
>  		.name = "pci_epf_test",
> 
> -- 
> 2.25.1
> 

Re: [PATCH 04/11] PCI: epf-test: Refactor pci_epf_test_unbind() function

[Manivannan Sadhasivam]

On Fri, Mar 22, 2024 at 05:09:03PM +0100, Niklas Cassel wrote:
> On Thu, Mar 14, 2024 at 08:53:43PM +0530, Manivannan Sadhasivam wrote:
> > Move the pci_epc_clear_bar() and pci_epf_free_space() code to respective
> > helper functions. This allows reusing the helpers in future commits.
> > 
> > This also requires moving the pci_epf_test_unbind() definition below
> > pci_epf_test_bind() to avoid forward declaration of the above helpers.
> > 
> > No functional change.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-test.c | 63 ++++++++++++++++++---------
> >  1 file changed, 42 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index 1dae0fce8fc4..2fac36553633 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -686,27 +686,6 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
> >  			   msecs_to_jiffies(1));
> >  }
> >  
> > -static void pci_epf_test_unbind(struct pci_epf *epf)
> > -{
> > -	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> > -	struct pci_epc *epc = epf->epc;
> > -	struct pci_epf_bar *epf_bar;
> > -	int bar;
> > -
> > -	cancel_delayed_work(&epf_test->cmd_handler);
> > -	pci_epf_test_clean_dma_chan(epf_test);
> > -	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> > -		epf_bar = &epf->bar[bar];
> > -
> > -		if (epf_test->reg[bar]) {
> > -			pci_epc_clear_bar(epc, epf->func_no, epf->vfunc_no,
> > -					  epf_bar);
> > -			pci_epf_free_space(epf, epf_test->reg[bar], bar,
> > -					   PRIMARY_INTERFACE);
> > -		}
> > -	}
> > -}
> > -
> >  static int pci_epf_test_set_bar(struct pci_epf *epf)
> >  {
> >  	int bar, add;
> > @@ -746,6 +725,22 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
> >  	return 0;
> >  }
> >  
> > +static void pci_epf_test_clear_bar(struct pci_epf *epf)
> > +{
> > +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> > +	struct pci_epc *epc = epf->epc;
> > +	struct pci_epf_bar *epf_bar;
> > +	int bar;
> > +
> > +	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> > +		epf_bar = &epf->bar[bar];
> > +
> > +		if (epf_test->reg[bar])
> > +			pci_epc_clear_bar(epc, epf->func_no, epf->vfunc_no,
> > +					  epf_bar);
> > +	}
> > +}
> > +
> >  static int pci_epf_test_epc_init(struct pci_epf *epf)
> >  {
> >  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> > @@ -885,6 +880,22 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> >  	return 0;
> >  }
> >  
> > +static void pci_epf_test_free_space(struct pci_epf *epf)
> > +{
> > +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> > +	struct pci_epf_bar *epf_bar;
> > +	int bar;
> > +
> > +	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> > +		epf_bar = &epf->bar[bar];
> > +
> > +		if (epf_test->reg[bar]) {
> > +			pci_epf_free_space(epf, epf_test->reg[bar], bar,
> > +					   PRIMARY_INTERFACE);
> > +		}
> 
> Nit: No need for braces here. (Just like you don't have braces in
> pci_epf_test_clear_bar()).
> 
> Like you said in the other thread, this commit clashes with changes done
> in my series.
> 

I think I should just rebase this series on top of yours.

> However, except for the small nit, the commit looks good:
> Reviewed-by: Niklas Cassel <cassel@kernel.org>
>

Thanks!
 
- Mani

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback

[Manivannan Sadhasivam]

To maintain uniformity across EPF drivers, let's move the DMA
initialization to EPC init callback. This will also allow us to deinit DMA
during PERST# assert in the further commits.

For EPC drivers without PERST#, DMA deinit will only happen during driver
unbind.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/endpoint/functions/pci-epf-mhi.c  | 16 ++++++++--------
 drivers/pci/endpoint/functions/pci-epf-test.c | 12 ++++++------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
index da894a9a447e..4e4300efd9d7 100644
--- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
+++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
@@ -737,6 +737,14 @@ static int pci_epf_mhi_epc_init(struct pci_epf *epf)
 	if (!epf_mhi->epc_features)
 		return -ENODATA;
 
+	if (info->flags & MHI_EPF_USE_DMA) {
+		ret = pci_epf_mhi_dma_init(epf_mhi);
+		if (ret) {
+			dev_err(dev, "Failed to initialize DMA: %d\n", ret);
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -749,14 +757,6 @@ static int pci_epf_mhi_link_up(struct pci_epf *epf)
 	struct device *dev = &epf->dev;
 	int ret;
 
-	if (info->flags & MHI_EPF_USE_DMA) {
-		ret = pci_epf_mhi_dma_init(epf_mhi);
-		if (ret) {
-			dev_err(dev, "Failed to initialize DMA: %d\n", ret);
-			return ret;
-		}
-	}
-
 	mhi_cntrl->mmio = epf_mhi->mmio;
 	mhi_cntrl->irq = epf_mhi->irq;
 	mhi_cntrl->mru = info->mru;
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 2fac36553633..8f1e0cb08814 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -753,6 +753,12 @@ static int pci_epf_test_epc_init(struct pci_epf *epf)
 	bool msi_capable = true;
 	int ret;
 
+	epf_test->dma_supported = true;
+
+	ret = pci_epf_test_init_dma_chan(epf_test);
+	if (ret)
+		epf_test->dma_supported = false;
+
 	epc_features = pci_epc_get_features(epc, epf->func_no, epf->vfunc_no);
 	if (epc_features) {
 		msix_capable = epc_features->msix_capable;
@@ -942,12 +948,6 @@ static int pci_epf_test_bind(struct pci_epf *epf)
 	if (ret)
 		return ret;
 
-	epf_test->dma_supported = true;
-
-	ret = pci_epf_test_init_dma_chan(epf_test);
-	if (ret)
-		epf_test->dma_supported = false;
-
 	return 0;
 }
 

-- 
2.25.1

Re: [PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback

[Niklas Cassel]

On Thu, Mar 14, 2024 at 08:53:44PM +0530, Manivannan Sadhasivam wrote:
> To maintain uniformity across EPF drivers, let's move the DMA
> initialization to EPC init callback. This will also allow us to deinit DMA
> during PERST# assert in the further commits.
> 
> For EPC drivers without PERST#, DMA deinit will only happen during driver
> unbind.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---

Reviewed-by: Niklas Cassel <cassel@kernel.org>


For the record, I was debugging a problem related to EPF DMA recently
and was dumping the DMA mask for the struct device of the epf driver.
I was a bit confused to see it as 32-bits, even though the EPC driver
has it set to 64-bits.

The current code works, because e.g., pci_epf_test_write(), etc,
does:
struct device *dma_dev = epf->epc->dev.parent;
dma_map_single(dma_dev, ...);

but it also means that all EPF drivers will do this uglyness.



However, if a EPF driver does e.g.
dma_alloc_coherent(), and sends in the struct *device for the EPF,
which is the most logical thing to do IMO, it will use the wrong DMA
mask.

Perhaps EPF or EPC code should make sure that the struct *device
for the EPF will get the same DMA mask as epf->epc->dev.parent,
so that EPF driver developer can use the struct *epf when calling
e.g. dma_alloc_coherent().

(This is however not strictly related to this patch, but I thought
that I should mention it.)


Kind regards,
Niklas

>  drivers/pci/endpoint/functions/pci-epf-mhi.c  | 16 ++++++++--------
>  drivers/pci/endpoint/functions/pci-epf-test.c | 12 ++++++------
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> index da894a9a447e..4e4300efd9d7 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> @@ -737,6 +737,14 @@ static int pci_epf_mhi_epc_init(struct pci_epf *epf)
>  	if (!epf_mhi->epc_features)
>  		return -ENODATA;
>  
> +	if (info->flags & MHI_EPF_USE_DMA) {
> +		ret = pci_epf_mhi_dma_init(epf_mhi);
> +		if (ret) {
> +			dev_err(dev, "Failed to initialize DMA: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> @@ -749,14 +757,6 @@ static int pci_epf_mhi_link_up(struct pci_epf *epf)
>  	struct device *dev = &epf->dev;
>  	int ret;
>  
> -	if (info->flags & MHI_EPF_USE_DMA) {
> -		ret = pci_epf_mhi_dma_init(epf_mhi);
> -		if (ret) {
> -			dev_err(dev, "Failed to initialize DMA: %d\n", ret);
> -			return ret;
> -		}
> -	}
> -
>  	mhi_cntrl->mmio = epf_mhi->mmio;
>  	mhi_cntrl->irq = epf_mhi->irq;
>  	mhi_cntrl->mru = info->mru;
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 2fac36553633..8f1e0cb08814 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -753,6 +753,12 @@ static int pci_epf_test_epc_init(struct pci_epf *epf)
>  	bool msi_capable = true;
>  	int ret;
>  
> +	epf_test->dma_supported = true;
> +
> +	ret = pci_epf_test_init_dma_chan(epf_test);
> +	if (ret)
> +		epf_test->dma_supported = false;
> +
>  	epc_features = pci_epc_get_features(epc, epf->func_no, epf->vfunc_no);
>  	if (epc_features) {
>  		msix_capable = epc_features->msix_capable;
> @@ -942,12 +948,6 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  	if (ret)
>  		return ret;
>  
> -	epf_test->dma_supported = true;
> -
> -	ret = pci_epf_test_init_dma_chan(epf_test);
> -	if (ret)
> -		epf_test->dma_supported = false;
> -
>  	return 0;
>  }
>  
> 
> -- 
> 2.25.1
> 

Re: [PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback

[Manivannan Sadhasivam]

On Fri, Mar 22, 2024 at 05:10:06PM +0100, Niklas Cassel wrote:
> On Thu, Mar 14, 2024 at 08:53:44PM +0530, Manivannan Sadhasivam wrote:
> > To maintain uniformity across EPF drivers, let's move the DMA
> > initialization to EPC init callback. This will also allow us to deinit DMA
> > during PERST# assert in the further commits.
> > 
> > For EPC drivers without PERST#, DMA deinit will only happen during driver
> > unbind.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> 
> Reviewed-by: Niklas Cassel <cassel@kernel.org>
> 
> 
> For the record, I was debugging a problem related to EPF DMA recently
> and was dumping the DMA mask for the struct device of the epf driver.
> I was a bit confused to see it as 32-bits, even though the EPC driver
> has it set to 64-bits.
> 
> The current code works, because e.g., pci_epf_test_write(), etc,
> does:
> struct device *dma_dev = epf->epc->dev.parent;
> dma_map_single(dma_dev, ...);
> 
> but it also means that all EPF drivers will do this uglyness.
> 

This ugliness is required as long as the dmaengine is associated only with the
EPC.

> 
> 
> However, if a EPF driver does e.g.
> dma_alloc_coherent(), and sends in the struct *device for the EPF,
> which is the most logical thing to do IMO, it will use the wrong DMA
> mask.
> 
> Perhaps EPF or EPC code should make sure that the struct *device
> for the EPF will get the same DMA mask as epf->epc->dev.parent,
> so that EPF driver developer can use the struct *epf when calling
> e.g. dma_alloc_coherent().
> 

Makes sense. I think it can be done during bind() in the EPC core. Feel free to
submit a patch if you like, otherwise I'll keep it in my todo list.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback

[Niklas Cassel]

On Tue, Mar 26, 2024 at 01:56:36PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Mar 22, 2024 at 05:10:06PM +0100, Niklas Cassel wrote:
> > On Thu, Mar 14, 2024 at 08:53:44PM +0530, Manivannan Sadhasivam wrote:
> > > To maintain uniformity across EPF drivers, let's move the DMA
> > > initialization to EPC init callback. This will also allow us to deinit DMA
> > > during PERST# assert in the further commits.
> > > 
> > > For EPC drivers without PERST#, DMA deinit will only happen during driver
> > > unbind.
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > 
> > Reviewed-by: Niklas Cassel <cassel@kernel.org>
> > 
> > 
> > For the record, I was debugging a problem related to EPF DMA recently
> > and was dumping the DMA mask for the struct device of the epf driver.
> > I was a bit confused to see it as 32-bits, even though the EPC driver
> > has it set to 64-bits.
> > 
> > The current code works, because e.g., pci_epf_test_write(), etc,
> > does:
> > struct device *dma_dev = epf->epc->dev.parent;
> > dma_map_single(dma_dev, ...);
> > 
> > but it also means that all EPF drivers will do this uglyness.
> > 
> 
> This ugliness is required as long as the dmaengine is associated only with the
> EPC.
> 
> > 
> > 
> > However, if a EPF driver does e.g.
> > dma_alloc_coherent(), and sends in the struct *device for the EPF,
> > which is the most logical thing to do IMO, it will use the wrong DMA
> > mask.
> > 
> > Perhaps EPF or EPC code should make sure that the struct *device
> > for the EPF will get the same DMA mask as epf->epc->dev.parent,
> > so that EPF driver developer can use the struct *epf when calling
> > e.g. dma_alloc_coherent().
> > 
> 
> Makes sense. I think it can be done during bind() in the EPC core. Feel free to
> submit a patch if you like, otherwise I'll keep it in my todo list.

So we still want to test:
-DMA API using the eDMA
-DMA API using the "dummy" memcpy dma-channel.

However, it seems like both pci-epf-mhi.c and pci-epf-test.c
do either:
-Use DMA API
or
-Use memcpy_fromio()/memcpy_toio() instead of DMA API


To me, it seems like we should always be able to use
DMA API (using either a eDMA or "dummy" memcpy).

I don't really see the need to have the path that does:
memcpy_fromio()/memcpy_toio().

I know that for DWC, when using memcpy (and this also
memcpy via DMA API), we need to map the address using
iATU first.

But that could probably be done using another flag,
perhaps rename that flag FLAG_USE_DMA to NEEDS_MAP or
something.
(Such that we can change these drivers to only have a
code path that uses DMA API.)
(...and making sure that inheriting the DMA mask does
not affect the DMA mask for DMA_MEMCPY.)

But perhaps I am missing something... and DMA_MEMCPY is
not always available?


Kind regards,
Niklas

Re: [PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback

[Niklas Cassel]

On Tue, Mar 26, 2024 at 12:05:42PM +0100, Niklas Cassel wrote:
> On Tue, Mar 26, 2024 at 01:56:36PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Mar 22, 2024 at 05:10:06PM +0100, Niklas Cassel wrote:
> > > On Thu, Mar 14, 2024 at 08:53:44PM +0530, Manivannan Sadhasivam wrote:
> > > > To maintain uniformity across EPF drivers, let's move the DMA
> > > > initialization to EPC init callback. This will also allow us to deinit DMA
> > > > during PERST# assert in the further commits.
> > > > 
> > > > For EPC drivers without PERST#, DMA deinit will only happen during driver
> > > > unbind.
> > > > 
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > 
> > > Reviewed-by: Niklas Cassel <cassel@kernel.org>
> > > 
> > > 
> > > For the record, I was debugging a problem related to EPF DMA recently
> > > and was dumping the DMA mask for the struct device of the epf driver.
> > > I was a bit confused to see it as 32-bits, even though the EPC driver
> > > has it set to 64-bits.
> > > 
> > > The current code works, because e.g., pci_epf_test_write(), etc,
> > > does:
> > > struct device *dma_dev = epf->epc->dev.parent;
> > > dma_map_single(dma_dev, ...);
> > > 
> > > but it also means that all EPF drivers will do this uglyness.
> > > 
> > 
> > This ugliness is required as long as the dmaengine is associated only with the
> > EPC.
> > 
> > > 
> > > 
> > > However, if a EPF driver does e.g.
> > > dma_alloc_coherent(), and sends in the struct *device for the EPF,
> > > which is the most logical thing to do IMO, it will use the wrong DMA
> > > mask.
> > > 
> > > Perhaps EPF or EPC code should make sure that the struct *device
> > > for the EPF will get the same DMA mask as epf->epc->dev.parent,
> > > so that EPF driver developer can use the struct *epf when calling
> > > e.g. dma_alloc_coherent().
> > > 
> > 
> > Makes sense. I think it can be done during bind() in the EPC core. Feel free to
> > submit a patch if you like, otherwise I'll keep it in my todo list.
> 
> So we still want to test:
> -DMA API using the eDMA
> -DMA API using the "dummy" memcpy dma-channel.
> 
> However, it seems like both pci-epf-mhi.c and pci-epf-test.c
> do either:
> -Use DMA API
> or
> -Use memcpy_fromio()/memcpy_toio() instead of DMA API
> 
> 
> To me, it seems like we should always be able to use
> DMA API (using either a eDMA or "dummy" memcpy).
> 
> I don't really see the need to have the path that does:
> memcpy_fromio()/memcpy_toio().
> 
> I know that for DWC, when using memcpy (and this also
> memcpy via DMA API), we need to map the address using
> iATU first.
> 
> But that could probably be done using another flag,
> perhaps rename that flag FLAG_USE_DMA to NEEDS_MAP or
> something.
> (Such that we can change these drivers to only have a
> code path that uses DMA API.)

Looking at pci-epf-mhi.c, it seems to use names like:
pci_epf_mhi_iatu_read() and pci_epf_mhi_edma_read().

This seems to be a very DWC focused naming.

AFAICT, EPF drivers should work on any PCIe EP controller that implements
the EPC API.

Yes, I understand that it is only Qualcomm that uses this MHI interface/bus,
but what is stopping Qualcomm from using a non-DWC based PCIe EP controller
in an upcoming SoC?

Surely that Qualcomm SoC could still implement the MHI interface/bus,
so perhaps the naming in this EPF driver should use somewhat less
"EPC vendor specific" function names?


Kind regards,
Niklas

Re: [PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback

[Manivannan Sadhasivam]

On Tue, Mar 26, 2024 at 03:27:27PM +0100, Niklas Cassel wrote:
> On Tue, Mar 26, 2024 at 12:05:42PM +0100, Niklas Cassel wrote:
> > On Tue, Mar 26, 2024 at 01:56:36PM +0530, Manivannan Sadhasivam wrote:
> > > On Fri, Mar 22, 2024 at 05:10:06PM +0100, Niklas Cassel wrote:
> > > > On Thu, Mar 14, 2024 at 08:53:44PM +0530, Manivannan Sadhasivam wrote:
> > > > > To maintain uniformity across EPF drivers, let's move the DMA
> > > > > initialization to EPC init callback. This will also allow us to deinit DMA
> > > > > during PERST# assert in the further commits.
> > > > > 
> > > > > For EPC drivers without PERST#, DMA deinit will only happen during driver
> > > > > unbind.
> > > > > 
> > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > ---
> > > > 
> > > > Reviewed-by: Niklas Cassel <cassel@kernel.org>
> > > > 
> > > > 
> > > > For the record, I was debugging a problem related to EPF DMA recently
> > > > and was dumping the DMA mask for the struct device of the epf driver.
> > > > I was a bit confused to see it as 32-bits, even though the EPC driver
> > > > has it set to 64-bits.
> > > > 
> > > > The current code works, because e.g., pci_epf_test_write(), etc,
> > > > does:
> > > > struct device *dma_dev = epf->epc->dev.parent;
> > > > dma_map_single(dma_dev, ...);
> > > > 
> > > > but it also means that all EPF drivers will do this uglyness.
> > > > 
> > > 
> > > This ugliness is required as long as the dmaengine is associated only with the
> > > EPC.
> > > 
> > > > 
> > > > 
> > > > However, if a EPF driver does e.g.
> > > > dma_alloc_coherent(), and sends in the struct *device for the EPF,
> > > > which is the most logical thing to do IMO, it will use the wrong DMA
> > > > mask.
> > > > 
> > > > Perhaps EPF or EPC code should make sure that the struct *device
> > > > for the EPF will get the same DMA mask as epf->epc->dev.parent,
> > > > so that EPF driver developer can use the struct *epf when calling
> > > > e.g. dma_alloc_coherent().
> > > > 
> > > 
> > > Makes sense. I think it can be done during bind() in the EPC core. Feel free to
> > > submit a patch if you like, otherwise I'll keep it in my todo list.
> > 
> > So we still want to test:
> > -DMA API using the eDMA
> > -DMA API using the "dummy" memcpy dma-channel.
> > 
> > However, it seems like both pci-epf-mhi.c and pci-epf-test.c
> > do either:
> > -Use DMA API
> > or
> > -Use memcpy_fromio()/memcpy_toio() instead of DMA API
> > 
> > 
> > To me, it seems like we should always be able to use
> > DMA API (using either a eDMA or "dummy" memcpy).
> > 
> > I don't really see the need to have the path that does:
> > memcpy_fromio()/memcpy_toio().
> > 
> > I know that for DWC, when using memcpy (and this also
> > memcpy via DMA API), we need to map the address using
> > iATU first.
> > 
> > But that could probably be done using another flag,
> > perhaps rename that flag FLAG_USE_DMA to NEEDS_MAP or
> > something.
> > (Such that we can change these drivers to only have a
> > code path that uses DMA API.)
> 
> Looking at pci-epf-mhi.c, it seems to use names like:
> pci_epf_mhi_iatu_read() and pci_epf_mhi_edma_read().
> 
> This seems to be a very DWC focused naming.
> 
> AFAICT, EPF drivers should work on any PCIe EP controller that implements
> the EPC API.
> 
> Yes, I understand that it is only Qualcomm that uses this MHI interface/bus,
> but what is stopping Qualcomm from using a non-DWC based PCIe EP controller
> in an upcoming SoC?
> 
> Surely that Qualcomm SoC could still implement the MHI interface/bus,
> so perhaps the naming in this EPF driver should use somewhat less
> "EPC vendor specific" function names?
> 

Yeah, agree. This needs to be cleaned up.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback

[Manivannan Sadhasivam]

On Tue, Mar 26, 2024 at 12:05:42PM +0100, Niklas Cassel wrote:
> On Tue, Mar 26, 2024 at 01:56:36PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Mar 22, 2024 at 05:10:06PM +0100, Niklas Cassel wrote:
> > > On Thu, Mar 14, 2024 at 08:53:44PM +0530, Manivannan Sadhasivam wrote:
> > > > To maintain uniformity across EPF drivers, let's move the DMA
> > > > initialization to EPC init callback. This will also allow us to deinit DMA
> > > > during PERST# assert in the further commits.
> > > > 
> > > > For EPC drivers without PERST#, DMA deinit will only happen during driver
> > > > unbind.
> > > > 
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > 
> > > Reviewed-by: Niklas Cassel <cassel@kernel.org>
> > > 
> > > 
> > > For the record, I was debugging a problem related to EPF DMA recently
> > > and was dumping the DMA mask for the struct device of the epf driver.
> > > I was a bit confused to see it as 32-bits, even though the EPC driver
> > > has it set to 64-bits.
> > > 
> > > The current code works, because e.g., pci_epf_test_write(), etc,
> > > does:
> > > struct device *dma_dev = epf->epc->dev.parent;
> > > dma_map_single(dma_dev, ...);
> > > 
> > > but it also means that all EPF drivers will do this uglyness.
> > > 
> > 
> > This ugliness is required as long as the dmaengine is associated only with the
> > EPC.
> > 
> > > 
> > > 
> > > However, if a EPF driver does e.g.
> > > dma_alloc_coherent(), and sends in the struct *device for the EPF,
> > > which is the most logical thing to do IMO, it will use the wrong DMA
> > > mask.
> > > 
> > > Perhaps EPF or EPC code should make sure that the struct *device
> > > for the EPF will get the same DMA mask as epf->epc->dev.parent,
> > > so that EPF driver developer can use the struct *epf when calling
> > > e.g. dma_alloc_coherent().
> > > 
> > 
> > Makes sense. I think it can be done during bind() in the EPC core. Feel free to
> > submit a patch if you like, otherwise I'll keep it in my todo list.
> 
> So we still want to test:
> -DMA API using the eDMA
> -DMA API using the "dummy" memcpy dma-channel.
> 

IMO, the test driver should just test one form of data transfer. Either CPU
memcpy (using iATU or something similar) or DMA. But I think the motive behind
using DMA memcpy is that to support platforms that do not pass DMA slave
channels in devicetree.

It is applicable to test driver but not to MHI driver since all DMA supported
MHI platforms will pass the DMA slave channels in devicetree.

> However, it seems like both pci-epf-mhi.c and pci-epf-test.c
> do either:
> -Use DMA API
> or
> -Use memcpy_fromio()/memcpy_toio() instead of DMA API
> 
> 
> To me, it seems like we should always be able to use
> DMA API (using either a eDMA or "dummy" memcpy).
> 

No, there are platforms that don't support DMA at all. Like Qcom SDX55, so we
still need to do CPU memcpy.

> I don't really see the need to have the path that does:
> memcpy_fromio()/memcpy_toio().
> 
> I know that for DWC, when using memcpy (and this also
> memcpy via DMA API), we need to map the address using
> iATU first.
> 
> But that could probably be done using another flag,
> perhaps rename that flag FLAG_USE_DMA to NEEDS_MAP or
> something.
> (Such that we can change these drivers to only have a
> code path that uses DMA API.)
> (...and making sure that inheriting the DMA mask does
> not affect the DMA mask for DMA_MEMCPY.)
> 
> But perhaps I am missing something... and DMA_MEMCPY is
> not always available?
> 

Yes.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback

[Niklas Cassel]

+CC Vinod

Hello Vinod,

I didn't know the answer, so I chose the "call a friend option" ;)
I hope that you can help me out :)


If you take a look at drivers/pci/endpoint/functions/pci-epf-test.c
https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/endpoint/functions/pci-epf-test.c#L448-L471

You can see that the driver always does pci_epc_map_addr(),
then it will either use:
DMA API, e.g. dma_map_single() etc.
or
memcpy_fromio()/memcpy_toio()

based on flag FLAG_USE_DMA.

This flag is set via ioctl, so if we run:
/usr/bin/pcitest -d
the flag will be set, without the -d parameter the flag won't be set.


If you look at how the DMA channel is requested:
https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/endpoint/functions/pci-epf-test.c#L224-L258

If will try to get a private DMA channel, if that fails,
it will use the "dummy memcpy" DMA channel.

If the FLAG_USE_DMA is set, the transfers itself will use:
https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/endpoint/functions/pci-epf-test.c#L139-L155
either dmaengine_prep_slave_single() or dmaengine_prep_dma_memcpy(),
depending on if we are using "dummy memcpy" or not.



If you take e.g. the DWC PCIe EP controller, it can have an embedded DMA
controller on the PCIe controller, and we will try to detect it when
initializing the PCIe EP controller using dw_pcie_edma_detect():
https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/controller/dwc/pcie-designware-ep.c#L759

For the PCIe EP controller that I am using, which have eDMA built-in,
I noticed that if I do not enable the eDMA driver (# CONFIG_DW_EDMA is not
set), I noticed that I can still run:
/usr/bin/pcitest -d

Which will use the "dummy memcpy" DMA channel.
Yes, the performance is poor, but it still works, so it appears that the
fallback code is working properly.


If I enable the eDMA driver (CONFIG_DW_EDMA=y),
I can run:
/usr/bin/pcitest -d

And the performance is good.


So my question is:
Is the "dummy memcpy" DMA channel always available?

Because if it is, I think we could drop the path in the pci-epf-test.c
driver which uses memcpy_fromio()/memcpy_toio() instead of DMA API.
(Since just having a single path to do I/O in the driver would simplify
the driver IMO.)

I assume that the "dummy memcpy" DMA channel just uses memcpy_fromio() and
memcpy_toio() under the hood, so I assume that using the memcpy_fromio()/
memcpy_toio/() is equivalent to using DMA API + dmaengine_prep_dma_memcpy().

Although it would be nice if we didn't need to have the two separate paths
in pci_epf_test_data_transfer() (dmaengine_prep_slave_single() vs
dmaengine_prep_dma_memcpy()) to support the "dummy memcpy" channel.
But I guess that is not possible...


I hope that you can bring some clarity Vinod.
(Please read my replies to Mani below before you compose your email,
as it does provide more insight to this mess.)

Mani, I tried to reply to you inline below, with my limited understanding
of how dmaengine works.


On Wed, Mar 27, 2024 at 11:48:19AM +0530, Manivannan Sadhasivam wrote:
> > So we still want to test:
> > -DMA API using the eDMA
> > -DMA API using the "dummy" memcpy dma-channel.
> > 
> 
> IMO, the test driver should just test one form of data transfer. Either CPU
> memcpy (using iATU or something similar) or DMA. But I think the motive behind
> using DMA memcpy is that to support platforms that do not pass DMA slave
> channels in devicetree.
> 
> It is applicable to test driver but not to MHI driver since all DMA supported
> MHI platforms will pass the DMA slave channels in devicetree.

I don't understand how device tree is relevant here, e.g. qcom-ep.c
specifies pcie_ep->pci.edma.nr_irqs = 1;
https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/controller/dwc/pcie-qcom-ep.c#L818
which is sufficient for you to be able to probe/detect eDMA successfully,
no need for anything in device tree at all.


> 
> > However, it seems like both pci-epf-mhi.c and pci-epf-test.c
> > do either:
> > -Use DMA API
> > or
> > -Use memcpy_fromio()/memcpy_toio() instead of DMA API
> > 
> > 
> > To me, it seems like we should always be able to use
> > DMA API (using either a eDMA or "dummy" memcpy).
> > 
> 
> No, there are platforms that don't support DMA at all. Like Qcom SDX55, so we
> still need to do CPU memcpy.

I assume that you mean the the PCIe controller used in SDX55 does not
have the eDMA on the PCIe controller, so dw_pcie_edma_detect() will
fail to detect any eDMA. That is fine no?

I assume that this SoC will still able to use the "dummy" memcpy dma-channel?


> 
> > I don't really see the need to have the path that does:
> > memcpy_fromio()/memcpy_toio().
> > 
> > I know that for DWC, when using memcpy (and this also
> > memcpy via DMA API), we need to map the address using
> > iATU first.
> > 
> > But that could probably be done using another flag,
> > perhaps rename that flag FLAG_USE_DMA to NEEDS_MAP or
> > something.
> > (Such that we can change these drivers to only have a
> > code path that uses DMA API.)
> > (...and making sure that inheriting the DMA mask does
> > not affect the DMA mask for DMA_MEMCPY.)

I was wrong here, pci-epf-test always calls pci_epc_map_addr()
regardless if FLAG_USE_DMA is set or not.

(Even though this should be unnecessary when using the eDMA.)

However, if we look at pci-epf-mhi.c we can see that it does
NOT call pci_epc_map_addr() when using DMA API + dmaengine.

Is it really safe to avoid pci_epc_map_addr() in all EPC controllers?
I assume that it should be safe for all "real" DMA channels.
We can see that it is not safe when using DMA API + "dummy" memcpy
dma-channel. (That is why I was asking if we need a NEEDS_MAP, or
MAP_NOT_NEEDED flag.)


> > 
> > But perhaps I am missing something... and DMA_MEMCPY is
> > not always available?

Right now pci-epf-test driver has three ways:
-DMA API + dmaengine dmaengine_prep_slave_single()
-DMA API + dmaengine dmaengine_prep_dma_memcpy()
-memcpy_toio()/memcpy_fromio().

pci-epf-mhi.c driver has two ways:
-DMA API + dmaengine dmaengine_prep_slave_single()
-memcpy_toio()/memcpy_fromio().


pci-epf-test.c:
-Always calls pci_epc_map_addr() when using DMA API.

pci-epf-mhi.c:
-Never calls pci_epc_map_addr() when using DMA API.


I honestly don't see any point of having three paths
for pci-epf-test. Ideally I would want one, max two.

If you think that:
-DMA API + dmaengine dmaengine_prep_slave_single()
+
-memcpy_toio()/memcpy_fromio().

is more logical than:
-DMA API + dmaengine dmaengine_prep_slave_single()
+
-DMA API + dmaengine dmaengine_prep_dma_memcpy()

Then I think we should rip out the:
-DMA API + dmaengine dmaengine_prep_dma_memcpy()
it serves no purpose... if you don't have a "real" DMA channel,
just run without the -d flag.

Or, if you argue that the dmaengine_prep_dma_memcpy() is there
to test the DMA API code (which I can't say that it does, since
it doesn't use the exact same code path as a "real" DMA channel, see:
https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/endpoint/functions/pci-epf-test.c#L139-L155
so this argument is questionable).

Put it under a --use_dummy_dma, and return failure by default
if no "real" DMA channel is found.


But even so, that would not address the pci-epf-test and
pci-mhi-test inconsistency WRT pci_epc_map_addr().

I think if we rip out:
-DMA API + dmaengine dmaengine_prep_dma_memcpy()
we could also move the pci_epc_map_addr() so that it is
only used for the memcpy_toio()/memcpy_fromio() path.

(Or if we add a --use_dummy_dma, we can move the pci_epc_map_addr() to
that path, and remove it from the dmaengine_prep_slave_single() path.)


Kind regards,
Niklas

Re: [PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback

[Vinod Koul]

On 27-03-24, 12:39, Niklas Cassel wrote:
> +CC Vinod
> 
> Hello Vinod,
> 
> I didn't know the answer, so I chose the "call a friend option" ;)
> I hope that you can help me out :)

Anytime :-)

> 
> 
> If you take a look at drivers/pci/endpoint/functions/pci-epf-test.c
> https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/endpoint/functions/pci-epf-test.c#L448-L471
> 
> You can see that the driver always does pci_epc_map_addr(),
> then it will either use:
> DMA API, e.g. dma_map_single() etc.
> or
> memcpy_fromio()/memcpy_toio()
> 
> based on flag FLAG_USE_DMA.
> 
> This flag is set via ioctl, so if we run:
> /usr/bin/pcitest -d
> the flag will be set, without the -d parameter the flag won't be set.
> 
> 
> If you look at how the DMA channel is requested:
> https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/endpoint/functions/pci-epf-test.c#L224-L258
> 
> If will try to get a private DMA channel, if that fails,
> it will use the "dummy memcpy" DMA channel.
> 
> If the FLAG_USE_DMA is set, the transfers itself will use:
> https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/endpoint/functions/pci-epf-test.c#L139-L155
> either dmaengine_prep_slave_single() or dmaengine_prep_dma_memcpy(),
> depending on if we are using "dummy memcpy" or not.
> 
> 
> 
> If you take e.g. the DWC PCIe EP controller, it can have an embedded DMA
> controller on the PCIe controller, and we will try to detect it when
> initializing the PCIe EP controller using dw_pcie_edma_detect():
> https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/controller/dwc/pcie-designware-ep.c#L759
> 
> For the PCIe EP controller that I am using, which have eDMA built-in,
> I noticed that if I do not enable the eDMA driver (# CONFIG_DW_EDMA is not
> set), I noticed that I can still run:
> /usr/bin/pcitest -d
> 
> Which will use the "dummy memcpy" DMA channel.
> Yes, the performance is poor, but it still works, so it appears that the
> fallback code is working properly.
> 
> 
> If I enable the eDMA driver (CONFIG_DW_EDMA=y),
> I can run:
> /usr/bin/pcitest -d
> 
> And the performance is good.
> 
> 
> So my question is:
> Is the "dummy memcpy" DMA channel always available?

That depends on the system, you may or maynot have such a system where
you have a generic memcpy dma controller which can provide you with
these channels

> 
> Because if it is, I think we could drop the path in the pci-epf-test.c
> driver which uses memcpy_fromio()/memcpy_toio() instead of DMA API.
> (Since just having a single path to do I/O in the driver would simplify
> the driver IMO.)
> 
> I assume that the "dummy memcpy" DMA channel just uses memcpy_fromio() and
> memcpy_toio() under the hood, so I assume that using the memcpy_fromio()/
> memcpy_toio/() is equivalent to using DMA API + dmaengine_prep_dma_memcpy().
> 
> Although it would be nice if we didn't need to have the two separate paths
> in pci_epf_test_data_transfer() (dmaengine_prep_slave_single() vs
> dmaengine_prep_dma_memcpy()) to support the "dummy memcpy" channel.
> But I guess that is not possible...

Based on my reading you might have this mechanism:
- eDMA provides dmaengine_prep_slave_single() which transfers data from
  mem to pci ep device, so fasted
- dmaengine_prep_dma_memcpy: This will copy the data but treat it as
  memory. I dont pci internals to figure out how both can work... So
  cant really make out why it is slowed
- memcpy_xxx that is IO mem functions, so ofc they will be slowest

I think the code is decent from fallback pov... chooses fastest path if
available on a system

> 
> 
> I hope that you can bring some clarity Vinod.
> (Please read my replies to Mani below before you compose your email,
> as it does provide more insight to this mess.)
> 
> Mani, I tried to reply to you inline below, with my limited understanding
> of how dmaengine works.
> 
> 
> On Wed, Mar 27, 2024 at 11:48:19AM +0530, Manivannan Sadhasivam wrote:
> > > So we still want to test:
> > > -DMA API using the eDMA
> > > -DMA API using the "dummy" memcpy dma-channel.
> > > 
> > 
> > IMO, the test driver should just test one form of data transfer. Either CPU
> > memcpy (using iATU or something similar) or DMA. But I think the motive behind
> > using DMA memcpy is that to support platforms that do not pass DMA slave
> > channels in devicetree.
> > 
> > It is applicable to test driver but not to MHI driver since all DMA supported
> > MHI platforms will pass the DMA slave channels in devicetree.
> 
> I don't understand how device tree is relevant here, e.g. qcom-ep.c
> specifies pcie_ep->pci.edma.nr_irqs = 1;
> https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/controller/dwc/pcie-qcom-ep.c#L818
> which is sufficient for you to be able to probe/detect eDMA successfully,
> no need for anything in device tree at all.
> 
> 
> > 
> > > However, it seems like both pci-epf-mhi.c and pci-epf-test.c
> > > do either:
> > > -Use DMA API
> > > or
> > > -Use memcpy_fromio()/memcpy_toio() instead of DMA API
> > > 
> > > 
> > > To me, it seems like we should always be able to use
> > > DMA API (using either a eDMA or "dummy" memcpy).
> > > 
> > 
> > No, there are platforms that don't support DMA at all. Like Qcom SDX55, so we
> > still need to do CPU memcpy.
> 
> I assume that you mean the the PCIe controller used in SDX55 does not
> have the eDMA on the PCIe controller, so dw_pcie_edma_detect() will
> fail to detect any eDMA. That is fine no?
> 
> I assume that this SoC will still able to use the "dummy" memcpy dma-channel?
> 
> 
> > 
> > > I don't really see the need to have the path that does:
> > > memcpy_fromio()/memcpy_toio().
> > > 
> > > I know that for DWC, when using memcpy (and this also
> > > memcpy via DMA API), we need to map the address using
> > > iATU first.
> > > 
> > > But that could probably be done using another flag,
> > > perhaps rename that flag FLAG_USE_DMA to NEEDS_MAP or
> > > something.
> > > (Such that we can change these drivers to only have a
> > > code path that uses DMA API.)
> > > (...and making sure that inheriting the DMA mask does
> > > not affect the DMA mask for DMA_MEMCPY.)
> 
> I was wrong here, pci-epf-test always calls pci_epc_map_addr()
> regardless if FLAG_USE_DMA is set or not.
> 
> (Even though this should be unnecessary when using the eDMA.)
> 
> However, if we look at pci-epf-mhi.c we can see that it does
> NOT call pci_epc_map_addr() when using DMA API + dmaengine.
> 
> Is it really safe to avoid pci_epc_map_addr() in all EPC controllers?
> I assume that it should be safe for all "real" DMA channels.
> We can see that it is not safe when using DMA API + "dummy" memcpy
> dma-channel. (That is why I was asking if we need a NEEDS_MAP, or
> MAP_NOT_NEEDED flag.)
> 
> 
> > > 
> > > But perhaps I am missing something... and DMA_MEMCPY is
> > > not always available?
> 
> Right now pci-epf-test driver has three ways:
> -DMA API + dmaengine dmaengine_prep_slave_single()
> -DMA API + dmaengine dmaengine_prep_dma_memcpy()
> -memcpy_toio()/memcpy_fromio().
> 
> pci-epf-mhi.c driver has two ways:
> -DMA API + dmaengine dmaengine_prep_slave_single()
> -memcpy_toio()/memcpy_fromio().
> 
> 
> pci-epf-test.c:
> -Always calls pci_epc_map_addr() when using DMA API.
> 
> pci-epf-mhi.c:
> -Never calls pci_epc_map_addr() when using DMA API.
> 
> 
> I honestly don't see any point of having three paths
> for pci-epf-test. Ideally I would want one, max two.
> 
> If you think that:
> -DMA API + dmaengine dmaengine_prep_slave_single()
> +
> -memcpy_toio()/memcpy_fromio().
> 
> is more logical than:
> -DMA API + dmaengine dmaengine_prep_slave_single()
> +
> -DMA API + dmaengine dmaengine_prep_dma_memcpy()
> 
> Then I think we should rip out the:
> -DMA API + dmaengine dmaengine_prep_dma_memcpy()
> it serves no purpose... if you don't have a "real" DMA channel,
> just run without the -d flag.
> 
> Or, if you argue that the dmaengine_prep_dma_memcpy() is there
> to test the DMA API code (which I can't say that it does, since
> it doesn't use the exact same code path as a "real" DMA channel, see:
> https://github.com/torvalds/linux/blob/v6.9-rc1/drivers/pci/endpoint/functions/pci-epf-test.c#L139-L155
> so this argument is questionable).
> 
> Put it under a --use_dummy_dma, and return failure by default
> if no "real" DMA channel is found.
> 
> 
> But even so, that would not address the pci-epf-test and
> pci-mhi-test inconsistency WRT pci_epc_map_addr().
> 
> I think if we rip out:
> -DMA API + dmaengine dmaengine_prep_dma_memcpy()
> we could also move the pci_epc_map_addr() so that it is
> only used for the memcpy_toio()/memcpy_fromio() path.
> 
> (Or if we add a --use_dummy_dma, we can move the pci_epc_map_addr() to
> that path, and remove it from the dmaengine_prep_slave_single() path.)
> 
> 
> Kind regards,
> Niklas

-- 
~Vinod

Re: [PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback

[Niklas Cassel]

On Fri, Mar 29, 2024 at 12:12:48AM +0530, Vinod Koul wrote:
> On 27-03-24, 12:39, Niklas Cassel wrote:
> > 
> > So my question is:
> > Is the "dummy memcpy" DMA channel always available?
> 
> That depends on the system, you may or maynot have such a system where
> you have a generic memcpy dma controller which can provide you with
> these channels

I misunderstood DMA_MEMCPY then, I assumed that it was a "software emulated"
DMA channel, which allowed the a driver to always use dmaengine + DMA API.

It actually uses a real DMA controller. I don't have any DMA controller in
the PCIe EP device tree node, but perhaps it can use any DMA controller that
has been registered with dmaengine?


> Based on my reading you might have this mechanism:
> - eDMA provides dmaengine_prep_slave_single() which transfers data from
>   mem to pci ep device, so fasted
> - dmaengine_prep_dma_memcpy: This will copy the data but treat it as
>   memory. I dont pci internals to figure out how both can work... So
>   cant really make out why it is slowed
> - memcpy_xxx that is IO mem functions, so ofc they will be slowest
> 
> I think the code is decent from fallback pov... chooses fastest path if
> available on a system

Indeed, it makes more sense to me now, thank you Vinod.


> > I was wrong here, pci-epf-test always calls pci_epc_map_addr()
> > regardless if FLAG_USE_DMA is set or not.
> > 
> > (Even though this should be unnecessary when using the eDMA.)
> > 
> > However, if we look at pci-epf-mhi.c we can see that it does
> > NOT call pci_epc_map_addr() when using DMA API + dmaengine.
> > 
> > Is it really safe to avoid pci_epc_map_addr() in all EPC controllers?
> > I assume that it should be safe for all "real" DMA channels.
> > We can see that it is not safe when using DMA API + "dummy" memcpy
> > dma-channel. (That is why I was asking if we need a NEEDS_MAP, or
> > MAP_NOT_NEEDED flag.)


> > pci-epf-test.c:
> > -Always calls pci_epc_map_addr() when using DMA API.
> > 
> > pci-epf-mhi.c:
> > -Never calls pci_epc_map_addr() when using DMA API.

Mani, I still think that this part is inconsistent between PCI EPF drivers.

Looking more at commit:
8353813c88ef ("PCI: endpoint: Enable DMA tests for endpoints with DMA capabilities")

Adding Frank on CC, since he is the author of that commit.

When the commit added support for eDMA to pci-epf-test, it added an extra
parameter to pci_epf_test_data_transfer(), to pass the PCI/DMA address of
the remote buffer, in addition to the already provided local physical address
that pci_epc_map_addr() has mapped the PCI/DMA address to.

So in the case of eDMA transfer, the pci_epc_map_addr() operation is still
being performed, even though pci-epf-test never uses the result of the
the mapping operation... This is just confusing and a waste of CPU cycles.

What I would like is more consistency between the EPF drivers.

I guess an if-statement that skips the pci_epc_map_addr() in pci-epf-test
if using eDMA would make pci-epf-mhi and pci-epf-test most consistent.


However, when reading the DWC databook:
-The eDMA and HDMA always goes via the iATU table.
If you do not want this, then you need to set the the appropriate bypass bit.


For eDMA:
""
When you do not want the iATU to translate outbound requests that are generated by the
internal DMA module, then you must implement one of the following approaches:
- Ensure that the combination of DMA channel programming and iATU control register
programming, causes no translation of DMA traffic to be done in the iATU.
or
- Activate the DMA bypass mode to allow request TLPs which are initiated by the DMA
controller to pass through the iATU untranslated. You can activate DMA bypass mode by
setting the DMA Bypass field of the iATU Control 2 Register (IATU_REGION_C-
TRL_OFF_2_OUTBOUND_0).
""

For HDMA:
""
When you do not want the iATU to translate outbound requests that are generated by the
internal HDMA module, then you must implement one of the following approaches:
- Ensure that the combination of HDMA channel programming and iATU control register
programming, causes no translation of DMA traffic to be done in the iATU.
or
- Activate the HDMA bypass mode to allow request TLPs which are initiated by the HDMA
controller to pass through the iATU untranslated. You can activate HDMA bypass mode by
setting the HDMA Bypass field of the iATU Control 2 Register (IATU_REGION_C-
TRL_OFF_2_OUTBOUND_0).
""

We also know that, if there is no match in the iATU table:
""
The default behavior of the ATU when there is no address match in the outbound direction or no
TLP attribute match in the inbound direction, is to pass the transaction through.
""

So even if we do not call pci_epc_map_addr(), the eDMA and HDMA will go via
the iATU table, it will most likely not find a match, so it will go through
untranslated.

So I think we need to answer these questions:
1) Do we want to rely on the fact that hopefully none of the iATUs in the DWC
controller has configured a mapping that might mess things up for us?
I don't see why the PCI/DMA address of the remote buffer, supplied to
pci-epf-test via test_reg BAR, might not fall within the physical iATU window
on the local EP system. (As long as the PCI EPF driver has mapped any address
using pci_epc_map_addr().)

This is a big argument that EPF drivers running on a DWC-based EPC should
definitely NOT call pci_epc_map_addr() needlessly when using eDMA, as it
can be catastrophic. (pci-epf-test needs to be patched.)


2) Can we really assume that both pci-epf-test and pci-epf-mhi does not need
to call pci_epc_map_addr() when using a DMA_SLAVE DMA controller?
This seems to be designed only with DWC in mind. Other PCIe endpoint
controllers might require this.
(Yes, for DWC-based controllers, this definitely should be skipped, but EPF
drivers are supposed to be independent from a specific EPC.)

I'm fine with just avoiding the pci_epc_map_addr() call when using DMA_SLAVE
DMA in pci-epf-test for now, as that is the only DMA controller that I'm
familiar with. This second question was more a question for how EPF drivers
are should be designed now and in the future.


Kind regards,
Niklas

Re: [PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback

[Manivannan Sadhasivam]

On Thu, Apr 04, 2024 at 10:44:40AM +0200, Niklas Cassel wrote:
> On Fri, Mar 29, 2024 at 12:12:48AM +0530, Vinod Koul wrote:
> > On 27-03-24, 12:39, Niklas Cassel wrote:
> > > 
> > > So my question is:
> > > Is the "dummy memcpy" DMA channel always available?
> > 
> > That depends on the system, you may or maynot have such a system where
> > you have a generic memcpy dma controller which can provide you with
> > these channels
> 
> I misunderstood DMA_MEMCPY then, I assumed that it was a "software emulated"
> DMA channel, which allowed the a driver to always use dmaengine + DMA API.
> 
> It actually uses a real DMA controller. I don't have any DMA controller in
> the PCIe EP device tree node, but perhaps it can use any DMA controller that
> has been registered with dmaengine?
> 

AFAIK, most of the dma controllers support both memcpy and separate tx/rx
channels except a few like dw-edma where memcpy is not supported.

But for just memcpy, clients can use any registered DMA controller in the
system. For slave channels, it is best to pass them in DT since the client may
not know how the channels are laid out in the DMA controller.

> 
> > Based on my reading you might have this mechanism:
> > - eDMA provides dmaengine_prep_slave_single() which transfers data from
> >   mem to pci ep device, so fasted
> > - dmaengine_prep_dma_memcpy: This will copy the data but treat it as
> >   memory. I dont pci internals to figure out how both can work... So
> >   cant really make out why it is slowed
> > - memcpy_xxx that is IO mem functions, so ofc they will be slowest
> > 
> > I think the code is decent from fallback pov... chooses fastest path if
> > available on a system
> 
> Indeed, it makes more sense to me now, thank you Vinod.
> 
> 
> > > I was wrong here, pci-epf-test always calls pci_epc_map_addr()
> > > regardless if FLAG_USE_DMA is set or not.
> > > 
> > > (Even though this should be unnecessary when using the eDMA.)
> > > 
> > > However, if we look at pci-epf-mhi.c we can see that it does
> > > NOT call pci_epc_map_addr() when using DMA API + dmaengine.
> > > 
> > > Is it really safe to avoid pci_epc_map_addr() in all EPC controllers?
> > > I assume that it should be safe for all "real" DMA channels.
> > > We can see that it is not safe when using DMA API + "dummy" memcpy
> > > dma-channel. (That is why I was asking if we need a NEEDS_MAP, or
> > > MAP_NOT_NEEDED flag.)
> 
> 
> > > pci-epf-test.c:
> > > -Always calls pci_epc_map_addr() when using DMA API.
> > > 
> > > pci-epf-mhi.c:
> > > -Never calls pci_epc_map_addr() when using DMA API.
> 
> Mani, I still think that this part is inconsistent between PCI EPF drivers.
> 
> Looking more at commit:
> 8353813c88ef ("PCI: endpoint: Enable DMA tests for endpoints with DMA capabilities")
> 
> Adding Frank on CC, since he is the author of that commit.
> 
> When the commit added support for eDMA to pci-epf-test, it added an extra
> parameter to pci_epf_test_data_transfer(), to pass the PCI/DMA address of
> the remote buffer, in addition to the already provided local physical address
> that pci_epc_map_addr() has mapped the PCI/DMA address to.
> 
> So in the case of eDMA transfer, the pci_epc_map_addr() operation is still
> being performed, even though pci-epf-test never uses the result of the
> the mapping operation... This is just confusing and a waste of CPU cycles.
> 
> What I would like is more consistency between the EPF drivers.
> 
> I guess an if-statement that skips the pci_epc_map_addr() in pci-epf-test
> if using eDMA would make pci-epf-mhi and pci-epf-test most consistent.
> 

Agree.

> 
> However, when reading the DWC databook:
> -The eDMA and HDMA always goes via the iATU table.
> If you do not want this, then you need to set the the appropriate bypass bit.
> 
> 
> For eDMA:
> ""
> When you do not want the iATU to translate outbound requests that are generated by the
> internal DMA module, then you must implement one of the following approaches:
> - Ensure that the combination of DMA channel programming and iATU control register
> programming, causes no translation of DMA traffic to be done in the iATU.
> or
> - Activate the DMA bypass mode to allow request TLPs which are initiated by the DMA
> controller to pass through the iATU untranslated. You can activate DMA bypass mode by
> setting the DMA Bypass field of the iATU Control 2 Register (IATU_REGION_C-
> TRL_OFF_2_OUTBOUND_0).
> ""
> 
> For HDMA:
> ""
> When you do not want the iATU to translate outbound requests that are generated by the
> internal HDMA module, then you must implement one of the following approaches:
> - Ensure that the combination of HDMA channel programming and iATU control register
> programming, causes no translation of DMA traffic to be done in the iATU.
> or
> - Activate the HDMA bypass mode to allow request TLPs which are initiated by the HDMA
> controller to pass through the iATU untranslated. You can activate HDMA bypass mode by
> setting the HDMA Bypass field of the iATU Control 2 Register (IATU_REGION_C-
> TRL_OFF_2_OUTBOUND_0).
> ""
> 
> We also know that, if there is no match in the iATU table:
> ""
> The default behavior of the ATU when there is no address match in the outbound direction or no
> TLP attribute match in the inbound direction, is to pass the transaction through.
> ""
> 
> So even if we do not call pci_epc_map_addr(), the eDMA and HDMA will go via
> the iATU table, it will most likely not find a match, so it will go through
> untranslated.
> 
> So I think we need to answer these questions:
> 1) Do we want to rely on the fact that hopefully none of the iATUs in the DWC
> controller has configured a mapping that might mess things up for us?
> I don't see why the PCI/DMA address of the remote buffer, supplied to
> pci-epf-test via test_reg BAR, might not fall within the physical iATU window
> on the local EP system. (As long as the PCI EPF driver has mapped any address
> using pci_epc_map_addr().)
> 
> This is a big argument that EPF drivers running on a DWC-based EPC should
> definitely NOT call pci_epc_map_addr() needlessly when using eDMA, as it
> can be catastrophic. (pci-epf-test needs to be patched.)
> 

Right. There is no need to do iATU translation for DMA. I avoid that in MHI
driver.

> 
> 2) Can we really assume that both pci-epf-test and pci-epf-mhi does not need
> to call pci_epc_map_addr() when using a DMA_SLAVE DMA controller?
> This seems to be designed only with DWC in mind. Other PCIe endpoint
> controllers might require this.
> (Yes, for DWC-based controllers, this definitely should be skipped, but EPF
> drivers are supposed to be independent from a specific EPC.)
> 

For TEST yes, but for MHI, no. In MHI, I kind of mix both iATU and DMA to ripe
most of the performance (small vs big transactions). But for the TEST driver, it
is fair to not call pci_epc_map_addr() when DMA_SLAVE is supported.

I do feel that we need to maintain the similarity within the EPF drivers, but it
is OK to let the drivers diverge a little for optimization.

> I'm fine with just avoiding the pci_epc_map_addr() call when using DMA_SLAVE
> DMA in pci-epf-test for now, as that is the only DMA controller that I'm
> familiar with. This second question was more a question for how EPF drivers
> are should be designed now and in the future.
> 

Regarding the DMA_MEMCPY code in TEST driver. We need to keep it for backwards
compatibility since not all platforms are passing the slave channels in
devicetree.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback

[Niklas Cassel]

On Mon, Apr 22, 2024 at 01:25:21PM +0530, Manivannan Sadhasivam wrote:
> > 
> > What I would like is more consistency between the EPF drivers.
> > 
> > I guess an if-statement that skips the pci_epc_map_addr() in pci-epf-test
> > if using eDMA would make pci-epf-mhi and pci-epf-test most consistent.
> > 
> 
> Agree.


> > 1) Do we want to rely on the fact that hopefully none of the iATUs in the DWC
> > controller has configured a mapping that might mess things up for us?
> > I don't see why the PCI/DMA address of the remote buffer, supplied to
> > pci-epf-test via test_reg BAR, might not fall within the physical iATU window
> > on the local EP system. (As long as the PCI EPF driver has mapped any address
> > using pci_epc_map_addr().)
> > 
> > This is a big argument that EPF drivers running on a DWC-based EPC should
> > definitely NOT call pci_epc_map_addr() needlessly when using eDMA, as it
> > can be catastrophic. (pci-epf-test needs to be patched.)
> > 
> 
> Right. There is no need to do iATU translation for DMA. I avoid that in MHI
> driver.

There is no need for pci_epc_map_addr() when using DMA_SLAVE *for DWC-based
controllers*.

Are we certain that this will not break pci-epf-test for non DWC-based
controllers?


> > 2) Can we really assume that both pci-epf-test and pci-epf-mhi does not need
> > to call pci_epc_map_addr() when using a DMA_SLAVE DMA controller?
> > This seems to be designed only with DWC in mind. Other PCIe endpoint
> > controllers might require this.
> > (Yes, for DWC-based controllers, this definitely should be skipped, but EPF
> > drivers are supposed to be independent from a specific EPC.)
> > 
> 
> For TEST yes, but for MHI, no. In MHI, I kind of mix both iATU and DMA to ripe
> most of the performance (small vs big transactions). But for the TEST driver, it
> is fair to not call pci_epc_map_addr() when DMA_SLAVE is supported.

I agree that we should definitely skip pci_epc_map_addr() in pci-epf-test when
using DMA_SLAVE on DWC-based controllers, but I don't feel comfortable in
submitting a patch that does this unconditionally for pci-epf-test.c,
as I don't know how the DMA hardware in:
drivers/pci/controller/cadence/pcie-cadence-ep.c
drivers/pci/controller/pcie-rcar-ep.c
drivers/pci/controller/pcie-rockchip-ep.c

works, and I do not want to regress them.

I did suggest that DWC-based drivers could set a DMA_SLAVE_SKIP_MEM_MAP flag
or similar when registering the eDMA, which pci-epf-test then could check,
but I got no response if anoyone else thought that this was a good idea.


Kind regards,
Niklas

[PATCH 06/11] PCI: endpoint: Introduce EPC 'deinit' event and notify the EPF drivers

[Manivannan Sadhasivam]

As like the EPC 'init' event, that is used to signal the EPF drivers about
the EPC initialization, let's introduce 'deinit' event that is used to
signal EPC deinitialization.

The EPC deinitialization applies only when any sort of fundamental reset
is supported by the endpoint controller as per the PCIe spec.

Reference: PCIe Base spec v5.0, sections 4.2.4.9.1 and 6.6.1.

Currently, some EPC drivers like pcie-qcom-ep and pcie-tegra194 support
PERST# as the fundamental reset. So the 'deinit' event will be notified to
the EPF drivers when PERST# assert happens in the above mentioned EPC
drivers.

The EPF drivers, on receiving the event through the deinit() callback
should reset the EPF state machine and also cleanup any configuration that
got affected by the fundamental reset like BAR, DMA etc...

This change also warrants skipping the cleanups in unbind() if already done
in deinit().

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom-ep.c     |  1 +
 drivers/pci/controller/dwc/pcie-tegra194.c    |  1 +
 drivers/pci/endpoint/functions/pci-epf-mhi.c  | 19 +++++++++++++++++++
 drivers/pci/endpoint/functions/pci-epf-test.c | 17 +++++++++++++++--
 drivers/pci/endpoint/pci-epc-core.c           | 25 +++++++++++++++++++++++++
 include/linux/pci-epc.h                       |  1 +
 include/linux/pci-epf.h                       |  2 ++
 7 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index 50b1635e3cbb..e4b742355d57 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -501,6 +501,7 @@ static void qcom_pcie_perst_assert(struct dw_pcie *pci)
 {
 	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
 
+	pci_epc_deinit_notify(pci->ep.epc);
 	dw_pcie_ep_cleanup(&pci->ep);
 	qcom_pcie_disable_resources(pcie_ep);
 	pcie_ep->link_status = QCOM_PCIE_EP_LINK_DISABLED;
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index e02deb31a72d..3e6e08b321fb 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1715,6 +1715,7 @@ static void pex_ep_event_pex_rst_assert(struct tegra_pcie_dw *pcie)
 	if (ret)
 		dev_err(pcie->dev, "Failed to go Detect state: %d\n", ret);
 
+	pci_epc_deinit_notify(pcie->pci.ep.epc);
 	dw_pcie_ep_cleanup(&pcie->pci.ep);
 
 	reset_control_assert(pcie->core_rst);
diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
index 4e4300efd9d7..83de96119718 100644
--- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
+++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
@@ -748,6 +748,24 @@ static int pci_epf_mhi_epc_init(struct pci_epf *epf)
 	return 0;
 }
 
+static void pci_epf_mhi_epc_deinit(struct pci_epf *epf)
+{
+	struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
+	const struct pci_epf_mhi_ep_info *info = epf_mhi->info;
+	struct pci_epf_bar *epf_bar = &epf->bar[info->bar_num];
+	struct mhi_ep_cntrl *mhi_cntrl = &epf_mhi->mhi_cntrl;
+	struct pci_epc *epc = epf->epc;
+
+	if (mhi_cntrl->mhi_dev) {
+		mhi_ep_power_down(mhi_cntrl);
+		if (info->flags & MHI_EPF_USE_DMA)
+			pci_epf_mhi_dma_deinit(epf_mhi);
+		mhi_ep_unregister_controller(mhi_cntrl);
+	}
+
+	pci_epc_clear_bar(epc, epf->func_no, epf->vfunc_no, epf_bar);
+}
+
 static int pci_epf_mhi_link_up(struct pci_epf *epf)
 {
 	struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
@@ -882,6 +900,7 @@ static void pci_epf_mhi_unbind(struct pci_epf *epf)
 
 static const struct pci_epc_event_ops pci_epf_mhi_epc_event_ops = {
 	.init = pci_epf_mhi_epc_init,
+	.deinit = pci_epf_mhi_epc_deinit,
 };
 
 static const struct pci_epc_bus_event_ops pci_epf_mhi_bus_event_ops = {
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 8f1e0cb08814..84cd47ebac22 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -804,6 +804,15 @@ static int pci_epf_test_epc_init(struct pci_epf *epf)
 	return 0;
 }
 
+static void pci_epf_test_epc_deinit(struct pci_epf *epf)
+{
+	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
+
+	cancel_delayed_work(&epf_test->cmd_handler);
+	pci_epf_test_clean_dma_chan(epf_test);
+	pci_epf_test_clear_bar(epf);
+}
+
 static int pci_epf_test_link_up(struct pci_epf *epf)
 {
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
@@ -816,6 +825,7 @@ static int pci_epf_test_link_up(struct pci_epf *epf)
 
 static const struct pci_epc_event_ops pci_epf_test_epc_event_ops = {
 	.init = pci_epf_test_epc_init,
+	.deinit = pci_epf_test_epc_deinit,
 };
 
 static const struct pci_epc_bus_event_ops pci_epf_test_bus_event_ops = {
@@ -954,10 +964,13 @@ static int pci_epf_test_bind(struct pci_epf *epf)
 static void pci_epf_test_unbind(struct pci_epf *epf)
 {
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
+	struct pci_epc *epc = epf->epc;
 
 	cancel_delayed_work(&epf_test->cmd_handler);
-	pci_epf_test_clean_dma_chan(epf_test);
-	pci_epf_test_clear_bar(epf);
+	if (epc->init_complete) {
+		pci_epf_test_clean_dma_chan(epf_test);
+		pci_epf_test_clear_bar(epf);
+	}
 	pci_epf_test_free_space(epf);
 }
 
diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index 5a522b2842e2..26378a9a56a7 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -779,6 +779,31 @@ void pci_epc_notify_pending_init(struct pci_epc *epc, struct pci_epf *epf)
 }
 EXPORT_SYMBOL_GPL(pci_epc_notify_pending_init);
 
+/**
+ * pci_epc_deinit_notify() - Notify the EPF device about EPC deinitialization
+ * @epc: the EPC device whose deinitialization is completed
+ *
+ * Invoke to notify the EPF device that the EPC deinitialization is completed.
+ */
+void pci_epc_deinit_notify(struct pci_epc *epc)
+{
+	struct pci_epf *epf;
+
+	if (IS_ERR_OR_NULL(epc))
+		return;
+
+	mutex_lock(&epc->list_lock);
+	list_for_each_entry(epf, &epc->pci_epf, list) {
+		mutex_lock(&epf->lock);
+		if (epf->epc_event_ops && epf->epc_event_ops->deinit)
+			epf->epc_event_ops->deinit(epf);
+		mutex_unlock(&epf->lock);
+	}
+	epc->init_complete = false;
+	mutex_unlock(&epc->list_lock);
+}
+EXPORT_SYMBOL_GPL(pci_epc_deinit_notify);
+
 /**
  * pci_epc_bme_notify() - Notify the EPF device that the EPC device has received
  *			  the BME event from the Root complex
diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
index adee6dbe4e45..976b2212e872 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -199,6 +199,7 @@ void pci_epc_linkup(struct pci_epc *epc);
 void pci_epc_linkdown(struct pci_epc *epc);
 void pci_epc_init_notify(struct pci_epc *epc);
 void pci_epc_notify_pending_init(struct pci_epc *epc, struct pci_epf *epf);
+void pci_epc_deinit_notify(struct pci_epc *epc);
 void pci_epc_bme_notify(struct pci_epc *epc);
 void pci_epc_remove_epf(struct pci_epc *epc, struct pci_epf *epf,
 			enum pci_epc_interface_type type);
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index ff8304e72f8e..52f69eaf505d 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -70,9 +70,11 @@ struct pci_epf_ops {
 /**
  * struct pci_epc_event_ops - Callbacks for capturing the EPC specific events
  * @init: Callback for the EPC initialization event
+ * @deinit: Callback for the EPC deinitialization event
  */
 struct pci_epc_event_ops {
 	int (*init)(struct pci_epf *epf);
+	void (*deinit)(struct pci_epf *epf);
 };
 
 /**

-- 
2.25.1

Re: [PATCH 06/11] PCI: endpoint: Introduce EPC 'deinit' event and notify the EPF drivers

[Niklas Cassel]

On Thu, Mar 14, 2024 at 08:53:45PM +0530, Manivannan Sadhasivam wrote:
> As like the EPC 'init' event, that is used to signal the EPF drivers about
> the EPC initialization, let's introduce 'deinit' event that is used to
> signal EPC deinitialization.
> 
> The EPC deinitialization applies only when any sort of fundamental reset
> is supported by the endpoint controller as per the PCIe spec.
> 
> Reference: PCIe Base spec v5.0, sections 4.2.4.9.1 and 6.6.1.
> 
> Currently, some EPC drivers like pcie-qcom-ep and pcie-tegra194 support
> PERST# as the fundamental reset. So the 'deinit' event will be notified to
> the EPF drivers when PERST# assert happens in the above mentioned EPC
> drivers.
> 
> The EPF drivers, on receiving the event through the deinit() callback
> should reset the EPF state machine and also cleanup any configuration that
> got affected by the fundamental reset like BAR, DMA etc...
> 
> This change also warrants skipping the cleanups in unbind() if already done
> in deinit().
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom-ep.c     |  1 +
>  drivers/pci/controller/dwc/pcie-tegra194.c    |  1 +
>  drivers/pci/endpoint/functions/pci-epf-mhi.c  | 19 +++++++++++++++++++
>  drivers/pci/endpoint/functions/pci-epf-test.c | 17 +++++++++++++++--
>  drivers/pci/endpoint/pci-epc-core.c           | 25 +++++++++++++++++++++++++
>  include/linux/pci-epc.h                       |  1 +
>  include/linux/pci-epf.h                       |  2 ++
>  7 files changed, 64 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index 50b1635e3cbb..e4b742355d57 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -501,6 +501,7 @@ static void qcom_pcie_perst_assert(struct dw_pcie *pci)
>  {
>  	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
>  
> +	pci_epc_deinit_notify(pci->ep.epc);

Why not just move pci_epc_deinit_notify() in to dw_pcie_ep_cleanup() ?
(So that we don't need to add pci_epc_deinit_notify() to all EPC drivers
with PERST?)

Regardless:
Reviewed-by: Niklas Cassel <cassel@kernel.org>

>  	dw_pcie_ep_cleanup(&pci->ep);
>  	qcom_pcie_disable_resources(pcie_ep);
>  	pcie_ep->link_status = QCOM_PCIE_EP_LINK_DISABLED;
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index e02deb31a72d..3e6e08b321fb 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -1715,6 +1715,7 @@ static void pex_ep_event_pex_rst_assert(struct tegra_pcie_dw *pcie)
>  	if (ret)
>  		dev_err(pcie->dev, "Failed to go Detect state: %d\n", ret);
>  
> +	pci_epc_deinit_notify(pcie->pci.ep.epc);
>  	dw_pcie_ep_cleanup(&pcie->pci.ep);
>  
>  	reset_control_assert(pcie->core_rst);
> diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> index 4e4300efd9d7..83de96119718 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> @@ -748,6 +748,24 @@ static int pci_epf_mhi_epc_init(struct pci_epf *epf)
>  	return 0;
>  }
>  
> +static void pci_epf_mhi_epc_deinit(struct pci_epf *epf)
> +{
> +	struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
> +	const struct pci_epf_mhi_ep_info *info = epf_mhi->info;
> +	struct pci_epf_bar *epf_bar = &epf->bar[info->bar_num];
> +	struct mhi_ep_cntrl *mhi_cntrl = &epf_mhi->mhi_cntrl;
> +	struct pci_epc *epc = epf->epc;
> +
> +	if (mhi_cntrl->mhi_dev) {
> +		mhi_ep_power_down(mhi_cntrl);
> +		if (info->flags & MHI_EPF_USE_DMA)
> +			pci_epf_mhi_dma_deinit(epf_mhi);
> +		mhi_ep_unregister_controller(mhi_cntrl);
> +	}
> +
> +	pci_epc_clear_bar(epc, epf->func_no, epf->vfunc_no, epf_bar);
> +}
> +
>  static int pci_epf_mhi_link_up(struct pci_epf *epf)
>  {
>  	struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
> @@ -882,6 +900,7 @@ static void pci_epf_mhi_unbind(struct pci_epf *epf)
>  
>  static const struct pci_epc_event_ops pci_epf_mhi_epc_event_ops = {
>  	.init = pci_epf_mhi_epc_init,
> +	.deinit = pci_epf_mhi_epc_deinit,
>  };
>  
>  static const struct pci_epc_bus_event_ops pci_epf_mhi_bus_event_ops = {
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 8f1e0cb08814..84cd47ebac22 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -804,6 +804,15 @@ static int pci_epf_test_epc_init(struct pci_epf *epf)
>  	return 0;
>  }
>  
> +static void pci_epf_test_epc_deinit(struct pci_epf *epf)
> +{
> +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> +
> +	cancel_delayed_work(&epf_test->cmd_handler);
> +	pci_epf_test_clean_dma_chan(epf_test);
> +	pci_epf_test_clear_bar(epf);
> +}
> +
>  static int pci_epf_test_link_up(struct pci_epf *epf)
>  {
>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> @@ -816,6 +825,7 @@ static int pci_epf_test_link_up(struct pci_epf *epf)
>  
>  static const struct pci_epc_event_ops pci_epf_test_epc_event_ops = {
>  	.init = pci_epf_test_epc_init,
> +	.deinit = pci_epf_test_epc_deinit,
>  };
>  
>  static const struct pci_epc_bus_event_ops pci_epf_test_bus_event_ops = {
> @@ -954,10 +964,13 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  static void pci_epf_test_unbind(struct pci_epf *epf)
>  {
>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> +	struct pci_epc *epc = epf->epc;
>  
>  	cancel_delayed_work(&epf_test->cmd_handler);
> -	pci_epf_test_clean_dma_chan(epf_test);
> -	pci_epf_test_clear_bar(epf);
> +	if (epc->init_complete) {
> +		pci_epf_test_clean_dma_chan(epf_test);
> +		pci_epf_test_clear_bar(epf);
> +	}
>  	pci_epf_test_free_space(epf);
>  }
>  
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 5a522b2842e2..26378a9a56a7 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -779,6 +779,31 @@ void pci_epc_notify_pending_init(struct pci_epc *epc, struct pci_epf *epf)
>  }
>  EXPORT_SYMBOL_GPL(pci_epc_notify_pending_init);
>  
> +/**
> + * pci_epc_deinit_notify() - Notify the EPF device about EPC deinitialization
> + * @epc: the EPC device whose deinitialization is completed
> + *
> + * Invoke to notify the EPF device that the EPC deinitialization is completed.
> + */
> +void pci_epc_deinit_notify(struct pci_epc *epc)
> +{
> +	struct pci_epf *epf;
> +
> +	if (IS_ERR_OR_NULL(epc))
> +		return;
> +
> +	mutex_lock(&epc->list_lock);
> +	list_for_each_entry(epf, &epc->pci_epf, list) {
> +		mutex_lock(&epf->lock);
> +		if (epf->epc_event_ops && epf->epc_event_ops->deinit)
> +			epf->epc_event_ops->deinit(epf);
> +		mutex_unlock(&epf->lock);
> +	}
> +	epc->init_complete = false;
> +	mutex_unlock(&epc->list_lock);
> +}
> +EXPORT_SYMBOL_GPL(pci_epc_deinit_notify);
> +
>  /**
>   * pci_epc_bme_notify() - Notify the EPF device that the EPC device has received
>   *			  the BME event from the Root complex
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index adee6dbe4e45..976b2212e872 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -199,6 +199,7 @@ void pci_epc_linkup(struct pci_epc *epc);
>  void pci_epc_linkdown(struct pci_epc *epc);
>  void pci_epc_init_notify(struct pci_epc *epc);
>  void pci_epc_notify_pending_init(struct pci_epc *epc, struct pci_epf *epf);
> +void pci_epc_deinit_notify(struct pci_epc *epc);
>  void pci_epc_bme_notify(struct pci_epc *epc);
>  void pci_epc_remove_epf(struct pci_epc *epc, struct pci_epf *epf,
>  			enum pci_epc_interface_type type);
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index ff8304e72f8e..52f69eaf505d 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -70,9 +70,11 @@ struct pci_epf_ops {
>  /**
>   * struct pci_epc_event_ops - Callbacks for capturing the EPC specific events
>   * @init: Callback for the EPC initialization event
> + * @deinit: Callback for the EPC deinitialization event
>   */
>  struct pci_epc_event_ops {
>  	int (*init)(struct pci_epf *epf);
> +	void (*deinit)(struct pci_epf *epf);
>  };
>  
>  /**
> 
> -- 
> 2.25.1
> 

Re: [PATCH 06/11] PCI: endpoint: Introduce EPC 'deinit' event and notify the EPF drivers

[Manivannan Sadhasivam]

On Fri, Mar 22, 2024 at 05:10:17PM +0100, Niklas Cassel wrote:
> On Thu, Mar 14, 2024 at 08:53:45PM +0530, Manivannan Sadhasivam wrote:
> > As like the EPC 'init' event, that is used to signal the EPF drivers about
> > the EPC initialization, let's introduce 'deinit' event that is used to
> > signal EPC deinitialization.
> > 
> > The EPC deinitialization applies only when any sort of fundamental reset
> > is supported by the endpoint controller as per the PCIe spec.
> > 
> > Reference: PCIe Base spec v5.0, sections 4.2.4.9.1 and 6.6.1.
> > 
> > Currently, some EPC drivers like pcie-qcom-ep and pcie-tegra194 support
> > PERST# as the fundamental reset. So the 'deinit' event will be notified to
> > the EPF drivers when PERST# assert happens in the above mentioned EPC
> > drivers.
> > 
> > The EPF drivers, on receiving the event through the deinit() callback
> > should reset the EPF state machine and also cleanup any configuration that
> > got affected by the fundamental reset like BAR, DMA etc...
> > 
> > This change also warrants skipping the cleanups in unbind() if already done
> > in deinit().
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom-ep.c     |  1 +
> >  drivers/pci/controller/dwc/pcie-tegra194.c    |  1 +
> >  drivers/pci/endpoint/functions/pci-epf-mhi.c  | 19 +++++++++++++++++++
> >  drivers/pci/endpoint/functions/pci-epf-test.c | 17 +++++++++++++++--
> >  drivers/pci/endpoint/pci-epc-core.c           | 25 +++++++++++++++++++++++++
> >  include/linux/pci-epc.h                       |  1 +
> >  include/linux/pci-epf.h                       |  2 ++
> >  7 files changed, 64 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > index 50b1635e3cbb..e4b742355d57 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > @@ -501,6 +501,7 @@ static void qcom_pcie_perst_assert(struct dw_pcie *pci)
> >  {
> >  	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> >  
> > +	pci_epc_deinit_notify(pci->ep.epc);
> 
> Why not just move pci_epc_deinit_notify() in to dw_pcie_ep_cleanup() ?
> (So that we don't need to add pci_epc_deinit_notify() to all EPC drivers
> with PERST?)
> 

This is mostly done to keep similarity with dw_pcie_ep_init_notify(). Even
though it is a helper, it explicitly says that the function sends init
notification. Otherwise, it will confuse developers on who is calling the
deinit_notify(). I believe there is already enough mess to confuse the
newcomers ;)

- Mani

> Regardless:
> Reviewed-by: Niklas Cassel <cassel@kernel.org>
> 
> >  	dw_pcie_ep_cleanup(&pci->ep);
> >  	qcom_pcie_disable_resources(pcie_ep);
> >  	pcie_ep->link_status = QCOM_PCIE_EP_LINK_DISABLED;
> > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > index e02deb31a72d..3e6e08b321fb 100644
> > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > @@ -1715,6 +1715,7 @@ static void pex_ep_event_pex_rst_assert(struct tegra_pcie_dw *pcie)
> >  	if (ret)
> >  		dev_err(pcie->dev, "Failed to go Detect state: %d\n", ret);
> >  
> > +	pci_epc_deinit_notify(pcie->pci.ep.epc);
> >  	dw_pcie_ep_cleanup(&pcie->pci.ep);
> >  
> >  	reset_control_assert(pcie->core_rst);
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > index 4e4300efd9d7..83de96119718 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > @@ -748,6 +748,24 @@ static int pci_epf_mhi_epc_init(struct pci_epf *epf)
> >  	return 0;
> >  }
> >  
> > +static void pci_epf_mhi_epc_deinit(struct pci_epf *epf)
> > +{
> > +	struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
> > +	const struct pci_epf_mhi_ep_info *info = epf_mhi->info;
> > +	struct pci_epf_bar *epf_bar = &epf->bar[info->bar_num];
> > +	struct mhi_ep_cntrl *mhi_cntrl = &epf_mhi->mhi_cntrl;
> > +	struct pci_epc *epc = epf->epc;
> > +
> > +	if (mhi_cntrl->mhi_dev) {
> > +		mhi_ep_power_down(mhi_cntrl);
> > +		if (info->flags & MHI_EPF_USE_DMA)
> > +			pci_epf_mhi_dma_deinit(epf_mhi);
> > +		mhi_ep_unregister_controller(mhi_cntrl);
> > +	}
> > +
> > +	pci_epc_clear_bar(epc, epf->func_no, epf->vfunc_no, epf_bar);
> > +}
> > +
> >  static int pci_epf_mhi_link_up(struct pci_epf *epf)
> >  {
> >  	struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
> > @@ -882,6 +900,7 @@ static void pci_epf_mhi_unbind(struct pci_epf *epf)
> >  
> >  static const struct pci_epc_event_ops pci_epf_mhi_epc_event_ops = {
> >  	.init = pci_epf_mhi_epc_init,
> > +	.deinit = pci_epf_mhi_epc_deinit,
> >  };
> >  
> >  static const struct pci_epc_bus_event_ops pci_epf_mhi_bus_event_ops = {
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index 8f1e0cb08814..84cd47ebac22 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -804,6 +804,15 @@ static int pci_epf_test_epc_init(struct pci_epf *epf)
> >  	return 0;
> >  }
> >  
> > +static void pci_epf_test_epc_deinit(struct pci_epf *epf)
> > +{
> > +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> > +
> > +	cancel_delayed_work(&epf_test->cmd_handler);
> > +	pci_epf_test_clean_dma_chan(epf_test);
> > +	pci_epf_test_clear_bar(epf);
> > +}
> > +
> >  static int pci_epf_test_link_up(struct pci_epf *epf)
> >  {
> >  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> > @@ -816,6 +825,7 @@ static int pci_epf_test_link_up(struct pci_epf *epf)
> >  
> >  static const struct pci_epc_event_ops pci_epf_test_epc_event_ops = {
> >  	.init = pci_epf_test_epc_init,
> > +	.deinit = pci_epf_test_epc_deinit,
> >  };
> >  
> >  static const struct pci_epc_bus_event_ops pci_epf_test_bus_event_ops = {
> > @@ -954,10 +964,13 @@ static int pci_epf_test_bind(struct pci_epf *epf)
> >  static void pci_epf_test_unbind(struct pci_epf *epf)
> >  {
> >  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> > +	struct pci_epc *epc = epf->epc;
> >  
> >  	cancel_delayed_work(&epf_test->cmd_handler);
> > -	pci_epf_test_clean_dma_chan(epf_test);
> > -	pci_epf_test_clear_bar(epf);
> > +	if (epc->init_complete) {
> > +		pci_epf_test_clean_dma_chan(epf_test);
> > +		pci_epf_test_clear_bar(epf);
> > +	}
> >  	pci_epf_test_free_space(epf);
> >  }
> >  
> > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> > index 5a522b2842e2..26378a9a56a7 100644
> > --- a/drivers/pci/endpoint/pci-epc-core.c
> > +++ b/drivers/pci/endpoint/pci-epc-core.c
> > @@ -779,6 +779,31 @@ void pci_epc_notify_pending_init(struct pci_epc *epc, struct pci_epf *epf)
> >  }
> >  EXPORT_SYMBOL_GPL(pci_epc_notify_pending_init);
> >  
> > +/**
> > + * pci_epc_deinit_notify() - Notify the EPF device about EPC deinitialization
> > + * @epc: the EPC device whose deinitialization is completed
> > + *
> > + * Invoke to notify the EPF device that the EPC deinitialization is completed.
> > + */
> > +void pci_epc_deinit_notify(struct pci_epc *epc)
> > +{
> > +	struct pci_epf *epf;
> > +
> > +	if (IS_ERR_OR_NULL(epc))
> > +		return;
> > +
> > +	mutex_lock(&epc->list_lock);
> > +	list_for_each_entry(epf, &epc->pci_epf, list) {
> > +		mutex_lock(&epf->lock);
> > +		if (epf->epc_event_ops && epf->epc_event_ops->deinit)
> > +			epf->epc_event_ops->deinit(epf);
> > +		mutex_unlock(&epf->lock);
> > +	}
> > +	epc->init_complete = false;
> > +	mutex_unlock(&epc->list_lock);
> > +}
> > +EXPORT_SYMBOL_GPL(pci_epc_deinit_notify);
> > +
> >  /**
> >   * pci_epc_bme_notify() - Notify the EPF device that the EPC device has received
> >   *			  the BME event from the Root complex
> > diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> > index adee6dbe4e45..976b2212e872 100644
> > --- a/include/linux/pci-epc.h
> > +++ b/include/linux/pci-epc.h
> > @@ -199,6 +199,7 @@ void pci_epc_linkup(struct pci_epc *epc);
> >  void pci_epc_linkdown(struct pci_epc *epc);
> >  void pci_epc_init_notify(struct pci_epc *epc);
> >  void pci_epc_notify_pending_init(struct pci_epc *epc, struct pci_epf *epf);
> > +void pci_epc_deinit_notify(struct pci_epc *epc);
> >  void pci_epc_bme_notify(struct pci_epc *epc);
> >  void pci_epc_remove_epf(struct pci_epc *epc, struct pci_epf *epf,
> >  			enum pci_epc_interface_type type);
> > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > index ff8304e72f8e..52f69eaf505d 100644
> > --- a/include/linux/pci-epf.h
> > +++ b/include/linux/pci-epf.h
> > @@ -70,9 +70,11 @@ struct pci_epf_ops {
> >  /**
> >   * struct pci_epc_event_ops - Callbacks for capturing the EPC specific events
> >   * @init: Callback for the EPC initialization event
> > + * @deinit: Callback for the EPC deinitialization event
> >   */
> >  struct pci_epc_event_ops {
> >  	int (*init)(struct pci_epf *epf);
> > +	void (*deinit)(struct pci_epf *epf);
> >  };
> >  
> >  /**
> > 
> > -- 
> > 2.25.1
> > 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 07/11] PCI: dwc: ep: Add a generic dw_pcie_ep_linkdown() API to handle Link Down event

[Manivannan Sadhasivam]

As per the PCIe base spec r5.0, section 5.2, Link Down event can happen
under any of the following circumstances:

1. Fundamental/Hot reset
2. Link disable transmission by upstream component
3. Moving from L2/L3 to L0

In those cases, Link Down causes some non-sticky DWC registers to loose the
state (like REBAR, etc...). So the drivers need to reinitialize them to
function properly once the link comes back again.

This is not a problem for drivers supporting PERST# IRQ, since they can
reinitialize the registers in the PERST# IRQ callback. But for the drivers
not supporting PERST#, there is no way they can reinitialize the registers
other than relying on Link Down IRQ received when the link goes down. So
let's add a DWC generic API dw_pcie_ep_linkdown() that reinitializes the
non-sticky registers and also notifies the EPF drivers about link going
down.

This API can also be used by the drivers supporting PERST# to handle the
scenario (2) mentioned above.

NOTE: For the sake of code organization, move the dw_pcie_ep_linkup()
definition just above dw_pcie_ep_linkdown().

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 93 ++++++++++++++++---------
 drivers/pci/controller/dwc/pcie-designware.h    |  5 ++
 2 files changed, 67 insertions(+), 31 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 3893a8c1a11c..5451057ca74b 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -14,18 +14,6 @@
 #include <linux/pci-epc.h>
 #include <linux/pci-epf.h>
 
-/**
- * dw_pcie_ep_linkup - Notify EPF drivers about link up event
- * @ep: DWC EP device
- */
-void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
-{
-	struct pci_epc *epc = ep->epc;
-
-	pci_epc_linkup(epc);
-}
-EXPORT_SYMBOL_GPL(dw_pcie_ep_linkup);
-
 /**
  * dw_pcie_ep_init_notify - Notify EPF drivers about EPC initialization
  *			    complete
@@ -672,6 +660,29 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
 	return 0;
 }
 
+static void dw_pcie_ep_init_non_sticky_registers(struct dw_pcie *pci)
+{
+	unsigned int offset;
+	unsigned int nbars;
+	u32 reg, i;
+
+	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
+
+	dw_pcie_dbi_ro_wr_en(pci);
+
+	if (offset) {
+		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
+		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
+			PCI_REBAR_CTRL_NBAR_SHIFT;
+
+		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
+			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
+	}
+
+	dw_pcie_setup(pci);
+	dw_pcie_dbi_ro_wr_dis(pci);
+}
+
 /**
  * dw_pcie_ep_init_registers - Initialize DWC EP specific registers
  * @ep: DWC EP device
@@ -686,13 +697,11 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep)
 	struct dw_pcie_ep_func *ep_func;
 	struct device *dev = pci->dev;
 	struct pci_epc *epc = ep->epc;
-	unsigned int offset, ptm_cap_base;
-	unsigned int nbars;
+	u32 ptm_cap_base, reg;
 	u8 hdr_type;
 	u8 func_no;
-	int i, ret;
 	void *addr;
-	u32 reg;
+	int ret;
 
 	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) &
 		   PCI_HEADER_TYPE_MASK;
@@ -755,20 +764,8 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep)
 	if (ep->ops->init)
 		ep->ops->init(ep);
 
-	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
 	ptm_cap_base = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_PTM);
 
-	dw_pcie_dbi_ro_wr_en(pci);
-
-	if (offset) {
-		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
-		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
-			PCI_REBAR_CTRL_NBAR_SHIFT;
-
-		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
-			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
-	}
-
 	/*
 	 * PTM responder capability can be disabled only after disabling
 	 * PTM root capability.
@@ -785,9 +782,6 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep)
 		dw_pcie_dbi_ro_wr_dis(pci);
 	}
 
-	dw_pcie_setup(pci);
-	dw_pcie_dbi_ro_wr_dis(pci);
-
 	return 0;
 
 err_remove_edma:
@@ -797,6 +791,43 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep)
 }
 EXPORT_SYMBOL_GPL(dw_pcie_ep_init_registers);
 
+/**
+ * dw_pcie_ep_linkup - Notify EPF drivers about Link Up event
+ * @ep: DWC EP device
+ */
+void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
+{
+	struct pci_epc *epc = ep->epc;
+
+	pci_epc_linkup(epc);
+}
+EXPORT_SYMBOL_GPL(dw_pcie_ep_linkup);
+
+/**
+ * dw_pcie_ep_linkdown - Notify EPF drivers about Link Down event
+ * @ep: DWC EP device
+ *
+ * Non-sticky registers are also initialized before sending the notification to
+ * the EPF drivers. This is needed since the registers need to be initialized
+ * before the link comes back again.
+ */
+void dw_pcie_ep_linkdown(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct pci_epc *epc = ep->epc;
+
+	/*
+	 * Initialize the non-sticky DWC registers as they would've reset post
+	 * Link Down. This is specifically needed for drivers not supporting
+	 * PERST# as they have no way to reinitialize the registers before the
+	 * link comes back again.
+	 */
+	dw_pcie_ep_init_non_sticky_registers(pci);
+
+	pci_epc_linkdown(epc);
+}
+EXPORT_SYMBOL_GPL(dw_pcie_ep_linkdown);
+
 /**
  * dw_pcie_ep_init - Initialize the endpoint device
  * @ep: DWC EP device
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index f8e5431a207b..152969545b0a 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -668,6 +668,7 @@ static inline void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus,
 
 #ifdef CONFIG_PCIE_DW_EP
 void dw_pcie_ep_linkup(struct dw_pcie_ep *ep);
+void dw_pcie_ep_linkdown(struct dw_pcie_ep *ep);
 int dw_pcie_ep_init(struct dw_pcie_ep *ep);
 int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep);
 void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep);
@@ -688,6 +689,10 @@ static inline void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
 {
 }
 
+static inline void dw_pcie_ep_linkdown(struct dw_pcie_ep *ep)
+{
+}
+
 static inline int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 {
 	return 0;

-- 
2.25.1

Re: [PATCH 07/11] PCI: dwc: ep: Add a generic dw_pcie_ep_linkdown() API to handle Link Down event

[Niklas Cassel]

On Thu, Mar 14, 2024 at 08:53:46PM +0530, Manivannan Sadhasivam wrote:
> As per the PCIe base spec r5.0, section 5.2, Link Down event can happen
> under any of the following circumstances:
> 
> 1. Fundamental/Hot reset
> 2. Link disable transmission by upstream component
> 3. Moving from L2/L3 to L0
> 
> In those cases, Link Down causes some non-sticky DWC registers to loose the
> state (like REBAR, etc...). So the drivers need to reinitialize them to
> function properly once the link comes back again.
> 
> This is not a problem for drivers supporting PERST# IRQ, since they can
> reinitialize the registers in the PERST# IRQ callback. But for the drivers
> not supporting PERST#, there is no way they can reinitialize the registers
> other than relying on Link Down IRQ received when the link goes down. So
> let's add a DWC generic API dw_pcie_ep_linkdown() that reinitializes the
> non-sticky registers and also notifies the EPF drivers about link going
> down.
> 
> This API can also be used by the drivers supporting PERST# to handle the
> scenario (2) mentioned above.
> 
> NOTE: For the sake of code organization, move the dw_pcie_ep_linkup()
> definition just above dw_pcie_ep_linkdown().
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH 07/11] PCI: dwc: ep: Add a generic dw_pcie_ep_linkdown() API to handle Link Down event

[Niklas Cassel]

Hello Mani,

On Thu, Mar 14, 2024 at 08:53:46PM +0530, Manivannan Sadhasivam wrote:
> As per the PCIe base spec r5.0, section 5.2, Link Down event can happen
> under any of the following circumstances:
> 
> 1. Fundamental/Hot reset
> 2. Link disable transmission by upstream component
> 3. Moving from L2/L3 to L0
> 
> In those cases, Link Down causes some non-sticky DWC registers to loose the
> state (like REBAR, etc...). So the drivers need to reinitialize them to
> function properly once the link comes back again.
> 
> This is not a problem for drivers supporting PERST# IRQ, since they can
> reinitialize the registers in the PERST# IRQ callback. But for the drivers
> not supporting PERST#, there is no way they can reinitialize the registers
> other than relying on Link Down IRQ received when the link goes down. So
> let's add a DWC generic API dw_pcie_ep_linkdown() that reinitializes the
> non-sticky registers and also notifies the EPF drivers about link going
> down.
> 
> This API can also be used by the drivers supporting PERST# to handle the
> scenario (2) mentioned above.
> 
> NOTE: For the sake of code organization, move the dw_pcie_ep_linkup()
> definition just above dw_pcie_ep_linkdown().
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 93 ++++++++++++++++---------
>  drivers/pci/controller/dwc/pcie-designware.h    |  5 ++
>  2 files changed, 67 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 3893a8c1a11c..5451057ca74b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -14,18 +14,6 @@
>  #include <linux/pci-epc.h>
>  #include <linux/pci-epf.h>
>  
> -/**
> - * dw_pcie_ep_linkup - Notify EPF drivers about link up event
> - * @ep: DWC EP device
> - */
> -void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> -{
> -	struct pci_epc *epc = ep->epc;
> -
> -	pci_epc_linkup(epc);
> -}
> -EXPORT_SYMBOL_GPL(dw_pcie_ep_linkup);
> -
>  /**
>   * dw_pcie_ep_init_notify - Notify EPF drivers about EPC initialization
>   *			    complete
> @@ -672,6 +660,29 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
>  	return 0;
>  }
>  
> +static void dw_pcie_ep_init_non_sticky_registers(struct dw_pcie *pci)
> +{
> +	unsigned int offset;
> +	unsigned int nbars;
> +	u32 reg, i;
> +
> +	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
> +
> +	dw_pcie_dbi_ro_wr_en(pci);
> +
> +	if (offset) {
> +		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
> +		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
> +			PCI_REBAR_CTRL_NBAR_SHIFT;
> +
> +		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
> +			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
> +	}
> +
> +	dw_pcie_setup(pci);
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +}
> +
>  /**
>   * dw_pcie_ep_init_registers - Initialize DWC EP specific registers
>   * @ep: DWC EP device
> @@ -686,13 +697,11 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep)
>  	struct dw_pcie_ep_func *ep_func;
>  	struct device *dev = pci->dev;
>  	struct pci_epc *epc = ep->epc;
> -	unsigned int offset, ptm_cap_base;
> -	unsigned int nbars;
> +	u32 ptm_cap_base, reg;
>  	u8 hdr_type;
>  	u8 func_no;
> -	int i, ret;
>  	void *addr;
> -	u32 reg;
> +	int ret;
>  
>  	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) &
>  		   PCI_HEADER_TYPE_MASK;
> @@ -755,20 +764,8 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep)
>  	if (ep->ops->init)
>  		ep->ops->init(ep);
>  
> -	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
>  	ptm_cap_base = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_PTM);
>  
> -	dw_pcie_dbi_ro_wr_en(pci);
> -
> -	if (offset) {
> -		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
> -		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
> -			PCI_REBAR_CTRL_NBAR_SHIFT;
> -
> -		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
> -			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
> -	}
> -
>  	/*
>  	 * PTM responder capability can be disabled only after disabling
>  	 * PTM root capability.
> @@ -785,9 +782,6 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep)
>  		dw_pcie_dbi_ro_wr_dis(pci);
>  	}
>  
> -	dw_pcie_setup(pci);
> -	dw_pcie_dbi_ro_wr_dis(pci);
> -

Your previous series had:

-       dw_pcie_setup(pci);
-       dw_pcie_dbi_ro_wr_dis(pci);
+       dw_pcie_ep_init_non_sticky_registers(pci);

Here.
I tested this series, but it did not work for me (the Resizable BARs did
not get resized) since you removed the call to
dw_pcie_ep_init_non_sticky_registers().

By readding the call to dw_pcie_ep_init_non_sticky_registers(),
the BARs get Resized again.


BTW do you have a git branch with both your series somewhere?
(Possibly even rebased on
https://lore.kernel.org/linux-pci/20240320113157.322695-1-cassel@kernel.org/T/#t
like you suggested in your other mail.)


Kind regards,
Niklas

Re: [PATCH 07/11] PCI: dwc: ep: Add a generic dw_pcie_ep_linkdown() API to handle Link Down event

[Manivannan Sadhasivam]

On Wed, Mar 27, 2024 at 07:06:30PM +0100, Niklas Cassel wrote:
> Hello Mani,
> 
> On Thu, Mar 14, 2024 at 08:53:46PM +0530, Manivannan Sadhasivam wrote:
> > As per the PCIe base spec r5.0, section 5.2, Link Down event can happen
> > under any of the following circumstances:
> > 
> > 1. Fundamental/Hot reset
> > 2. Link disable transmission by upstream component
> > 3. Moving from L2/L3 to L0
> > 
> > In those cases, Link Down causes some non-sticky DWC registers to loose the
> > state (like REBAR, etc...). So the drivers need to reinitialize them to
> > function properly once the link comes back again.
> > 
> > This is not a problem for drivers supporting PERST# IRQ, since they can
> > reinitialize the registers in the PERST# IRQ callback. But for the drivers
> > not supporting PERST#, there is no way they can reinitialize the registers
> > other than relying on Link Down IRQ received when the link goes down. So
> > let's add a DWC generic API dw_pcie_ep_linkdown() that reinitializes the
> > non-sticky registers and also notifies the EPF drivers about link going
> > down.
> > 
> > This API can also be used by the drivers supporting PERST# to handle the
> > scenario (2) mentioned above.
> > 
> > NOTE: For the sake of code organization, move the dw_pcie_ep_linkup()
> > definition just above dw_pcie_ep_linkdown().
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-ep.c | 93 ++++++++++++++++---------
> >  drivers/pci/controller/dwc/pcie-designware.h    |  5 ++
> >  2 files changed, 67 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 3893a8c1a11c..5451057ca74b 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -14,18 +14,6 @@
> >  #include <linux/pci-epc.h>
> >  #include <linux/pci-epf.h>
> >  
> > -/**
> > - * dw_pcie_ep_linkup - Notify EPF drivers about link up event
> > - * @ep: DWC EP device
> > - */
> > -void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> > -{
> > -	struct pci_epc *epc = ep->epc;
> > -
> > -	pci_epc_linkup(epc);
> > -}
> > -EXPORT_SYMBOL_GPL(dw_pcie_ep_linkup);
> > -
> >  /**
> >   * dw_pcie_ep_init_notify - Notify EPF drivers about EPC initialization
> >   *			    complete
> > @@ -672,6 +660,29 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
> >  	return 0;
> >  }
> >  
> > +static void dw_pcie_ep_init_non_sticky_registers(struct dw_pcie *pci)
> > +{
> > +	unsigned int offset;
> > +	unsigned int nbars;
> > +	u32 reg, i;
> > +
> > +	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
> > +
> > +	dw_pcie_dbi_ro_wr_en(pci);
> > +
> > +	if (offset) {
> > +		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
> > +		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
> > +			PCI_REBAR_CTRL_NBAR_SHIFT;
> > +
> > +		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
> > +			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
> > +	}
> > +
> > +	dw_pcie_setup(pci);
> > +	dw_pcie_dbi_ro_wr_dis(pci);
> > +}
> > +
> >  /**
> >   * dw_pcie_ep_init_registers - Initialize DWC EP specific registers
> >   * @ep: DWC EP device
> > @@ -686,13 +697,11 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep)
> >  	struct dw_pcie_ep_func *ep_func;
> >  	struct device *dev = pci->dev;
> >  	struct pci_epc *epc = ep->epc;
> > -	unsigned int offset, ptm_cap_base;
> > -	unsigned int nbars;
> > +	u32 ptm_cap_base, reg;
> >  	u8 hdr_type;
> >  	u8 func_no;
> > -	int i, ret;
> >  	void *addr;
> > -	u32 reg;
> > +	int ret;
> >  
> >  	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) &
> >  		   PCI_HEADER_TYPE_MASK;
> > @@ -755,20 +764,8 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep)
> >  	if (ep->ops->init)
> >  		ep->ops->init(ep);
> >  
> > -	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
> >  	ptm_cap_base = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_PTM);
> >  
> > -	dw_pcie_dbi_ro_wr_en(pci);
> > -
> > -	if (offset) {
> > -		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
> > -		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
> > -			PCI_REBAR_CTRL_NBAR_SHIFT;
> > -
> > -		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
> > -			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
> > -	}
> > -
> >  	/*
> >  	 * PTM responder capability can be disabled only after disabling
> >  	 * PTM root capability.
> > @@ -785,9 +782,6 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep)
> >  		dw_pcie_dbi_ro_wr_dis(pci);
> >  	}
> >  
> > -	dw_pcie_setup(pci);
> > -	dw_pcie_dbi_ro_wr_dis(pci);
> > -
> 
> Your previous series had:
> 
> -       dw_pcie_setup(pci);
> -       dw_pcie_dbi_ro_wr_dis(pci);
> +       dw_pcie_ep_init_non_sticky_registers(pci);
> 
> Here.
> I tested this series, but it did not work for me (the Resizable BARs did
> not get resized) since you removed the call to
> dw_pcie_ep_init_non_sticky_registers().
> 
> By readding the call to dw_pcie_ep_init_non_sticky_registers(),
> the BARs get Resized again.
> 
> 

Ah, looks like rebase has gone bad. Will fix it in v2.

> BTW do you have a git branch with both your series somewhere?
> (Possibly even rebased on
> https://lore.kernel.org/linux-pci/20240320113157.322695-1-cassel@kernel.org/T/#t
> like you suggested in your other mail.)
> 

There it is: https://git.codelinaro.org/manivannan.sadhasivam/linux/-/tree/b4/pci-epf-rework

- Mani

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 08/11] PCI: qcom-ep: Use the generic dw_pcie_ep_linkdown() API to handle Link Down event

[Manivannan Sadhasivam]

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.

Reviewed-by: Niklas Cassel <cassel@kernel.org>
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 e4b742355d57..811f250e967a 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -635,7 +635,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

[PATCH 09/11] PCI: epf-test: Handle Link Down event

[Manivannan Sadhasivam]

As per the PCIe base spec r5.0, section 5.2, Link Down event can happen
under any of the following circumstances:

1. Fundamental/Hot reset
2. Link disable transmission by upstream component
3. Moving from L2/L3 to L0

When the event happens, the EPC driver capable of detecting it may pass the
notification to the EPF driver through link_down() callback in 'struct
pci_epc_bus_event_ops'.

While the PCIe spec has not defined the actual behavior of the endpoint
when the Link Down event happens, we may assume that atleast the ongoing
transactions need to be stopped as the link won't be active. So let's
cancel the command handler work in the callback implementation
pci_epf_test_link_down(). The work will be started again in
pci_epf_test_link_up() once the link comes back again.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 84cd47ebac22..97245228c9eb 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -823,6 +823,15 @@ static int pci_epf_test_link_up(struct pci_epf *epf)
 	return 0;
 }
 
+static int pci_epf_test_link_down(struct pci_epf *epf)
+{
+	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
+
+	cancel_delayed_work(&epf_test->cmd_handler);
+
+	return 0;
+}
+
 static const struct pci_epc_event_ops pci_epf_test_epc_event_ops = {
 	.init = pci_epf_test_epc_init,
 	.deinit = pci_epf_test_epc_deinit,
@@ -830,6 +839,7 @@ static const struct pci_epc_event_ops pci_epf_test_epc_event_ops = {
 
 static const struct pci_epc_bus_event_ops pci_epf_test_bus_event_ops = {
 	.link_up = pci_epf_test_link_up,
+	.link_down = pci_epf_test_link_down,
 };
 
 static int pci_epf_test_alloc_space(struct pci_epf *epf)

-- 
2.25.1

Re: [PATCH 09/11] PCI: epf-test: Handle Link Down event

[Niklas Cassel]

On Thu, Mar 14, 2024 at 08:53:48PM +0530, Manivannan Sadhasivam wrote:
> As per the PCIe base spec r5.0, section 5.2, Link Down event can happen
> under any of the following circumstances:
> 
> 1. Fundamental/Hot reset
> 2. Link disable transmission by upstream component
> 3. Moving from L2/L3 to L0
> 
> When the event happens, the EPC driver capable of detecting it may pass the
> notification to the EPF driver through link_down() callback in 'struct
> pci_epc_bus_event_ops'.
> 
> While the PCIe spec has not defined the actual behavior of the endpoint
> when the Link Down event happens, we may assume that atleast the ongoing

Nit:
s/atleast/at least/

Reviewed-by: Niklas Cassel <cassel@kernel.org>

> transactions need to be stopped as the link won't be active. So let's
> cancel the command handler work in the callback implementation
> pci_epf_test_link_down(). The work will be started again in
> pci_epf_test_link_up() once the link comes back again.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 84cd47ebac22..97245228c9eb 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -823,6 +823,15 @@ static int pci_epf_test_link_up(struct pci_epf *epf)
>  	return 0;
>  }
>  
> +static int pci_epf_test_link_down(struct pci_epf *epf)
> +{
> +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> +
> +	cancel_delayed_work(&epf_test->cmd_handler);
> +
> +	return 0;
> +}
> +
>  static const struct pci_epc_event_ops pci_epf_test_epc_event_ops = {
>  	.init = pci_epf_test_epc_init,
>  	.deinit = pci_epf_test_epc_deinit,
> @@ -830,6 +839,7 @@ static const struct pci_epc_event_ops pci_epf_test_epc_event_ops = {
>  
>  static const struct pci_epc_bus_event_ops pci_epf_test_bus_event_ops = {
>  	.link_up = pci_epf_test_link_up,
> +	.link_down = pci_epf_test_link_down,
>  };
>  
>  static int pci_epf_test_alloc_space(struct pci_epf *epf)
> 
> -- 
> 2.25.1
> 

[PATCH 10/11] PCI: qcom-ep: Rework {start/stop}_link() callbacks implementation

[Manivannan Sadhasivam]

DWC specific start_link() and stop_link() callbacks are supposed to start
and stop the link training of the PCIe bus. But the current implementation
of this driver enables/disables the PERST# IRQ.

Even though this is not causing any issues, this creates inconsistency
among the controller drivers. So for the sake of consistency, let's just
start/stop the link training in these callbacks.

Also, PERST# IRQ is now enabled from the start itself, thus allowing the
controller driver to initialize the registers when PERST# gets deasserted
without waiting for the user intervention though configfs.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom-ep.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index 811f250e967a..653e4ace0a07 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -122,6 +122,9 @@
 /* PARF_CFG_BITS register fields */
 #define PARF_CFG_BITS_REQ_EXIT_L1SS_MSI_LTR_EN	BIT(1)
 
+/* PARF_LTSSM register fields */
+#define LTSSM_EN				BIT(8)
+
 /* ELBI registers */
 #define ELBI_SYS_STTS				0x08
 #define ELBI_CS2_ENABLE				0xa4
@@ -250,8 +253,12 @@ static int qcom_pcie_dw_link_up(struct dw_pcie *pci)
 static int qcom_pcie_dw_start_link(struct dw_pcie *pci)
 {
 	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
+	u32 val;
 
-	enable_irq(pcie_ep->perst_irq);
+	/* Enable LTSSM */
+	val = readl_relaxed(pcie_ep->parf + PARF_LTSSM);
+	val |= LTSSM_EN;
+	writel_relaxed(val, pcie_ep->parf + PARF_LTSSM);
 
 	return 0;
 }
@@ -259,8 +266,12 @@ static int qcom_pcie_dw_start_link(struct dw_pcie *pci)
 static void qcom_pcie_dw_stop_link(struct dw_pcie *pci)
 {
 	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
+	u32 val;
 
-	disable_irq(pcie_ep->perst_irq);
+	/* Disable LTSSM */
+	val = readl_relaxed(pcie_ep->parf + PARF_LTSSM);
+	val &= ~LTSSM_EN;
+	writel_relaxed(val, pcie_ep->parf + PARF_LTSSM);
 }
 
 static void qcom_pcie_dw_write_dbi2(struct dw_pcie *pci, void __iomem *base,
@@ -484,11 +495,6 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
 
 	dw_pcie_ep_init_notify(&pcie_ep->pci.ep);
 
-	/* Enable LTSSM */
-	val = readl_relaxed(pcie_ep->parf + PARF_LTSSM);
-	val |= BIT(8);
-	writel_relaxed(val, pcie_ep->parf + PARF_LTSSM);
-
 	return 0;
 
 err_disable_resources:
@@ -707,7 +713,6 @@ static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev,
 	}
 
 	pcie_ep->perst_irq = gpiod_to_irq(pcie_ep->reset);
-	irq_set_status_flags(pcie_ep->perst_irq, IRQ_NOAUTOEN);
 	ret = devm_request_threaded_irq(&pdev->dev, pcie_ep->perst_irq, NULL,
 					qcom_pcie_ep_perst_irq_thread,
 					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,

-- 
2.25.1

Re: [PATCH 10/11] PCI: qcom-ep: Rework {start/stop}_link() callbacks implementation

[Niklas Cassel]

On Thu, Mar 14, 2024 at 08:53:49PM +0530, Manivannan Sadhasivam wrote:
> DWC specific start_link() and stop_link() callbacks are supposed to start
> and stop the link training of the PCIe bus. But the current implementation
> of this driver enables/disables the PERST# IRQ.
> 
> Even though this is not causing any issues, this creates inconsistency
> among the controller drivers. So for the sake of consistency, let's just
> start/stop the link training in these callbacks.
> 
> Also, PERST# IRQ is now enabled from the start itself, thus allowing the
> controller driver to initialize the registers when PERST# gets deasserted
> without waiting for the user intervention though configfs.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---

Nice change:
Reviewed-by: Niklas Cassel <cassel@kernel.org>

If you dump LTSSM after a PERST assert + deassert,
using e.g. dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1);
to dump the debug registers (see dw_pcie_link_up())
do you see that PCIE_PORT_DEBUG1_LINK_IN_TRAINING is set?

I was thinking that perhaps there was a thought behind
this original design, that you had to explicitly set
LTSSM_EN after a fundamental core reset, because it
would get cleared?

(It it is implemented like signals and not registers,
then this change should be fine.)


Kind regards,
Niklas


>  drivers/pci/controller/dwc/pcie-qcom-ep.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index 811f250e967a..653e4ace0a07 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -122,6 +122,9 @@
>  /* PARF_CFG_BITS register fields */
>  #define PARF_CFG_BITS_REQ_EXIT_L1SS_MSI_LTR_EN	BIT(1)
>  
> +/* PARF_LTSSM register fields */
> +#define LTSSM_EN				BIT(8)
> +
>  /* ELBI registers */
>  #define ELBI_SYS_STTS				0x08
>  #define ELBI_CS2_ENABLE				0xa4
> @@ -250,8 +253,12 @@ static int qcom_pcie_dw_link_up(struct dw_pcie *pci)
>  static int qcom_pcie_dw_start_link(struct dw_pcie *pci)
>  {
>  	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> +	u32 val;
>  
> -	enable_irq(pcie_ep->perst_irq);
> +	/* Enable LTSSM */
> +	val = readl_relaxed(pcie_ep->parf + PARF_LTSSM);
> +	val |= LTSSM_EN;
> +	writel_relaxed(val, pcie_ep->parf + PARF_LTSSM);
>  
>  	return 0;
>  }
> @@ -259,8 +266,12 @@ static int qcom_pcie_dw_start_link(struct dw_pcie *pci)
>  static void qcom_pcie_dw_stop_link(struct dw_pcie *pci)
>  {
>  	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> +	u32 val;
>  
> -	disable_irq(pcie_ep->perst_irq);
> +	/* Disable LTSSM */
> +	val = readl_relaxed(pcie_ep->parf + PARF_LTSSM);
> +	val &= ~LTSSM_EN;
> +	writel_relaxed(val, pcie_ep->parf + PARF_LTSSM);
>  }
>  
>  static void qcom_pcie_dw_write_dbi2(struct dw_pcie *pci, void __iomem *base,
> @@ -484,11 +495,6 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
>  
>  	dw_pcie_ep_init_notify(&pcie_ep->pci.ep);
>  
> -	/* Enable LTSSM */
> -	val = readl_relaxed(pcie_ep->parf + PARF_LTSSM);
> -	val |= BIT(8);
> -	writel_relaxed(val, pcie_ep->parf + PARF_LTSSM);
> -
>  	return 0;
>  
>  err_disable_resources:
> @@ -707,7 +713,6 @@ static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev,
>  	}
>  
>  	pcie_ep->perst_irq = gpiod_to_irq(pcie_ep->reset);
> -	irq_set_status_flags(pcie_ep->perst_irq, IRQ_NOAUTOEN);
>  	ret = devm_request_threaded_irq(&pdev->dev, pcie_ep->perst_irq, NULL,
>  					qcom_pcie_ep_perst_irq_thread,
>  					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> 
> -- 
> 2.25.1
> 

Re: [PATCH 10/11] PCI: qcom-ep: Rework {start/stop}_link() callbacks implementation

[Manivannan Sadhasivam]

On Fri, Mar 22, 2024 at 05:10:54PM +0100, Niklas Cassel wrote:
> On Thu, Mar 14, 2024 at 08:53:49PM +0530, Manivannan Sadhasivam wrote:
> > DWC specific start_link() and stop_link() callbacks are supposed to start
> > and stop the link training of the PCIe bus. But the current implementation
> > of this driver enables/disables the PERST# IRQ.
> > 
> > Even though this is not causing any issues, this creates inconsistency
> > among the controller drivers. So for the sake of consistency, let's just
> > start/stop the link training in these callbacks.
> > 
> > Also, PERST# IRQ is now enabled from the start itself, thus allowing the
> > controller driver to initialize the registers when PERST# gets deasserted
> > without waiting for the user intervention though configfs.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> 
> Nice change:
> Reviewed-by: Niklas Cassel <cassel@kernel.org>
> 
> If you dump LTSSM after a PERST assert + deassert,
> using e.g. dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1);
> to dump the debug registers (see dw_pcie_link_up())
> do you see that PCIE_PORT_DEBUG1_LINK_IN_TRAINING is set?
> 
> I was thinking that perhaps there was a thought behind
> this original design, that you had to explicitly set
> LTSSM_EN after a fundamental core reset, because it
> would get cleared?
> 

Well, you are right. I was hoping to get an answer from Kishon/Vidya, but you
throwed the light. I will drop these 2 patches.

Thanks!

- Mani

> (It it is implemented like signals and not registers,
> then this change should be fine.)
> 
> 
> Kind regards,
> Niklas
> 
> 
> >  drivers/pci/controller/dwc/pcie-qcom-ep.c | 21 +++++++++++++--------
> >  1 file changed, 13 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > index 811f250e967a..653e4ace0a07 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > @@ -122,6 +122,9 @@
> >  /* PARF_CFG_BITS register fields */
> >  #define PARF_CFG_BITS_REQ_EXIT_L1SS_MSI_LTR_EN	BIT(1)
> >  
> > +/* PARF_LTSSM register fields */
> > +#define LTSSM_EN				BIT(8)
> > +
> >  /* ELBI registers */
> >  #define ELBI_SYS_STTS				0x08
> >  #define ELBI_CS2_ENABLE				0xa4
> > @@ -250,8 +253,12 @@ static int qcom_pcie_dw_link_up(struct dw_pcie *pci)
> >  static int qcom_pcie_dw_start_link(struct dw_pcie *pci)
> >  {
> >  	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> > +	u32 val;
> >  
> > -	enable_irq(pcie_ep->perst_irq);
> > +	/* Enable LTSSM */
> > +	val = readl_relaxed(pcie_ep->parf + PARF_LTSSM);
> > +	val |= LTSSM_EN;
> > +	writel_relaxed(val, pcie_ep->parf + PARF_LTSSM);
> >  
> >  	return 0;
> >  }
> > @@ -259,8 +266,12 @@ static int qcom_pcie_dw_start_link(struct dw_pcie *pci)
> >  static void qcom_pcie_dw_stop_link(struct dw_pcie *pci)
> >  {
> >  	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> > +	u32 val;
> >  
> > -	disable_irq(pcie_ep->perst_irq);
> > +	/* Disable LTSSM */
> > +	val = readl_relaxed(pcie_ep->parf + PARF_LTSSM);
> > +	val &= ~LTSSM_EN;
> > +	writel_relaxed(val, pcie_ep->parf + PARF_LTSSM);
> >  }
> >  
> >  static void qcom_pcie_dw_write_dbi2(struct dw_pcie *pci, void __iomem *base,
> > @@ -484,11 +495,6 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
> >  
> >  	dw_pcie_ep_init_notify(&pcie_ep->pci.ep);
> >  
> > -	/* Enable LTSSM */
> > -	val = readl_relaxed(pcie_ep->parf + PARF_LTSSM);
> > -	val |= BIT(8);
> > -	writel_relaxed(val, pcie_ep->parf + PARF_LTSSM);
> > -
> >  	return 0;
> >  
> >  err_disable_resources:
> > @@ -707,7 +713,6 @@ static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev,
> >  	}
> >  
> >  	pcie_ep->perst_irq = gpiod_to_irq(pcie_ep->reset);
> > -	irq_set_status_flags(pcie_ep->perst_irq, IRQ_NOAUTOEN);
> >  	ret = devm_request_threaded_irq(&pdev->dev, pcie_ep->perst_irq, NULL,
> >  					qcom_pcie_ep_perst_irq_thread,
> >  					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > 
> > -- 
> > 2.25.1
> > 
> 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 11/11] PCI: tegra194: Rework {start/stop}_link() callbacks implementation

[Manivannan Sadhasivam]

DWC specific start_link() and stop_link() callbacks are supposed to start
and stop the link training of the PCIe bus. But the current endpoint
implementation of this driver enables/disables the PERST# IRQ.

Even though this is not causing any issues, this creates inconsistency
among the EP controller drivers. So for the sake of consistency, let's just
start/stop the link training in these callbacks.

Also, PERST# IRQ is now enabled from the start itself, thus allowing the
controller driver to initialize the registers when PERST# gets deasserted
without waiting for the user intervention though configfs.

Cc: Vidya Sagar <vidyas@nvidia.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-tegra194.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 3e6e08b321fb..03d6f248bc6f 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -964,7 +964,11 @@ static int tegra_pcie_dw_start_link(struct dw_pcie *pci)
 	bool retry = true;
 
 	if (pcie->of_data->mode == DW_PCIE_EP_TYPE) {
-		enable_irq(pcie->pex_rst_irq);
+		/* Enable LTSSM */
+		val = appl_readl(pcie, APPL_CTRL);
+		val |= APPL_CTRL_LTSSM_EN;
+		appl_writel(pcie, val, APPL_CTRL);
+
 		return 0;
 	}
 
@@ -1049,8 +1053,12 @@ static int tegra_pcie_dw_link_up(struct dw_pcie *pci)
 static void tegra_pcie_dw_stop_link(struct dw_pcie *pci)
 {
 	struct tegra_pcie_dw *pcie = to_tegra_pcie(pci);
+	u32 val;
 
-	disable_irq(pcie->pex_rst_irq);
+	/* Disable LTSSM */
+	val = appl_readl(pcie, APPL_CTRL);
+	val &= ~APPL_CTRL_LTSSM_EN;
+	appl_writel(pcie, val, APPL_CTRL);
 }
 
 static const struct dw_pcie_ops tegra_dw_pcie_ops = {
@@ -1702,11 +1710,6 @@ static void pex_ep_event_pex_rst_assert(struct tegra_pcie_dw *pcie)
 	if (pcie->ep_state == EP_STATE_DISABLED)
 		return;
 
-	/* Disable LTSSM */
-	val = appl_readl(pcie, APPL_CTRL);
-	val &= ~APPL_CTRL_LTSSM_EN;
-	appl_writel(pcie, val, APPL_CTRL);
-
 	ret = readl_poll_timeout(pcie->appl_base + APPL_DEBUG, val,
 				 ((val & APPL_DEBUG_LTSSM_STATE_MASK) >>
 				 APPL_DEBUG_LTSSM_STATE_SHIFT) ==
@@ -1913,11 +1916,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
 		appl_writel(pcie, val, APPL_LTR_MSG_2);
 	}
 
-	/* Enable LTSSM */
-	val = appl_readl(pcie, APPL_CTRL);
-	val |= APPL_CTRL_LTSSM_EN;
-	appl_writel(pcie, val, APPL_CTRL);
-
 	pcie->ep_state = EP_STATE_ENABLED;
 	dev_dbg(dev, "Initialization of endpoint is completed\n");
 
@@ -2060,8 +2058,6 @@ static int tegra_pcie_config_ep(struct tegra_pcie_dw *pcie,
 		return -ENOMEM;
 	}
 
-	irq_set_status_flags(pcie->pex_rst_irq, IRQ_NOAUTOEN);
-
 	pcie->ep_state = EP_STATE_DISABLED;
 
 	ret = devm_request_threaded_irq(dev, pcie->pex_rst_irq, NULL,

-- 
2.25.1

Re: [PATCH 11/11] PCI: tegra194: Rework {start/stop}_link() callbacks implementation

[Niklas Cassel]

On Thu, Mar 14, 2024 at 08:53:50PM +0530, Manivannan Sadhasivam wrote:
> DWC specific start_link() and stop_link() callbacks are supposed to start
> and stop the link training of the PCIe bus. But the current endpoint
> implementation of this driver enables/disables the PERST# IRQ.
> 
> Even though this is not causing any issues, this creates inconsistency
> among the EP controller drivers. So for the sake of consistency, let's just
> start/stop the link training in these callbacks.
> 
> Also, PERST# IRQ is now enabled from the start itself, thus allowing the
> controller driver to initialize the registers when PERST# gets deasserted
> without waiting for the user intervention though configfs.
> 
> Cc: Vidya Sagar <vidyas@nvidia.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---

Reviewed-by: Niklas Cassel <cassel@kernel.org>