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

From: Henrik Grimler <henrik@grimler.se>
To: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Cc: chanho61.park@samsung.com, semen.protsenko@linaro.org,
	linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org
Subject: Re: [PATCH 1/2] soc: samsung: exynos-chipid: print entire PRO_ID reg when probing
Date: Sun, 31 Oct 2021 22:29:00 +0100	[thread overview]
Message-ID: <YX8KnNl0atfDwccR@grimlerstat.localdomain> (raw)
In-Reply-To: <878a4657-c68a-743e-7c89-bf69b416b824@canonical.com>

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