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

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

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