scsi: qcom-ufs: Add support for bus voting using ICB framework
Subsystems (4)
PATCH 1/1 scsi: qcom-ufs: Add support for bus voting using ICB framework
In reply to a message not in the archiveAdapt to the new ICB framework for bus bandwidth voting.
This requires the source/destination port ids.
Also this requires a tuple of values.
The tuple is for two different paths - from UFS master to BIMC slave. The other is from CPU master to UFS slave. This tuple consists of the average and peak bandwidth.
--- .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 12 ++ drivers/scsi/ufs/ufs-qcom.c | 234 ++++++++++++++++----- drivers/scsi/ufs/ufs-qcom.h | 20 ++ 3 files changed, 218 insertions(+), 48 deletions(-)
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +12 -0
@@ -45,6 +45,18 @@ Optional properties: Note: If above properties are not defined it can be assumed that the supply regulators or clocks are always on. +* Following bus parameters are required: +interconnects +interconnect-names +- Please refer to Documentation/devicetree/bindings/interconnect/ + for more details on the above. +qcom,msm-bus,name - string describing the bus path +qcom,msm-bus,num-cases - number of configurations in which ufs can operate in +qcom,msm-bus,num-paths - number of paths to vote for +qcom,msm-bus,vectors-KBps - Takes a tuple <ib ab>, <ib ab> (2 tuples for 2 num-paths) + The number of these entries *must* be same as + num-cases. + Example: ufshc@0xfc598000 { compatible = "jedec,ufs-1.1";
drivers/scsi/ufs/ufs-qcom.c +186 -48
294 lines not on this page · raw message
drivers/scsi/ufs/ufs-qcom.h +20 -0
@@ -207,6 +207,25 @@ struct ufs_qcom_testbus {u8 select_minor; }; +struct qcom_bus_vectors { +uint64_t ab; +uint64_t ib; +}; + +struct qcom_bus_path { +unsigned int num_paths; +struct qcom_bus_vectors *vec; +}; + +struct qcom_bus_scale_data { +struct qcom_bus_path *usecase; +unsigned int num_usecase; +struct icc_path *ufs_ddr; +struct icc_path *cpu_ufs; + +const char *name; +}; +struct ufs_qcom_host {/* * Set this capability if host controller supports the QUniPro mode
@@ -242,6 +261,7 @@ struct ufs_qcom_host {/* Bitmask for enabling debug prints */ u32 dbg_print_en;struct ufs_qcom_testbus testbus; +struct qcom_bus_scale_data *qbsd; }; static inline u32
Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
Hi Asutosh,
Thanks for the patch!
Quoted 1 line · Asutosh Das
Adapt to the new ICB framework for bus bandwidth voting.
It's actually called interconnect API or interconnect framework.
Quoted 9 lines
This requires the source/destination port ids. Also this requires a tuple of values.
The tuple is for two different paths - from UFS master to BIMC slave. The other is from CPU master to UFS slave. This tuple consists of the average and peak bandwidth.
Signed-off-by Asutosh Das <[email protected]>---
You can put here (below the --- line) the text from the cover letter. Usually people do not add separate cover letters for single patches.
Quoted 10 lines
.../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 12 ++ drivers/scsi/ufs/ufs-qcom.c | 234 ++++++++++++++++----- drivers/scsi/ufs/ufs-qcom.h | 20 ++ 3 files changed, 218 insertions(+), 48 deletions(-)Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +0
-0
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt Optional properties: · line 50
Quoted 6 lines
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +3
-0@@ -45,6 +45,18 @@ Optional properties: Note: If above properties are not defined it can be assumed that the supply regulators or clocks are always on. +* Following bus parameters are required: +interconnects +interconnect-names
Is the above really required? Are the interconnect bandwidth requests required to enable something critical to UFS functionality? Would UFS still work without any bandwidth scaling, although for example slower? Could you please clarify.
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt Optional properties: · line 58
Quoted 8 lines
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +8
-0+- Please refer to Documentation/devicetree/bindings/interconnect/ + for more details on the above. +qcom,msm-bus,name - string describing the bus path +qcom,msm-bus,num-cases - number of configurations in which ufs can operate in +qcom,msm-bus,num-paths - number of paths to vote for +qcom,msm-bus,vectors-KBps - Takes a tuple <ib ab>, <ib ab> (2 tuples for 2 num-paths) + The number of these entries *must* be same as + num-cases.
DT bindings should be submitted as a separate patch. Anyway, people frown upon putting configuration data in DT. Could we put this data into the driver as a static table instead of DT? Also maybe use ab/pb for average/peak bandwidth.
Quoted 9 lines
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +1
-0+ Example: ufshc@0xfc598000 { compatible = "jedec,ufs-1.1";drivers/scsi/ufs/ufs-qcom.c +0
-0
drivers/scsi/ufs/ufs-qcom.c line 20
Quoted 5 lines
drivers/scsi/ufs/ufs-qcom.c +1
-0@@ -16,6 +16,7 @@#include <linux/of.h> #include <linux/platform_device.h> #include <linux/phy/phy.h> +#include <linux/interconnect.h> #include <linux/phy/phy-qcom-ufs.h>
Alphabetical order?
drivers/scsi/ufs/ufs-qcom.c line 32
Quoted 8 lines
drivers/scsi/ufs/ufs-qcom.c +2
-0#include "ufshcd.h" @@ -27,6 +28,10 @@#define UFS_QCOM_DEFAULT_DBG_PRINT_EN \ (UFS_QCOM_DBG_PRINT_REGS_EN | UFS_QCOM_DBG_PRINT_TEST_BUS_EN) +#define UFS_DDR "ufs-ddr" +#define CPU_UFS "cpu-ufs"
Not sure if it's really useful to have them as #defines. Also maybe use "ufs-mem" instead of "ufs-ddr" to be more consistent with other users.
Quoted 121 lines
drivers/scsi/ufs/ufs-qcom.c +74
-13+ +enum {TSTBUS_UAWM ,TSTBUS_UARM ,@@ -714,7 +719,6 @@ static int ufs_qcom_get_pwr_dev_param(struct ufs_qcom_dev_params *qcom_param,return 0 ; } -#ifdef CONFIG_MSM_BUS_SCALING static int ufs_qcom_get_bus_vote (struct ufs_qcom_host *host,const char *speed_mode) {@@ -768,24 +772,83 @@ static void ufs_qcom_get_speed_mode(struct ufs_pa_layer_attr *p, char *result) } } +static int ufs_qcom_get_ib_ab (struct ufs_qcom_host *host,int index, +struct qcom_bus_vectors *ufs_ddr_vec, +struct qcom_bus_vectors *cpu_ufs_vec) +{ +struct qcom_bus_path *usecase; + +if (!host->qbsd) +return -EINVAL ; + +if (index > host->qbsd->num_usecase) +return -EINVAL ; + + usecase = host->qbsd->usecase; + +/* +* +* usecase:0 usecase:0 +* ufs->ddr cpu->ufs +* |vec[0&1] | vec[2&3]| +* +----+----+----+----+ +* | ab | ib | ab | ib | +* |----+----+----+----+ +* . +* . +* . +* usecase:n usecase:n +* ufs->ddr cpu->ufs +* |vec[0&1] | vec[2&3]| +* +----+----+----+----+ +* | ab | ib | ab | ib | +* |----+----+----+----+ +*/ + +/* index refers to offset in usecase */ + ufs_ddr_vec->ab = usecase[index].vec[0 ].ab; + ufs_ddr_vec->ib = usecase[index].vec[0 ].ib; + + cpu_ufs_vec->ab = usecase[index].vec[1 ].ab; + cpu_ufs_vec->ib = usecase[index].vec[1 ].ib; + +return 0 ; +} +static int ufs_qcom_set_bus_vote (struct ufs_qcom_host *host,int vote) {int err =0 ; +struct qcom_bus_scale_data *d = host->qbsd; +struct qcom_bus_vectors path0, path1; +struct device *dev = host->hba->dev; -if (vote != host->bus_vote.curr_vote) { - err =msm_bus_scale_client_update_request ( - host->bus_vote.client_handle, vote); -if (err) { -dev_err (host->hba->dev, -"%s: msm_bus_scale_client_update_request() failed: bus_client_handle=0x%x, vote=%d, err=%d\n" , - __func__, host->bus_vote.client_handle, - vote, err); - goto out; - } + err =ufs_qcom_get_ib_ab (host, vote, &path0, &path1); +if (err) { +dev_err (dev,"Error: failed (%d) to get ib/ab\n" , + err); +return err; + } - host->bus_vote.curr_vote = vote; +dev_dbg (dev,"Setting vote: %d: ufs-ddr: ab: %llu ib: %llu\n" , vote, + path0.ab, path0.ib); + err =icc_set_bw (d->ufs_ddr, path0.ab, path0.ib); +if (err) { +dev_err (dev,"Error: (%d) failed setting (%s) bus vote\n" , err, +UFS_DDR ); +return err; } -out: + +dev_dbg (dev,"Setting: cpu-ufs: ab: %llu ib: %llu\n" , path1.ab, + path1.ib); + err =icc_set (d->cpu_ufs, path1.ab, path1.ib); +if (err) { +dev_err (dev,"Error: (%d) failed setting (%s) bus vote\n" , err, +CPU_UFS ); +return err; + } + + host->bus_vote.curr_vote = vote; +return err; }@@ -807,6 +870,7 @@ static int ufs_qcom_update_bus_bw_vote(struct ufs_qcom_host *host)dev_err (host->hba->dev,"%s: failed %d\n" , __func__, err); else host->bus_vote.saved_vote = vote; +return err; }@@ -837,34 +901,114 @@ static int ufs_qcom_update_bus_bw_vote(struct ufs_qcom_host *host)return count; }
drivers/scsi/ufs/ufs-qcom.c static int ufs_qcom_update_bus_bw_vote(struct ufs_qcom_host *host) · line 913
Quoted 10 lines
drivers/scsi/ufs/ufs-qcom.c +10
-0+static struct qcom_bus_scale_data *ufs_qcom_get_bus_scale_data (struct device + *dev) + +{ +struct platform_device *pdev =to_platform_device (dev); +struct device_node *of_node = dev->of_node; +struct qcom_bus_scale_data *qsd; +struct qcom_bus_path *usecase =NULL ; +int ret =0 , i =0 , j, num_paths, len; +const uint32_t *vec_arr =NULL ;
Please use u32 instead of uint32_t.
Quoted 62 lines
drivers/scsi/ufs/ufs-qcom.c +62
-0+bool mem_err = false; + +if (!pdev) { +dev_err (dev,"Null platform device!\n" ); +return NULL ; + } + + qsd =devm_kzalloc (dev,sizeof (struct qcom_bus_scale_data ),GFP_KERNEL ); +if (!qsd) +return NULL ; + + ret =of_property_read_string (of_node,"qcom,msm-bus,name" , &qsd->name); +if (ret) { +dev_err (dev,"Error: (%d) Bus name missing!\n" , ret); +return NULL ; + } + + ret =of_property_read_u32 (of_node,"qcom,msm-bus,num-cases" , + &qsd->num_usecase); +if (ret) { +pr_err ("Error: num-usecases not found\n" ); + goto err; + } + + usecase =devm_kzalloc (dev, (sizeof (struct qcom_bus_path ) * + qsd->num_usecase),GFP_KERNEL ); +if (!usecase) +return NULL ; + + ret =of_property_read_u32 (of_node,"qcom,msm-bus,num-paths" , + &num_paths); +if (ret) { +pr_err ("Error: num_paths not found\n" ); +return NULL ; + } + + vec_arr =of_get_property (of_node,"qcom,msm-bus,vectors-KBps" , &len); +if (vec_arr ==NULL ) { +pr_err ("Error: Vector array not found\n" ); +return NULL ; + } + +for (i =0 ; i < qsd->num_usecase; i++) { + usecase[i].num_paths = num_paths; + usecase[i].vec =devm_kzalloc (dev, num_paths * +sizeof (struct qcom_bus_vectors ), +GFP_KERNEL ); +if (!usecase[i].vec) { + mem_err = true; +dev_err (dev,"Error: Failed to alloc mem for vectors\n" ); + goto err; + } + +for (j =0 ; j < num_paths; j++) { +int idx = ((i * num_paths) + j) *2 ; + + usecase[i].vec[j].ab = (uint64_t ) +be32_to_cpu (vec_arr[idx]); + usecase[i].vec[j].ib = (uint64_t ) +be32_to_cpu (vec_arr[idx +1 ]); + } + }
drivers/scsi/ufs/ufs-qcom.c static int ufs_qcom_update_bus_bw_vote(struct ufs_qcom_host *host) · line 985
Quoted 10 lines
drivers/scsi/ufs/ufs-qcom.c +10
-0+ + qsd->usecase = usecase; +return qsd; +err: +if (mem_err) { +for (; i >0 ; i--) +kfree (usecase[i].vec); + } +return NULL ; +}
We wouldn't need all the above DT parsing if we add a sdm845 bandwidth usecase table. Could you give it a try?
Thanks,
Georgi
Quoted 26 lines · Asutosh Das
Adapt to the new ICB framework for bus bandwidth voting.
This requires the source/destination port ids. Also this requires a tuple of values.
The tuple is for two different paths - from UFS master to BIMC slave. The other is from CPU master to UFS slave. This tuple consists of the average and peak bandwidth.
Signed-off-by Asutosh Das <[email protected]>--- .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 12 ++ drivers/scsi/ufs/ufs-qcom.c | 234 ++++++++++++++++----- drivers/scsi/ufs/ufs-qcom.h | 20 ++ 3 files changed, 218 insertions(+), 48 deletions(-)Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +1
-0@@ -45,6 +45,18 @@ Optional properties: Note: If above properties are not defined it can be assumed that the supply regulators or clocks are always on. +* Following bus parameters are required:
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt Optional properties: · line 58
Quoted 10 lines
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +10
-0+interconnects +interconnect-names +- Please refer to Documentation/devicetree/bindings/interconnect/ + for more details on the above. +qcom,msm-bus,name - string describing the bus path +qcom,msm-bus,num-cases - number of configurations in which ufs can operate in +qcom,msm-bus,num-paths - number of paths to vote for +qcom,msm-bus,vectors-KBps - Takes a tuple <ib ab>, <ib ab> (2 tuples for 2 num-paths) + The number of these entries *must* be same as + num-cases.
I think we can do away with all of the qcom* ones, right? This should be achievable with just interconnects and interconnect-names.
Also, is this patch based on a downstream tree? I don't recognize a lot of the context. We'll need a patch that's based on an upstream tree.
I'll wait to review the rest of the patch until rev 2, since it's hard to reason about the patch with all the downstream stuff in there. -Evan
Quoted 8 lines · Georgi Djakov
Hi Asutosh,
Thanks for the patch!
Quoted 1 line · Asutosh Das
Adapt to the new ICB framework for bus bandwidth voting.
It's actually called interconnect API or interconnect framework.
Thanks! will change it.
Quoted 8 lines
Quoted 8 lines
This requires the source/destination port ids. Also this requires a tuple of values.
The tuple is for two different paths - from UFS master to BIMC slave. The other is from CPU master to UFS slave. This tuple consists of the average and peak bandwidth.
Quoted 5 lines
Quoted 2 lines
Signed-off-by Asutosh Das <[email protected]>---
You can put here (below the --- line) the text from the cover letter. Usually people do not add separate cover letters for single patches.
Ok - will do so.
Quoted 14 lines
Quoted 14 lines
.../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 12 ++ drivers/scsi/ufs/ufs-qcom.c | 234 ++++++++++++++++----- drivers/scsi/ufs/ufs-qcom.h | 20 ++ 3 files changed, 218 insertions(+), 48 deletions(-)Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +0
-0@@ -45,6 +45,18 @@ Optional properties: Note: If above properties are not defined it can be assumed that the supply regulators or clocks are always on.
Quoted 8 lines
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt Optional properties: · line 50
Quoted 3 lines
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +3
-0+* Following bus parameters are required: +interconnects +interconnect-namesIs the above really required? Are the interconnect bandwidth requests required to enable something critical to UFS functionality? Would UFS still work without any bandwidth scaling, although for example slower? Could you please clarify.
Yes - UFS will still work without any bandwidth scaling - but the performance would be terrible.
Quoted 14 lines
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt Optional properties: · line 58
Quoted 9 lines
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +8
-0+- Please refer to Documentation/devicetree/bindings/interconnect/ + for more details on the above. +qcom,msm-bus,name - string describing the bus path +qcom,msm-bus,num-cases - number of configurations in which ufs can operate in +qcom,msm-bus,num-paths - number of paths to vote for +qcom,msm-bus,vectors-KBps - Takes a tuple <ib ab>, <ib ab> (2 tuples for 2 num-paths) + The number of these entries *must* be same as + num-cases.DT bindings should be submitted as a separate patch. Anyway, people frown upon putting configuration data in DT. Could we put this data into the driver as a static table instead of DT? Also maybe use ab/pb for average/peak bandwidth.
The ab/ib value change depending on the target - that's the reasoning for putting it in dts file. However, I'm open to ideas as to how else to handle this.
Agree to changing it to pb.
Quoted 17 lines
Quoted 10 lines
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +1
-0+ Example: ufshc@0xfc598000 { compatible = "jedec,ufs-1.1";drivers/scsi/ufs/ufs-qcom.c +0
-0drivers/scsi/ufs/ufs-qcom.c line 20
Quoted 5 lines
drivers/scsi/ufs/ufs-qcom.c +1
-0@@ -16,6 +16,7 @@#include <linux/of.h> #include <linux/platform_device.h> #include <linux/phy/phy.h> +#include <linux/interconnect.h> #include <linux/phy/phy-qcom-ufs.h> Alphabetical order?
Agreed.
Quoted 7 lines
Quoted 7 lines
drivers/scsi/ufs/ufs-qcom.c +0
-0#include "ufshcd.h" @@ -27,6 +28,10 @@#define UFS_QCOM_DEFAULT_DBG_PRINT_EN \ (UFS_QCOM_DBG_PRINT_REGS_EN | UFS_QCOM_DBG_PRINT_TEST_BUS_EN)
Quoted 5 lines
drivers/scsi/ufs/ufs-qcom.c line 32
Quoted 2 lines
drivers/scsi/ufs/ufs-qcom.c +2
-0+#define UFS_DDR "ufs-ddr" +#define CPU_UFS "cpu-ufs"Not sure if it's really useful to have them as #defines. Also maybe use "ufs-mem" instead of "ufs-ddr" to be more consistent with other users.
I can declare these as strings too - I don't have a preference.
Please let me know if you've a preference.
The reason for ufs-ddr was that it describes the path from ufs device to ddr. Am fine with ufs-mem too.
Quoted 122 lines
Quoted 122 lines
drivers/scsi/ufs/ufs-qcom.c +74
-13+ +enum {TSTBUS_UAWM ,TSTBUS_UARM ,@@ -714,7 +719,6 @@ static int ufs_qcom_get_pwr_dev_param(struct ufs_qcom_dev_params *qcom_param,return 0 ; } -#ifdef CONFIG_MSM_BUS_SCALING static int ufs_qcom_get_bus_vote (struct ufs_qcom_host *host,const char *speed_mode) {@@ -768,24 +772,83 @@ static void ufs_qcom_get_speed_mode(struct ufs_pa_layer_attr *p, char *result) } } +static int ufs_qcom_get_ib_ab (struct ufs_qcom_host *host,int index, +struct qcom_bus_vectors *ufs_ddr_vec, +struct qcom_bus_vectors *cpu_ufs_vec) +{ +struct qcom_bus_path *usecase; + +if (!host->qbsd) +return -EINVAL ; + +if (index > host->qbsd->num_usecase) +return -EINVAL ; + + usecase = host->qbsd->usecase; + +/* +* +* usecase:0 usecase:0 +* ufs->ddr cpu->ufs +* |vec[0&1] | vec[2&3]| +* +----+----+----+----+ +* | ab | ib | ab | ib | +* |----+----+----+----+ +* . +* . +* . +* usecase:n usecase:n +* ufs->ddr cpu->ufs +* |vec[0&1] | vec[2&3]| +* +----+----+----+----+ +* | ab | ib | ab | ib | +* |----+----+----+----+ +*/ + +/* index refers to offset in usecase */ + ufs_ddr_vec->ab = usecase[index].vec[0 ].ab; + ufs_ddr_vec->ib = usecase[index].vec[0 ].ib; + + cpu_ufs_vec->ab = usecase[index].vec[1 ].ab; + cpu_ufs_vec->ib = usecase[index].vec[1 ].ib; + +return 0 ; +} +static int ufs_qcom_set_bus_vote (struct ufs_qcom_host *host,int vote) {int err =0 ; +struct qcom_bus_scale_data *d = host->qbsd; +struct qcom_bus_vectors path0, path1; +struct device *dev = host->hba->dev; -if (vote != host->bus_vote.curr_vote) { - err =msm_bus_scale_client_update_request ( - host->bus_vote.client_handle, vote); -if (err) { -dev_err (host->hba->dev, -"%s: msm_bus_scale_client_update_request() failed: bus_client_handle=0x%x, vote=%d, err=%d\n" , - __func__, host->bus_vote.client_handle, - vote, err); - goto out; - } + err =ufs_qcom_get_ib_ab (host, vote, &path0, &path1); +if (err) { +dev_err (dev,"Error: failed (%d) to get ib/ab\n" , + err); +return err; + } - host->bus_vote.curr_vote = vote; +dev_dbg (dev,"Setting vote: %d: ufs-ddr: ab: %llu ib: %llu\n" , vote, + path0.ab, path0.ib); + err =icc_set_bw (d->ufs_ddr, path0.ab, path0.ib); +if (err) { +dev_err (dev,"Error: (%d) failed setting (%s) bus vote\n" , err, +UFS_DDR ); +return err; } -out: + +dev_dbg (dev,"Setting: cpu-ufs: ab: %llu ib: %llu\n" , path1.ab, + path1.ib); + err =icc_set (d->cpu_ufs, path1.ab, path1.ib); +if (err) { +dev_err (dev,"Error: (%d) failed setting (%s) bus vote\n" , err, +CPU_UFS ); +return err; + } + + host->bus_vote.curr_vote = vote; +return err; }@@ -807,6 +870,7 @@ static int ufs_qcom_update_bus_bw_vote(struct ufs_qcom_host *host)dev_err (host->hba->dev,"%s: failed %d\n" , __func__, err); else host->bus_vote.saved_vote = vote; +return err; }@@ -837,34 +901,114 @@ static int ufs_qcom_update_bus_bw_vote(struct ufs_qcom_host *host)return count; }
Quoted 12 lines
drivers/scsi/ufs/ufs-qcom.c static int ufs_qcom_update_bus_bw_vote(struct ufs_qcom_host *host) · line 913
Quoted 10 lines
drivers/scsi/ufs/ufs-qcom.c +10
-0+static struct qcom_bus_scale_data *ufs_qcom_get_bus_scale_data (struct device + *dev) + +{ +struct platform_device *pdev =to_platform_device (dev); +struct device_node *of_node = dev->of_node; +struct qcom_bus_scale_data *qsd; +struct qcom_bus_path *usecase =NULL ; +int ret =0 , i =0 , j, num_paths, len; +const uint32_t *vec_arr =NULL ;Please use u32 instead of
uint32_t.
Agreed.
Quoted 68 lines
Quoted 68 lines
drivers/scsi/ufs/ufs-qcom.c +67
-0+bool mem_err = false; + +if (!pdev) { +dev_err (dev,"Null platform device!\n" ); +return NULL ; + } + + qsd =devm_kzalloc (dev,sizeof (struct qcom_bus_scale_data ),GFP_KERNEL ); +if (!qsd) +return NULL ; + + ret =of_property_read_string (of_node,"qcom,msm-bus,name" , &qsd->name); +if (ret) { +dev_err (dev,"Error: (%d) Bus name missing!\n" , ret); +return NULL ; + } + + ret =of_property_read_u32 (of_node,"qcom,msm-bus,num-cases" , + &qsd->num_usecase); +if (ret) { +pr_err ("Error: num-usecases not found\n" ); + goto err; + } + + usecase =devm_kzalloc (dev, (sizeof (struct qcom_bus_path ) * + qsd->num_usecase),GFP_KERNEL ); +if (!usecase) +return NULL ; + + ret =of_property_read_u32 (of_node,"qcom,msm-bus,num-paths" , + &num_paths); +if (ret) { +pr_err ("Error: num_paths not found\n" ); +return NULL ; + } + + vec_arr =of_get_property (of_node,"qcom,msm-bus,vectors-KBps" , &len); +if (vec_arr ==NULL ) { +pr_err ("Error: Vector array not found\n" ); +return NULL ; + } + +for (i =0 ; i < qsd->num_usecase; i++) { + usecase[i].num_paths = num_paths; + usecase[i].vec =devm_kzalloc (dev, num_paths * +sizeof (struct qcom_bus_vectors ), +GFP_KERNEL ); +if (!usecase[i].vec) { + mem_err = true; +dev_err (dev,"Error: Failed to alloc mem for vectors\n" ); + goto err; + } + +for (j =0 ; j < num_paths; j++) { +int idx = ((i * num_paths) + j) *2 ; + + usecase[i].vec[j].ab = (uint64_t ) +be32_to_cpu (vec_arr[idx]); + usecase[i].vec[j].ib = (uint64_t ) +be32_to_cpu (vec_arr[idx +1 ]); + } + } + + qsd->usecase = usecase; +return qsd; +err: +if (mem_err) {
Quoted 8 lines
drivers/scsi/ufs/ufs-qcom.c static int ufs_qcom_update_bus_bw_vote(struct ufs_qcom_host *host) · line 985
Quoted 5 lines
drivers/scsi/ufs/ufs-qcom.c +5
-0+for (; i >0 ; i--) +kfree (usecase[i].vec); + } +return NULL ; +}We wouldn't need all the above DT parsing if we add a sdm845 bandwidth usecase table. Could you give it a try?
Replied above - Please let me know if you've any suggestions on how else to handle this, as it can change with targets.
Quoted 4 lines
Thanks,
Georgi
Hi Georgi - thanks for the comments.
Replied to your comments inline.
-asd
Signature · 1 line
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 26 lines · Evan Green
Quoted 25 lines · Asutosh Das
Adapt to the new ICB framework for bus bandwidth voting.
This requires the source/destination port ids. Also this requires a tuple of values.
The tuple is for two different paths - from UFS master to BIMC slave. The other is from CPU master to UFS slave. This tuple consists of the average and peak bandwidth.
Signed-off-by Asutosh Das <[email protected]>--- .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 12 ++ drivers/scsi/ufs/ufs-qcom.c | 234 ++++++++++++++++----- drivers/scsi/ufs/ufs-qcom.h | 20 ++ 3 files changed, 218 insertions(+), 48 deletions(-)Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +0
-0@@ -45,6 +45,18 @@ Optional properties: Note: If above properties are not defined it can be assumed that the supply regulators or clocks are always on.
Quoted 14 lines
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt Optional properties: · line 58
Quoted 11 lines
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +11
-0+* Following bus parameters are required: +interconnects +interconnect-names +- Please refer to Documentation/devicetree/bindings/interconnect/ + for more details on the above. +qcom,msm-bus,name - string describing the bus path +qcom,msm-bus,num-cases - number of configurations in which ufs can operate in +qcom,msm-bus,num-paths - number of paths to vote for +qcom,msm-bus,vectors-KBps - Takes a tuple <ib ab>, <ib ab> (2 tuples for 2 num-paths) + The number of these entries *must* be same as + num-cases.I think we can do away with all of the qcom* ones, right? This should be achievable with just interconnects and interconnect-names.
Let me give that a bit more thought - though I'm not sure how that'd work.
Quoted 4 lines
Also, is this patch based on a downstream tree? I don't recognize a lot of the context. We'll need a patch that's based on an upstream tree.
This was developed on the internal Chrome AU and ported to ufs-next. Let me check internally on this anyway.
Quoted 5 lines
I'll wait to review the rest of the patch until rev 2, since it's hard to reason about the patch with all the downstream stuff in there. -Evan
Hi Evan - thanks for the comments.
-asd
Signature · 1 line
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 42 lines · Asutosh Das (asd)
Quoted 26 lines · Evan Green
Quoted 25 lines · Asutosh Das
Adapt to the new ICB framework for bus bandwidth voting.
This requires the source/destination port ids. Also this requires a tuple of values.
The tuple is for two different paths - from UFS master to BIMC slave. The other is from CPU master to UFS slave. This tuple consists of the average and peak bandwidth.
Signed-off-by Asutosh Das <[email protected]>--- .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 12 ++ drivers/scsi/ufs/ufs-qcom.c | 234 ++++++++++++++++----- drivers/scsi/ufs/ufs-qcom.h | 20 ++ 3 files changed, 218 insertions(+), 48 deletions(-)Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +0
-0@@ -45,6 +45,18 @@ Optional properties: Note: If above properties are not defined it can be assumed that the supply regulators or clocks are always on.Quoted 14 lines
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt Optional properties: · line 58
Quoted 11 lines
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +11
-0+* Following bus parameters are required: +interconnects +interconnect-names +- Please refer to Documentation/devicetree/bindings/interconnect/ + for more details on the above. +qcom,msm-bus,name - string describing the bus path +qcom,msm-bus,num-cases - number of configurations in which ufs can operate in +qcom,msm-bus,num-paths - number of paths to vote for +qcom,msm-bus,vectors-KBps - Takes a tuple <ib ab>, <ib ab> (2 tuples for 2 num-paths) + The number of these entries *must* be same as + num-cases.I think we can do away with all of the qcom* ones, right? This should be achievable with just interconnects and interconnect-names.
Quoted 3 lines
Let me give that a bit more thought - though I'm not sure how that'd work.
From the downstream kernel that I have, it looks like these are
basically used to define the bandwidth values for each gear/mode in UFS. My understanding is that the DT folks generally balk at having configuration data in the device tree. I'm hopeful that we can have a snippet of code that actually computes the required bandwidth for a certain combination of gear speed and lanes. But if that is somehow not possible, this can at worst be a table in code of bandwidths[gear]. But let's try for the computation first.
Quoted 8 lines
Quoted 5 lines
Also, is this patch based on a downstream tree? I don't recognize a lot of the context. We'll need a patch that's based on an upstream tree.
This was developed on the internal Chrome AU and ported to ufs-next. Let me check internally on this anyway.
Whoops, you're right, the context I was confused about does appear to be upstream. I was unaware there was dangling code to the old downstream bus scaling stuff in the upstream kernel. I think we should get rid of all of that and start fresh.
Also, like the dangling downstream code was, the new stuff should probably be surrounded by ifdefs for the new interconnect core.
I also think we don't really need the max_bus_bw thing, so when we rip out all the downstream leftovers we don't need to put that back in.
-Evan
Hi Asutosh,
Quoted 1 line · Asutosh Das (asd)
On 1/24/2019 5:16 PM, Georgi Djakov wrote:
[..]>>> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
Quoted 16 lines
Quoted 8 lines
Quoted 8 lines
b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txtindex a99ed55..94249ef 100644Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +0
-0@@ -45,6 +45,18 @@ Optional properties:Note: If above properties are not defined it can be assumed that the supply
regulators or clocks are always on.Quoted 8 lines
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt Optional properties: · line 50
Quoted 3 lines
+* Following bus parameters are required:
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +2
-0+interconnects +interconnect-namesIs the above really required? Are the interconnect bandwidth requests required to enable something critical to UFS functionality? Would UFS still work without any bandwidth scaling, although for example slower? Could you please clarify.
Quoted 2 lines
Yes - UFS will still work without any bandwidth scaling - but the performance would be terrible.
Ok, thanks for clarifying! Then the properties should be optional. Maybe we can also mention in the commit text how much the performance would improve with this patch.
Quoted 16 lines
Quoted 16 lines
Quoted 11 lines
+- Please refer to
Documentation/devicetree/bindings/interconnect/+ for more details on the above.Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +2
-0+qcom,msm-bus,name - string describing the bus path +qcom,msm-bus,num-cases - number of configurations in which ufs canoperate in
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +2
-0+qcom,msm-bus,num-paths - number of paths to vote for +qcom,msm-bus,vectors-KBps - Takes a tuple <ib ab>, <ib ab> (2 tuplesfor 2 num-paths)
+ The number of these entries *must* be same as + num-cases.DT bindings should be submitted as a separate patch. Anyway, people frown upon putting configuration data in DT. Could we put this data into the driver as a static table instead of DT? Also maybe use ab/pb for average/peak bandwidth.
Quoted 3 lines
The ab/ib value change depending on the target - that's the reasoning for putting it in dts file. However, I'm open to ideas as to how else to handle this.
As Evan already suggested, it would be best if we can calculate the bandwidth. Can we do that based on the number of lanes, clock rate and ufs standard version?
If calculating is really not possible and we have strong arguments for that, we could add a more specific compatible DT string - for example qcom,sdm845-ufshc and use per SoC bandwidth tables.
Thanks,
Georgi