| Message ID | 20230901114336.31339-1-quic_nitirawa@quicinc.com |
|---|---|
| Headers | show |
| Series | scsi: ufs: qcom: Align programming sequence as per HW spec | expand |
On 9/1/2023 9:00 PM, Bjorn Andersson wrote: > On Fri, Sep 01, 2023 at 05:13:32PM +0530, Nitin Rawat wrote: >> Configure PA_VS_CORE_CLK_40NS_CYCLES with frequency of unipro core clk >> for Qualcomm UFS controller V4.0 and onwards to align with the hardware >> specification. >> >> Co-developed-by: Naveen Kumar Goud Arepalli <quic_narepall@quicinc.com> >> Signed-off-by: Naveen Kumar Goud Arepalli <quic_narepall@quicinc.com> >> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> >> --- >> drivers/ufs/host/ufs-qcom.c | 47 ++++++++++++++++++++++++++++--------- >> drivers/ufs/host/ufs-qcom.h | 2 ++ >> 2 files changed, 38 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >> index fe36003faaa8..018e391c276e 100644 >> --- a/drivers/ufs/host/ufs-qcom.c >> +++ b/drivers/ufs/host/ufs-qcom.c >> @@ -93,8 +93,9 @@ static const struct __ufs_qcom_bw_table { >> static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS]; >> >> static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host); >> -static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba, >> - u32 clk_cycles); >> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, >> + u32 clk_cycles, >> + u32 clk_40ns_cycles); >> >> static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd) >> { >> @@ -690,8 +691,8 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba, >> * set unipro core clock cycles to 150 & clear clock >> * divider >> */ >> - err = ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(hba, >> - 150); >> + err = ufs_qcom_set_core_clk_ctrl(hba, >> + 150, 6); > > There's no reason to maintain the line wrap here, and the new > indentation looks wrong. Sure..will remove it next patchset. > >> >> /* >> * Some UFS devices (and may be host) have issues if LCC is >> @@ -1295,12 +1296,12 @@ static void ufs_qcom_exit(struct ufs_hba *hba) >> phy_power_off(host->generic_phy); >> phy_exit(host->generic_phy); >> } >> - >> -static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba, >> - u32 cycles_in_1us) >> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, >> + u32 cycles_in_1us, > > Following my comment on this rename in the last commit, here you do > change the prototype, so here you need the rename to clarify the intent, > so make it here instead. We will change the order of patch so that we need change the prototype in 1 place. -Nitin > >> + u32 cycles_in_40ns) >> { >> struct ufs_qcom_host *host = ufshcd_get_variant(hba); >> - u32 core_clk_ctrl_reg; >> + u32 core_clk_ctrl_reg, reg; > > Please keep one variable declaration per line, in particular when using > names like "core_clk_ctrl_reg" which looks much more like a type than a > variable name... > > I would have preferred "reg" instead of core_clk_ctrl_reg" and you could > have used that same variable for both sections. > >> int ret; >> >> ret = ufshcd_dme_get(hba, >> @@ -1325,9 +1326,33 @@ static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba, >> /* Clear CORE_CLK_DIV_EN */ >> core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT; >> >> - return ufshcd_dme_set(hba, >> + err = ufshcd_dme_set(hba, > > I might be confused, but didn't you remove "err" in patch 1? Does this > patch compile? > >> UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL), >> core_clk_ctrl_reg); >> + /* >> + * UFS host controller V4.0.0 onwards needs to program >> + * PA_VS_CORE_CLK_40NS_CYCLES attribute per programmed >> + * frequency of unipro core clk of UFS host controller. >> + */ >> + if (!err && (host->hw_ver.major >= 4)) { > > Rather than if (previous-block-didn't-fail), add an explicit return if > err is unfavourable. That will make it more obvious that this section > relates to version >= 4 and nothing else. Sure..Will take care in next patchset > > > That said, you now have one function: > > core_clk_ctrl(int a, int b) > { > do stuff based on a; > > if (version > 4) > do stuff based on b; > } > > It's pretty much one function for cycles_in_1us, with a separate > function for cycles_in_40us bolted on at the end. How about just > splitting it in two functions instead, and provide a small wrapper that > calls one or both functions? Sure...Will move cycles_in_40us to different function. > >> + if (cycles_in_40ns > PA_VS_CORE_CLK_40NS_CYCLES_MASK) >> + return -EINVAL; >> + >> + err = ufshcd_dme_get(hba, >> + UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES), >> + ®); > > Leaving this line unwrapped seems to end up at 89 characters wide. > Perfectly reasonable to do, in the name of readability. > Sure..Will address this in next patchset. >> + if (err) >> + return err; >> + >> + reg &= ~PA_VS_CORE_CLK_40NS_CYCLES_MASK; >> + reg |= cycles_in_40ns; >> + >> + err = ufshcd_dme_set(hba, >> + UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES), >> + reg); > > Same as above. Sure..Will address this in next patchset. > > Regards, > Bjorn Thanks, Nitin
On 9/1/2023 9:10 PM, Bjorn Andersson wrote: > On Fri, Sep 01, 2023 at 05:13:33PM +0530, Nitin Rawat wrote: >> Add Support to configure CORE_CLK_1US_CYCLES, PA_VS_CORE_CLK_40NS_CYCLES >> for multiple unipro clock frequencies. Currently this is handled only for >> only 150Mhz and 75MHz. >> >> Since different qualcomm targets support different unipro frequency, add >> support to handle all other frequencies like 403MHz, 300MHz, 202MHz, >> 150 MHz, 75Mhz, 37.5 MHz. >> > > Please flip your commit message around, start with the problem > description (the second paragraph), then the description of what you're > changing. Sure will address this in next patchset. > >> Co-developed-by: Naveen Kumar Goud Arepalli <quic_narepall@quicinc.com> >> Signed-off-by: Naveen Kumar Goud Arepalli <quic_narepall@quicinc.com> >> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> >> --- >> drivers/ufs/host/ufs-qcom.c | 96 ++++++++++++++++++++++++++++--------- >> drivers/ufs/host/ufs-qcom.h | 10 ++++ >> 2 files changed, 84 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >> index 018e391c276e..e3648e936498 100644 >> --- a/drivers/ufs/host/ufs-qcom.c >> +++ b/drivers/ufs/host/ufs-qcom.c >> @@ -94,8 +94,7 @@ static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS]; >> >> static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host); >> static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, >> - u32 clk_cycles, >> - u32 clk_40ns_cycles); >> + bool is_max_freq); > > Odd indentation here. That said, I doubt that you need the line wrap > now. Sure will address this in next patchset. > >> >> static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd) >> { >> @@ -686,13 +685,14 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba, >> return -EINVAL; >> } >> >> - if (ufs_qcom_cap_qunipro(host)) >> - /* >> - * set unipro core clock cycles to 150 & clear clock >> - * divider >> - */ >> - err = ufs_qcom_set_core_clk_ctrl(hba, >> - 150, 6); >> + if (ufs_qcom_cap_qunipro(host)) { >> + err = ufs_qcom_set_core_clk_ctrl(hba, true); >> + if (err) { >> + dev_err(hba->dev, >> + "%s cfg core clk ctrl failed\n", >> + __func__); > > Please don't build error messages using __func__, describe the actual > error, in English. Sure will address this in next patchset. > >> + } >> + } >> >> /* >> * Some UFS devices (and may be host) have issues if LCC is >> @@ -1297,13 +1297,67 @@ static void ufs_qcom_exit(struct ufs_hba *hba) >> phy_exit(host->generic_phy); >> } >> static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, >> - u32 cycles_in_1us, >> - u32 cycles_in_40ns) >> + bool is_max_freq) > > You changed this function prototype in both patch 1 and patch 2, and now > you're changing it again. What if you make this transition in patch 1, > then you only need to change the prototype once. Sure we will change the order of patch so that prototype will change in 1 place. > >> { >> struct ufs_qcom_host *host = ufshcd_get_variant(hba); >> + struct list_head *head = &hba->clk_list_head; >> u32 core_clk_ctrl_reg, reg; >> + struct ufs_clk_info *clki; >> + u32 cycles_in_1us, cycles_in_40ns; >> int ret; >> >> + list_for_each_entry(clki, head, list) { >> + if (!IS_ERR_OR_NULL(clki->clk) && >> + !strcmp(clki->name, "core_clk_unipro")) { >> + if (is_max_freq) >> + cycles_in_1us = clki->max_freq; >> + else >> + cycles_in_1us = clk_get_rate(clki->clk); >> + break; >> + } >> + } >> + >> + if ((cycles_in_1us % (1000 * 1000)) != 0) >> + cycles_in_1us = cycles_in_1us/(1000 * 1000) + 1; >> + else >> + cycles_in_1us = cycles_in_1us/(1000 * 1000); >> + > > Iiuc, the following 34 lines only apply to hw_ver >= 4. Sure...Will address this in next patchset. > >> + /* >> + * Generic formulae for cycles_in_40ns = (freq_unipro/25) is not >> + * applicable for all frequencies. For ex: ceil(37.5 MHz/25) will >> + * be 2 and ceil(403 MHZ/25) will be 17 whereas Hardware >> + * specification expect to be 16. Hence use exact hardware spec >> + * mandated value for cycles_in_40ns instead of calculating using >> + * generic formulae. >> + */ >> + switch (cycles_in_1us) { >> + case UNIPRO_CORE_CLK_FREQ_403_MHZ: >> + cycles_in_40ns = 16; >> + break; >> + case UNIPRO_CORE_CLK_FREQ_300_MHZ: >> + cycles_in_40ns = 12; >> + break; >> + case UNIPRO_CORE_CLK_FREQ_201_5_MHZ: >> + cycles_in_40ns = 8; >> + break; >> + case UNIPRO_CORE_CLK_FREQ_150_MHZ: >> + cycles_in_40ns = 6; >> + break; >> + case UNIPRO_CORE_CLK_FREQ_100_MHZ: >> + cycles_in_40ns = 4; >> + break; >> + case UNIPRO_CORE_CLK_FREQ_75_MHZ: >> + cycles_in_40ns = 3; >> + break; >> + case UNIPRO_CORE_CLK_FREQ_37_5_MHZ: >> + cycles_in_40ns = 2; >> + break; >> + default: >> + ret = -EINVAL; > - In next patchset we are returning Error from default case. > 5 lines below, you're overwriting this value and moving on as if nothing > happened. Doesn't your compiler complain that you then will use > cycles_in_40ns without first initializing it in this case? > >> + dev_err(hba->dev, "UNIPRO clk freq %u MHz not supported\n", >> + cycles_in_1us); >> + } >> + >> ret = ufshcd_dme_get(hba, >> UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL), >> &core_clk_ctrl_reg); >> @@ -1326,7 +1380,7 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, >> /* Clear CORE_CLK_DIV_EN */ >> core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT; > > The following 6 changes just corrects the compilation error you > shouldn't have introduced in the previous patch. > >> >> - err = ufshcd_dme_set(hba, >> + ret = ufshcd_dme_set(hba, >> UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL), >> core_clk_ctrl_reg); >> /* >> @@ -1334,25 +1388,25 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, >> * PA_VS_CORE_CLK_40NS_CYCLES attribute per programmed >> * frequency of unipro core clk of UFS host controller. >> */ >> - if (!err && (host->hw_ver.major >= 4)) { >> + if (!ret && (host->hw_ver.major >= 4)) { >> if (cycles_in_40ns > PA_VS_CORE_CLK_40NS_CYCLES_MASK) >> return -EINVAL; >> >> - err = ufshcd_dme_get(hba, >> + ret = ufshcd_dme_get(hba, >> UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES), >> ®); >> - if (err) >> - return err; >> + if (ret) >> + return ret; >> >> reg &= ~PA_VS_CORE_CLK_40NS_CYCLES_MASK; >> reg |= cycles_in_40ns; >> >> - err = ufshcd_dme_set(hba, >> + ret = ufshcd_dme_set(hba, >> UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES), >> reg); >> } >> >> - return err; >> + return ret; >> } >> >> static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba) >> @@ -1368,8 +1422,7 @@ static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba) >> if (!ufs_qcom_cap_qunipro(host)) >> return 0; >> >> - /* set unipro core clock cycles to 150 and clear clock divider */ >> - return ufs_qcom_set_core_clk_ctrl(hba, 150, 6); >> + return ufs_qcom_set_core_clk_ctrl(hba, true); >> } >> >> static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba) >> @@ -1404,8 +1457,7 @@ static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba) >> if (!ufs_qcom_cap_qunipro(host)) >> return 0; >> >> - /* set unipro core clock cycles to 75 and clear clock divider */ >> - return ufs_qcom_set_core_clk_ctrl(hba, 75, 3); >> + return ufs_qcom_set_core_clk_ctrl(hba, false); >> } >> >> static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, >> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h >> index d81bf1a1b77a..bc176ef58e3e 100644 >> --- a/drivers/ufs/host/ufs-qcom.h >> +++ b/drivers/ufs/host/ufs-qcom.h >> @@ -9,6 +9,7 @@ >> #include <linux/reset.h> >> #include <soc/qcom/ice.h> >> #include <ufs/ufshcd.h> >> +#include <linux/math.h> > > Why is math.h needed in this header file now? Removed it. > > Regards, > Bjorn > >> >> #define MAX_UFS_QCOM_HOSTS 1 >> #define MAX_U32 (~(u32)0) >> @@ -79,6 +80,15 @@ enum { >> UFS_MEM_CQIS_VS = 0x8, >> }; >> >> +/* QCOM UFS host controller core clk frequencies */ >> +#define UNIPRO_CORE_CLK_FREQ_37_5_MHZ 38 >> +#define UNIPRO_CORE_CLK_FREQ_75_MHZ 75 >> +#define UNIPRO_CORE_CLK_FREQ_100_MHZ 100 >> +#define UNIPRO_CORE_CLK_FREQ_150_MHZ 150 >> +#define UNIPRO_CORE_CLK_FREQ_300_MHZ 300 >> +#define UNIPRO_CORE_CLK_FREQ_201_5_MHZ 202 >> +#define UNIPRO_CORE_CLK_FREQ_403_MHZ 403 >> + >> #define UFS_CNTLR_2_x_x_VEN_REGS_OFFSET(x) (0x000 + x) >> #define UFS_CNTLR_3_x_x_VEN_REGS_OFFSET(x) (0x400 + x) >> >> -- >> 2.17.1 >>
On 9/1/2023 9:13 PM, Bjorn Andersson wrote: > On Fri, Sep 01, 2023 at 05:13:34PM +0530, Nitin Rawat wrote: >> Currently CORE_CLK_1US_CYCLES, PA_VS_CORE_CLK_40NS_CYCLES are configured >> in clk scaling post change ops. >> >> Move this to clk scaling pre change ops to align completely with hardware >> specification. This doesn't bring any functionality change. >> > > How can applying the clock scaling configuration, and "aligning with > hardware specification" not "bring any functionality change"? > > If the code is called in a way where there is no difference between pre > and post callbacks, then state that - but it begs the question, why do > we have this "flexible" (complex) callback scheme if it doesn't matter. > > Regards, > Bjorn Hi Bjorn, Here my intention is to align the sequence completely with HPG. Functionality w.r.t to clock scaling is not impacted here. I'll update the commit text to capture more details in next patchset. Thanks, Nitin
This patch aligns programming sequence as per Qualcomm UFS hardware specification. changes from v5: - Addressed Mani comment to FIELD_PREP and FIELD_FIT. - Optimised ufs_qcom_set_core_clk_ctrl API. - Updated commit text for few patches to capture more details. Changes from v4: - Addressed bjorn comment to split single patch to multiple patches. Changes from v3: -Addressed bjorn comment to update commit msg to capture change details. Changes from v2: - Addressed bao comment, removed duplicate clock timer cfg API call Changes from v1: - Addressed bao comment, removed wrapper function - Tab alignment Nitin Rawat (6): scsi: ufs: qcom: Align mask for core_clk_1us_cycles scsi: ufs: qcom: Configure PA_VS_CORE_CLK_40NS_CYCLES scsi: ufs: qcom: Add multiple frequency support for unipro clk attributes scsi: ufs: qcom: Align unipro clk attributes configuration as per HPG scsi: ufs: qcom: Refactor ufs_qcom_cfg_timers function. scsi: ufs: qcom: Configure clk HW division based on scaling conditions. drivers/ufs/host/ufs-qcom.c | 240 ++++++++++++++++++++++++++---------- drivers/ufs/host/ufs-qcom.h | 17 ++- 2 files changed, 193 insertions(+), 64 deletions(-) -- 2.17.1