Skip to content

ASoC: SOF: DMIC: Fixed length IPC #1924

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

Merged
kv2019i merged 3 commits into from
Apr 9, 2020
Merged

ASoC: SOF: DMIC: Fixed length IPC #1924

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
d767e9c
ASoC: SOF: Intel: Fix typo in header file comment text
singalsu Mar 20, 2020
154bd1f
ASoC: SOF: Intel: Change DMIC load IPC to fixed length
singalsu Mar 20, 2020
57f8967
ASoC: SOF: Intel: Rename deprecated DMIC IPC struct field
singalsu Mar 24, 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
14 changes: 9 additions & 5 deletions include/sound/sof/dai-intel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@
/* bclk idle */
#define SOF_DAI_INTEL_SSP_CLKCTRL_BCLK_IDLE_HIGH BIT(5)

/* DMIC max. four controllers for eight microphone channels */
#define SOF_DAI_INTEL_DMIC_NUM_CTRL 4
Copy link
Collaborator

@lyakh lyakh Apr 7, 2020

Choose a reason for hiding this comment

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

sorry for chiming in so late with this. This can be made a separate PR: should we add a check of num_pdm_active <= SOF_DAI_INTEL_DMIC_NUM_CTRL? I think it would be good to tighten up our topology consistency checks.


