[v2] PCI: qcom: Enable ASPM on host bridge and devices

[PATCH v2 1/2] PCI: dwc: Add host_post_init() callback

Posted by Manivannan Sadhasivam 1 year, 11 months ago

This callback can be used by the platform drivers to do configuration once
all the devices are scanned. Like changing LNKCTL of all downstream devices
to enable ASPM etc...

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 3 +++
 drivers/pci/controller/dwc/pcie-designware.h      | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index a7170fd0e847..7991f0e179b2 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -502,6 +502,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 	if (ret)
 		goto err_stop_link;
 
+	if (pp->ops->host_post_init)
+		pp->ops->host_post_init(pp);
+
 	return 0;
 
 err_stop_link:
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index ef0b2efa9f93..efb4d4754fc8 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -301,6 +301,7 @@ enum dw_pcie_ltssm {
 struct dw_pcie_host_ops {
 	int (*host_init)(struct dw_pcie_rp *pp);
 	void (*host_deinit)(struct dw_pcie_rp *pp);
+	void (*host_post_init)(struct dw_pcie_rp *pp);
 	int (*msi_host_init)(struct dw_pcie_rp *pp);
 	void (*pme_turn_off)(struct dw_pcie_rp *pp);
 };
-- 
2.25.1

Re: [PATCH v2 1/2] PCI: dwc: Add host_post_init() callback

Posted by Bjorn Helgaas 1 year, 11 months ago

On Tue, Oct 10, 2023 at 09:29:13PM +0530, Manivannan Sadhasivam wrote:
> This callback can be used by the platform drivers to do configuration once
> all the devices are scanned. Like changing LNKCTL of all downstream devices
> to enable ASPM etc...
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 3 +++
>  drivers/pci/controller/dwc/pcie-designware.h      | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index a7170fd0e847..7991f0e179b2 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -502,6 +502,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  	if (ret)
>  		goto err_stop_link;
>  
> +	if (pp->ops->host_post_init)
> +		pp->ops->host_post_init(pp);

I know we talked about this a little bit in the context of enabling
ASPM for devices below qcom 1.9.0 controllers at
https://lore.kernel.org/r/20231011050058.GC3508@thinkpad

But I didn't realize at the time that this callback adds a fundamental
difference between devices present at boot-time (these devices can be
affected by this callback) and any devices hot-added later (we never
run this callback again, so anything done by .host_post_init() never
applies to them).

We merged this for now, and it helps enable ASPM for builtin devices
on qcom, but I don't feel good about this from a larger DWC
perspective.  If other drivers use this and they support hot-add, I
think we're going to have problems.

>  	return 0;
>  
>  err_stop_link:
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index ef0b2efa9f93..efb4d4754fc8 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -301,6 +301,7 @@ enum dw_pcie_ltssm {
>  struct dw_pcie_host_ops {
>  	int (*host_init)(struct dw_pcie_rp *pp);
>  	void (*host_deinit)(struct dw_pcie_rp *pp);
> +	void (*host_post_init)(struct dw_pcie_rp *pp);
>  	int (*msi_host_init)(struct dw_pcie_rp *pp);
>  	void (*pme_turn_off)(struct dw_pcie_rp *pp);
>  };
> -- 
> 2.25.1
>

Re: [PATCH v2 1/2] PCI: dwc: Add host_post_init() callback

Posted by Manivannan Sadhasivam 1 year, 10 months ago

On Mon, Oct 16, 2023 at 03:58:49PM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 10, 2023 at 09:29:13PM +0530, Manivannan Sadhasivam wrote:
> > This callback can be used by the platform drivers to do configuration once
> > all the devices are scanned. Like changing LNKCTL of all downstream devices
> > to enable ASPM etc...
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 3 +++
> >  drivers/pci/controller/dwc/pcie-designware.h      | 1 +
> >  2 files changed, 4 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index a7170fd0e847..7991f0e179b2 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -502,6 +502,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >  	if (ret)
> >  		goto err_stop_link;
> >  
> > +	if (pp->ops->host_post_init)
> > +		pp->ops->host_post_init(pp);
> 
> I know we talked about this a little bit in the context of enabling
> ASPM for devices below qcom 1.9.0 controllers at
> https://lore.kernel.org/r/20231011050058.GC3508@thinkpad
> 
> But I didn't realize at the time that this callback adds a fundamental
> difference between devices present at boot-time (these devices can be
> affected by this callback) and any devices hot-added later (we never
> run this callback again, so anything done by .host_post_init() never
> applies to them).
> 
> We merged this for now, and it helps enable ASPM for builtin devices
> on qcom, but I don't feel good about this from a larger DWC
> perspective.  If other drivers use this and they support hot-add, I
> think we're going to have problems.
> 

If someone is going to add same ASPM code in host_post_init() callback, they
will most likely aware of the hotplug issue. I see this as an interim solution
overall and we should fix the PCI core to handle this. But I do not see any
straightforward way to enable ASPM by default in PCI core as the misbehaving
devices can pull the system down (atleast in some x86 cases).

- Mani

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

Re: [PATCH v2 1/2] PCI: dwc: Add host_post_init() callback

Posted by Bjorn Helgaas 1 year, 10 months ago

On Tue, Oct 17, 2023 at 01:29:52PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Oct 16, 2023 at 03:58:49PM -0500, Bjorn Helgaas wrote:
> > On Tue, Oct 10, 2023 at 09:29:13PM +0530, Manivannan Sadhasivam wrote:
> > > This callback can be used by the platform drivers to do configuration once
> > > all the devices are scanned. Like changing LNKCTL of all downstream devices
> > > to enable ASPM etc...
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-designware-host.c | 3 +++
> > >  drivers/pci/controller/dwc/pcie-designware.h      | 1 +
> > >  2 files changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index a7170fd0e847..7991f0e179b2 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -502,6 +502,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > >  	if (ret)
> > >  		goto err_stop_link;
> > >  
> > > +	if (pp->ops->host_post_init)
> > > +		pp->ops->host_post_init(pp);
> > 
> > I know we talked about this a little bit in the context of enabling
> > ASPM for devices below qcom 1.9.0 controllers at
> > https://lore.kernel.org/r/20231011050058.GC3508@thinkpad
> > 
> > But I didn't realize at the time that this callback adds a fundamental
> > difference between devices present at boot-time (these devices can be
> > affected by this callback) and any devices hot-added later (we never
> > run this callback again, so anything done by .host_post_init() never
> > applies to them).
> > 
> > We merged this for now, and it helps enable ASPM for builtin devices
> > on qcom, but I don't feel good about this from a larger DWC
> > perspective.  If other drivers use this and they support hot-add, I
> > think we're going to have problems.
> 
> If someone is going to add same ASPM code in host_post_init()
> callback, they will most likely aware of the hotplug issue. I see
> this as an interim solution overall and we should fix the PCI core
> to handle this. But I do not see any straightforward way to enable
> ASPM by default in PCI core as the misbehaving devices can pull the
> system down (atleast in some x86 cases).

Definitely an interim solution, but the interim is going to be long :)

I don't see the PCI core ASPM issue being resolved very soon; it's
been this way ever since the beginning.  I guess there's no point in
me whining without any proposals.

Bjorn