[RFC DO-NOT-MERGE PATCH] arm64: dts: renesas: R8A77961: Add Renesas M3-W+ (M3 ES3.0) SoC support

[RFC DO-NOT-MERGE PATCH] arm64: dts: renesas: R8A77961: Add Renesas M3-W+ (M3 ES3.0) SoC support

[Eugeniu Rosca]

Similar to the revision update from H3-ES1.x to H3-ES2.0, the update
from M3-ES1.x to M3-ES3.0, in addition to fixing HW bugs/erratas, drops
entire silicon IPs [1-2] (for cost efficiency and other reasons).

However, unlike in the H3 ES1.x->ES2.0 revision update, the M3-ES3.0
came with a new SoC id, i.e. r8a77961 (according to both [2] and
the updated SoC HW manual Rev.2.00 Jul 2019). The choice to allocate a
new identifier seems to strengthen the HW differences between M3-ES1.x
and M3-ES3.0 (as it is the case for M3N/r8a77965).

Given the above, there are several ways to differentiate between
M3-ES1.x and M3-ES3.0:

A. The BSP way [1]. Move/rename r8a7796.dtsi to r8a7796-es1.dtsi and
keep using r8a7796.dtsi for M3-ES3.x.

Pros:
 * Resembles commit 291e0c4994d081 ("arm64: dts: r8a7795: Add support
   for R-Car H3 ES2.0")
 * Reuses the r8a7796 (e.g. sysc, cpg-mssr) drivers for r8a77961 (i.e.
   minimizes the bring-up effort)
Cons:
 * Deliberately diverges from the vendor documentation [2] by
   ignoring the new SoC identifier r8a77961, i.e. leading to
   inconsistencies in the names of the drivers and DTS

B. The approach taken in this patch, i.e. create a brand new
  r8a77961.dtsi (similar to r8a77965.dtsi).

Pros:
 * Reflects the reality documented by HW designers [2]
 * Maintains drivers/DTS naming consistency and avoids mismatch between
   documentation and code
Cons:
 * higher bring-up effort than (A)
 * more discussion is needed on whether it makes sense to separate:
   - DTS only
   - DTS + Kconfig (ARCH_R8A77961)
   - DTS + Kconfig (ARCH_R8A77961) + drivers (sysc, cpg-mssr, other?)

Comments appreciated!

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 arch/arm64/boot/dts/renesas/r8a77961.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 arch/arm64/boot/dts/renesas/r8a77961.dtsi

diff --git a/arch/arm64/boot/dts/renesas/r8a77961.dtsi b/arch/arm64/boot/dts/renesas/r8a77961.dtsi
new file mode 100644
index 000000000000..7f784619be32
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/r8a77961.dtsi
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree Source for the R-Car M3-W+ (R8A77961) ES3.x SoC
+ *
+ * Copyright (C) 2019 Renesas Electronics Corp.
+ */
+
+#include "r8a7796.dtsi"
+
+/*
+ * Here comes the delta between M3-W (M3 ES1.x) and M3-W+ (M3 ES3.0)
+ * described in:
+ * [1] https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=8ba438fd03d5
+ *     ("arm64: dts: r8a7796: Add support for R-Car M3 ES3.0")
+ * [2] [Confidential] Engineering Change Notice for R8A77961
+ */
-- 
2.23.0

Re: [RFC DO-NOT-MERGE PATCH] arm64: dts: renesas: R8A77961: Add Renesas M3-W+ (M3 ES3.0) SoC support

[Geert Uytterhoeven]

Hi Eugeniu,

Thanks for bringing this up!

On Wed, Aug 21, 2019 at 2:45 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> Similar to the revision update from H3-ES1.x to H3-ES2.0, the update
> from M3-ES1.x to M3-ES3.0, in addition to fixing HW bugs/erratas, drops
> entire silicon IPs [1-2] (for cost efficiency and other reasons).
>
> However, unlike in the H3 ES1.x->ES2.0 revision update, the M3-ES3.0
> came with a new SoC id, i.e. r8a77961 (according to both [2] and

Actually R-Car H3 ES2.0 is r8a77951, while ES1.x is r8a77950.
But we ignored the fifth digit (see below).

> the updated SoC HW manual Rev.2.00 Jul 2019). The choice to allocate a
> new identifier seems to strengthen the HW differences between M3-ES1.x
> and M3-ES3.0 (as it is the case for M3N/r8a77965).

While H3 ES2.0 was an evolutionary step, obsoleting H3 ES1.x, it looks
like M3-W and M3-W+ may exist as two separate products, next to each
other.

> Given the above, there are several ways to differentiate between
> M3-ES1.x and M3-ES3.0:
>
> A. The BSP way [1]. Move/rename r8a7796.dtsi to r8a7796-es1.dtsi and
> keep using r8a7796.dtsi for M3-ES3.x.
>
> Pros:
>  * Resembles commit 291e0c4994d081 ("arm64: dts: r8a7795: Add support
>    for R-Car H3 ES2.0")
>  * Reuses the r8a7796 (e.g. sysc, cpg-mssr) drivers for r8a77961 (i.e.
>    minimizes the bring-up effort)
> Cons:
>  * Deliberately diverges from the vendor documentation [2] by
>    ignoring the new SoC identifier r8a77961, i.e. leading to
>    inconsistencies in the names of the drivers and DTS
>
> B. The approach taken in this patch, i.e. create a brand new
>   r8a77961.dtsi (similar to r8a77965.dtsi).
>
> Pros:
>  * Reflects the reality documented by HW designers [2]
>  * Maintains drivers/DTS naming consistency and avoids mismatch between
>    documentation and code
> Cons:
>  * higher bring-up effort than (A)
>  * more discussion is needed on whether it makes sense to separate:
>    - DTS only
>    - DTS + Kconfig (ARCH_R8A77961)
>    - DTS + Kconfig (ARCH_R8A77961) + drivers (sysc, cpg-mssr, other?)
>
> Comments appreciated!

When we started work on H3 ES2.0, it was considered an evolutionary step
from ES1.x, not a different SoC.  We also were used to 4-digit IDs in
compatible values, as before the 5th digit was typically used to
indicate a minor difference, like a different package, or a different
ROM option.  Hence we ignored the 5th digit, reused the compatible
values for H3 ES1.x, and went with soc_device_match() to differentiate,
where needed.

However, it turned out H3 ES2.0 was more like a different SoC in the
same family: it has more similarities with R-Car M3-W ES1.0 than with
R-Car H3 ES1.x.  In the mean time, with the advent of R-Car D3 and M3-N,
we also got used to 5 digits.  Hence in hindsight, it might have been
better if we had considered H3 ES1.x and ES2.0 to be two different
SoCs.

Given R-Car M3-W and M3-W+ may co-exist as separate SoCs, I think
approach B is the best approach, using separate DTS, compatible values,
Kconfig, and drivers, like we did for e.g. R-Car M3-N.

What do you think?
Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC DO-NOT-MERGE PATCH] arm64: dts: renesas: R8A77961: Add Renesas M3-W+ (M3 ES3.0) SoC support

[Eugeniu Rosca]

Hi Geert,

Thanks for taking some time to reflect retrospectively on the bring-up
experiences from the past and to summarize the lessons learned.

On Fri, Aug 23, 2019 at 04:18:09PM +0200, Geert Uytterhoeven wrote:
[..]
> Actually R-Car H3 ES2.0 is r8a77951, while ES1.x is r8a77950.

That's a great detail which I missed. I confirm below:
 - SoC HW manual "Rev.1.00 Apr 2018" maps R8A77951 to H3 ver2.0
 - SoC HW manuals "Rev.1.50 Nov 2018" and "Rev.2.00 Jul 2019" map
   R8A77951 to H3 ver3.0
 - Older Renesas docs refer to the earlier H3 ES1.x SoC as R8A77950

So, on the one hand, there _is_ a 'part number' update from H3 ver1 to
rev2 and, on the other hand, there is no part number update from ver2 to
ver3. My understanding of the latter (as a side note) is that there are
no added/dropped HW features in ver3, hence the same 'part number'.

[..]

> When we started work on H3 ES2.0, it was considered an evolutionary step
> from ES1.x, not a different SoC.  We also were used to 4-digit IDs in
> compatible values, as before the 5th digit was typically used to
> indicate a minor difference, like a different package, or a different
> ROM option.  Hence we ignored the 5th digit, reused the compatible
> values for H3 ES1.x, and went with soc_device_match() to differentiate,
> where needed.

Honestly, this still sounds sensible and intuitive, assuming the delta
between the SoC models (differing in the 5th digit) is relatively low
(not sure how to quantify it though and where the threshold is).

One of the concerns related to soc_device_match() is that it sometimes
doesn't play nicely with spinlocks, generating lockdep splats [1].

> However, it turned out H3 ES2.0 was more like a different SoC in the
> same family: it has more similarities with R-Car M3-W ES1.0 than with
> R-Car H3 ES1.x. 
>
> In the mean time, with the advent of R-Car D3 and M3-N,
> we also got used to 5 digits.  Hence in hindsight, it might have been
> better if we had considered H3 ES1.x and ES2.0 to be two different
> SoCs.

Thinking of the way H3-ES1.x support was added, now that H3-ES1.x is
declared obsolete, we could probably reduce the image size by
some tens of KiB (more?) by simply disabling CONFIG_ARCH_R8A77950 if
its support was implemented as standalone CONFIG_ARCH? We now have to
compile and link the H3-ES1.x functionality even if nobody needs it,
which is a bit unfortunate.

> 
> Given R-Car M3-W and M3-W+ may co-exist as separate SoCs, I think
> approach B is the best approach, using separate DTS, compatible values,
> Kconfig, and drivers, like we did for e.g. R-Car M3-N.
> 
> What do you think?

I still think B is the best option in terms of transparency (clear
mapping between documentation and code), but I try to reconcile below
recent concerns (any support appreciated):

 - After reviewing the "Engineering Change Notice for R8A77961", it
   seems to me that the number of differences between R8A77960 and
   R8A77961 is really low (much much lower than in the case of
   R8A77950->R8A77951 transition). Most (if not all) of the IP blocks
   present in R8A77960 and removed in R8A77961 [2] (i.e. fcpci0, fcpcs,
   ivdp1c, vdpb) are not even supported in vanilla.
 - I guess with this low amount of differences, R8A77961 will be an
   almost perfect twin of R8A77960. If so, then is the additional
   maintenance effort (resulting after bring-up) still justified?
 - Duplicating 'drivers/pinctrl/sh-pfc/pfc-r8a7796.c' (as it was done
   for M3-N, with 99% similarity in the contents) will increase the
   image size by roughly 50KiB [3]. Additional (albeit less significant)
   size increase is expected from the addition of other SoC-specific
   drivers.
 - [Minor] According to your feedback and to the best of our knowledge,
   both M3-W and M3-W+ are expected to reach the end user, which might
   create less motivation to really separate the two SoCs via CONFIG.

What do you think about the above?
Thanks!

[1] lockdep splats generated by soc_device_match()
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ba164a49f8f739
https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=8037f4932ec5
https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=08cd9d10caff
https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=34d3b527b70c
https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=918f22c7b2ae
https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=196d1399ffa8
https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=5ed4a312252e
[2] https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=8ba438fd03d5
[3] $ size drivers/pinctrl/sh-pfc/pfc-r8a7796*.o
   text        data     bss     dec     hex filename
  51660       0       0   51660    c9cc drivers/pinctrl/sh-pfc/pfc-r8a77965.o
  51628       0       0   51628    c9ac drivers/pinctrl/sh-pfc/pfc-r8a7796.o

-- 
Best Regards,
Eugeniu.

Re: [RFC DO-NOT-MERGE PATCH] arm64: dts: renesas: R8A77961: Add Renesas M3-W+ (M3 ES3.0) SoC support

[Geert Uytterhoeven]

Hi Eugeniu,

On Wed, Aug 28, 2019 at 7:10 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> On Fri, Aug 23, 2019 at 04:18:09PM +0200, Geert Uytterhoeven wrote:
> [..]
> > Actually R-Car H3 ES2.0 is r8a77951, while ES1.x is r8a77950.
>
> That's a great detail which I missed. I confirm below:
>  - SoC HW manual "Rev.1.00 Apr 2018" maps R8A77951 to H3 ver2.0
>  - SoC HW manuals "Rev.1.50 Nov 2018" and "Rev.2.00 Jul 2019" map
>    R8A77951 to H3 ver3.0
>  - Older Renesas docs refer to the earlier H3 ES1.x SoC as R8A77950
>
> So, on the one hand, there _is_ a 'part number' update from H3 ver1 to
> rev2 and, on the other hand, there is no part number update from ver2 to
> ver3. My understanding of the latter (as a side note) is that there are
> no added/dropped HW features in ver3, hence the same 'part number'.

Perhaps. Note that part numbers are controlled by marketing, so there
may or may not be technical reasons...

> > When we started work on H3 ES2.0, it was considered an evolutionary step
> > from ES1.x, not a different SoC.  We also were used to 4-digit IDs in
> > compatible values, as before the 5th digit was typically used to
> > indicate a minor difference, like a different package, or a different
> > ROM option.  Hence we ignored the 5th digit, reused the compatible
> > values for H3 ES1.x, and went with soc_device_match() to differentiate,
> > where needed.
>
> Honestly, this still sounds sensible and intuitive, assuming the delta
> between the SoC models (differing in the 5th digit) is relatively low
> (not sure how to quantify it though and where the threshold is).
>
> One of the concerns related to soc_device_match() is that it sometimes
> doesn't play nicely with spinlocks, generating lockdep splats [1].

soc_device_match() is not intended to be called all over the place in a
driver.  Just call it once from the driver's .probe() method, and make use
of the soc_device_attribute.data field to store SoC-specific features.

> > However, it turned out H3 ES2.0 was more like a different SoC in the
> > same family: it has more similarities with R-Car M3-W ES1.0 than with
> > R-Car H3 ES1.x.
> >
> > In the mean time, with the advent of R-Car D3 and M3-N,
> > we also got used to 5 digits.  Hence in hindsight, it might have been
> > better if we had considered H3 ES1.x and ES2.0 to be two different
> > SoCs.
>
> Thinking of the way H3-ES1.x support was added, now that H3-ES1.x is
> declared obsolete, we could probably reduce the image size by
> some tens of KiB (more?) by simply disabling CONFIG_ARCH_R8A77950 if
> its support was implemented as standalone CONFIG_ARCH? We now have to
> compile and link the H3-ES1.x functionality even if nobody needs it,
> which is a bit unfortunate.

We could make separate Kconfig symbols for r8a77950 and r8a77951.
The biggest win would be the PFC driver.
For now, I think there are still several ES1.x SoCs in use.

> > Given R-Car M3-W and M3-W+ may co-exist as separate SoCs, I think
> > approach B is the best approach, using separate DTS, compatible values,
> > Kconfig, and drivers, like we did for e.g. R-Car M3-N.
> >
> > What do you think?
>
> I still think B is the best option in terms of transparency (clear
> mapping between documentation and code), but I try to reconcile below
> recent concerns (any support appreciated):
>
>  - After reviewing the "Engineering Change Notice for R8A77961", it
>    seems to me that the number of differences between R8A77960 and
>    R8A77961 is really low (much much lower than in the case of
>    R8A77950->R8A77951 transition). Most (if not all) of the IP blocks
>    present in R8A77960 and removed in R8A77961 [2] (i.e. fcpci0, fcpcs,
>    ivdp1c, vdpb) are not even supported in vanilla.

Well, DT is about describing hardware, not about describing (current)
Linux functionality ;-)

