diff mbox series

[1/2] soc: samsung: exynos-chipid: print entire PRO_ID reg when probing

Message ID 20211031165645.1182368-1-henrik@grimler.se (mailing list archive)
State New, archived
Headers show
Series [1/2] soc: samsung: exynos-chipid: print entire PRO_ID reg when probing | expand

Commit Message

Henrik Grimler Oct. 31, 2021, 4:56 p.m. UTC 
Older Exynos socs has one reg PRO_ID containing both product id and
revision information. Newer Exynos socs has one Product_ID reg with
product id, and one CHIPID_REV reg with revision information.

In commit c072c4ef7ef0 ("soc: samsung: exynos-chipid: Pass revision
reg offsets") the driver was changed so that the revision part of
PRO_ID is masked to 0 when printed during probing. This can give a
false impression that the revision is 0, so lets change so entire
PRO_ID reg is printed again.

Signed-off-by: Henrik Grimler <henrik@grimler.se>
---
Has been tested on exynos4412-i9300, which is compatible with
exynos4210-chipid, and on an exynos8895 device compatible with
exynos850-chipid.
---
drivers/soc/samsung/exynos-chipid.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


base-commit: b417d1e88f32645ed62a00d43c347b4386a0a021

Comments

Krzysztof Kozlowski Oct. 31, 2021, 8:35 p.m. UTC | #1 
On 31/10/2021 17:56, Henrik Grimler wrote:
> Older Exynos socs has one reg PRO_ID containing both product id and
> revision information. Newer Exynos socs has one Product_ID reg with
> product id, and one CHIPID_REV reg with revision information.
> 
> In commit c072c4ef7ef0 ("soc: samsung: exynos-chipid: Pass revision
> reg offsets") the driver was changed so that the revision part of
> PRO_ID is masked to 0 when printed during probing. This can give a
> false impression that the revision is 0, so lets change so entire
> PRO_ID reg is printed again.
> 
> Signed-off-by: Henrik Grimler <henrik@grimler.se>
> ---
> Has been tested on exynos4412-i9300, which is compatible with
> exynos4210-chipid, and on an exynos8895 device compatible with
> exynos850-chipid.
> ---

Hi,

Thanks for the patch.

I miss here however the most important information - why do you need it?
The answer to "why" should be in commit msg.

The change was kind of intentional and accepted, because revision ID is
printed next to the product ID. Printing revision ID with product ID
could be confusing...

Best regards,
Krzysztof


Best regards,
Krzysztof
Henrik Grimler Oct. 31, 2021, 9:29 p.m. UTC | #2 
Hi,

On Sun, Oct 31, 2021 at 09:35:20PM +0100, Krzysztof Kozlowski wrote:
> On 31/10/2021 17:56, Henrik Grimler wrote:
> > Older Exynos socs has one reg PRO_ID containing both product id and
> > revision information. Newer Exynos socs has one Product_ID reg with
> > product id, and one CHIPID_REV reg with revision information.
> > 
> > In commit c072c4ef7ef0 ("soc: samsung: exynos-chipid: Pass revision
> > reg offsets") the driver was changed so that the revision part of
> > PRO_ID is masked to 0 when printed during probing. This can give a
> > false impression that the revision is 0, so lets change so entire
> > PRO_ID reg is printed again.
> > 
> > Signed-off-by: Henrik Grimler <henrik@grimler.se>
> > ---
> > Has been tested on exynos4412-i9300, which is compatible with
> > exynos4210-chipid, and on an exynos8895 device compatible with
> > exynos850-chipid.
> > ---
> 
> Hi,
> 
> Thanks for the patch.
> 
> I miss here however the most important information - why do you need it?
> The answer to "why" should be in commit msg.

In dmesg we currently print something like:

    Exynos: CPU[EXYNOS4412] PRO_ID[0xe4412000] REV[0x11] Detected

where PRO_ID is given in datasheet as:

    [31:12] Product ID
      [9:8] Package information
      [7:4] Main Revision Number
      [3:0] Sub Revision Number

By printing PRO_ID[0xe4412000] it gives the impression that Package
information, Main Revision Number and Sub Revision Number are all 0.

> The change was kind of intentional and accepted, because revision ID is
> printed next to the product ID. Printing revision ID with product ID
> could be confusing...

Sure, I see the reason for only printing the product id. Would you
accept a patch write Product_ID instead of PRO_ID in the printed
message? So that we print:

    Exynos: CPU[EXYNOS4412] Product_ID[0xe4412000] REV[0x11] Detected

There's then less room for confusion regarding the revision, since
Product_ID should contain only the Product ID, unlike PRO_ID which
should contain both Product ID and revision info.

> Best regards,
> Krzysztof

Best regards,
Henrik Grimler
Krzysztof Kozlowski Nov. 1, 2021, 7:43 a.m. UTC | #3 
On 31/10/2021 22:29, Henrik Grimler wrote:
> Hi,
> 
> On Sun, Oct 31, 2021 at 09:35:20PM +0100, Krzysztof Kozlowski wrote:
>> On 31/10/2021 17:56, Henrik Grimler wrote:
>>> Older Exynos socs has one reg PRO_ID containing both product id and
>>> revision information. Newer Exynos socs has one Product_ID reg with
>>> product id, and one CHIPID_REV reg with revision information.
>>>
>>> In commit c072c4ef7ef0 ("soc: samsung: exynos-chipid: Pass revision
>>> reg offsets") the driver was changed so that the revision part of
>>> PRO_ID is masked to 0 when printed during probing. This can give a
>>> false impression that the revision is 0, so lets change so entire
>>> PRO_ID reg is printed again.
>>>
>>> Signed-off-by: Henrik Grimler <henrik@grimler.se>
>>> ---
>>> Has been tested on exynos4412-i9300, which is compatible with
>>> exynos4210-chipid, and on an exynos8895 device compatible with
>>> exynos850-chipid.
>>> ---
>>
>> Hi,
>>
>> Thanks for the patch.
>>
>> I miss here however the most important information - why do you need it?
>> The answer to "why" should be in commit msg.
> 
> In dmesg we currently print something like:
> 
>     Exynos: CPU[EXYNOS4412] PRO_ID[0xe4412000] REV[0x11] Detected
> 
> where PRO_ID is given in datasheet as:
> 
>     [31:12] Product ID
>       [9:8] Package information
>       [7:4] Main Revision Number
>       [3:0] Sub Revision Number
> 
> By printing PRO_ID[0xe4412000] it gives the impression that Package
> information, Main Revision Number and Sub Revision Number are all 0.
> 
>> The change was kind of intentional and accepted, because revision ID is
>> printed next to the product ID. Printing revision ID with product ID
>> could be confusing...
> 
> Sure, I see the reason for only printing the product id. Would you
> accept a patch write Product_ID instead of PRO_ID in the printed
> message? So that we print:
> 
>     Exynos: CPU[EXYNOS4412] Product_ID[0xe4412000] REV[0x11] Detected
> 
> There's then less room for confusion regarding the revision, since
> Product_ID should contain only the Product ID, unlike PRO_ID which
> should contain both Product ID and revision info.

Yes, let it be then:
Exynos: CPU[EXYNOS4412] ProductID[0xe4412000] Rev[0x11] detected


Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
index a28053ec7e6a..7fe44f71920d 100644
--- a/drivers/soc/samsung/exynos-chipid.c
+++ b/drivers/soc/samsung/exynos-chipid.c
@@ -33,6 +33,7 @@  struct exynos_chipid_variant {
 };
 
 struct exynos_chipid_info {
+	u32 pro_id;
 	u32 product_id;
 	u32 revision;
 };
@@ -79,6 +80,7 @@  static int exynos_chipid_get_chipid_info(struct regmap *regmap,
 	ret = regmap_read(regmap, EXYNOS_CHIPID_REG_PRO_ID, &val);
 	if (ret < 0)
 		return ret;
+	soc_info->pro_id = val;
 	soc_info->product_id = val & EXYNOS_MASK;
 
 	if (data->rev_reg != EXYNOS_CHIPID_REG_PRO_ID) {
@@ -146,7 +148,7 @@  static int exynos_chipid_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, soc_dev);
 
 	dev_info(&pdev->dev, "Exynos: CPU[%s] PRO_ID[0x%x] REV[0x%x] Detected\n",
-		 soc_dev_attr->soc_id, soc_info.product_id, soc_info.revision);
+		 soc_dev_attr->soc_id, soc_info.pro_id, soc_info.revision);
 
 	return 0;