| Message ID | 20190821124441.22319-1-erosca@de.adit-jv.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [RFC,DO-NOT-MERGE] arm64: dts: renesas: R8A77961: Add Renesas M3-W+ (M3 ES3.0) SoC support | expand |
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
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
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.
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
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.
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.
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 + */
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