Support extended manifest config data for clock config #2297

fredoh9 · 2020-07-17T23:11:06Z

This need to be finalized to support SOF firmware change, thesofproject/sof#2893.

fredoh9 · 2020-07-17T23:59:52Z

Testing is pending as I couldn't get extended manifest from the firmware. I compiled @abonislawski 's branch with debug option as well. But I couldn't get ext_man_size.

[    8.044090] snd_sof_fw_ext_man_parse: remaning=1861222146 ext_man_size=0
[    8.044094] sof-audio-pci 0000:00:0e.0: firmware doesn't contain extended manifest

@abonislawski , can you proivde test firmware? I have test devcies, apl and cnl. Thank you!

abonislawski · 2020-07-20T06:53:22Z

@fredoh9 at the moment ext_manifest is not included in any FW image.
Just add this patch to test it:
thesofproject/sof#2957

fredoh9 · 2020-07-21T01:08:31Z

I'm able to see extended manifest data with merging a commit from thesofproject/sof#2957. And I built own firmware for APL.
Currently finalizing this PR, will update soon.

sound/soc/sof/loader.c

fredoh9 · 2020-07-21T22:08:52Z

I tested with CML Helios,

[    2.667307] sof-audio-pci 0000:00:1f.3: loading firmware
[    2.668727] sof-audio-pci 0000:00:1f.3: request_firmware intel/sof/community/sof-cml.ri successful
[    2.668731] sof-audio-pci 0000:00:1f.3: found sof_ext_man header type 1 size 0x1A0
[    2.668735] sof-audio-pci 0000:00:1f.3: found sof_ext_man header type 0 size 0x50
[    2.668739] sof-audio-pci 0000:00:1f.3: Firmware info: version 1:5:0-4dd38
[    2.668743] sof-audio-pci 0000:00:1f.3: Firmware: ABI 3:17:0 Kernel ABI 3:17:0
[    2.668747] sof-audio-pci 0000:00:1f.3: Firmware debug build 1 on Jul 21 2020-10:02:59 - options:
                GDB: disabled
                lock debug: disabled
                lock vdebug: disabled
[    2.668750] sof-audio-pci 0000:00:1f.3: found sof_ext_man header type 2 size 0x70
[    2.668754] sof-audio-pci 0000:00:1f.3: Firmware info: used compiler XCC 12:0:8 <RG-2017.8-linux> used optimization flags O2
[    2.668764] sof-audio-pci 0000:00:1f.3: found sof_ext_man header type 3 size 0x30
[    2.668767] sof-audio-pci 0000:00:1f.3: warning: unknown sof_ext_man header type 3 size 0x30
[    2.668769] sof-audio-pci 0000:00:1f.3: found sof_ext_man header type 5 size 0x30
[    2.668772] sof-audio-pci 0000:00:1f.3: clk_config CAVS_LPRO: 1

sound/soc/sof/loader.c

include/sound/sof/ext_manifest.h

sound/soc/sof/sof-priv.h

include/sound/sof/ext_manifest.h

kv2019i

Thanks @fredoh9 -- this starts to look good. One bigger issue I'm left wondering is that what is the first actual use of this information. Unless we have a real user for the info, I don't think we can send the series forward. What would be the first user and could we add it to this PR?

sound/soc/sof/intel/ext_manifest.h

sound/soc/sof/intel/hda-loader.c

sound/soc/sof/intel/hda.h

fredoh9 · 2020-10-12T17:52:18Z

Thanks @fredoh9 -- this starts to look good. One bigger issue I'm left wondering is that what is the first actual use of this information. Unless we have a real user for the info, I don't think we can send the series forward. What would be the first user and could we add it to this PR?

I will submit new PR for supporting ICL ICCMAX. It will use of clk_config_lpro.

lyakh · 2020-10-15T07:41:42Z

No more formal objections, but yes, as @kv2019i pointed out, without use-cases it's difficult to review properly

fredoh9 · 2020-10-15T18:06:40Z

@lyakh @kv2019i @ranj063 I dropped LAST_ELEM and MAX related changes. Initially I thought it is minor and necessary. But it drags too much discussion. Current version is exactly same header with merged firmware codes except EMPTY.
I will focus on ICL usage with clk_config_lpro.

kv2019i · 2020-10-19T09:56:23Z

@ktrzcinx I'll ping you here as well. As this is smaller of the two (and @fredoh9 doesn't have the ICCMAX patches in the same series), maybe it would be easier to push this in first, and you could then rebase. Is the implementation here good for you?

ktrzcinx · 2020-10-19T10:20:30Z

In this PR one thing which concerns me is inconsistence between elem_num calculation as for flex array (which one I thing is done properly) and SOF_EXT_MAN_CAVS_CONFIG_LAST_ELEM usage to define sof_ext_man_cavs_config_data->elems as array with constant size.

xiulipan

Please make alignment with #2459

ABI header need alignment
parser need to be in loader.c and clk config should be in ops.
The size is really a problem. A question here, how could we know how many config elem we have? like the elem_num is calculated by alignment size.

xiulipan · 2020-10-20T02:27:21Z

include/sound/sof/ext_manifest.h

+struct sof_ext_man_config_data {
+	struct sof_ext_man_elem_header hdr;
+
+	struct sof_config_elem elems[SOF_EXT_MAN_CONFIG_LAST_ELEM];


@fredoh9 I also have some solution for the size , see thesofproject/sof#3527 this will reduce our change to minimum..
Agreed with @kv2019i in #2459 @ktrzcinx @fredoh9 I think you may need to decided who's PR goes first. We'd better make some alignment with header first.

xiulipan · 2020-10-20T02:37:25Z

sound/soc/sof/intel/ext_manifest.h

+	struct sof_ext_man_elem_header hdr;
+
+	struct sof_config_elem elems[SOF_EXT_MAN_CAVS_CONFIG_LAST_ELEM];
+} __packed;


Some finding about the elem_size here. see https://github.com/thesofproject/linux/pull/2459/files#r508172578
@fredoh9 @ktrzcinx how could we get real elem size with align up here?

.hdr.elem_size = ALIGN_UP(sizeof(struct ext_man_config_data), EXT_MAN_ALIGN),

xiulipan · 2020-10-20T02:43:58Z

sound/soc/sof/intel/cnl.c

 	.post_fw_run = hda_dsp_post_fw_run,

+	/* parse platform specific extended manifest */
+	.parse_platform_ext_manifest = hda_dsp_ext_man_get_cavs_config_data,


Why add this parse as ops?
Will we have different type of ext_manifest for different platform?
I think you can make something same with #2459

@xiulipan yes we will. For example, why would the IMX platforms have the LPRO config elem?

@ranj063 A good question here, but I think we should parse the platform manifest in one place and config them to each platform.
Otherwise we will need to add a almost same function for parse in each platform.

@xiulipan I'm afraid this is a bit wrong audience. The ABI was already added to FW with 3.17 in thesofproject/sof#2893
It's possible to just ignore this and agree a new interface to pass clock information in non-hw-specific way, but this needs to be raised in FW.

@ranj063 @fredoh9 @kv2019i My only concern here is about the generic part in hda_dsp_ext_man_get_cavs_config_data, if we need implement this for each platform there will be too many duplicate code. I will suggest to only wrap specific handler to platform and fallback to unknown handler to unsupported platforms.

xiulipan · 2020-10-20T02:46:14Z

sound/soc/sof/intel/hda-loader.c

+			/* skip empty token */
+			break;
+		case SOF_EXT_MAN_CAVS_CONFIG_CAVS_LPRO:
+			hda->clk_config_lpro = config_data->elems[i].value;


I think you would better to have some ops for the clk_config setting for each platform.
The parse should be unify for all platform, but how to se the clk should be platform specific.

xiulipan · 2020-10-23T05:17:03Z

sound/soc/sof/intel/cnl.c

 	.post_fw_run = hda_dsp_post_fw_run,

+	/* parse platform specific extended manifest */
+	.parse_platform_ext_manifest = hda_dsp_ext_man_get_cavs_config_data,


@ranj063 @fredoh9 @kv2019i My only concern here is about the generic part in hda_dsp_ext_man_get_cavs_config_data, if we need implement this for each platform there will be too many duplicate code. I will suggest to only wrap specific handler to platform and fallback to unknown handler to unsupported platforms.

xiulipan · 2020-10-23T05:21:28Z

sound/soc/sof/intel/hda-loader.c