>  - I guess with this low amount of differences, R8A77961 will be an
>    almost perfect twin of R8A77960. If so, then is the additional
>    maintenance effort (resulting after bring-up) still justified?

I think so.  We may discover more subtle differences later.

>  - Duplicating 'drivers/pinctrl/sh-pfc/pfc-r8a7796.c' (as it was done
>    for M3-N, with 99% similarity in the contents) will increase the
>    image size by roughly 50KiB [3]. Additional (albeit less significant)
>    size increase is expected from the addition of other SoC-specific
>    drivers.

As e.g. FCPs do not use external pins, we may be able to reuse pfc-r8a7796.c
for M3-W+.  I'll need to have a closer look...

>  - [Minor] According to your feedback and to the best of our knowledge,
>    both M3-W and M3-W+ are expected to reach the end user, which might
>    create less motivation to really separate the two SoCs via CONFIG.

We do have separate Kconfig options for other SoCs that are considered
separate parts.

Please remember DT has the stable ABI, so the DT bindings is what we
need to get right in the first place.  It's much easier to change our
mind later within the kernel (driver and Kconfig organization; although
Kconfig changes may cause you some troubles).

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC DO-NOT-MERGE PATCH] arm64: dts: renesas: R8A77961: Add Renesas M3-W+ (M3 ES3.0) SoC support

