diff mbox series

[v2,2/4] soc: qcom: smem: introduce qcom_smem_get_msm_id()

Message ID 20230524162329.819770-2-robimarko@gmail.com
State New
Headers
Series [v2,1/4] soc: qcom: socinfo: move SMEM item struct and defines to a header |

Commit Message

Robert Marko May 24, 2023, 4:23 p.m. UTC 
  Introduce a helper to return the SoC SMEM ID, which is used to identify the
exact SoC model as there may be differences in the same SoC family.

Currently, cpufreq-nvmem does this completely in the driver and there has
been more interest expresed for other drivers to use this information so
lets expose a common helper to prevent redoing it in individual drivers
since this field is present on every SMEM table version.

Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 drivers/soc/qcom/smem.c       | 19 +++++++++++++++++++
 include/linux/soc/qcom/smem.h |  2 ++
 2 files changed, 21 insertions(+)

Comments

Trilok Soni May 24, 2023, 6:16 p.m. UTC | #1 
On 5/24/2023 9:23 AM, Robert Marko wrote:
> Introduce a helper to return the SoC SMEM ID, which is used to identify the
> exact SoC model as there may be differences in the same SoC family.
> 
> Currently, cpufreq-nvmem does this completely in the driver and there has
> been more interest expresed for other drivers to use this information so
> lets expose a common helper to prevent redoing it in individual drivers
> since this field is present on every SMEM table version.
> 
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
>   drivers/soc/qcom/smem.c       | 19 +++++++++++++++++++
>   include/linux/soc/qcom/smem.h |  2 ++
>   2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 6be7ea93c78c..0d6ba9bce8cb 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -14,6 +14,7 @@
>   #include <linux/sizes.h>
>   #include <linux/slab.h>
>   #include <linux/soc/qcom/smem.h>
> +#include <linux/soc/qcom/socinfo.h>
>   
>   /*
>    * The Qualcomm shared memory system is a allocate only heap structure that
> @@ -772,6 +773,24 @@ phys_addr_t qcom_smem_virt_to_phys(void *p)
>   }
>   EXPORT_SYMBOL(qcom_smem_virt_to_phys);
>   
> +/**
> + * qcom_smem_get_msm_id() - return the SoC ID
> + *
> + * Look up SoC ID from HW/SW build ID and return it.
> + */
> +int qcom_smem_get_msm_id(void)
> +{
> +	size_t len;
> +	struct socinfo *info;
> +
> +	info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, &len);
> +	if (IS_ERR(info))
> +		return PTR_ERR(info);
> +
> +	return __le32_to_cpu(info->id);
> +}
> +EXPORT_SYMBOL(qcom_smem_get_msm_id);

EXPORT_SYMBOL_GPL please?

Please change it for other symbols in the driver as well w/ separate patch.

---Trilok Soni
Konrad Dybcio May 24, 2023, 6:27 p.m. UTC | #2 
On 24.05.2023 20:16, Trilok Soni wrote:
> On 5/24/2023 9:23 AM, Robert Marko wrote:
>> Introduce a helper to return the SoC SMEM ID, which is used to identify the
>> exact SoC model as there may be differences in the same SoC family.
>>
>> Currently, cpufreq-nvmem does this completely in the driver and there has
>> been more interest expresed for other drivers to use this information so
>> lets expose a common helper to prevent redoing it in individual drivers
>> since this field is present on every SMEM table version.
>>
>> Signed-off-by: Robert Marko <robimarko@gmail.com>
>> ---
>>   drivers/soc/qcom/smem.c       | 19 +++++++++++++++++++
>>   include/linux/soc/qcom/smem.h |  2 ++
>>   2 files changed, 21 insertions(+)
>>
>> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
>> index 6be7ea93c78c..0d6ba9bce8cb 100644
>> --- a/drivers/soc/qcom/smem.c
>> +++ b/drivers/soc/qcom/smem.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/sizes.h>
>>   #include <linux/slab.h>
>>   #include <linux/soc/qcom/smem.h>
>> +#include <linux/soc/qcom/socinfo.h>
>>     /*
>>    * The Qualcomm shared memory system is a allocate only heap structure that
>> @@ -772,6 +773,24 @@ phys_addr_t qcom_smem_virt_to_phys(void *p)
>>   }
>>   EXPORT_SYMBOL(qcom_smem_virt_to_phys);
>>   +/**
>> + * qcom_smem_get_msm_id() - return the SoC ID
>> + *
>> + * Look up SoC ID from HW/SW build ID and return it.
>> + */
>> +int qcom_smem_get_msm_id(void)
On top of Trilok's point, this should return le32, or at least unsigned int.

Konrad
>> +{
>> +    size_t len;
>> +    struct socinfo *info;
>> +
>> +    info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, &len);
>> +    if (IS_ERR(info))
>> +        return PTR_ERR(info);
>> +
>> +    return __le32_to_cpu(info->id);
>> +}
>> +EXPORT_SYMBOL(qcom_smem_get_msm_id);
> 
> EXPORT_SYMBOL_GPL please?
> 
> Please change it for other symbols in the driver as well w/ separate patch.
> 
> ---Trilok Soni
> 
>
Christian Marangi May 24, 2023, 7:57 p.m. UTC | #3 
On Wed, May 24, 2023 at 08:27:03PM +0200, Konrad Dybcio wrote:
> 
> 
> On 24.05.2023 20:16, Trilok Soni wrote:
> > On 5/24/2023 9:23 AM, Robert Marko wrote:
> >> Introduce a helper to return the SoC SMEM ID, which is used to identify the
> >> exact SoC model as there may be differences in the same SoC family.
> >>
> >> Currently, cpufreq-nvmem does this completely in the driver and there has
> >> been more interest expresed for other drivers to use this information so
> >> lets expose a common helper to prevent redoing it in individual drivers
> >> since this field is present on every SMEM table version.
> >>
> >> Signed-off-by: Robert Marko <robimarko@gmail.com>
> >> ---
> >>   drivers/soc/qcom/smem.c       | 19 +++++++++++++++++++
> >>   include/linux/soc/qcom/smem.h |  2 ++
> >>   2 files changed, 21 insertions(+)
> >>
> >> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> >> index 6be7ea93c78c..0d6ba9bce8cb 100644
> >> --- a/drivers/soc/qcom/smem.c
> >> +++ b/drivers/soc/qcom/smem.c
> >> @@ -14,6 +14,7 @@
> >>   #include <linux/sizes.h>
> >>   #include <linux/slab.h>
> >>   #include <linux/soc/qcom/smem.h>
> >> +#include <linux/soc/qcom/socinfo.h>
> >>     /*
> >>    * The Qualcomm shared memory system is a allocate only heap structure that
> >> @@ -772,6 +773,24 @@ phys_addr_t qcom_smem_virt_to_phys(void *p)
> >>   }
> >>   EXPORT_SYMBOL(qcom_smem_virt_to_phys);
> >>   +/**
> >> + * qcom_smem_get_msm_id() - return the SoC ID
> >> + *
> >> + * Look up SoC ID from HW/SW build ID and return it.
> >> + */
> >> +int qcom_smem_get_msm_id(void)
> On top of Trilok's point, this should return le32, or at least unsigned int.
> 

Mhhh why unsigned? We would lose error and qcom_smem_get can return all
sort of errors. Also I think le32 is problematic as we are converting
the value with __le32_to_cpu.

> >> +{
> >> +    size_t len;
> >> +    struct socinfo *info;
> >> +
> >> +    info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, &len);
> >> +    if (IS_ERR(info))
> >> +        return PTR_ERR(info);
> >> +
> >> +    return __le32_to_cpu(info->id);
> >> +}
> >> +EXPORT_SYMBOL(qcom_smem_get_msm_id);
> > 
> > EXPORT_SYMBOL_GPL please?
> > 
> > Please change it for other symbols in the driver as well w/ separate patch.
> > 
> > ---Trilok Soni
> > 
> >
Konrad Dybcio May 24, 2023, 8:53 p.m. UTC | #4 
On 24.05.2023 21:57, Christian Marangi wrote:
> On Wed, May 24, 2023 at 08:27:03PM +0200, Konrad Dybcio wrote:
>>
>>
>> On 24.05.2023 20:16, Trilok Soni wrote:
>>> On 5/24/2023 9:23 AM, Robert Marko wrote:
>>>> Introduce a helper to return the SoC SMEM ID, which is used to identify the
>>>> exact SoC model as there may be differences in the same SoC family.
>>>>
>>>> Currently, cpufreq-nvmem does this completely in the driver and there has
>>>> been more interest expresed for other drivers to use this information so
>>>> lets expose a common helper to prevent redoing it in individual drivers
>>>> since this field is present on every SMEM table version.
>>>>
>>>> Signed-off-by: Robert Marko <robimarko@gmail.com>
>>>> ---
>>>>   drivers/soc/qcom/smem.c       | 19 +++++++++++++++++++
>>>>   include/linux/soc/qcom/smem.h |  2 ++
>>>>   2 files changed, 21 insertions(+)
>>>>
>>>> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
>>>> index 6be7ea93c78c..0d6ba9bce8cb 100644
>>>> --- a/drivers/soc/qcom/smem.c
>>>> +++ b/drivers/soc/qcom/smem.c
>>>> @@ -14,6 +14,7 @@
>>>>   #include <linux/sizes.h>
>>>>   #include <linux/slab.h>
>>>>   #include <linux/soc/qcom/smem.h>
>>>> +#include <linux/soc/qcom/socinfo.h>
>>>>     /*
>>>>    * The Qualcomm shared memory system is a allocate only heap structure that
>>>> @@ -772,6 +773,24 @@ phys_addr_t qcom_smem_virt_to_phys(void *p)
>>>>   }
>>>>   EXPORT_SYMBOL(qcom_smem_virt_to_phys);
>>>>   +/**
>>>> + * qcom_smem_get_msm_id() - return the SoC ID
>>>> + *
>>>> + * Look up SoC ID from HW/SW build ID and return it.
>>>> + */
>>>> +int qcom_smem_get_msm_id(void)
>> On top of Trilok's point, this should return le32, or at least unsigned int.
>>
> 
> Mhhh why unsigned? We would lose error and qcom_smem_get can return all
> sort of errors. Also I think le32 is problematic as we are converting
> the value with __le32_to_cpu.
Hm right.. Qcom didn't really think this through then, but hopefully
they don't randomly jump from e.g. 547 to 1<<31