+int hda_dsp_ext_man_get_cavs_config_data(struct snd_sof_dev *sdev,
+					 const struct sof_ext_man_elem_header *hdr)
+{
+	const struct sof_ext_man_cavs_config_data *config_data =
+		container_of(hdr, struct sof_ext_man_cavs_config_data, hdr);
+	struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
+	int i, elem_num;
+
+	/* calculate total number of config data elements */
+	elem_num = (hdr->size - sizeof(struct sof_ext_man_elem_header)) / sizeof(struct sof_config_elem);
+	if (elem_num <= 0) {
+		dev_err(sdev->dev, "cavs config data is inconsistent: %d\n", elem_num);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < elem_num; i++)
+		switch (config_data->elems[i].token) {
+		case SOF_EXT_MAN_CAVS_CONFIG_EMPTY:
+			/* skip empty token */
+			break;
+		case SOF_EXT_MAN_CAVS_CONFIG_CAVS_LPRO:
+			hda->clk_config_lpro = config_data->elems[i].value;
+			dev_dbg(sdev->dev, "FW clock config: %s\n", hda->clk_config_lpro ? "LPRO" : "HPRO");
+			break;
+		case SOF_EXT_MAN_CAVS_CONFIG_OUTBOX_SIZE:
+		case SOF_EXT_MAN_CAVS_CONFIG_INBOX_SIZE:
+			/* This elem is supported but config data is not being used yet */
+			break;
+		default:
+			dev_warn(sdev->dev, "unsupported token type: %d\n",
+				 config_data->elems[i].token);
+		}
+
+	return 0;
+}


I will suggest to move this function to loader.c and only do something like

ext_man_config_cvas_lpro(...); break; case SOF_EXT_MAN_CAVS_CONFIG_OUTBOX_SIZE: case SOF_EXT_MAN_CAVS_CONFIG_INBOX_SIZE: ext_man_config_box_size(...); break; default: dev_warn(sdev->dev, "unsupported token type: %d\n", config_data->elems[i].token); } }

Then in ext_man_config_cvas_lpro and ext_man_config_box_size we can have platform ops check and fallback to
warn unsupported ext_man

plbossart · 2020-10-30T21:35:43Z

@fredoh9 if the firmware part was merged, what's the direction here for the driver? @ranj063 I'll let you drive this since I don't have any context (despite this being started in July...)

fredoh9 · 2020-10-31T14:46:16Z

@plbossart Sorry for delay, i couldn't finish icl iccmax implementation. I will submit a PR for it today. And I will work with @ranj063

kv2019i · 2020-11-02T11:36:55Z

#2459 is now merged, @fredoh9 you can now rebase this one.

fredoh9 · 2020-11-02T18:02:46Z

#2459 is now merged, @fredoh9 you can now rebase this one.

Sure, I will rebase!

Add parse_platform_ext_manifest() op to parse platform-specific config data in the extended manifest. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

Define the parse_platform_ext_manifest() op for HDA platforms to parse the SOF_EXT_MAN_CAVS_CONFIG_CAVS_LPRO config item to determine if the FW is configured for LPRO. The default clock configuration is assumed to be HPRO in the absence of this item in the extended manifest. New member clk_config_lpro is added to struct sof_intel_hda_dev to store the FW clock config information and that this will be used later to perform platform-specific operations in the post_fw_run op. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

OUTBOX_SIZE, INBOX_SIZE are supported but not being used yet. Handle these elements to avoid warning about unknown token type. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

fredoh9 · 2020-11-04T18:59:20Z

Closing this. Will be reviewed and merged with ICL ICCMAX implementation. #2548

fredoh9 requested review from bardliao, dbaluta, juimonen, keyonjie, kv2019i, lgirdwood, lyakh, plbossart and ranj063 as code owners July 17, 2020 23:11

fredoh9 force-pushed the fix/ext_lpro_abi_update branch from b7cee8d to 3530dcf Compare July 17, 2020 23:24

fredoh9 changed the title ~~[WIP] Support extended manifest codec config~~ [WIP][Don't Review Yet] Support extended manifest codec config Jul 17, 2020

fredoh9 force-pushed the fix/ext_lpro_abi_update branch from 3530dcf to a77904e Compare July 21, 2020 19:45

fredoh9 changed the title ~~[WIP][Don't Review Yet] Support extended manifest codec config~~ Support extended manifest config data for clock config Jul 21, 2020

ranj063 reviewed Jul 21, 2020

View reviewed changes

sound/soc/sof/loader.c Show resolved Hide resolved

ranj063 reviewed Jul 21, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

fredoh9 force-pushed the fix/ext_lpro_abi_update branch from a77904e to f553993 Compare July 21, 2020 21:59

fredoh9 requested a review from ranj063 July 21, 2020 22:30

ranj063 reviewed Jul 21, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

ranj063 reviewed Jul 21, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

fredoh9 force-pushed the fix/ext_lpro_abi_update branch from f553993 to 1e5fc3b Compare July 22, 2020 01:15

lyakh reviewed Jul 22, 2020

View reviewed changes

include/sound/sof/ext_manifest.h Show resolved Hide resolved

sound/soc/sof/sof-priv.h Outdated Show resolved Hide resolved

fredoh9 mentioned this pull request Jul 22, 2020

ICCMAX implementation for FW boot in ICL #2310

Closed

fredoh9 force-pushed the fix/ext_lpro_abi_update branch 2 times, most recently from 8d59191 to b8b3314 Compare July 24, 2020 18:32

lyakh requested changes Jul 27, 2020

View reviewed changes

include/sound/sof/ext_manifest.h Outdated Show resolved Hide resolved

fredoh9 force-pushed the fix/ext_lpro_abi_update branch from b8b3314 to 4a4b2f5 Compare July 29, 2020 17:13

fredoh9 mentioned this pull request Oct 9, 2020

ext_manifest: define EMPTY with explicit 0 thesofproject/sof#3510

Closed

fredoh9 force-pushed the fix/ext_lpro_abi_update branch from d6e8f01 to 4b34c3c Compare October 9, 2020 20:34

fredoh9 mentioned this pull request Oct 9, 2020

ext_manifest: clean up IPC extended data and handle PROBE_INFO #2491

Closed

kv2019i requested changes Oct 12, 2020

View reviewed changes

sound/soc/sof/intel/ext_manifest.h Show resolved Hide resolved

sound/soc/sof/intel/ext_manifest.h Outdated Show resolved Hide resolved

sound/soc/sof/intel/hda-loader.c Show resolved Hide resolved

sound/soc/sof/intel/hda.h Outdated Show resolved Hide resolved

fredoh9 force-pushed the fix/ext_lpro_abi_update branch 2 times, most recently from 82c8b05 to 7ec7010 Compare October 15, 2020 05:01

fredoh9 requested review from kv2019i and lyakh October 15, 2020 05:08

fredoh9 force-pushed the fix/ext_lpro_abi_update branch from 7ec7010 to d918d7e Compare October 15, 2020 18:00

xiulipan suggested changes Oct 20, 2020

View reviewed changes

xiulipan mentioned this pull request Oct 21, 2020

ext_manifest: add new test cases for extmanifest thesofproject/sof-test#456

Open

This was referenced Oct 21, 2020

ASoC: SOF: loader: do not warn about unknown firmware headers #2519

Merged

[RFC]ext_man: fix size issue for ext_man_config_data elem thesofproject/sof#3527

Closed

xiulipan suggested changes Oct 23, 2020

View reviewed changes

fredoh9 added 3 commits November 2, 2020 13:04

ASoC: SOF: ops: add parse_platform_ext_manifest() op

f288370

Add parse_platform_ext_manifest() op to parse platform-specific config data in the extended manifest. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

ASoC: SOF: ext_manifest: parse cavs extra config data elem

8cf6b73

OUTBOX_SIZE, INBOX_SIZE are supported but not being used yet. Handle these elements to avoid warning about unknown token type. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

fredoh9 force-pushed the fix/ext_lpro_abi_update branch from d918d7e to 8cf6b73 Compare November 2, 2020 21:07

fredoh9 requested a review from xiulipan November 3, 2020 21:59

fredoh9 closed this Nov 4, 2020

Support extended manifest config data for clock config #2297

Support extended manifest config data for clock config #2297

Uh oh!

Conversation

fredoh9 commented Jul 17, 2020

Uh oh!

fredoh9 commented Jul 17, 2020

Uh oh!

abonislawski commented Jul 20, 2020

Uh oh!

fredoh9 commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fredoh9 commented Jul 21, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fredoh9 commented Oct 12, 2020

Uh oh!

lyakh commented Oct 15, 2020

Uh oh!

fredoh9 commented Oct 15, 2020

Uh oh!

kv2019i commented Oct 19, 2020

Uh oh!

ktrzcinx commented Oct 19, 2020

Uh oh!

xiulipan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart commented Oct 30, 2020

Uh oh!

fredoh9 commented Oct 31, 2020

Uh oh!

kv2019i commented Nov 2, 2020

Uh oh!

fredoh9 commented Nov 2, 2020

Uh oh!

fredoh9 commented Nov 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

fredoh9 commented Jul 21, 2020 •

edited

Loading

ranj063 Oct 20, 2020 •

edited

Loading