/* SSP Configuration Request - SOF_IPC_DAI_SSP_CONFIG */
struct sof_ipc_dai_ssp_params {
struct sof_ipc_hdr hdr;
Expand Down Expand Up @@ -135,7 +138,7 @@ struct sof_ipc_dai_dmic_pdm_ctrl {
* version number used in configuration data is checked vs. version used by
* device driver src/drivers/dmic.c need to match. It is incremented from
* initial value 1 if updates done for the to driver would alter the operation
* of the microhone.
* of the microphone.
*
* Note: The microphone clock (pdmclk_min, pdmclk_max, duty_min, duty_max)
* parameters need to be set as defined in microphone data sheet. E.g. clock
Expand Down Expand Up @@ -170,12 +173,13 @@ struct sof_ipc_dai_dmic_params {
uint32_t fifo_fs; /**< FIFO sample rate in Hz (8000..96000) */
uint32_t reserved_1; /**< Reserved */
uint16_t fifo_bits; /**< FIFO word length (16 or 32) */
uint16_t reserved_2; /**< Reserved */
uint16_t fifo_bits_b; /**< Deprecated since firmware ABI 3.0.1 */
Copy link
Collaborator

@lyakh lyakh Apr 6, 2020

Choose a reason for hiding this comment

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

I'm wondering how we're going to handle such deprecation? Do we set a grace period of, say, a year and then remove this backward compatibility and then claim the field as "reserved?"

Copy link
Collaborator

@kv2019i kv2019i Apr 6, 2020

Choose a reason for hiding this comment

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

@lyakh I don't think you really ever get back reserved bits via deprecation unless you end up in a situation where you absolutely have to. I.e. rare enough event, so better worry about the process when the time comes.


uint16_t duty_min; /**< Min. mic clock duty cycle in % (20..80) */
uint16_t duty_max; /**< Max. mic clock duty cycle in % (min..80) */

uint32_t num_pdm_active; /**< Number of active pdm controllers */
uint32_t num_pdm_active; /**< Number of active pdm controllers. */
/**< Range is 1..SOF_DAI_INTEL_DMIC_NUM_CTRL */

uint32_t wake_up_time; /**< Time from clock start to data (us) */
uint32_t min_clock_on_time; /**< Min. time that clk is kept on (us) */
Expand All @@ -184,8 +188,8 @@ struct sof_ipc_dai_dmic_params {
/* reserved for future use */
uint32_t reserved[5];

/**< variable number of pdm controller config */
struct sof_ipc_dai_dmic_pdm_ctrl pdm[0];
/**< PDM controllers configuration */
struct sof_ipc_dai_dmic_pdm_ctrl pdm[SOF_DAI_INTEL_DMIC_NUM_CTRL];
} __packed;

#endif
83 changes: 30 additions & 53 deletions sound/soc/sof/topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2891,18 +2891,13 @@ static int sof_link_dmic_load(struct snd_soc_component *scomp, int index,
{
struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp);
struct snd_soc_tplg_private *private = &cfg->priv;
struct sof_ipc_dai_config *ipc_config;
struct sof_ipc_reply reply;
struct sof_ipc_fw_ready *ready = &sdev->fw_ready;
struct sof_ipc_fw_version *v = &ready->version;
u32 size;
size_t size = sizeof(*config);
int ret, j;

/*
* config is only used for the common params in dmic_params structure
* that does not include the PDM controller config array
* Set the common params to 0.
*/
/* Ensure the entire DMIC config struct is zeros */
Copy link
Collaborator

@lyakh lyakh Apr 7, 2020

Choose a reason for hiding this comment

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

This comment could be just drop, it doesn't seem to add a lot of information. In fact, looking at it: the below memset() is completely redundant. The caller has already 0-initialised the whole config. @singalsu could you make an additional PR to remove this too?

memset(&config->dmic, 0, sizeof(struct sof_ipc_dai_dmic_params));

/* get DMIC tokens */
Expand All @@ -2915,33 +2910,16 @@ static int sof_link_dmic_load(struct snd_soc_component *scomp, int index,
return ret;
}

/*
* allocate memory for dmic dai config accounting for the
* variable number of active pdm controllers
* This will be the ipc payload for setting dai config
*/
size = sizeof(*config) + sizeof(struct sof_ipc_dai_dmic_pdm_ctrl) *
config->dmic.num_pdm_active;

ipc_config = kzalloc(size, GFP_KERNEL);
if (!ipc_config)
return -ENOMEM;

/* copy the common dai config and dmic params */
memcpy(ipc_config, config, sizeof(*config));

/*
* alloc memory for private member
* Used to track the pdm config array index currently being parsed
*/
sdev->private = kzalloc(sizeof(u32), GFP_KERNEL);
if (!sdev->private) {
Copy link
Collaborator

@lyakh lyakh Apr 6, 2020

Choose a reason for hiding this comment

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

Do I understand correctly that this now conflicts with #1766? But the conflict seems to be easy to resolve.

Copy link
Collaborator

@kv2019i kv2019i Apr 6, 2020

Choose a reason for hiding this comment

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

@juimonen FYI, Seppo is out this week, so maybe let's put this #1924 in first, and you'll update the other one?

Copy link
Collaborator

@lyakh lyakh Apr 7, 2020

Choose a reason for hiding this comment

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

@kv2019i you want me to update it? Sure can do, but so far I'm not aware of many cases of such take-overs. Is it urgent? Or was it just a typo?

kfree(ipc_config);
if (!sdev->private)
return -ENOMEM;
}

/* get DMIC PDM tokens */
ret = sof_parse_tokens(scomp, &ipc_config->dmic.pdm[0], dmic_pdm_tokens,
ret = sof_parse_tokens(scomp, &config->dmic.pdm[0], dmic_pdm_tokens,
ARRAY_SIZE(dmic_pdm_tokens), private->array,
le32_to_cpu(private->size));
if (ret != 0) {
Expand All @@ -2951,44 +2929,44 @@ static int sof_link_dmic_load(struct snd_soc_component *scomp, int index,
}

/* set IPC header size */
ipc_config->hdr.size = size;
config->hdr.size = size;

/* debug messages */
dev_dbg(scomp->dev, "tplg: config DMIC%d driver version %d\n",
ipc_config->dai_index, ipc_config->dmic.driver_ipc_version);
config->dai_index, config->dmic.driver_ipc_version);
dev_dbg(scomp->dev, "pdmclk_min %d pdm_clkmax %d duty_min %hd\n",
ipc_config->dmic.pdmclk_min, ipc_config->dmic.pdmclk_max,
ipc_config->dmic.duty_min);
config->dmic.pdmclk_min, config->dmic.pdmclk_max,
config->dmic.duty_min);
dev_dbg(scomp->dev, "duty_max %hd fifo_fs %d num_pdms active %d\n",
ipc_config->dmic.duty_max, ipc_config->dmic.fifo_fs,
ipc_config->dmic.num_pdm_active);
dev_dbg(scomp->dev, "fifo word length %hd\n",
ipc_config->dmic.fifo_bits);
config->dmic.duty_max, config->dmic.fifo_fs,
config->dmic.num_pdm_active);
dev_dbg(scomp->dev, "fifo word length %hd\n", config->dmic.fifo_bits);

for (j = 0; j < ipc_config->dmic.num_pdm_active; j++) {
for (j = 0; j < config->dmic.num_pdm_active; j++) {
dev_dbg(scomp->dev, "pdm %hd mic a %hd mic b %hd\n",
ipc_config->dmic.pdm[j].id,
ipc_config->dmic.pdm[j].enable_mic_a,
ipc_config->dmic.pdm[j].enable_mic_b);
config->dmic.pdm[j].id,
config->dmic.pdm[j].enable_mic_a,
config->dmic.pdm[j].enable_mic_b);
dev_dbg(scomp->dev, "pdm %hd polarity a %hd polarity b %hd\n",
ipc_config->dmic.pdm[j].id,
ipc_config->dmic.pdm[j].polarity_mic_a,
ipc_config->dmic.pdm[j].polarity_mic_b);
config->dmic.pdm[j].id,
config->dmic.pdm[j].polarity_mic_a,
config->dmic.pdm[j].polarity_mic_b);
dev_dbg(scomp->dev, "pdm %hd clk_edge %hd skew %hd\n",
ipc_config->dmic.pdm[j].id,
ipc_config->dmic.pdm[j].clk_edge,
ipc_config->dmic.pdm[j].skew);
config->dmic.pdm[j].id,
config->dmic.pdm[j].clk_edge,
config->dmic.pdm[j].skew);
}

if (SOF_ABI_VER(v->major, v->minor, v->micro) < SOF_ABI_VER(3, 0, 1)) {
/* this takes care of backwards compatible handling of fifo_bits_b */
ipc_config->dmic.reserved_2 = ipc_config->dmic.fifo_bits;
}
/*
* this takes care of backwards compatible handling of fifo_bits_b.
* It is deprecated since firmware ABI version 3.0.1.
*/
if (SOF_ABI_VER(v->major, v->minor, v->micro) < SOF_ABI_VER(3, 0, 1))
config->dmic.fifo_bits_b = config->dmic.fifo_bits;

/* send message to DSP */
ret = sof_ipc_tx_message(sdev->ipc,
ipc_config->hdr.cmd, ipc_config, size, &reply,
sizeof(reply));
ret = sof_ipc_tx_message(sdev->ipc, config->hdr.cmd, config, size,
&reply, sizeof(reply));

if (ret < 0) {
dev_err(scomp->dev,
Expand All @@ -2998,14 +2976,13 @@ static int sof_link_dmic_load(struct snd_soc_component *scomp, int index,
}

/* set config for all DAI's with name matching the link name */
ret = sof_set_dai_config(sdev, size, link, ipc_config);
ret = sof_set_dai_config(sdev, size, link, config);
if (ret < 0)
dev_err(scomp->dev, "error: failed to save DAI config for DMIC%d\n",
config->dai_index);

err:
kfree(sdev->private);
kfree(ipc_config);

return ret;
}
Expand Down