| Message ID | 20230525210214.78235-5-robimarko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/5] soc: qcom: socinfo: move SMEM item struct and defines to a header | expand |
On 25.05.2023 23:02, Robert Marko wrote: > Now that SMEM exports a helper to get the SMEM SoC ID lets utilize it. > Currently qcom_cpufreq_get_msm_id() is encoding the returned SMEM SoC ID > into an enum, however there is no reason to do so and we can just match > directly on the SMEM SoC ID as returned by qcom_smem_get_soc_id(). > > Signed-off-by: Robert Marko <robimarko@gmail.com> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > Changes in v4: > * Adapt to name change to qcom_smem_get_soc_id() > > Changes in v3: > * Adapt to helper using argument now > > Changes in v2: > * Utilize helper exported by SMEM instead of refactoring > qcom_cpufreq_get_msm_id() > --- > drivers/cpufreq/qcom-cpufreq-nvmem.c | 56 +++++----------------------- > 1 file changed, 10 insertions(+), 46 deletions(-) > > diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c > index 60e99be2d3db..a88b6fe5db50 100644 > --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c > +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c > @@ -29,16 +29,8 @@ > #include <linux/slab.h> > #include <linux/soc/qcom/smem.h> > > -#define MSM_ID_SMEM 137 > - > #include <dt-bindings/arm/qcom,ids.h> > > -enum _msm8996_version { > - MSM8996_V3, > - MSM8996_SG, > - NUM_OF_MSM8996_VERSIONS, > -}; > - > struct qcom_cpufreq_drv; > > struct qcom_cpufreq_match_data { > @@ -135,60 +127,32 @@ static void get_krait_bin_format_b(struct device *cpu_dev, > dev_dbg(cpu_dev, "PVS version: %d\n", *pvs_ver); > } > > -static enum _msm8996_version qcom_cpufreq_get_msm_id(void) > -{ > - size_t len; > - u32 *msm_id; > - enum _msm8996_version version; > - > - msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY, MSM_ID_SMEM, &len); > - if (IS_ERR(msm_id)) > - return NUM_OF_MSM8996_VERSIONS; > - > - /* The first 4 bytes are format, next to them is the actual msm-id */ > - msm_id++; > - > - switch ((enum _msm_id)*msm_id) { > - case QCOM_ID_MSM8996: > - case QCOM_ID_APQ8096: > - version = MSM8996_V3; > - break; > - case QCOM_ID_MSM8996SG: > - case QCOM_ID_APQ8096SG: > - version = MSM8996_SG; > - break; > - default: > - version = NUM_OF_MSM8996_VERSIONS; > - } > - > - return version; > -} > - > static int qcom_cpufreq_kryo_name_version(struct device *cpu_dev, > struct nvmem_cell *speedbin_nvmem, > char **pvs_name, > struct qcom_cpufreq_drv *drv) > { > size_t len; > + u32 msm_id; __le32 > u8 *speedbin; > - enum _msm8996_version msm8996_version; > + int ret; > *pvs_name = NULL; > > - msm8996_version = qcom_cpufreq_get_msm_id(); > - if (NUM_OF_MSM8996_VERSIONS == msm8996_version) { > - dev_err(cpu_dev, "Not Snapdragon 820/821!"); > - return -ENODEV; > - } > + ret = qcom_smem_get_soc_id(&msm_id); > + if (ret) > + return ret; Now since it can return a PTR_ERR, you should check for IS_ERR and return ERR_PTR if that happens LGTM otherwise! Konrad > > speedbin = nvmem_cell_read(speedbin_nvmem, &len); > if (IS_ERR(speedbin)) > return PTR_ERR(speedbin); > > - switch (msm8996_version) { > - case MSM8996_V3: > + switch (msm_id) { > + case QCOM_ID_MSM8996: > + case QCOM_ID_APQ8096: > drv->versions = 1 << (unsigned int)(*speedbin); > break; > - case MSM8996_SG: > + case QCOM_ID_MSM8996SG: > + case QCOM_ID_APQ8096SG: > drv->versions = 1 << ((unsigned int)(*speedbin) + 4); > break; > default:
On Fri, May 26, 2023 at 01:18:02AM +0200, Konrad Dybcio wrote: > > > On 25.05.2023 23:02, Robert Marko wrote: > > Now that SMEM exports a helper to get the SMEM SoC ID lets utilize it. > > Currently qcom_cpufreq_get_msm_id() is encoding the returned SMEM SoC ID > > into an enum, however there is no reason to do so and we can just match > > directly on the SMEM SoC ID as returned by qcom_smem_get_soc_id(). > > > > Signed-off-by: Robert Marko <robimarko@gmail.com> > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > Changes in v4: > > * Adapt to name change to qcom_smem_get_soc_id() > > > > Changes in v3: > > * Adapt to helper using argument now > > > > Changes in v2: > > * Utilize helper exported by SMEM instead of refactoring > > qcom_cpufreq_get_msm_id() > > --- > > drivers/cpufreq/qcom-cpufreq-nvmem.c | 56 +++++----------------------- > > 1 file changed, 10 insertions(+), 46 deletions(-) > > > > diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c > > index 60e99be2d3db..a88b6fe5db50 100644 > > --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c > > +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c > > @@ -29,16 +29,8 @@ > > #include <linux/slab.h> > > #include <linux/soc/qcom/smem.h> > > > > -#define MSM_ID_SMEM 137 > > - > > #include <dt-bindings/arm/qcom,ids.h> > > > > -enum _msm8996_version { > > - MSM8996_V3, > > - MSM8996_SG, > > - NUM_OF_MSM8996_VERSIONS, > > -}; > > - > > struct qcom_cpufreq_drv; > > > > struct qcom_cpufreq_match_data { > > @@ -135,60 +127,32 @@ static void get_krait_bin_format_b(struct device *cpu_dev, > > dev_dbg(cpu_dev, "PVS version: %d\n", *pvs_ver); > > } > > > > -static enum _msm8996_version qcom_cpufreq_get_msm_id(void) > > -{ > > - size_t len; > > - u32 *msm_id; > > - enum _msm8996_version version; > > - > > - msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY, MSM_ID_SMEM, &len); > > - if (IS_ERR(msm_id)) > > - return NUM_OF_MSM8996_VERSIONS; > > - > > - /* The first 4 bytes are format, next to them is the actual msm-id */ > > - msm_id++; > > - > > - switch ((enum _msm_id)*msm_id) { > > - case QCOM_ID_MSM8996: > > - case QCOM_ID_APQ8096: > > - version = MSM8996_V3; > > - break; > > - case QCOM_ID_MSM8996SG: > > - case QCOM_ID_APQ8096SG: > > - version = MSM8996_SG; > > - break; > > - default: > > - version = NUM_OF_MSM8996_VERSIONS; > > - } > > - > > - return version; > > -} > > - > > static int qcom_cpufreq_kryo_name_version(struct device *cpu_dev, > > struct nvmem_cell *speedbin_nvmem, > > char **pvs_name, > > struct qcom_cpufreq_drv *drv) > > { > > size_t len; > > + u32 msm_id; > __le32 > > > u8 *speedbin; > > - enum _msm8996_version msm8996_version; > > + int ret; > > *pvs_name = NULL; > > > > - msm8996_version = qcom_cpufreq_get_msm_id(); > > - if (NUM_OF_MSM8996_VERSIONS == msm8996_version) { > > - dev_err(cpu_dev, "Not Snapdragon 820/821!"); > > - return -ENODEV; > > - } > > + ret = qcom_smem_get_soc_id(&msm_id); > > + if (ret) > > + return ret; > Now since it can return a PTR_ERR, you should check for IS_ERR > and return ERR_PTR if that happens No, the PTR_ERR() extracted the error value out of the pointer, so it's just an integer now (or zero on success). So this is looking correct to me. > > LGTM otherwise! > > Konrad > > > > speedbin = nvmem_cell_read(speedbin_nvmem, &len); > > if (IS_ERR(speedbin)) > > return PTR_ERR(speedbin); > > > > - switch (msm8996_version) { > > - case MSM8996_V3: > > + switch (msm_id) { > > + case QCOM_ID_MSM8996: And here are those cpu-endian constants... If msm_id is a __le32 then all these constants needs to be cpu_to_le32(). Regards, Bjorn > > + case QCOM_ID_APQ8096: > > drv->versions = 1 << (unsigned int)(*speedbin); > > break; > > - case MSM8996_SG: > > + case QCOM_ID_MSM8996SG: > > + case QCOM_ID_APQ8096SG: > > drv->versions = 1 << ((unsigned int)(*speedbin) + 4); > > break; > > default:
On 26.05.2023 04:43, Bjorn Andersson wrote: > On Fri, May 26, 2023 at 01:18:02AM +0200, Konrad Dybcio wrote: >> >> >> On 25.05.2023 23:02, Robert Marko wrote: >>> Now that SMEM exports a helper to get the SMEM SoC ID lets utilize it. >>> Currently qcom_cpufreq_get_msm_id() is encoding the returned SMEM SoC ID >>> into an enum, however there is no reason to do so and we can just match >>> directly on the SMEM SoC ID as returned by qcom_smem_get_soc_id(). >>> >>> Signed-off-by: Robert Marko <robimarko@gmail.com> >>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org> >>> --- >>> Changes in v4: >>> * Adapt to name change to qcom_smem_get_soc_id() >>> >>> Changes in v3: >>> * Adapt to helper using argument now >>> >>> Changes in v2: >>> * Utilize helper exported by SMEM instead of refactoring >>> qcom_cpufreq_get_msm_id() >>> --- >>> drivers/cpufreq/qcom-cpufreq-nvmem.c | 56 +++++----------------------- >>> 1 file changed, 10 insertions(+), 46 deletions(-) >>> >>> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c >>> index 60e99be2d3db..a88b6fe5db50 100644 >>> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c >>> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c >>> @@ -29,16 +29,8 @@ >>> #include <linux/slab.h> >>> #include <linux/soc/qcom/smem.h> >>> >>> -#define MSM_ID_SMEM 137 >>> - >>> #include <dt-bindings/arm/qcom,ids.h> >>> >>> -enum _msm8996_version { >>> - MSM8996_V3, >>> - MSM8996_SG, >>> - NUM_OF_MSM8996_VERSIONS, >>> -}; >>> - >>> struct qcom_cpufreq_drv; >>> >>> struct qcom_cpufreq_match_data { >>> @@ -135,60 +127,32 @@ static void get_krait_bin_format_b(struct device *cpu_dev, >>> dev_dbg(cpu_dev, "PVS version: %d\n", *pvs_ver); >>> } >>> >>> -static enum _msm8996_version qcom_cpufreq_get_msm_id(void) >>> -{ >>> - size_t len; >>> - u32 *msm_id; >>> - enum _msm8996_version version; >>> - >>> - msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY, MSM_ID_SMEM, &len); >>> - if (IS_ERR(msm_id)) >>> - return NUM_OF_MSM8996_VERSIONS; >>> - >>> - /* The first 4 bytes are format, next to them is the actual msm-id */ >>> - msm_id++; >>> - >>> - switch ((enum _msm_id)*msm_id) { >>> - case QCOM_ID_MSM8996: >>> - case QCOM_ID_APQ8096: >>> - version = MSM8996_V3; >>> - break; >>> - case QCOM_ID_MSM8996SG: >>> - case QCOM_ID_APQ8096SG: >>> - version = MSM8996_SG; >>> - break; >>> - default: >>> - version = NUM_OF_MSM8996_VERSIONS; >>> - } >>> - >>> - return version; >>> -} >>> - >>> static int qcom_cpufreq_kryo_name_version(struct device *cpu_dev, >>> struct nvmem_cell *speedbin_nvmem, >>> char **pvs_name, >>> struct qcom_cpufreq_drv *drv) >>> { >>> size_t len; >>> + u32 msm_id; >> __le32 >> >>> u8 *speedbin; >>> - enum _msm8996_version msm8996_version; >>> + int ret; >>> *pvs_name = NULL; >>> >>> - msm8996_version = qcom_cpufreq_get_msm_id(); >>> - if (NUM_OF_MSM8996_VERSIONS == msm8996_version) { >>> - dev_err(cpu_dev, "Not Snapdragon 820/821!"); >>> - return -ENODEV; >>> - } >>> + ret = qcom_smem_get_soc_id(&msm_id); >>> + if (ret) >>> + return ret; >> Now since it can return a PTR_ERR, you should check for IS_ERR >> and return ERR_PTR if that happens > > No, the PTR_ERR() extracted the error value out of the pointer, so it's > just an integer now (or zero on success). So this is looking correct to > me. Riiight the function that returns a pointer to an error is *within* the one we're calling.. So much so for me reviewing late at night.. Konrad > >> >> LGTM otherwise! >> >> Konrad >>> >>> speedbin = nvmem_cell_read(speedbin_nvmem, &len); >>> if (IS_ERR(speedbin)) >>> return PTR_ERR(speedbin); >>> >>> - switch (msm8996_version) { >>> - case MSM8996_V3: >>> + switch (msm_id) { >>> + case QCOM_ID_MSM8996: > > And here are those cpu-endian constants... If msm_id is a __le32 then > all these constants needs to be cpu_to_le32(). > > Regards, > Bjorn > >>> + case QCOM_ID_APQ8096: >>> drv->versions = 1 << (unsigned int)(*speedbin); >>> break; >>> - case MSM8996_SG: >>> + case QCOM_ID_MSM8996SG: >>> + case QCOM_ID_APQ8096SG: >>> drv->versions = 1 << ((unsigned int)(*speedbin) + 4); >>> break; >>> default:
diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c index 60e99be2d3db..a88b6fe5db50 100644 --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c @@ -29,16 +29,8 @@ #include <linux/slab.h> #include <linux/soc/qcom/smem.h> -#define MSM_ID_SMEM 137 - #include <dt-bindings/arm/qcom,ids.h> -enum _msm8996_version { - MSM8996_V3, - MSM8996_SG, - NUM_OF_MSM8996_VERSIONS, -}; - struct qcom_cpufreq_drv; struct qcom_cpufreq_match_data { @@ -135,60 +127,32 @@ static void get_krait_bin_format_b(struct device *cpu_dev, dev_dbg(cpu_dev, "PVS version: %d\n", *pvs_ver); } -static enum _msm8996_version qcom_cpufreq_get_msm_id(void) -{ - size_t len; - u32 *msm_id; - enum _msm8996_version version; - - msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY, MSM_ID_SMEM, &len); - if (IS_ERR(msm_id)) - return NUM_OF_MSM8996_VERSIONS; - - /* The first 4 bytes are format, next to them is the actual msm-id */ - msm_id++; - - switch ((enum _msm_id)*msm_id) { - case QCOM_ID_MSM8996: - case QCOM_ID_APQ8096: - version = MSM8996_V3; - break; - case QCOM_ID_MSM8996SG: - case QCOM_ID_APQ8096SG: - version = MSM8996_SG; - break; - default: - version = NUM_OF_MSM8996_VERSIONS; - } - - return version; -} - static int qcom_cpufreq_kryo_name_version(struct device *cpu_dev, struct nvmem_cell *speedbin_nvmem, char **pvs_name, struct qcom_cpufreq_drv *drv) { size_t len; + u32 msm_id; u8 *speedbin; - enum _msm8996_version msm8996_version; + int ret; *pvs_name = NULL; - msm8996_version = qcom_cpufreq_get_msm_id(); - if (NUM_OF_MSM8996_VERSIONS == msm8996_version) { - dev_err(cpu_dev, "Not Snapdragon 820/821!"); - return -ENODEV; - } + ret = qcom_smem_get_soc_id(&msm_id); + if (ret) + return ret; speedbin = nvmem_cell_read(speedbin_nvmem, &len); if (IS_ERR(speedbin)) return PTR_ERR(speedbin); - switch (msm8996_version) { - case MSM8996_V3: + switch (msm_id) { + case QCOM_ID_MSM8996: + case QCOM_ID_APQ8096: drv->versions = 1 << (unsigned int)(*speedbin); break; - case MSM8996_SG: + case QCOM_ID_MSM8996SG: + case QCOM_ID_APQ8096SG: drv->versions = 1 << ((unsigned int)(*speedbin) + 4); break; default: