Skip to content

Support extended manifest config data for clock config #2297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fredoh9 wants to merge 3 commits into from
Closed

Support extended manifest config data for clock config #2297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f288370
ASoC: SOF: ops: add parse_platform_ext_manifest() op
fredoh9 Oct 7, 2020
768db78
ASoC: SOF: Intel: hda: define parse_platform_ext_manifest op
fredoh9 Oct 5, 2020
8cf6b73
ASoC: SOF: ext_manifest: parse cavs extra config data elem
fredoh9 Oct 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/sound/sof/ext_manifest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ enum sof_ext_man_elem_type {
SOF_EXT_MAN_ELEM_CC_VERSION = SOF_IPC_EXT_CC_INFO,
SOF_EXT_MAN_ELEM_DBG_ABI = SOF_IPC_EXT_USER_ABI_INFO,
SOF_EXT_MAN_ELEM_CONFIG_DATA = 5, /**< ABI3.17 */
SOF_EXT_MAN_ELEM_PLATFORM_CONFIG_DATA = 6,
};

/* extended manifest element header */
Expand Down
3 changes: 3 additions & 0 deletions sound/soc/sof/intel/apl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ const struct snd_sof_dsp_ops sof_apl_ops = {
.pre_fw_run = hda_dsp_pre_fw_run,
.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,

/* dsp core power up/down */
.core_power_up = hda_dsp_enable_core,
.core_power_down = hda_dsp_core_reset_power_down,
Expand Down
3 changes: 3 additions & 0 deletions sound/soc/sof/intel/cnl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,9 @@ const struct snd_sof_dsp_ops sof_cnl_ops = {
.pre_fw_run = hda_dsp_pre_fw_run,
.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,
Copy link

@xiulipan xiulipan Oct 20, 2020

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@ranj063 ranj063 Oct 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

@xiulipan xiulipan Oct 20, 2020

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

@kv2019i kv2019i Oct 22, 2020

Choose a reason for hiding this comment

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

@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.

Copy link

@xiulipan xiulipan Oct 23, 2020

Choose a reason for hiding this comment

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

@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.


/* dsp core power up/down */
.core_power_up = hda_dsp_enable_core,
.core_power_down = hda_dsp_core_reset_power_down,
Expand Down
36 changes: 36 additions & 0 deletions sound/soc/sof/intel/ext_manifest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */
/*
* This file is provided under a dual BSD/GPLv2 license. When using or
* redistributing this file, you may do so under either license.
*
* Copyright(c) 2020 Intel Corporation. All rights reserved.
*/

/*
* Intel extended manifest is a extra place to store Intel cavs specific
* metadata about firmware, for example LPRO/HPRO configuration is
* Intel cavs specific. This part of output binary is not signed.
*/

#ifndef __INTEL_CAVS_EXT_MANIFEST_H__
#define __INTEL_CAVS_EXT_MANIFEST_H__

#include <sound/sof/ext_manifest.h>

/* EXT_MAN_ELEM_PLATFORM_CONFIG_DATA elements identificators */
enum sof_cavs_config_elem_type {
SOF_EXT_MAN_CAVS_CONFIG_EMPTY = 0,
SOF_EXT_MAN_CAVS_CONFIG_CAVS_LPRO = 1,
SOF_EXT_MAN_CAVS_CONFIG_OUTBOX_SIZE = 2,
SOF_EXT_MAN_CAVS_CONFIG_INBOX_SIZE = 3,
SOF_EXT_MAN_CAVS_CONFIG_LAST_ELEM, /**< keep it at the end of enum list */
};

/* EXT_MAN_ELEM_PLATFORM_CONFIG_DATA elements */
struct sof_ext_man_cavs_config_data {
struct sof_ext_man_elem_header hdr;

struct sof_config_elem elems[SOF_EXT_MAN_CAVS_CONFIG_LAST_ELEM];
} __packed;
Copy link
Collaborator

@lyakh lyakh Oct 8, 2020

Choose a reason for hiding this comment

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

this got me thinking. This means, that every time you add a new config element type this structure will change size. So if you run a kernel and a firmware with different minor versions the kernel won't know what size of this structure should actually be there. I think you need a normal variable length array here like

struct sof_ext_man_cavs_config_data {
	struct sof_ext_man_elem_header hdr;
	uint32_t n_elem;
	struct sof_config_elem elems[];
} __packed;

Copy link
Member

@ktrzcinx ktrzcinx Oct 8, 2020

Choose a reason for hiding this comment

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

It was my point to, but variable explicit defining number of elements will be hard to achieve from FW side, there should be calculation based on sof_ext_man_cavs_config_data.hdr.elem_size.

Copy link
Collaborator

@ranj063 ranj063 Oct 8, 2020

Choose a reason for hiding this comment

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

why not have a MAX_ELEMS instead then?

Copy link
Collaborator Author

@fredoh9 fredoh9 Oct 8, 2020

Choose a reason for hiding this comment

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

Initially I thought that defining MAX enum is easy and better way but I'm investigating @lyakh @ktrzcinx 's comments right now.
@ranj063 After returning error, it failed all HDA platform. firmware had extra configs for OUTBOX/INBOX size which we don't use. That break this logic. I can see possible errror when firmware add more field. We should not return error, I will take care of this also.

Copy link
Collaborator

@lyakh lyakh Oct 9, 2020

Choose a reason for hiding this comment

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

It was my point to, but variable explicit defining number of elements will be hard to achieve from FW side, there should be calculation based on sof_ext_man_cavs_config_data.hdr.elem_size.

@ktrzcinx you mean the xcc's problem with variable size arrays?

Copy link
Member

@ktrzcinx ktrzcinx Oct 9, 2020

Choose a reason for hiding this comment

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

xcc's problem with variable size arrays is one issue, another is is explicit defining number of elements from FW side. From FW side those are global, const structs, filling them looks like this:

const struct ext_man_config_data ext_man_config
	__aligned(EXT_MAN_ALIGN) __section(".fw_metadata") = {
	.hdr.type = EXT_MAN_ELEM_CONFIG_DATA,
	.hdr.elem_size = ALIGN_UP(sizeof(struct ext_man_config_data),
				  EXT_MAN_ALIGN),
	.elems = {
		{EXT_MAN_CONFIG_IPC_MSG_SIZE, SOF_IPC_MSG_MAX_SIZE},
	},
};

As I know inserting .elems elements counter to this list would need some hacks. Or are we going about inserting .elems capacity, then it shouldn't be any problem (except for ABI change), but result will be the same as it is now (elem counter calculation from struct size).

Copy link
Collaborator Author

@fredoh9 fredoh9 Oct 9, 2020

Choose a reason for hiding this comment

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

I dropped variable size arrays. After investigating SOF firmware side change, first XCC bug and second, implementation is unnecessary complicated(even ignoring XCC bug).

Instead I bring back LAST_ELEM but LAST_ELEM is initialized with last element of that enum. And also added first enum EMPTY as Explicit 0, this is more clear(even i forgot why it started from 1).

Even with LAST_ELEM, calculating number of elem from header size is good idea. So I kept that also.

To sync up with SOF firmware header, I submit a PR to SOF side as well. thesofproject/sof#3510

Copy link
Collaborator

@lyakh lyakh Oct 13, 2020

Choose a reason for hiding this comment

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

I still disagree, that setting the array size to the number of currently defined parameters is a good idea. If these parameters are going to be used, then we'll get more than 1 of them pretty quickly, with each time having to modify the ABI. If this will never be extended beyond the original 1 value, well, maybe we don't need an array then in the first place at all.

Copy link
Member

@ktrzcinx ktrzcinx Oct 13, 2020

Choose a reason for hiding this comment

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

then we'll get more than 1 of them pretty quickly, with each time having to modify the ABI

If this array is flex array from kernel point of view and unknown elements are ignored, then ABI change won't be needed after extend (eg. multiple usage of single key).

Copy link

@xiulipan xiulipan Oct 20, 2020

Choose a reason for hiding this comment

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

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),


#endif /* __INTEL_CAVS_EXT_MANIFEST_H__ */
37 changes: 37 additions & 0 deletions sound/soc/sof/intel/hda-loader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include <sound/hdaudio_ext.h>
#include <sound/hda_register.h>
#include <sound/sof.h>
#include "ext_manifest.h"
#include "../ops.h"
#include "hda.h"

Expand Down Expand Up @@ -470,3 +471,39 @@ int hda_dsp_post_fw_run(struct snd_sof_dev *sdev)
/* re-enable clock gating and power gating */
return hda_dsp_ctrl_clock_power_gating(sdev, true);
}

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;
Copy link

@xiulipan xiulipan Oct 20, 2020

Choose a reason for hiding this comment

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

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.

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;
}
Comment on lines +475 to +509
Copy link

@xiulipan xiulipan Oct 23, 2020

Choose a reason for hiding this comment

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

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

7 changes: 7 additions & 0 deletions sound/soc/sof/intel/hda.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,9 @@ struct sof_intel_hda_dev {

/* sdw context allocated by SoundWire driver */
struct sdw_intel_ctx *sdw;

/* FW clock config, 0:HPRO, 1:LPRO */
bool clk_config_lpro;
};

static inline struct hdac_bus *sof_to_bus(struct snd_sof_dev *s)
Expand Down Expand Up @@ -618,6 +621,10 @@ int hda_dsp_cl_boot_firmware_skl(struct snd_sof_dev *sdev);
int hda_dsp_pre_fw_run(struct snd_sof_dev *sdev);
int hda_dsp_post_fw_run(struct snd_sof_dev *sdev);

/* parse platform specific ext manifest ops */
int hda_dsp_ext_man_get_cavs_config_data(struct snd_sof_dev *sdev,
const struct sof_ext_man_elem_header *hdr);

/*
* HDA Controller Operations.
*/
Expand Down
3 changes: 3 additions & 0 deletions sound/soc/sof/intel/tgl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ const struct snd_sof_dsp_ops sof_tgl_ops = {
.pre_fw_run = hda_dsp_pre_fw_run,
.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,

/* dsp core power up/down */
.core_power_up = hda_dsp_enable_core,
.core_power_down = hda_dsp_core_reset_power_down,
Expand Down
3 changes: 3 additions & 0 deletions sound/soc/sof/loader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,9 @@ static int snd_sof_fw_ext_man_parse(struct snd_sof_dev *sdev,
case SOF_EXT_MAN_ELEM_CONFIG_DATA:
ret = ext_man_get_config_data(sdev, elem_hdr);
break;
case SOF_EXT_MAN_ELEM_PLATFORM_CONFIG_DATA:
ret = snd_sof_dsp_parse_platform_ext_manifest(sdev, elem_hdr);
break;
default:
dev_info(sdev->dev, "unknown sof_ext_man header type %d size 0x%X\n",
elem_hdr->type, elem_hdr->size);
Expand Down
10 changes: 10 additions & 0 deletions sound/soc/sof/ops.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,16 @@ static inline int snd_sof_dsp_post_fw_run(struct snd_sof_dev *sdev)
return 0;
}

/* parse platform specific extended manifest */
static inline int snd_sof_dsp_parse_platform_ext_manifest(struct snd_sof_dev *sdev,
const struct sof_ext_man_elem_header *hdr)
{
if (sof_ops(sdev)->parse_platform_ext_manifest)
return sof_ops(sdev)->parse_platform_ext_manifest(sdev, hdr);

return 0;
}

/* misc */

/**
Expand Down
5 changes: 5 additions & 0 deletions sound/soc/sof/sof-priv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <sound/sof/pm.h>
#include <sound/sof/trace.h>
#include <uapi/sound/sof/fw.h>
#include <sound/sof/ext_manifest.h>

/* debug flags */
#define SOF_DBG_ENABLE_TRACE BIT(0)
Expand Down Expand Up @@ -208,6 +209,10 @@ struct snd_sof_dsp_ops {
int (*pre_fw_run)(struct snd_sof_dev *sof_dev); /* optional */
int (*post_fw_run)(struct snd_sof_dev *sof_dev); /* optional */

/* parse platform specific extended manifest */
int (*parse_platform_ext_manifest)(struct snd_sof_dev *sof_dev,
const struct sof_ext_man_elem_header *hdr); /* optional */

/* DSP PM */
int (*suspend)(struct snd_sof_dev *sof_dev,
u32 target_state); /* optional */
Expand Down