| Message ID | 20221117104957.254648-2-bmasney@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | qcom: add basic interconnect support to UFS | expand |
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 >
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",
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",
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",
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(+)