[PATCH 0/2] arm64/msm8996: enable UFS interconnect

[PATCH 0/2] arm64/msm8996: enable UFS interconnect

[Dmitry Baryshkov]

MSM8996 requires a vote on UFS interconnects to work in a stable manner.
The first patch is a rework of older patch from Brian, see [1]

[1] https://lore.kernel.org/all/20221117104957.254648-2-bmasney@redhat.com/

Brian Masney (1):
  scsi: ufs: ufs-qcom: add basic interconnect support

Dmitry Baryshkov (1):
  arm64: dts: qcom: msm8996: enable UFS interconnects

 arch/arm64/boot/dts/qcom/msm8996.dtsi |  4 ++++
 drivers/ufs/host/ufs-qcom.c           | 26 ++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

-- 
2.39.0

[PATCH 1/2] scsi: ufs: ufs-qcom: add basic interconnect support

[Dmitry Baryshkov]

From: Brian Masney <bmasney@redhat.com>

The firmware on the Qualcomm platforms expects the interconnect votes to
be present. Let's add very basic support where the maximum throughput is
requested to match what's done in a few other drivers.

This will not break boot on systems where the interconnects and
interconnect-names properties are not specified in device tree for UFS
since the interconnect framework will silently return.

Signed-off-by: Brian Masney <bmasney@redhat.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 8ad1415e10b6..8267a4056120 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -14,6 +14,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/reset-controller.h>
 #include <linux/devfreq.h>
+#include <linux/interconnect.h>
 
 #include <ufs/ufshcd.h>
 #include "ufshcd-pltfrm.h"
@@ -936,6 +937,23 @@ static const struct reset_control_ops ufs_qcom_reset_ops = {
 	.deassert = ufs_qcom_reset_deassert,
 };
 
+static int ufs_qcom_icc_init(struct device *dev, char *pathname,
+		u32 avg_bw, u32 peak_bw)
+{
+	struct icc_path *path;
+	int ret;
+
+	path = devm_of_icc_get(dev, pathname);
+	if (IS_ERR(path))
+		return dev_err_probe(dev, PTR_ERR(path), "failed to acquire interconnect path\n");
+
+	ret = icc_set_bw(path, avg_bw, peak_bw);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to set bandwidth request\n");
+
+	return 0;
+}
+
 /**
  * ufs_qcom_init - bind phy with controller
  * @hba: host controller instance
@@ -1005,6 +1023,14 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 	ufs_qcom_get_controller_revision(hba, &host->hw_ver.major,
 		&host->hw_ver.minor, &host->hw_ver.step);
 
+	err = ufs_qcom_icc_init(dev, "ufs-ddr", 4096000, 0);
+	if (err)
+		goto out_variant_clear;
+
+	err = ufs_qcom_icc_init(dev, "cpu-ufs", 1000, 0);
+	if (err)
+		goto out_variant_clear;
+
 	/*
 	 * for newer controllers, device reference clock control bit has
 	 * moved inside UFS controller register address space itself.
-- 
2.39.0

[PATCH 2/2] arm64: dts: qcom: msm8996: enable UFS interconnects

[Dmitry Baryshkov]

Specify interconnects to be used by the UFS host controller.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8996.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 55180586f7b6..0c2f7be9f205 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -2037,6 +2037,10 @@ ufshc: ufshc@624000 {
 				<0 0>,
 				<0 0>;
 
+			interconnects = <&a2noc MASTER_UFS &bimc SLAVE_EBI_CH0>,
+					<&bimc MASTER_AMPSS_M0 &cnoc SLAVE_UFS_CFG>;
+			interconnect-names = "ufs-ddr", "cpu-ufs";
+
 			lanes-per-direction = <1>;
 			#reset-cells = <1>;
 			status = "disabled";
-- 
2.39.0

Re: (subset) [PATCH 0/2] arm64/msm8996: enable UFS interconnect

[Bjorn Andersson]

On Thu, 19 Jan 2023 16:43:24 +0200, Dmitry Baryshkov wrote:
> MSM8996 requires a vote on UFS interconnects to work in a stable manner.
> The first patch is a rework of older patch from Brian, see [1]
> 
> [1] https://lore.kernel.org/all/20221117104957.254648-2-bmasney@redhat.com/
> 
> Brian Masney (1):
>   scsi: ufs: ufs-qcom: add basic interconnect support
> 
> [...]

Applied, thanks!

[2/2] arm64: dts: qcom: msm8996: enable UFS interconnects
      commit: bc72f13e4456afa34ccbd1dfc61aaea18f877b88

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

[PATCH 0/2] qcom: add basic interconnect support to UFS

[Brian Masney]

This patch set adds very basic support for the interconnect framework
to the Qualcomm portion of the UFS framework since the firmware on
these platforms expects the interconnect votes to be present. The
maximum throughput is requested to match what's already done in a few
other drivers.

Here's the relevant entries from the interconnect_summary file in
debugfs that shows the two ICC paths are setup for the first UFS
host controller on the SA8540p automotive board (sc8280xp).

 node                                  tag          avg         peak
--------------------------------------------------------------------
xm_ufs_mem                                            0   4294967295
  1d84000.ufs                            0            0   4294967295
qns_a1noc_snoc                                        0   4294967295
  1d84000.ufs                            0            0   4294967295
qnm_gemnoc_cnoc                                     115   4294967295
  1d84000.ufs                            0            0   4294967295
  a6f8800.usb                            0            0        40000
  884000.serial                          0          115          115
qhs_ufs_mem_cfg                                       0   4294967295
  1d84000.ufs                            0            0   4294967295
chm_apps                                            115   4294967295
  1d84000.ufs                            0            0   4294967295
  a6f8800.usb                            0            0        40000
  884000.serial                          0          115          115
qnm_snoc_sf                                     1000000   4294967295
  1d84000.ufs                            0            0   4294967295
  a6f8800.usb                            0      1000000      2500000
qns_gem_noc_cnoc                                    115   4294967295
  1d84000.ufs                            0            0   4294967295
  a6f8800.usb                            0            0        40000
  884000.serial                          0          115          115
qns_llcc                                        1000000   4294967295
  1d84000.ufs                            0            0   4294967295
  a6f8800.usb                            0      1000000      2500000
llcc_mc                                         1000000   4294967295
  1d84000.ufs                            0            0   4294967295
  a6f8800.usb                            0      1000000      2500000
ebi                                             1000000   4294967295
  1d84000.ufs                            0            0   4294967295
  a6f8800.usb                            0      1000000      2500000
qnm_aggre1_noc                                        0   4294967295
  1d84000.ufs                            0            0   4294967295
qns_gemnoc_sf                                   1000000   4294967295
  1d84000.ufs                            0            0   4294967295
  a6f8800.usb                            0      1000000      2500000


Brian Masney (2):
  scsi: ufs: ufs-qcom: add basic interconnect support
  arm64: dts: qcom: sc8280xp: add interconnect properties to ufs nodes

 arch/arm64/boot/dts/qcom/sc8280xp.dtsi |  8 ++++++++
 drivers/ufs/host/ufs-qcom.c            | 25 +++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

-- 
2.38.1

[PATCH 1/2] scsi: ufs: ufs-qcom: add basic interconnect support

[Brian Masney]

The firmware on the Qualcomm platforms expects the interconnect votes to
be present. Let's add very basic support where the maximum throughput is
requested to match what's done in a few other drivers.

This will not break boot on systems where the interconnects and
interconnect-names properties are not specified in device tree for UFS
since the interconnect framework will silently return.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/ufs/host/ufs-qcom.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 8ad1415e10b6..55bf8dd88985 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -7,6 +7,7 @@
 #include <linux/time.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/interconnect.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -936,6 +937,22 @@ static const struct reset_control_ops ufs_qcom_reset_ops = {
 	.deassert = ufs_qcom_reset_deassert,
 };
 
+static int ufs_qcom_icc_init(struct device *dev, char *pathname)
+{
+	struct icc_path *path;
+	int ret;
+
+	path = devm_of_icc_get(dev, pathname);
+	if (IS_ERR(path))
+		return dev_err_probe(dev, PTR_ERR(path), "failed to acquire interconnect path\n");
+
+	ret = icc_set_bw(path, 0, UINT_MAX);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to set bandwidth request\n");
+
+	return 0;
+}
+
 /**
  * ufs_qcom_init - bind phy with controller
  * @hba: host controller instance
@@ -991,6 +1008,14 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 			err = dev_err_probe(dev, PTR_ERR(host->generic_phy), "Failed to get PHY\n");
 			goto out_variant_clear;
 		}
+
+		err = ufs_qcom_icc_init(dev, "ufs-ddr");
+		if (err)
+			goto out_variant_clear;
+
+		err = ufs_qcom_icc_init(dev, "cpu-ufs");
+		if (err)
+			goto out_variant_clear;
 	}
 
 	host->device_reset = devm_gpiod_get_optional(dev, "reset",
-- 
2.38.1

Re: [PATCH 1/2] scsi: ufs: ufs-qcom: add basic interconnect support

[Manivannan Sadhasivam]

On Thu, Nov 17, 2022 at 05:49:56AM -0500, Brian Masney wrote:
> The firmware on the Qualcomm platforms expects the interconnect votes to
> be present. Let's add very basic support where the maximum throughput is
> requested to match what's done in a few other drivers.
> 
> This will not break boot on systems where the interconnects and
> interconnect-names properties are not specified in device tree for UFS
> since the interconnect framework will silently return.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 8ad1415e10b6..55bf8dd88985 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -7,6 +7,7 @@
>  #include <linux/time.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> +#include <linux/interconnect.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> @@ -936,6 +937,22 @@ static const struct reset_control_ops ufs_qcom_reset_ops = {
>  	.deassert = ufs_qcom_reset_deassert,
>  };
>  
> +static int ufs_qcom_icc_init(struct device *dev, char *pathname)
> +{
> +	struct icc_path *path;
> +	int ret;
> +
> +	path = devm_of_icc_get(dev, pathname);
> +	if (IS_ERR(path))
> +		return dev_err_probe(dev, PTR_ERR(path), "failed to acquire interconnect path\n");
> +
> +	ret = icc_set_bw(path, 0, UINT_MAX);

Please use icc macros for setting the bandwidth. Like, GBps_to_icc(),
MBps_to_icc() etc...

Also, during the init stage you can set a minimum bandwidth that will allow the
controller to get probed successfully. Then, you should update the bandwidth
based on the gear in pwr_change_notify() callback.

> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to set bandwidth request\n");
> +
> +	return 0;
> +}
> +
>  /**
>   * ufs_qcom_init - bind phy with controller
>   * @hba: host controller instance
> @@ -991,6 +1008,14 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>  			err = dev_err_probe(dev, PTR_ERR(host->generic_phy), "Failed to get PHY\n");
>  			goto out_variant_clear;
>  		}
> +
> +		err = ufs_qcom_icc_init(dev, "ufs-ddr");
> +		if (err)
> +			goto out_variant_clear;
> +
> +		err = ufs_qcom_icc_init(dev, "cpu-ufs");
> +		if (err)
> +			goto out_variant_clear;

It'd be nice to have a single function that initializes both paths.

Thanks,
Mani

>  	}
>  
>  	host->device_reset = devm_gpiod_get_optional(dev, "reset",
> -- 
> 2.38.1
> 

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

Re: [PATCH 1/2] scsi: ufs: ufs-qcom: add basic interconnect support

[Dmitry Baryshkov]

On 17/11/2022 12:49, Brian Masney wrote:
> The firmware on the Qualcomm platforms expects the interconnect votes to
> be present. Let's add very basic support where the maximum throughput is
> requested to match what's done in a few other drivers.
> 
> This will not break boot on systems where the interconnects and
> interconnect-names properties are not specified in device tree for UFS
> since the interconnect framework will silently return.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>   drivers/ufs/host/ufs-qcom.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 8ad1415e10b6..55bf8dd88985 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -7,6 +7,7 @@
>   #include <linux/time.h>
>   #include <linux/clk.h>
>   #include <linux/delay.h>
> +#include <linux/interconnect.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/platform_device.h>
> @@ -936,6 +937,22 @@ static const struct reset_control_ops ufs_qcom_reset_ops = {
>   	.deassert = ufs_qcom_reset_deassert,
>   };
>   
> +static int ufs_qcom_icc_init(struct device *dev, char *pathname)
> +{
> +	struct icc_path *path;
> +	int ret;
> +
> +	path = devm_of_icc_get(dev, pathname);
> +	if (IS_ERR(path))
> +		return dev_err_probe(dev, PTR_ERR(path), "failed to acquire interconnect path\n");
> +
> +	ret = icc_set_bw(path, 0, UINT_MAX);

I noticed that this patch bumps peak_bw (and leaves average_bw as 0), 
while vendor kernels bump average_bw, but ib (peak_bw) is set to 0.

> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to set bandwidth request\n");
> +
> +	return 0;
> +}
> +
>   /**
>    * ufs_qcom_init - bind phy with controller
>    * @hba: host controller instance
> @@ -991,6 +1008,14 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>   			err = dev_err_probe(dev, PTR_ERR(host->generic_phy), "Failed to get PHY\n");
>   			goto out_variant_clear;
>   		}
> +
> +		err = ufs_qcom_icc_init(dev, "ufs-ddr");
> +		if (err)
> +			goto out_variant_clear;
> +
> +		err = ufs_qcom_icc_init(dev, "cpu-ufs");
> +		if (err)
> +			goto out_variant_clear;
>   	}
>   
>   	host->device_reset = devm_gpiod_get_optional(dev, "reset",

-- 
With best wishes
Dmitry

Re: [PATCH 1/2] scsi: ufs: ufs-qcom: add basic interconnect support

[Konrad Dybcio]

On 17.11.2022 11:49, Brian Masney wrote:
> The firmware on the Qualcomm platforms expects the interconnect votes to
> be present. Let's add very basic support where the maximum throughput is
> requested to match what's done in a few other drivers.
> 
> This will not break boot on systems where the interconnects and
> interconnect-names properties are not specified in device tree for UFS
> since the interconnect framework will silently return.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
Hi everyone!

This was never merged, but it's actually strictly necessary!

For example UFS dies on SM8450 if we add sync_state to its interconnect
driver, as there's no votes cast.

Can we look into this again?

Konrad
>  drivers/ufs/host/ufs-qcom.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 8ad1415e10b6..55bf8dd88985 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -7,6 +7,7 @@
>  #include <linux/time.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> +#include <linux/interconnect.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> @@ -936,6 +937,22 @@ static const struct reset_control_ops ufs_qcom_reset_ops = {
>  	.deassert = ufs_qcom_reset_deassert,
>  };
>  
> +static int ufs_qcom_icc_init(struct device *dev, char *pathname)
> +{
> +	struct icc_path *path;
> +	int ret;
> +
> +	path = devm_of_icc_get(dev, pathname);
> +	if (IS_ERR(path))
> +		return dev_err_probe(dev, PTR_ERR(path), "failed to acquire interconnect path\n");
> +
> +	ret = icc_set_bw(path, 0, UINT_MAX);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to set bandwidth request\n");
> +
> +	return 0;
> +}
> +
>  /**
>   * ufs_qcom_init - bind phy with controller
>   * @hba: host controller instance
> @@ -991,6 +1008,14 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>  			err = dev_err_probe(dev, PTR_ERR(host->generic_phy), "Failed to get PHY\n");
>  			goto out_variant_clear;
>  		}
> +
> +		err = ufs_qcom_icc_init(dev, "ufs-ddr");
> +		if (err)
> +			goto out_variant_clear;
> +
> +		err = ufs_qcom_icc_init(dev, "cpu-ufs");
> +		if (err)
> +			goto out_variant_clear;
>  	}
>  
>  	host->device_reset = devm_gpiod_get_optional(dev, "reset",