[v2,2/2] PCI: qcom: Add basic interconnect support

  On Qualcomm platforms like SC8280XP and SA8540P, interconnect bandwidth
must be requested before enabling interconnect clocks.

Add basic support for managing an optional "pcie-mem" interconnect path
by setting a low constraint before enabling clocks and updating it after
the link is up.

Note that it is not possible for a controller driver to set anything but
a maximum peak bandwidth as expected average bandwidth will vary with
use case and actual use (and power policy?). This very much remains an
unresolved problem with the interconnect framework.

Also note that no constraint is set for the SC8280XP/SA8540P "cpu-pcie"
path for now as it is not clear what an appropriate constraint would be
(and the system does not crash when left unspecified).

Fixes: 70574511f3fc ("PCI: qcom: Add support for SC8280XP")
Reviewed-by: Brian Masney <bmasney@redhat.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 76 ++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)
  

Thanks for the patches, Johan!

On 21.10.22 9:46, Johan Hovold wrote:
> On Qualcomm platforms like SC8280XP and SA8540P, interconnect bandwidth
> must be requested before enabling interconnect clocks.
> 
> Add basic support for managing an optional "pcie-mem" interconnect path
> by setting a low constraint before enabling clocks and updating it after
> the link is up.
> 
> Note that it is not possible for a controller driver to set anything but
> a maximum peak bandwidth as expected average bandwidth will vary with
> use case and actual use (and power policy?). This very much remains an
> unresolved problem with the interconnect framework.
> 
> Also note that no constraint is set for the SC8280XP/SA8540P "cpu-pcie"
> path for now as it is not clear what an appropriate constraint would be
> (and the system does not crash when left unspecified).
> 
> Fixes: 70574511f3fc ("PCI: qcom: Add support for SC8280XP")
> Reviewed-by: Brian Masney <bmasney@redhat.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Acked-by: Georgi Djakov <djakov@kernel.org>

Also CC-ing Stan's new e-mail address.

Thanks,
Georgi

> ---
>   drivers/pci/controller/dwc/pcie-qcom.c | 76 ++++++++++++++++++++++++++
>   1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 7db94a22238d..0c13f976626f 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -12,6 +12,7 @@
>   #include <linux/crc8.h>
>   #include <linux/delay.h>
>   #include <linux/gpio/consumer.h>
> +#include <linux/interconnect.h>
>   #include <linux/interrupt.h>
>   #include <linux/io.h>
>   #include <linux/iopoll.h>
> @@ -224,6 +225,7 @@ struct qcom_pcie {
>   	union qcom_pcie_resources res;
>   	struct phy *phy;
>   	struct gpio_desc *reset;
> +	struct icc_path *icc_mem;
>   	const struct qcom_pcie_cfg *cfg;
>   };
>   
> @@ -1644,6 +1646,74 @@ static const struct dw_pcie_ops dw_pcie_ops = {
>   	.start_link = qcom_pcie_start_link,
>   };
>   
> +static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +	int ret;
> +
> +	pcie->icc_mem = devm_of_icc_get(pci->dev, "pcie-mem");
> +	if (IS_ERR(pcie->icc_mem)) {
> +		ret = PTR_ERR(pcie->icc_mem);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Some Qualcomm platforms require interconnect bandwidth constraints
> +	 * to be set before enabling interconnect clocks.
> +	 *
> +	 * Set an initial peak bandwidth corresponding to single-lane Gen 1
> +	 * for the pcie-mem path.
> +	 */
> +	ret = icc_set_bw(pcie->icc_mem, 0, MBps_to_icc(250));
> +	if (ret) {
> +		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +	u32 offset, status, bw;
> +	int speed, width;
> +	int ret;
> +
> +	if (!pcie->icc_mem)
> +		return;
> +
> +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
> +
> +	/* Only update constraints if link is up. */
> +	if (!(status & PCI_EXP_LNKSTA_DLLLA))
> +		return;
> +
> +	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
> +	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
> +
> +	switch (speed) {
> +	case 1:
> +		bw = MBps_to_icc(250);
> +		break;
> +	case 2:
> +		bw = MBps_to_icc(500);
> +		break;
> +	default:
> +	case 3:
> +		bw = MBps_to_icc(985);
> +		break;
> +	}
> +
> +	ret = icc_set_bw(pcie->icc_mem, 0, width * bw);
> +	if (ret) {
> +		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> +			ret);
> +	}
> +}
> +
>   static int qcom_pcie_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -1704,6 +1774,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>   		goto err_pm_runtime_put;
>   	}
>   
> +	ret = qcom_pcie_icc_init(pcie);
> +	if (ret)
> +		goto err_pm_runtime_put;
> +
>   	ret = pcie->cfg->ops->get_resources(pcie);
>   	if (ret)
>   		goto err_pm_runtime_put;
> @@ -1722,6 +1796,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>   		goto err_phy_exit;
>   	}
>   
> +	qcom_pcie_icc_update(pcie);
> +
>   	return 0;
>   
>   err_phy_exit:
  

On Fri, Oct 21, 2022 at 08:46:16AM +0200, Johan Hovold wrote:
> On Qualcomm platforms like SC8280XP and SA8540P, interconnect bandwidth
> must be requested before enabling interconnect clocks.
> 
> Add basic support for managing an optional "pcie-mem" interconnect path
> by setting a low constraint before enabling clocks and updating it after
> the link is up.
> 
> Note that it is not possible for a controller driver to set anything but
> a maximum peak bandwidth as expected average bandwidth will vary with
> use case and actual use (and power policy?). This very much remains an
> unresolved problem with the interconnect framework.
> 
> Also note that no constraint is set for the SC8280XP/SA8540P "cpu-pcie"
> path for now as it is not clear what an appropriate constraint would be
> (and the system does not crash when left unspecified).
> 

I initially thought we should move this to dwc core but I'm not sure if the
interconnect path is going to be the same for all platforms. So keeping it
within Qcom driver is good for now. 

> Fixes: 70574511f3fc ("PCI: qcom: Add support for SC8280XP")
> Reviewed-by: Brian Masney <bmasney@redhat.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 76 ++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 7db94a22238d..0c13f976626f 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -12,6 +12,7 @@
>  #include <linux/crc8.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/interconnect.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
> @@ -224,6 +225,7 @@ struct qcom_pcie {
>  	union qcom_pcie_resources res;
>  	struct phy *phy;
>  	struct gpio_desc *reset;
> +	struct icc_path *icc_mem;
>  	const struct qcom_pcie_cfg *cfg;
>  };
>  
> @@ -1644,6 +1646,74 @@ static const struct dw_pcie_ops dw_pcie_ops = {
>  	.start_link = qcom_pcie_start_link,
>  };
>  
> +static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +	int ret;
> +
> +	pcie->icc_mem = devm_of_icc_get(pci->dev, "pcie-mem");
> +	if (IS_ERR(pcie->icc_mem)) {
> +		ret = PTR_ERR(pcie->icc_mem);
> +		return ret;

return PTR_ERR(pcie->icc_mem);

> +	}
> +
> +	/*
> +	 * Some Qualcomm platforms require interconnect bandwidth constraints
> +	 * to be set before enabling interconnect clocks.
> +	 *
> +	 * Set an initial peak bandwidth corresponding to single-lane Gen 1
> +	 * for the pcie-mem path.
> +	 */
> +	ret = icc_set_bw(pcie->icc_mem, 0, MBps_to_icc(250));
> +	if (ret) {
> +		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> +			ret);

Move "ret);" to prior line. No need to keep up within 80 columns.

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +	u32 offset, status, bw;
> +	int speed, width;
> +	int ret;
> +
> +	if (!pcie->icc_mem)
> +		return;
> +
> +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
> +
> +	/* Only update constraints if link is up. */
> +	if (!(status & PCI_EXP_LNKSTA_DLLLA))
> +		return;

What if the link comes back later? I'd suggest to call this function from
qcom_pcie_link_up(), whenever link is up.

> +
> +	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
> +	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
> +
> +	switch (speed) {
> +	case 1:
> +		bw = MBps_to_icc(250);
> +		break;
> +	case 2:
> +		bw = MBps_to_icc(500);
> +		break;
> +	default:
> +	case 3:

Why do you need explicit "case 3" and not just default case?

> +		bw = MBps_to_icc(985);
> +		break;
> +	}
> +
> +	ret = icc_set_bw(pcie->icc_mem, 0, width * bw);
> +	if (ret) {
> +		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> +			ret);

Move "ret);" to prior line and save braces.

Thanks,
Mani

> +	}
> +}
> +
>  static int qcom_pcie_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -1704,6 +1774,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  		goto err_pm_runtime_put;
>  	}
>  
> +	ret = qcom_pcie_icc_init(pcie);
> +	if (ret)
> +		goto err_pm_runtime_put;
> +
>  	ret = pcie->cfg->ops->get_resources(pcie);
>  	if (ret)
>  		goto err_pm_runtime_put;
> @@ -1722,6 +1796,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  		goto err_phy_exit;
>  	}
>  
> +	qcom_pcie_icc_update(pcie);
> +
>  	return 0;
>  
>  err_phy_exit:
> -- 
> 2.37.3
>
  

On Tue, Nov 01, 2022 at 08:05:29PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Oct 21, 2022 at 08:46:16AM +0200, Johan Hovold wrote:
> > On Qualcomm platforms like SC8280XP and SA8540P, interconnect bandwidth
> > must be requested before enabling interconnect clocks.
> > 
> > Add basic support for managing an optional "pcie-mem" interconnect path
> > by setting a low constraint before enabling clocks and updating it after
> > the link is up.
> > 
> > Note that it is not possible for a controller driver to set anything but
> > a maximum peak bandwidth as expected average bandwidth will vary with
> > use case and actual use (and power policy?). This very much remains an
> > unresolved problem with the interconnect framework.
> > 
> > Also note that no constraint is set for the SC8280XP/SA8540P "cpu-pcie"
> > path for now as it is not clear what an appropriate constraint would be
> > (and the system does not crash when left unspecified).
 
> > +static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> > +{
> > +	struct dw_pcie *pci = pcie->pci;
> > +	int ret;
> > +
> > +	pcie->icc_mem = devm_of_icc_get(pci->dev, "pcie-mem");
> > +	if (IS_ERR(pcie->icc_mem)) {
> > +		ret = PTR_ERR(pcie->icc_mem);
> > +		return ret;
> 
> return PTR_ERR(pcie->icc_mem);

Sure.

> > +	}
> > +
> > +	/*
> > +	 * Some Qualcomm platforms require interconnect bandwidth constraints
> > +	 * to be set before enabling interconnect clocks.
> > +	 *
> > +	 * Set an initial peak bandwidth corresponding to single-lane Gen 1
> > +	 * for the pcie-mem path.
> > +	 */
> > +	ret = icc_set_bw(pcie->icc_mem, 0, MBps_to_icc(250));
> > +	if (ret) {
> > +		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> > +			ret);
> 
> Move "ret);" to prior line. No need to keep up within 80 columns.

80 chars is still a soft limit and in this case there's no real gain in
terms of readability from breaking it.

But sure, I can remove the line break.
 
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> > +{
> > +	struct dw_pcie *pci = pcie->pci;
> > +	u32 offset, status, bw;
> > +	int speed, width;
> > +	int ret;
> > +
> > +	if (!pcie->icc_mem)
> > +		return;
> > +
> > +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > +	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
> > +
> > +	/* Only update constraints if link is up. */
> > +	if (!(status & PCI_EXP_LNKSTA_DLLLA))
> > +		return;
> 
> What if the link comes back later? I'd suggest to call this function from
> qcom_pcie_link_up(), whenever link is up.

I actually tried that initially but realised it doesn't work.

First, the link-up callback can be called in atomic context which
prevents using icc_set_bw() directly (this can be worked around of
course).

Second, the link-up callback isn't even called if the link comes up
later.

If anyone needs this to deal with FPGA-type use cases when the link
comes up later, then dwc3 core would need to be extended first. 
 
> > +
> > +	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
> > +	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
> > +
> > +	switch (speed) {
> > +	case 1:
> > +		bw = MBps_to_icc(250);
> > +		break;
> > +	case 2:
> > +		bw = MBps_to_icc(500);
> > +		break;
> > +	default:
> > +	case 3:
> 
> Why do you need explicit "case 3" and not just default case?

Because it's the gen3 bandwidth which is set in case the controller
ever reports anything else but the supported gen1, gen2 or gen3 speed
here.

I first had a WARN_ON_ONCE() here as an aid to anyone ever extending the
driver with support for gen4, but then removed it in case there are any
misbehaving controllers out there. I guess I can add it back and see if
anyone complains.

> > +		bw = MBps_to_icc(985);
> > +		break;
> > +	}
> > +
> > +	ret = icc_set_bw(pcie->icc_mem, 0, width * bw);
> > +	if (ret) {
> > +		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> > +			ret);

Johan
  

On Wed, Nov 02, 2022 at 09:38:31AM +0100, Johan Hovold wrote:
> On Tue, Nov 01, 2022 at 08:05:29PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Oct 21, 2022 at 08:46:16AM +0200, Johan Hovold wrote:
> > > On Qualcomm platforms like SC8280XP and SA8540P, interconnect bandwidth
> > > must be requested before enabling interconnect clocks.
> > > 
> > > Add basic support for managing an optional "pcie-mem" interconnect path
> > > by setting a low constraint before enabling clocks and updating it after
> > > the link is up.
> > > 
> > > Note that it is not possible for a controller driver to set anything but
> > > a maximum peak bandwidth as expected average bandwidth will vary with
> > > use case and actual use (and power policy?). This very much remains an
> > > unresolved problem with the interconnect framework.
> > > 
> > > Also note that no constraint is set for the SC8280XP/SA8540P "cpu-pcie"
> > > path for now as it is not clear what an appropriate constraint would be
> > > (and the system does not crash when left unspecified).

> > > +	}
> > > +
> > > +	/*
> > > +	 * Some Qualcomm platforms require interconnect bandwidth constraints
> > > +	 * to be set before enabling interconnect clocks.
> > > +	 *
> > > +	 * Set an initial peak bandwidth corresponding to single-lane Gen 1
> > > +	 * for the pcie-mem path.
> > > +	 */
> > > +	ret = icc_set_bw(pcie->icc_mem, 0, MBps_to_icc(250));
> > > +	if (ret) {
> > > +		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> > > +			ret);
> > 
> > Move "ret);" to prior line. No need to keep up within 80 columns.
> 
> 80 chars is still a soft limit and in this case there's no real gain in
> terms of readability from breaking it.
> 
> But sure, I can remove the line break.

After looking at the result, I changed my mind here and will stick to
the 80 char limit.

Johan