On Sun, Oct 31, 2021 at 09:56:01PM +0100, Krzysztof Kozlowski wrote:
> On 31/10/2021 18:53, David Virag wrote:
> > Exynos 7885 has product id "0xE7885000". Add this id to the ids with
> > the name.
> >
>
> Thanks for the patch!
>
> > The downstream driver sets sub_rev to 2 if we are on Exynos 7885, we
> > detected sub_rev 1 and the 27th bit of the revision register is set.
>
> There is no revision register in older Exynos boards, so it seems you
> speak about new version, but please mention it explicitly.
>
Yes, the 7885 has the new version with seperate registers, that can be
used with the 850 compatible.
> > This is presumably because Samsung might have set the wrong bits on
> > rev2 of the SoC in the chipid, but we may never know as we have no
> > manual.
> >
> > Both the SM-A530F/jackpotlte with Exynos7885 and the SM-M305/m30lte
> > with Exynos7904 (rebranded Exynos7885 with lower clock speeds) seem
> > to have this bit set to 1 and have a sub_rev of 1 otherwise, but the
> > downstream driver corrects it to 2.
> > Let's replicate this behaviour in upstream too!
>
> No, let's don't replicate weird vendor behavior without understanding
> it, unless there is reason to. Please describe the reason or drop it.
>
Fair enough, I included it because as I understand Samsung made a
mistake in this revision's chipid regs and set a wrong bit, so if that
bit is set we should report revision 2 as it is actually rev2 just with
a broken chipid. At least this is what I think has happened but we'll
probably never know. Will remove in v2 then.
> >
> > Signed-off-by: David Virag <virag.david003@xxxxxxxxx>
> > ---
> > drivers/soc/samsung/exynos-chipid.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
> > index a28053ec7e6a..ec8c76275aec 100644
> > --- a/drivers/soc/samsung/exynos-chipid.c
> > +++ b/drivers/soc/samsung/exynos-chipid.c
> > @@ -55,6 +55,7 @@ static const struct exynos_soc_id {
> > { "EXYNOS5440", 0xE5440000 },
> > { "EXYNOS5800", 0xE5422000 },
> > { "EXYNOS7420", 0xE7420000 },
> > + { "EXYNOS7885", 0xE7885000 },
>
> This looks good, but please rebase on:
> https://lore.kernel.org/linux-samsung-soc/20211031205212.59505-1-krzysztof.kozlowski@xxxxxxxxxxxxx/T/#u
> because we use one compatible for entire family and I would like to have
> it documented which family is this here.
>
Sure.
> > { "EXYNOS850", 0xE3830000 },
> > { "EXYNOSAUTOV9", 0xAAA80000 },
> > };
> > @@ -88,6 +89,14 @@ static int exynos_chipid_get_chipid_info(struct regmap *regmap,
> > }
> > main_rev = (val >> data->main_rev_shift) & EXYNOS_REV_PART_MASK;
> > sub_rev = (val >> data->sub_rev_shift) & EXYNOS_REV_PART_MASK;
> > +
> > + //Exynos 7885 revision 2 apparently has the 27th bit set instead of having
> > + //a sub_rev of 2. Correct for this!
>
> Not a Linux kernel comment. This will go away anyway, but please read
> the coding style and use scripts/checkpatch.pl for future patches.
>
I did run checkpatch on it and it said nothing but yeah I forgot about
that. My bad!
> > + if (soc_info->product_id == 0xE7885000) {
> > + if ((sub_rev == 1) && (val & 0x04000000))
> > + sub_rev = 2;
> > + }
> > +
> > soc_info->revision = (main_rev << EXYNOS_REV_PART_SHIFT) | sub_rev;
> >
> > return 0;
> >
>
>
> Best regards,
> Krzysztof
Best regards,
David