Konrad
> 
>>>> +{
>>>> +    size_t len;
>>>> +    struct socinfo *info;
>>>> +
>>>> +    info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, &len);
>>>> +    if (IS_ERR(info))
>>>> +        return PTR_ERR(info);
>>>> +
>>>> +    return __le32_to_cpu(info->id);
>>>> +}
>>>> +EXPORT_SYMBOL(qcom_smem_get_msm_id);
>>>
>>> EXPORT_SYMBOL_GPL please?
>>>
>>> Please change it for other symbols in the driver as well w/ separate patch.
>>>
>>> ---Trilok Soni
>>>
>>>
>
Bjorn Andersson May 25, 2023, 2:54 a.m. UTC | #5 
On Wed, May 24, 2023 at 08:27:03PM +0200, Konrad Dybcio wrote:
> 
> 
> On 24.05.2023 20:16, Trilok Soni wrote:
> > On 5/24/2023 9:23 AM, Robert Marko wrote:
> >> Introduce a helper to return the SoC SMEM ID, which is used to identify the
> >> exact SoC model as there may be differences in the same SoC family.
> >>
> >> Currently, cpufreq-nvmem does this completely in the driver and there has
> >> been more interest expresed for other drivers to use this information so
> >> lets expose a common helper to prevent redoing it in individual drivers
> >> since this field is present on every SMEM table version.
> >>
> >> Signed-off-by: Robert Marko <robimarko@gmail.com>
> >> ---
> >>   drivers/soc/qcom/smem.c       | 19 +++++++++++++++++++
> >>   include/linux/soc/qcom/smem.h |  2 ++
> >>   2 files changed, 21 insertions(+)
> >>
> >> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> >> index 6be7ea93c78c..0d6ba9bce8cb 100644
> >> --- a/drivers/soc/qcom/smem.c
> >> +++ b/drivers/soc/qcom/smem.c
> >> @@ -14,6 +14,7 @@
> >>   #include <linux/sizes.h>
> >>   #include <linux/slab.h>
> >>   #include <linux/soc/qcom/smem.h>
> >> +#include <linux/soc/qcom/socinfo.h>
> >>     /*
> >>    * The Qualcomm shared memory system is a allocate only heap structure that
> >> @@ -772,6 +773,24 @@ phys_addr_t qcom_smem_virt_to_phys(void *p)
> >>   }
> >>   EXPORT_SYMBOL(qcom_smem_virt_to_phys);
> >>   +/**
> >> + * qcom_smem_get_msm_id() - return the SoC ID
> >> + *
> >> + * Look up SoC ID from HW/SW build ID and return it.
> >> + */
> >> +int qcom_smem_get_msm_id(void)
> On top of Trilok's point, this should return le32, or at least unsigned int.
> 

Returning the value in CPU-native endian sounds very reasonable, so we
don't need __le32_to_cpu() on the calling side.

If we want to worry about this value going beyond 31 bits the
appropriate way would be to take, and assign, a unsigned int* argument
and return 0/-errno to indicate success/failure.

Regards,
Bjorn

> Konrad
> >> +{
> >> +    size_t len;
> >> +    struct socinfo *info;
> >> +
> >> +    info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, &len);
> >> +    if (IS_ERR(info))
> >> +        return PTR_ERR(info);
> >> +
> >> +    return __le32_to_cpu(info->id);
> >> +}
> >> +EXPORT_SYMBOL(qcom_smem_get_msm_id);
> > 
> > EXPORT_SYMBOL_GPL please?
> > 
> > Please change it for other symbols in the driver as well w/ separate patch.
> > 
> > ---Trilok Soni
> > 
> >
diff mbox series

Patch

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 6be7ea93c78c..0d6ba9bce8cb 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -14,6 +14,7 @@ 
 #include <linux/sizes.h>
 #include <linux/slab.h>
 #include <linux/soc/qcom/smem.h>
+#include <linux/soc/qcom/socinfo.h>
 
 /*
  * The Qualcomm shared memory system is a allocate only heap structure that
@@ -772,6 +773,24 @@  phys_addr_t qcom_smem_virt_to_phys(void *p)
 }
 EXPORT_SYMBOL(qcom_smem_virt_to_phys);
 
+/**
+ * qcom_smem_get_msm_id() - return the SoC ID
+ *
+ * Look up SoC ID from HW/SW build ID and return it.
+ */
+int qcom_smem_get_msm_id(void)
+{
+	size_t len;
+	struct socinfo *info;
+
+	info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, &len);
+	if (IS_ERR(info))
+		return PTR_ERR(info);
+
+	return __le32_to_cpu(info->id);
+}
+EXPORT_SYMBOL(qcom_smem_get_msm_id);
+
 static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
 {
 	struct smem_header *header;
diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
index 86e1b358688a..6448607239e6 100644
--- a/include/linux/soc/qcom/smem.h
+++ b/include/linux/soc/qcom/smem.h
@@ -11,4 +11,6 @@  int qcom_smem_get_free_space(unsigned host);
 
 phys_addr_t qcom_smem_virt_to_phys(void *p);
 
+int qcom_smem_get_msm_id(void);
+
 #endif