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

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

[Henrik Grimler]

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(-)

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;
 

base-commit: b417d1e88f32645ed62a00d43c347b4386a0a021
-- 
2.33.1

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

[Henrik Grimler]

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(-)

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;
 

base-commit: b417d1e88f32645ed62a00d43c347b4386a0a021
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] soc: samsung: exynos-chipid: be more informative when soc is unknown

[Henrik Grimler]

Print the obtained product id that is incompatible.

Signed-off-by: Henrik Grimler <henrik@grimler.se>
---
 drivers/soc/samsung/exynos-chipid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
index 7fe44f71920d..885d578fd005 100644
--- a/drivers/soc/samsung/exynos-chipid.c
+++ b/drivers/soc/samsung/exynos-chipid.c
@@ -132,7 +132,7 @@ static int exynos_chipid_probe(struct platform_device *pdev)
 						"%x", soc_info.revision);
 	soc_dev_attr->soc_id = product_id_to_soc_id(soc_info.product_id);
 	if (!soc_dev_attr->soc_id) {
-		pr_err("Unknown SoC\n");
+		pr_err("Exynos: Unknown SoC: 0x%x\n", soc_info.product_id);
 		return -ENODEV;
 	}
 
-- 
2.33.1

[PATCH 2/2] soc: samsung: exynos-chipid: be more informative when soc is unknown

[Henrik Grimler]

Print the obtained product id that is incompatible.

Signed-off-by: Henrik Grimler <henrik@grimler.se>
---
 drivers/soc/samsung/exynos-chipid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
index 7fe44f71920d..885d578fd005 100644
--- a/drivers/soc/samsung/exynos-chipid.c
+++ b/drivers/soc/samsung/exynos-chipid.c
@@ -132,7 +132,7 @@ static int exynos_chipid_probe(struct platform_device *pdev)
 						"%x", soc_info.revision);
 	soc_dev_attr->soc_id = product_id_to_soc_id(soc_info.product_id);
 	if (!soc_dev_attr->soc_id) {
-		pr_err("Unknown SoC\n");
+		pr_err("Exynos: Unknown SoC: 0x%x\n", soc_info.product_id);
 		return -ENODEV;
 	}
 
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Krzysztof Kozlowski]

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

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

[Krzysztof Kozlowski]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] soc: samsung: exynos-chipid: be more informative when soc is unknown

[Krzysztof Kozlowski]

On 31/10/2021 17:56, Henrik Grimler wrote:
> Print the obtained product id that is incompatible.
> 

Why? Please mention it in the commit msg.


Best regards,
Krzysztof

Re: [PATCH 2/2] soc: samsung: exynos-chipid: be more informative when soc is unknown

[Krzysztof Kozlowski]

On 31/10/2021 17:56, Henrik Grimler wrote:
> Print the obtained product id that is incompatible.
> 

Why? Please mention it in the commit msg.


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Henrik Grimler]

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

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

[Henrik Grimler]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Krzysztof Kozlowski]

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

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

[Krzysztof Kozlowski]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel