Re: [meta-arago][master/kirkstone][PATCH 7/7] meta-arago: Use new J7 SoC names over specific board names

[Denys Dmytriyenko <denis@denix.org>]

On Mon, Oct 10, 2022 at 08:26:26AM -0500, Andrew Davis wrote:
> On 10/7/22 3:10 PM, Denys Dmytriyenko wrote:
> >On Thu, Oct 06, 2022 at 10:06:28AM -0500, Andrew Davis via lists.yoctoproject.org wrote:
> >>Now that we have SoC names, we can avoid adding features based on the
> >>board name. We expect folks to create their own boards based on these
> >>SoCs, and so using the TI made EVM board name everywhere adds extra churn
> >>when adding a new board. Plus it is more correct for most of these
> >>features as they depend on the SoC, not on the EVM board.
> >>
> >>One other thing we do here is to not use the generic "j7" name,
> >>the current and future J7 devices are far to feature diverse
> >>to group at this level. Grouping like that will lead to the wrong
> >>things getting enabled as new J7 SoCs are added.
> >
> >Hmm, this second part is rather backwards (IMO) in some places, see below.
> >
> >
> >>diff --git a/meta-arago-distro/recipes-core/images/tisdk-core-bundle.inc b/meta-arago-distro/recipes-core/images/tisdk-core-bundle.inc
> >>index 3c31ba18..296eef7a 100644
> >>--- a/meta-arago-distro/recipes-core/images/tisdk-core-bundle.inc
> >>+++ b/meta-arago-distro/recipes-core/images/tisdk-core-bundle.inc
> >>@@ -24,7 +24,9 @@ DTB_FILTER:am57xx-hs-evm = "${DTB_FILTER:am57xx-evm}"
> >>  DTB_FILTER:ti43x = "am43"
> >>  DTB_FILTER:omapl138 = "da850"
> >>  DTB_FILTER:am65xx = "am65"
> >>-DTB_FILTER:j7 = "j721e"
> >>+DTB_FILTER:j721e = "j721e"
> >>+DTB_FILTER:j7200 = "j7200"
> >>+DTB_FILTER:j721s2 = "j721s2"
> >
> >Yes, here it does make perfect sense.
> >
> >
> >>diff --git a/meta-arago-distro/recipes-core/packagegroups/packagegroup-arago-tisdk-addons.bb b/meta-arago-distro/recipes-core/packagegroups/packagegroup-arago-tisdk-addons.bb
> >>index f4e72a89..c01e9497 100644
> >>--- a/meta-arago-distro/recipes-core/packagegroups/packagegroup-arago-tisdk-addons.bb
> >>+++ b/meta-arago-distro/recipes-core/packagegroups/packagegroup-arago-tisdk-addons.bb
> >>@@ -61,9 +61,9 @@ UTILS:append:am64xx = " ti-rtos-firmware pru-icss"
> >>  UTILS:append:am62xx = " ti-rtos-firmware"
> >>  #UTILS:append:am65xx = " ti-rtos-firmware pru-icss pru-pwm-fw"
> >>  UTILS:append:am65xx = " ti-rtos-firmware pru-icss"
> >>-UTILS:append:j7 = " ti-rtos-firmware"
> >>-UTILS:append:j721e-evm = " pru-icss"
> >>-UTILS:append:j721e-hs-evm = " pru-icss"
> >>+UTILS:append:j721e = " ti-rtos-firmware pru-icss"
> >>+UTILS:append:j7200 = " ti-rtos-firmware"
> >>+UTILS:append:j721s2 = " ti-rtos-firmware"
> >
> >Here - not so much. ti-rtos-firmware is applicable to all j7. So, I'd leave
> >that line alone and only replace adding pru-icss to specific EVMs to adding it
> >to j721e SoC family.
> >
> >Or go even further - ti-rtos-firmware is common to all k3 platforms, so all
> >the individual am65xx, am64xx and am62xx, along with j7, could be replaced
> >with one line:
> >
> >UTILS:append:k3 = " ti-rtos-firmware"
> >
> >And then add pri-icss or other extra FW to specific SoC families only.
> >
> 
> If you want I can make this re-arrangement in a follow up patch.
> 
> >
> >>diff --git a/meta-arago-distro/recipes-core/packagegroups/ti-analytics.bb b/meta-arago-distro/recipes-core/packagegroups/ti-analytics.bb
> >>index e16e4d51..e6e0b915 100644
> >>--- a/meta-arago-distro/recipes-core/packagegroups/ti-analytics.bb
> >>+++ b/meta-arago-distro/recipes-core/packagegroups/ti-analytics.bb
> >>@@ -19,7 +19,9 @@ ANALYTICS = ""
> >>  #    ${@['','qt-opencv-opencl-opengl-multithreaded'][oe.utils.all_distro_features(d, 'opencv opencl opengl', True, False) and bb.utils.contains('MACHINE_FEATURES', 'gpu dsp', True, False, d)]} \
> >>  #    ${@['','barcode-roi'][oe.utils.all_distro_features(d, 'opencv', True, False) and bb.utils.contains('MACHINE_FEATURES', 'dsp', True, False, d)]} \
> >>  #"
> >>-ANALYTICS:j7 = ""
> >>+ANALYTICS:j721e = ""
> >>+ANALYTICS:j7200 = ""
> >>+ANALYTICS:j721s2 = ""
> >
> >Well, this is probably completely wrong by now, anyway.
> >
> 
> Yes it probably is, but then my point in this patch was to
> not make any changes to functionality (or as few as possible).
> So if it is broken now, it was broken just the same before.
> 
> >
> >>  ANALYTICS:omapl138 = ""
> >>  RDEPENDS:${PN} = "\
> >>diff --git a/meta-arago-distro/recipes-core/packagegroups/ti-test.bb b/meta-arago-distro/recipes-core/packagegroups/ti-test.bb
> >>index 99a6cc82..5f56f8be 100644
> >>--- a/meta-arago-distro/recipes-core/packagegroups/ti-test.bb
> >>+++ b/meta-arago-distro/recipes-core/packagegroups/ti-test.bb
> >>@@ -78,7 +78,15 @@ ARAGO_TI_TEST:append:k3 = " \
> >>      k3conf \
> >>  "
> >>-ARAGO_TI_TEST:append:j7 = " \
> >>+ARAGO_TI_TEST:append:j721e = " \
> >>+    ufs-utils \
> >>+"
> >>+
> >>+ARAGO_TI_TEST:append:j7200 = " \
> >>+    ufs-utils \
> >>+"
> >>+
> >>+ARAGO_TI_TEST:append:j721s2 = " \
> >>      ufs-utils \
> >>  "
> >
> >ufs-utils is quite generic and should probably be added to k3 or maybe even
> >all platforms...
> >
> 
> Same as above, would you like this done in a follow up patch? This patch only
> fans out the name keeping the same functionality as before. If that exposes
> existing issues or opportunities for cleanup, then I can make those next.

It would have been fine to do further cleanup/optimization/collapsing of the 
code in a followup patch, if you weren't first expanding it here...

In other words, it seems replacing "j7" override with more specific 
"j721e/j7200/j721s2" overrides only makes sense for DTB_FILTER case and 
should be left alone in other 3 places.

-- 
Denys