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
Add the missing interconnects and interconnect-names properties to the
ufs_mem_hc and ufs_card_hc nodes.
Signed-off-by: Brian Masney <[email protected]>
---
arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index 6bc12e507d21..17c8dc8d4767 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -856,6 +856,10 @@ ufs_mem_hc: ufs@1d84000 {
iommus = <&apps_smmu 0xe0 0x0>;
+ interconnects = <&aggre1_noc MASTER_UFS_MEM 0 &mc_virt SLAVE_EBI1 0>,
+ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_UFS_MEM_CFG 0>;
+ interconnect-names = "ufs-ddr", "cpu-ufs";
+
clocks = <&gcc GCC_UFS_PHY_AXI_CLK>,
<&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>,
<&gcc GCC_UFS_PHY_AHB_CLK>,
@@ -924,6 +928,10 @@ ufs_card_hc: ufs@1da4000 {
iommus = <&apps_smmu 0x4a0 0x0>;
+ interconnects = <&aggre2_noc MASTER_UFS_CARD 0 &mc_virt SLAVE_EBI1 0>,
+ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_UFS_CARD_CFG 0>;
+ interconnect-names = "ufs-ddr", "cpu-ufs";
+
clocks = <&gcc GCC_UFS_CARD_AXI_CLK>,
<&gcc GCC_AGGRE_UFS_CARD_AXI_CLK>,
<&gcc GCC_UFS_CARD_AHB_CLK>,
--
2.38.1
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 <[email protected]>
---
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
On 17/11/2022 11:49, Brian Masney wrote:
> 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).
I wonder whether this is solving the same or orthogonal problem as my
old patchset here:
https://lore.kernel.org/all/[email protected]/
Best regards,
Krzysztof
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 <[email protected]>
> ---
> 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
>
--
மணிவண்ணன் சதாசிவம்
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 <[email protected]>
> ---
> 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
On 17/11/2022 13:12, Krzysztof Kozlowski wrote:
> On 17/11/2022 11:49, Brian Masney wrote:
>> 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).
>
> I wonder whether this is solving the same or orthogonal problem as my
> old patchset here:
>
> https://lore.kernel.org/all/[email protected]/
More or less it does. Vendor kernel scales both paths according to the
gear selected. I was surprised to see just two entries there. sdm845 has
22 entries in its msm-bus scaling table. What was the reason for just
two entries in your case?
What was the net result for that patchset? Is it going to be merged
anytime?
I think we can start with just a version of this patchset that enables
static ICC config and then upgrade that with proper OPP tables, WDYT?
(I wrote 'a version' since I had to modify the patch to set avg_bw
instead of setting the peak_bw and to pass different values instead of
UINT_MAX, I'll send it).
--
With best wishes
Dmitry
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 <[email protected]>
> ---
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",