[Eugeniu Rosca]

Hi Geert,

Many thanks for the recent clarifications.
I got some M3 ES3.0 reference targets on my desk.
So, if time permits, I might push some bring-up patches to you.

-- 
Best Regards,
Eugeniu

Re: [RFC DO-NOT-MERGE PATCH] arm64: dts: renesas: R8A77961: Add Renesas M3-W+ (M3 ES3.0) SoC support

[Simon Horman]

On Fri, Aug 23, 2019 at 04:18:09PM +0200, Geert Uytterhoeven wrote:
> Hi Eugeniu,
> 
> Thanks for bringing this up!
> 
> On Wed, Aug 21, 2019 at 2:45 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> > Similar to the revision update from H3-ES1.x to H3-ES2.0, the update
> > from M3-ES1.x to M3-ES3.0, in addition to fixing HW bugs/erratas, drops
> > entire silicon IPs [1-2] (for cost efficiency and other reasons).
> >
> > However, unlike in the H3 ES1.x->ES2.0 revision update, the M3-ES3.0
> > came with a new SoC id, i.e. r8a77961 (according to both [2] and
> 
> Actually R-Car H3 ES2.0 is r8a77951, while ES1.x is r8a77950.
> But we ignored the fifth digit (see below).
> 
> > the updated SoC HW manual Rev.2.00 Jul 2019). The choice to allocate a
> > new identifier seems to strengthen the HW differences between M3-ES1.x
> > and M3-ES3.0 (as it is the case for M3N/r8a77965).
> 
> While H3 ES2.0 was an evolutionary step, obsoleting H3 ES1.x, it looks
> like M3-W and M3-W+ may exist as two separate products, next to each
> other.
> 
> > Given the above, there are several ways to differentiate between
> > M3-ES1.x and M3-ES3.0:
> >
> > A. The BSP way [1]. Move/rename r8a7796.dtsi to r8a7796-es1.dtsi and
> > keep using r8a7796.dtsi for M3-ES3.x.
> >
> > Pros:
> >  * Resembles commit 291e0c4994d081 ("arm64: dts: r8a7795: Add support
> >    for R-Car H3 ES2.0")
> >  * Reuses the r8a7796 (e.g. sysc, cpg-mssr) drivers for r8a77961 (i.e.
> >    minimizes the bring-up effort)
> > Cons:
> >  * Deliberately diverges from the vendor documentation [2] by
> >    ignoring the new SoC identifier r8a77961, i.e. leading to
> >    inconsistencies in the names of the drivers and DTS
> >
> > B. The approach taken in this patch, i.e. create a brand new
> >   r8a77961.dtsi (similar to r8a77965.dtsi).
> >
> > Pros:
> >  * Reflects the reality documented by HW designers [2]
> >  * Maintains drivers/DTS naming consistency and avoids mismatch between
> >    documentation and code
> > Cons:
> >  * higher bring-up effort than (A)
> >  * more discussion is needed on whether it makes sense to separate:
> >    - DTS only
> >    - DTS + Kconfig (ARCH_R8A77961)
> >    - DTS + Kconfig (ARCH_R8A77961) + drivers (sysc, cpg-mssr, other?)
> >
> > Comments appreciated!
> 
> When we started work on H3 ES2.0, it was considered an evolutionary step
> from ES1.x, not a different SoC.  We also were used to 4-digit IDs in
> compatible values, as before the 5th digit was typically used to
> indicate a minor difference, like a different package, or a different
> ROM option.  Hence we ignored the 5th digit, reused the compatible
> values for H3 ES1.x, and went with soc_device_match() to differentiate,
> where needed.
> 
> However, it turned out H3 ES2.0 was more like a different SoC in the
> same family: it has more similarities with R-Car M3-W ES1.0 than with
> R-Car H3 ES1.x.  In the mean time, with the advent of R-Car D3 and M3-N,
> we also got used to 5 digits.  Hence in hindsight, it might have been
> better if we had considered H3 ES1.x and ES2.0 to be two different
> SoCs.
> 
> Given R-Car M3-W and M3-W+ may co-exist as separate SoCs, I think
> approach B is the best approach, using separate DTS, compatible values,
> Kconfig, and drivers, like we did for e.g. R-Car M3-N.
> 
> What do you think?
> Thanks!

+1

Your recollection of the decisions made around H3 and 4/5 digit identifiers
matches mine. And I agree that in hindsight we may have been better to use
a 5 digit identifier for H3 ES2.0.  So I think it would be a good idea to
learn from this and use a 5 digit identifier for M3-W+. We can always
register unused identifiers if the need arises.

For may the main drawback of this approach is that it is inconsistent
with the one we took for H3, which may lead to confusion. But I think
that we are better off to favour evolution over reuse in this case.
Or in other words, it seems like a good opportunity to learn from
our mistakes.

Re: [RFC DO-NOT-MERGE PATCH] arm64: dts: renesas: R8A77961: Add Renesas M3-W+ (M3 ES3.0) SoC support

[Eugeniu Rosca]

Hi Simon,

On Sat, Aug 31, 2019 at 10:01:02AM +0200, Simon Horman wrote:
[..]
> +1
> 
> Your recollection of the decisions made around H3 and 4/5 digit identifiers
> matches mine. And I agree that in hindsight we may have been better to use
> a 5 digit identifier for H3 ES2.0.  So I think it would be a good idea to
> learn from this and use a 5 digit identifier for M3-W+. We can always
> register unused identifiers if the need arises.
> 
> For may the main drawback of this approach is that it is inconsistent
> with the one we took for H3, which may lead to confusion. But I think
> that we are better off to favour evolution over reuse in this case.
> Or in other words, it seems like a good opportunity to learn from
> our mistakes.

Thank you for casting your thoughts. Much appreciated.

-- 
Best Regards,
Eugeniu