Skip to content

Conversation

@jajanusz
Copy link
Contributor

We switch to PCH-based names instead of CPU config names for tigerlake+
platforms, because we need to support multiple variants of PCHs based
on one CPU famile - for example TGL-H & TGL-LP.

Signed-off-by: Janusz Jankowski janusz.jankowski@linux.intel.com

We switch to PCH-based names instead of CPU config names for tigerlake+
platforms, because we need to support multiple variants of PCHs based
on one CPU famile - for example TGL-H & TGL-LP.

Signed-off-by: Janusz Jankowski <janusz.jankowski@linux.intel.com>
@jajanusz
Copy link
Contributor Author

Please do not merge before CI is ready for that (it has to be green). We need to wait for CI guys to update fw names.

@lgirdwood
Copy link
Member

@zrombel @aiChaoSONG please approve when CI is ready and this PR is good to merge.

@plbossart
Copy link
Member

Please do not merge before CI is ready for that (it has to be green). We need to wait for CI guys to update fw names.

Which CI, firmware only? I would think this has no impact on kernel tests, right?

Copy link
Contributor

@xiulipan xiulipan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jajanusz I think this patch only change the defconfig name. How about the ri file name? I think for both tgph_defconfig and tgplp_defconfig they both generated sof-tgl.ri

PS: can we make this change more backward compatible, eg: add a soft-link to tigerlake_defconfig → tgplp_defconfig.
The release 1.6 and TGL-10 drop still need to use tigerlake_defconfig

I have an old PR to change the FW binary name, you can use this as reference. #3204

@jajanusz
Copy link
Contributor Author

jajanusz commented Sep 24, 2020

Please do not merge before CI is ready for that (it has to be green). We need to wait for CI guys to update fw names.

Which CI, firmware only? I would think this has no impact on kernel tests, right?

It applies only to our CI cos steps for building are written there... Your CI doesn't have this problem because you just run shell script from this repo and it is updated with PR.

@jajanusz
Copy link
Contributor Author

@jajanusz I think this patch only change the defconfig name. How about the ri file name? I think for both tgph_defconfig and tgplp_defconfig they both generated sof-tgl.ri

PS: can we make this change more backward compatible, eg: add a soft-link to tigerlake_defconfig → tgplp_defconfig.
The release 1.6 and TGL-10 drop still need to use tigerlake_defconfig

I have an old PR to change the FW binary name, you can use this as reference. #3204

It doesn not change file name because file name is not linked to PCH, it is by default named after signing config for RIMAGE.
Sorry but I'm not going to change names here (you'd have also do it for other platforms, f.e. EHL is also outputted as sof-tgl, what's with that?).
Kconfig should describe contents of firmware, not how file is named.
I will not change binary names in this PR. You rename result binaries for driver anyway. Also for release notes and describing building steps you can do it without renaming files -> #3385
It feels less hacky with this one, like something that is supported by buildsystem.

@lgirdwood
Copy link
Member

@jajanusz IIUC, we will build different binaries, one binary for each PCH variant with it's own kconfig.
I'm not following the binary file renaming discussion here, won't we need a binary file name change too so that the kernel can load the correct FW based on PCH ? i.e. does this PR use #3385 or is there a subsequent PR that links this all together.

@jajanusz
Copy link
Contributor Author

@jajanusz IIUC, we will build different binaries, one binary for each PCH variant with it's own kconfig.
I'm not following the binary file renaming discussion here, won't we need a binary file name change too so that the kernel can load the correct FW based on PCH ? i.e. does this PR use #3385 or is there a subsequent PR that links this all together.

We don't need to change default binary filename, kernel team renames them anyway while upstream. F.e. you have sof-cnl for cnl,cml,whl and currently we end up with kernel team renaming them / doing symlinks, so no change here.

There is no upcoming PR, #3385 can just be optionally used for better release notes for clients.

