LinuxLists.cc - [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them

2024-04-05 08:41:56

Subject: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them

In preparation for parsing the chip "feature code" (FC) and "product
code" (PC) (essentially the parameters that let us conclusively
characterize the sillicon we're running on, including various speed
bins), move the socinfo version defines to the public header and
include some more FC/PC defines.

Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/soc/qcom/socinfo.c | 8 --------
include/linux/soc/qcom/socinfo.h | 36 ++++++++++++++++++++++++++++++++++++
2 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
index 277c07a6603d..cf4616a468f2 100644
--- a/drivers/soc/qcom/socinfo.c
+++ b/drivers/soc/qcom/socinfo.c
@@ -21,14 +21,6 @@

#include <dt-bindings/arm/qcom,ids.h>

-/*
- * SoC version type with major number in the upper 16 bits and minor
- * number in the lower 16 bits.
- */
-#define SOCINFO_MAJOR(ver) (((ver) >> 16) & 0xffff)
-#define SOCINFO_MINOR(ver) ((ver) & 0xffff)
-#define SOCINFO_VERSION(maj, min) ((((maj) & 0xffff) << 16)|((min) & 0xffff))
-
/* Helper macros to create soc_id table */
#define qcom_board_id(id) QCOM_ID_ ## id, __stringify(id)
#define qcom_board_id_named(id, name) QCOM_ID_ ## id, (name)
diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h
index e78777bb0f4a..ba7f683bd32c 100644
--- a/include/linux/soc/qcom/socinfo.h
+++ b/include/linux/soc/qcom/socinfo.h
@@ -3,6 +3,8 @@
#ifndef __QCOM_SOCINFO_H__
#define __QCOM_SOCINFO_H__

+#include <linux/types.h>
+
/*
* SMEM item id, used to acquire handles to respective
* SMEM region.
@@ -12,6 +14,14 @@
#define SMEM_SOCINFO_BUILD_ID_LENGTH 32
#define SMEM_SOCINFO_CHIP_ID_LENGTH 32

+/*
+ * SoC version type with major number in the upper 16 bits and minor
+ * number in the lower 16 bits.
+ */
+#define SOCINFO_MAJOR(ver) (((ver) >> 16) & 0xffff)
+#define SOCINFO_MINOR(ver) ((ver) & 0xffff)
+#define SOCINFO_VERSION(maj, min) ((((maj) & 0xffff) << 16)|((min) & 0xffff))
+
/* Socinfo SMEM item structure */
struct socinfo {
__le32 fmt;
@@ -74,4 +84,30 @@ struct socinfo {
__le32 boot_core;
};

+/* Internal feature codes */
+enum feature_code {
+ /* External feature codes */
+ SOCINFO_FC_UNKNOWN = 0x0,
+ SOCINFO_FC_AA,
+ SOCINFO_FC_AB,
+ SOCINFO_FC_AC,
+ SOCINFO_FC_AD,
+ SOCINFO_FC_AE,
+ SOCINFO_FC_AF,
+ SOCINFO_FC_AG,
+ SOCINFO_FC_AH,
+ SOCINFO_FC_EXT_RESERVE,
+};
+
+/* Internal feature codes */
+/* Valid values: 0 <= n <= 0xf */
+#define SOCINFO_FC_Yn(n) (0xf1 + n)
+#define SOCINFO_FC_INT_RESERVE SOCINFO_FC_Yn(0x10)
+
+/* Product codes */
+#define SOCINFO_PC_UNKNOWN 0
+/* Valid values: 0 <= n <= 8, the rest is reserved */
+#define SOCINFO_PCn(n) (n + 1)
+#define SOCINFO_PC_RESERVE (BIT(31) - 1)
+
#endif

--
2.40.1

2024-04-06 02:23:24

by Dmitry Baryshkov

[permalink] [raw]

Subject: Re: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them

On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote:
> In preparation for parsing the chip "feature code" (FC) and "product
> code" (PC) (essentially the parameters that let us conclusively
> characterize the sillicon we're running on, including various speed
> bins), move the socinfo version defines to the public header and
> include some more FC/PC defines.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/soc/qcom/socinfo.c | 8 --------
> include/linux/soc/qcom/socinfo.h | 36 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 36 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> index 277c07a6603d..cf4616a468f2 100644
> --- a/drivers/soc/qcom/socinfo.c
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -21,14 +21,6 @@
>
> #include <dt-bindings/arm/qcom,ids.h>
>
> -/*
> - * SoC version type with major number in the upper 16 bits and minor
> - * number in the lower 16 bits.
> - */
> -#define SOCINFO_MAJOR(ver) (((ver) >> 16) & 0xffff)
> -#define SOCINFO_MINOR(ver) ((ver) & 0xffff)
> -#define SOCINFO_VERSION(maj, min) ((((maj) & 0xffff) << 16)|((min) & 0xffff))
> -
> /* Helper macros to create soc_id table */
> #define qcom_board_id(id) QCOM_ID_ ## id, __stringify(id)
> #define qcom_board_id_named(id, name) QCOM_ID_ ## id, (name)
> diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h
> index e78777bb0f4a..ba7f683bd32c 100644
> --- a/include/linux/soc/qcom/socinfo.h
> +++ b/include/linux/soc/qcom/socinfo.h
> @@ -3,6 +3,8 @@
> #ifndef __QCOM_SOCINFO_H__
> #define __QCOM_SOCINFO_H__
>
> +#include <linux/types.h>
> +
> /*
> * SMEM item id, used to acquire handles to respective
> * SMEM region.
> @@ -12,6 +14,14 @@
> #define SMEM_SOCINFO_BUILD_ID_LENGTH 32
> #define SMEM_SOCINFO_CHIP_ID_LENGTH 32
>
> +/*
> + * SoC version type with major number in the upper 16 bits and minor
> + * number in the lower 16 bits.
> + */
> +#define SOCINFO_MAJOR(ver) (((ver) >> 16) & 0xffff)
> +#define SOCINFO_MINOR(ver) ((ver) & 0xffff)
> +#define SOCINFO_VERSION(maj, min) ((((maj) & 0xffff) << 16)|((min) & 0xffff))
> +
> /* Socinfo SMEM item structure */
> struct socinfo {
> __le32 fmt;
> @@ -74,4 +84,30 @@ struct socinfo {
> __le32 boot_core;
> };
>
> +/* Internal feature codes */
> +enum feature_code {
> + /* External feature codes */
> + SOCINFO_FC_UNKNOWN = 0x0,
> + SOCINFO_FC_AA,
> + SOCINFO_FC_AB,
> + SOCINFO_FC_AC,
> + SOCINFO_FC_AD,
> + SOCINFO_FC_AE,
> + SOCINFO_FC_AF,
> + SOCINFO_FC_AG,
> + SOCINFO_FC_AH,
> + SOCINFO_FC_EXT_RESERVE,
> +};
> +
> +/* Internal feature codes */
> +/* Valid values: 0 <= n <= 0xf */
> +#define SOCINFO_FC_Yn(n) (0xf1 + n)
> +#define SOCINFO_FC_INT_RESERVE SOCINFO_FC_Yn(0x10)
> +
> +/* Product codes */
> +#define SOCINFO_PC_UNKNOWN 0
> +/* Valid values: 0 <= n <= 8, the rest is reserved */
> +#define SOCINFO_PCn(n) (n + 1)
> +#define SOCINFO_PC_RESERVE (BIT(31) - 1)

Please move these defines into the next patch.

> +
> #endif
>
> --
> 2.40.1
>

--
With best wishes
Dmitry

2024-04-11 18:55:41

by Elliot Berman

[permalink] [raw]

Subject: Re: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them

2024-04-11 20:05:45

by Konrad Dybcio

[permalink] [raw]

Subject: Re: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them

On 4/11/24 20:55, Elliot Berman wrote:
> On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote:
>> In preparation for parsing the chip "feature code" (FC) and "product
>> code" (PC) (essentially the parameters that let us conclusively
>> characterize the sillicon we're running on, including various speed
>> bins), move the socinfo version defines to the public header and
>> include some more FC/PC defines.
>>
>> Signed-off-by: Konrad Dybcio <[email protected]>
>> ---

[...]

>> + SOCINFO_FC_EXT_RESERVE,
>> +};
>
> SOCINFO_FC_EXT_RESERVE was a convenient limit since we mapped
> SOCINFO_FC_AA -> string "AA" via an array, and we've only needed the 8
> feature codes so far.
>
> We should remove the EXT_RESERVE and test for the Y0-YF (internal
> feature code) values instead.

OK

>
>> +
>> +/* Internal feature codes */
>> +/* Valid values: 0 <= n <= 0xf */
>> +#define SOCINFO_FC_Yn(n) (0xf1 + n)
>> +#define SOCINFO_FC_INT_RESERVE SOCINFO_FC_Yn(0x10)
>
> We probably should've named this SOCINFO_FC_INT_MAX. Reserve implies
> it's reserved for some future use, but it's really the max value it
> could be.

So, should SOCINFO_FC_Yn(0x10) also be considered valid, or is (0xf)
the last one?

>
>> +
>> +/* Product codes */
>> +#define SOCINFO_PC_UNKNOWN 0
>> +/* Valid values: 0 <= n <= 8, the rest is reserved */
>> +#define SOCINFO_PCn(n) (n + 1)
>> +#define SOCINFO_PC_RESERVE (BIT(31) - 1)
>
> Similar comments here as the SOCINFO_FC_EXT_*. It's more like known
> values are [0,8], but more values could come in future chipsets.

Ok, sounds good, I'll remove the comment then

Konrad

2024-04-11 20:30:55

by Elliot Berman

[permalink] [raw]

Subject: Re: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them

On Thu, Apr 11, 2024 at 10:05:30PM +0200, Konrad Dybcio wrote:
>
>
> On 4/11/24 20:55, Elliot Berman wrote:
> > On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote:
> > > In preparation for parsing the chip "feature code" (FC) and "product
> > > code" (PC) (essentially the parameters that let us conclusively
> > > characterize the sillicon we're running on, including various speed
> > > bins), move the socinfo version defines to the public header and
> > > include some more FC/PC defines.
> > >
> > > Signed-off-by: Konrad Dybcio <[email protected]>
> > > ---
>
> [...]
>
> > > + SOCINFO_FC_EXT_RESERVE,
> > > +};
> >
> > SOCINFO_FC_EXT_RESERVE was a convenient limit since we mapped
> > SOCINFO_FC_AA -> string "AA" via an array, and we've only needed the 8
> > feature codes so far.
> >
> > We should remove the EXT_RESERVE and test for the Y0-YF (internal
> > feature code) values instead.
>
> OK
>
> >
> > > +
> > > +/* Internal feature codes */
> > > +/* Valid values: 0 <= n <= 0xf */
> > > +#define SOCINFO_FC_Yn(n) (0xf1 + n)
> > > +#define SOCINFO_FC_INT_RESERVE SOCINFO_FC_Yn(0x10)
> >
> > We probably should've named this SOCINFO_FC_INT_MAX. Reserve implies
> > it's reserved for some future use, but it's really the max value it
> > could be.
>
> So, should SOCINFO_FC_Yn(0x10) also be considered valid, or is (0xf)
> the last one?
>

0xf is the last one.

Thanks,
Elliot

> >
> > > +
> > > +/* Product codes */
> > > +#define SOCINFO_PC_UNKNOWN 0
> > > +/* Valid values: 0 <= n <= 8, the rest is reserved */
> > > +#define SOCINFO_PCn(n) (n + 1)
> > > +#define SOCINFO_PC_RESERVE (BIT(31) - 1)
> >
> > Similar comments here as the SOCINFO_FC_EXT_*. It's more like known
> > values are [0,8], but more values could come in future chipsets.
>
> Ok, sounds good, I'll remove the comment then
>
> Konrad

2024-04-11 20:31:54

by Konrad Dybcio

[permalink] [raw]

Subject: Re: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them

On 4/11/24 22:09, Elliot Berman wrote:
> On Thu, Apr 11, 2024 at 10:05:30PM +0200, Konrad Dybcio wrote:
>>
>>
>> On 4/11/24 20:55, Elliot Berman wrote:
>>> On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote:
>>>> In preparation for parsing the chip "feature code" (FC) and "product
>>>> code" (PC) (essentially the parameters that let us conclusively
>>>> characterize the sillicon we're running on, including various speed
>>>> bins), move the socinfo version defines to the public header and
>>>> include some more FC/PC defines.
>>>>
>>>> Signed-off-by: Konrad Dybcio <[email protected]>
>>>> ---

[...]

>
> 0xf is the last one.

One more question, are the "internal/external feature codes" referring to
internality/externality of the chips (i.e. "are they QC-lab-only engineering
samples), or what else does that represent?

Konrad

2024-04-11 23:50:44

by Elliot Berman

[permalink] [raw]

Subject: Re: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them

On Thu, Apr 11, 2024 at 10:24:08PM +0200, Konrad Dybcio wrote:
>
>
> On 4/11/24 22:09, Elliot Berman wrote:
> > On Thu, Apr 11, 2024 at 10:05:30PM +0200, Konrad Dybcio wrote:
> > >
> > >
> > > On 4/11/24 20:55, Elliot Berman wrote:
> > > > On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote:
> > > > > In preparation for parsing the chip "feature code" (FC) and "product
> > > > > code" (PC) (essentially the parameters that let us conclusively
> > > > > characterize the sillicon we're running on, including various speed
> > > > > bins), move the socinfo version defines to the public header and
> > > > > include some more FC/PC defines.
> > > > >
> > > > > Signed-off-by: Konrad Dybcio <[email protected]>
> > > > > ---
>
> [...]
>
> >
> > 0xf is the last one.
>
> One more question, are the "internal/external feature codes" referring to
> internality/externality of the chips (i.e. "are they QC-lab-only engineering
> samples), or what else does that represent?

Yes, QC-lab-only engineering samples is the right interpretation of
these feature codes.

Thanks,
Elliot

2024-04-12 00:10:47

by Konrad Dybcio

[permalink] [raw]

Subject: Re: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them

On 4/12/24 01:49, Elliot Berman wrote:
> On Thu, Apr 11, 2024 at 10:24:08PM +0200, Konrad Dybcio wrote:
>>
>>
>> On 4/11/24 22:09, Elliot Berman wrote:
>>> On Thu, Apr 11, 2024 at 10:05:30PM +0200, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 4/11/24 20:55, Elliot Berman wrote:
>>>>> On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote:
>>>>>> In preparation for parsing the chip "feature code" (FC) and "product
>>>>>> code" (PC) (essentially the parameters that let us conclusively
>>>>>> characterize the sillicon we're running on, including various speed
>>>>>> bins), move the socinfo version defines to the public header and
>>>>>> include some more FC/PC defines.
>>>>>>
>>>>>> Signed-off-by: Konrad Dybcio <[email protected]>
>>>>>> ---
>>
>> [...]
>>
>>>
>>> 0xf is the last one.
>>
>> One more question, are the "internal/external feature codes" referring to
>> internality/externality of the chips (i.e. "are they QC-lab-only engineering
>> samples), or what else does that represent?
>
> Yes, QC-lab-only engineering samples is the right interpretation of
> these feature codes.

Do you think it would be beneficial to keep the logic for these ESes in
the upstream GPU driver? Otherwise, I can yank out half of the added lines.

Konrad

2024-04-12 00:50:04

by Elliot Berman

[permalink] [raw]

Subject: Re: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them

On Fri, Apr 12, 2024 at 02:10:30AM +0200, Konrad Dybcio wrote:
>
>
> On 4/12/24 01:49, Elliot Berman wrote:
> > On Thu, Apr 11, 2024 at 10:24:08PM +0200, Konrad Dybcio wrote:
> > >
> > >
> > > On 4/11/24 22:09, Elliot Berman wrote:
> > > > On Thu, Apr 11, 2024 at 10:05:30PM +0200, Konrad Dybcio wrote:
> > > > >
> > > > >
> > > > > On 4/11/24 20:55, Elliot Berman wrote:
> > > > > > On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote:
> > > > > > > In preparation for parsing the chip "feature code" (FC) and "product
> > > > > > > code" (PC) (essentially the parameters that let us conclusively
> > > > > > > characterize the sillicon we're running on, including various speed
> > > > > > > bins), move the socinfo version defines to the public header and
> > > > > > > include some more FC/PC defines.
> > > > > > >
> > > > > > > Signed-off-by: Konrad Dybcio <[email protected]>
> > > > > > > ---
> > >
> > > [...]
> > >
> > > >
> > > > 0xf is the last one.
> > >
> > > One more question, are the "internal/external feature codes" referring to
> > > internality/externality of the chips (i.e. "are they QC-lab-only engineering
> > > samples), or what else does that represent?
> >
> > Yes, QC-lab-only engineering samples is the right interpretation of
> > these feature codes.
>
> Do you think it would be beneficial to keep the logic for these ESes in
> the upstream GPU driver? Otherwise, I can yank out half of the added lines.
>

Should be fine to yank, IMO.