I think that you want to have something like that for tgllp_defconfig it will create sof-tgllp and for tglh_defconfig it will create sof-tglh, but to achieve this we would need to rename all defconfigs to PCH names (otherwise you'd have outputs like sof-skylake.ri) OR include output binary name in defconfigs, what is unacceptable because Kconfig shouldn't describe such things. Output binaries are named after platform signing schema (usually related to CPU name) and you can rename them manually or with #3385

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 25, 2020

f.e. EHL is also outputted as sof-tgl, what's with that?).

I always wondered what was the naming logic and I would have never suspected something like RIMAGE_SIGNING_SCHEMA! Based on this interesting discussion it looks like I was not the only one missing some parts of the whole picture. The toolchain names make this even more confusing: https://thesofproject.github.io/latest/getting_started/build-guide/build-from-scratch.html#toolchains

@jajanusz as you sound like the only one on top of this all, please submit a separate documentation PR with a summary of how names like "tgl" are used now (platform, signing scheme, toolchain, defconfig,...) and also how they should be used and not be used going forward. So the naming logic and directions are clearly stated, agreed (through the doc review) and easily found. Otherwise the discussions in #3385 and here are likely to repeat themselves plus these PRs are not the most convenient place to find information.

I can think of two places for this summary: either sof-docs or the top of src/arch/extensa/CMakeLists.txt but maybe there is a better place.

PS: I agree that build_apl/sof-apl.ri is at best pointless and at worst making scripting more difficult and less "scalable". build_apl/sof.ri would be more consistent and just as easy to copy/install/rename to /lib/firmware/intel/sof-api.ri or whatever else. ls -l /boot/vmlinuz* shows many different names yet none of these names come from the Linux build system itself; the naming happens after the build.

@lgirdwood
Copy link
Member

lgirdwood commented Sep 25, 2020

@plbossart are you good regarding binary file naming from kernel side ?

@xiulipan
Copy link
Contributor

@jajanusz Agreed with @marc-hb. We have some rename with "RIMAGE_SIGNING_SCHEMA" in jasperlake. It used CONFIG_ICELAKE but have output filename sof-jsl.ri. I think we should do same with the tgl-lp and tgl-h. I actually do not care if we name it with PCH or platform names. I just want to make sure we have some different binary names to help us identify which kconfig is used for this binary.

CONFIG_ICELAKE=y
CONFIG_RIMAGE_SIGNING_SCHEMA="jsl"
CONFIG_INTEL_DMIC=y
CONFIG_INTEL_SSP=y
CONFIG_INTEL_ALH=y
CONFIG_CORE_COUNT=2
CONFIG_LP_MEMORY_BANKS=1
CONFIG_HP_MEMORY_BANKS=16

@jajanusz
Copy link
Contributor Author

@xiulipan Sorry I think it shouldn't be done like this. As I wrote Kconfig shouldn't have something like file names and info about which config should be used for which platform is in defconfig name already, alternatively info like that can be in build scripts. Also JSL is patological situation here that should never happen. Someone duplicated icl machine definition in rimage and named it jsl just to have "jsl" in file name...

@marc-hb I agree, docs need updates.

@jajanusz
Copy link
Contributor Author

@xiulipan @lgirdwood so can we merge it? I guess 1.6 is out so should be safe.

@marc-hb
Copy link
Collaborator

marc-hb commented Oct 3, 2020

@marc-hb I agree, docs need updates

Filed new docs issue #3491

@marc-hb
Copy link
Collaborator

marc-hb commented Oct 5, 2020

This unsurprisingly broke various TGL things in CI; just before the majority of our CI team went for one-week national holiday.

@fredoh9 is trying to pick up the pieces. Internal issue number is 576

@marc-hb
Copy link
Collaborator

marc-hb commented Oct 9, 2020

@fredoh9 is trying to pick up the pieces. Internal issue number is 576

Mystery solved: a "quick defconfig hack" was manually put in Jenkins to test the final force push of this PR (Sep 23rd) before merge. This temporary hack was gone when this PR was merged later on Sep 30th, which broke the TGL build in Jenkins, which had a cascade effect on other CI things after Sep 30th. Things look OK now. Great job @fredoh9; thanks for covering for the CI team during the holiday.

marc-hb added a commit to marc-hb/sof that referenced this pull request Sep 27, 2021
ADL and ADL-S binaries share FW build configuration with TGL and TGL-H.

See also issue thesofproject#3491 and commit 15e03fd ("config: intel: use PCH
name for tigerlake") and the corresponding review in PR thesofproject#3451.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
lgirdwood pushed a commit that referenced this pull request Sep 29, 2021
ADL and ADL-S binaries share FW build configuration with TGL and TGL-H.

See also issue #3491 and commit 15e03fd ("config: intel: use PCH
name for tigerlake") and the corresponding review in PR #3451.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb added a commit to marc-hb/sof that referenced this pull request Oct 4, 2021
ADL and ADL-S binaries share FW build configuration with TGL and TGL-H.

See also issue thesofproject#3491 and commit 15e03fd ("config: intel: use PCH
name for tigerlake") and the corresponding review in PR thesofproject#3451.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
(cherry picked from commit 5a7a135)
lgirdwood pushed a commit that referenced this pull request Oct 5, 2021
ADL and ADL-S binaries share FW build configuration with TGL and TGL-H.

See also issue #3491 and commit 15e03fd ("config: intel: use PCH
name for tigerlake") and the corresponding review in PR #3451.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
(cherry picked from commit 5a7a135)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants