| Message ID | 20180213165837.1620-1-srinivas.kandagatla@linaro.org |
|---|---|
| Headers | show |
| Series | ASoC: qcom: Add support to QDSP based Audio | expand |
Thanks for your review comments On 19/02/18 10:30, Rohit Kumar wrote: >> diff --git a/sound/soc/qcom/Makefile b/sound/soc/qcom/Makefile >> index d5280355c24f..748f5e891dcf 100644 >> --- a/sound/soc/qcom/Makefile >> +++ b/sound/soc/qcom/Makefile >> @@ -13,6 +13,11 @@ obj-$(CONFIG_SND_SOC_LPASS_APQ8016) += >> snd-soc-lpass-apq8016.o >> # Machine >> snd-soc-storm-objs := storm.o >> snd-soc-apq8016-sbc-objs := apq8016_sbc.o >> +snd-soc-msm8996-objs := apq8096.o > This needs to be moved out of this patch Will fix it in next version. >> obj-$(CONFIG_SND_SOC_STORM) += snd-soc-storm.o >> obj-$(CONFIG_SND_SOC_APQ8016_SBC) += snd-soc-apq8016-sbc.o >> +obj-$(CONFIG_SND_SOC_MSM8996) += snd-soc-msm8996.o >> + >> +#DSP lib _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Thanks for testing this out, On 19/02/18 10:32, Rohit Kumar wrote: >> >> diff --git a/sound/soc/qcom/qdsp6/Makefile >> b/sound/soc/qcom/qdsp6/Makefile >> index 660afcab98fd..c7833842b878 100644 >> --- a/sound/soc/qcom/qdsp6/Makefile >> +++ b/sound/soc/qcom/qdsp6/Makefile >> @@ -1,5 +1,5 @@ >> obj-$(CONFIG_SND_SOC_QDSP6_COMMON) += q6dsp-common.o >> -obj-$(CONFIG_SND_SOC_QDSP6_AFE) += q6afe.o >> +obj-$(CONFIG_SND_SOC_QDSP6_AFE) += q6afe.o q6afe-dai.o > depmod: ERROR: Cycle detected: q6afe -> q6afe_dai -> q6afe > need to use like: > +qdsp6_afe-objs := q6afe.o q6afe-dai.o > +obj-$(CONFIG_SND_SOC_QDSP6_AFE) += qdsp6_afe.o > similarly for asm and adm Yep, will fix this in next version, thanks, srini >> obj-$(CONFIG_SND_SOC_QDSP6_ADM) += q6adm.o q6routing.o >> obj-$(CONFIG_SND_SOC_QDSP6_ASM) += q6asm.o >> obj-$(CONFIG_SND_SOC_QDSP6_CORE) += q6core.o >> diff --git a/sound/soc/qcom/qdsp6/q6afe-dai.c >> b/sound/soc/qcom/qdsp6/q6afe-dai.c >> new file mode 100644 >> index 000000000000..f6a618e9db9e _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, Feb 13, 2018 at 04:58:16PM +0000, srinivas.kandagatla@linaro.org wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Please submit patches using subject lines reflecting the style for the subsystem. This makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. > +- but must contain the following property: > + > +- compatible: > +- qcom,apr-svc-id > +- qcom,apr-svc-name > +- #sound-dai-cells Property or properties? It's a bit surprising that both the ID and the name are mandatory, does the name actually get used for non-diagnostic reasons? _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Mon, Feb 19, 2018 at 04:00:32PM +0530, Rohit Kumar wrote: > > On 2/13/2018 10:28 PM, srinivas.kandagatla@linaro.org wrote: > > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > > > This patch adds support to Q6AFE (Audio Front End) module on Q6DSP. > > > > AFE module sits right at the other end of cpu where the codec/audio > > devices are connected. Please delete unneeded context from mails when replying. Doing this makes it much easier to find your reply in the message, helping ensure it won't be missed by people scrolling through the irrelevant quoted material. _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, Feb 13, 2018 at 04:58:17PM +0000, srinivas.kandagatla@linaro.org wrote: > +// SPDX-License-Identifier: GPL-2.0 > +#ifndef __DT_BINDINGS_Q6_AFE_H__ > +#define __DT_BINDINGS_Q6_AFE_H__ > + > +/* Audio Front End (AFE) Ports */ > +#define AFE_PORT_HDMI_RX 8 > + > +#endif /* __DT_BINDINGS_Q6_AFE_H__ */ Please use a C++ comment here as well for consistency, it looks more intentional. > +config SND_SOC_QDSP6_AFE > + tristate > + default n > + n is the default anyway, no need to specify it (I know some uses already slipped through here but it'd be better to fix those). > index 000000000000..0a5af06bb50e > --- /dev/null > +++ b/sound/soc/qcom/qdsp6/q6afe.c > @@ -0,0 +1,520 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2011-2017, The Linux Foundation > + * Copyright (c) 2018, Linaro Limited > + */ Same here with the comment, just make the whole comment a C++ comment rather than having one comment using both styles. Similar things apply elsewhere. > +/* Port map of index vs real hw port ids */ > +static struct afe_port_map port_maps[AFE_PORT_MAX] = { > + [AFE_PORT_HDMI_RX] = { AFE_PORT_ID_MULTICHAN_HDMI_RX, > + AFE_PORT_HDMI_RX, 1, 1}, > +}; Is this not device specific in any way? It looks likely to be. > +static struct q6afe_port *afe_find_port(struct q6afe *afe, int token) > +{ > + struct q6afe_port *p = NULL; > + > + spin_lock(&afe->port_list_lock); > + list_for_each_entry(p, &afe->port_list, node) > + if (p->token == token) > + break; > + > + spin_unlock(&afe->port_list_lock); Why do we need to lock the port list, what are we protecting it against? We don't write here which suggests either there's some kind of race condition in this lookup or the lock is not doing anything. > +static int afe_callback(struct apr_device *adev, > + struct apr_client_message *data) > +{ > + struct q6afe *afe = dev_get_drvdata(&adev->dev); > + struct aprv2_ibasic_rsp_result_t *res; > + struct q6afe_port *port; > + > + if (!data->payload_size) > + return 0; > + > + res = data->payload; > + if (data->opcode == APR_BASIC_RSP_RESULT) { > + if (res->status) { Shouldn't we use a switch statement here in case we want to handle something else? > + switch (res->opcode) { > + case AFE_PORT_CMD_SET_PARAM_V2: > + case AFE_PORT_CMD_DEVICE_STOP: > + case AFE_PORT_CMD_DEVICE_START: > + afe->state = AFE_CMD_RESP_AVAIL; > + port = afe_find_port(afe, data->token); > + if (port) > + wake_up(&port->wait); No locking here? It seems a bit odd that the AFE global state is being set to say there's a response available but we wake a specific port. > + ret = apr_send_pkt(afe->apr, data); > + if (ret < 0) { > + dev_err(afe->dev, "packet not transmitted\n"); > + ret = -EINVAL; > + goto err; Why squash the error code here? We don't even print it. > + } > + > + ret = wait_event_timeout(*wait, (afe->state == AFE_CMD_RESP_AVAIL), > + msecs_to_jiffies(TIMEOUT_MS)); > + if (!ret) { > + ret = -ETIMEDOUT; > + } else if (afe->status > 0) { > + dev_err(afe->dev, "DSP returned error[%s]\n", > + q6dsp_strerror(afe->status)); > + ret = q6dsp_errno(afe->status); If we time out here and the DSP delivers a response later it looks like we'll get data corruption - the DSP will happily deliver the response into shared state. > +static int afe_port_start(struct q6afe_port *port, > + union afe_port_config *afe_config) > +{ > + struct afe_audioif_config_command config = {0,}; > + struct q6afe *afe = port->afe; > + int port_id = port->id; > + int ret, param_id = port->cfg_type; > + > + config.port = *afe_config; > + > + ret = q6afe_port_set_param_v2(port, &config, param_id, > + sizeof(*afe_config)); > + if (ret) { > + dev_err(afe->dev, "AFE enable for port 0x%x failed %d\n", > + port_id, ret); > + return ret; > + } > + return afe_send_cmd_port_start(port); Why not just inline this function here? It appears to have only this user. > + int index = 0; We set index to 0... > + > + port_id = port->id; > + index = port->token; ...the unconditionally overwrite it? > +/** > + * q6afe_port_start() - Start a afe port > + * > + * @port: Instance of port to start > + * > + * Return: Will be an negative on packet size on success. > + */ > +int q6afe_port_start(struct q6afe_port *port) > +{ > + return afe_port_start(port, &port->port_cfg); > +} > +EXPORT_SYMBOL_GPL(q6afe_port_start); This is the third level of wrapper for the port start command in this file. Do we *really* need all these wrappers? > +struct q6afe_port *q6afe_port_get_from_id(struct device *dev, int id) > +{ > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL); > + if (!port) > + return ERR_PTR(-ENOMEM); The _port_get_ function is allocating data but... > +/** > + * q6afe_port_put() - Release port reference > + * > + * @port: Instance of port to put > + */ > +void q6afe_port_put(struct q6afe_port *port) > +{ > + struct q6afe *afe = port->afe; > + > + spin_lock(&afe->port_list_lock); > + list_del(&port->node); > + spin_unlock(&afe->port_list_lock); > +} > +EXPORT_SYMBOL_GPL(q6afe_port_put); ...the _port_put() function isn't freeing it. That seems leaky. I'm also a bit worried about this being a spinlock that we're using for allocation, and not even spin_lock_irqsave(). Presumably this is protecting against interrupts otherwise we'd not need a spinlock but for that to work we'd need the _irqsave()? _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, Feb 13, 2018 at 04:58:22PM +0000, srinivas.kandagatla@linaro.org wrote: > + num_regions = is_contiguous ? 1 : periods; > + buf_sz = is_contiguous ? (period_sz * periods) : period_sz; Please write normal if statements, it's much easier to read. > + buf_sz = PAGE_ALIGN(buf_sz); I don't understand what this is doing, buf_sz is a length not an address so why are we attempting to align it? _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, Feb 13, 2018 at 04:58:23PM +0000, srinivas.kandagatla@linaro.org wrote: > uint32_t used; > @@ -131,7 +191,7 @@ static int q6asm_apr_send_session_pkt(struct q6asm *a, struct audio_client *ac, > > rc = wait_event_timeout(a->mem_wait, (a->mem_state <= 0), 5 * HZ); > if (!rc) { > - dev_err(a->dev, "CMD timeout \n"); > + dev_err(a->dev, "CMD timeout\n"); > rc = -ETIMEDOUT; > } else if (a->mem_state < 0) { > rc = q6dsp_errno(a->mem_state); This should be folded into whatever patch is being fixed. > + open.hdr.opcode = ASM_STREAM_CMD_OPEN_WRITE_V3; > + open.mode_flags = 0x00; > + open.mode_flags |= ASM_LEGACY_STREAM_SESSION; What is a legacy stream and why are we using it in new code? _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, Feb 13, 2018 at 04:58:26PM +0000, srinivas.kandagatla@linaro.org wrote: > +static int q6hdmi_format_put(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct q6afe_dai_data *dai_data = kcontrol->private_data; > + int value = ucontrol->value.integer.value[0]; > + > + dai_data->port_config[AFE_PORT_HDMI_RX].hdmi.datatype = value; > + > + return 0; > +} No validation, and do we not need to tell a currently running stream if the format changed on it (or block such changes if they're not going to work, which seems more likely)? > +static int q6hdmi_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params, > + struct snd_soc_dai *dai) > +{ > + struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev); > + int channels = params_channels(params); > + struct q6afe_hdmi_cfg *hdmi = &dai_data->port_config[dai->id].hdmi; > + > + hdmi->sample_rate = params_rate(params); > + switch (params_format(params)) { > + case SNDRV_PCM_FORMAT_S16_LE: > + hdmi->bit_width = 16; > + break; > + case SNDRV_PCM_FORMAT_S24_LE: > + hdmi->bit_width = 24; > + break; > + } This silently accepts invalid values. > + /*refer to HDMI spec CEA-861-E: Table 28 Audio InfoFrame Data Byte 4*/ Coding style, spaces around the /* */. > +static int q6afe_dai_startup(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev); > + > + dai_data->is_port_started[dai->id] = false; > + > + return 0; > +} If this is needed it makes me a bit worried that we've got some kind of bug with not shutting things down properly somewhere - what's going on here? > +static void q6afe_dai_shutdown(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev); > + int rc; > + > + rc = q6afe_port_stop(dai_data->port[dai->id]); > + if (rc < 0) > + dev_err(dai->dev, "fail to close AFE port\n"); Better to print the error code so users have more information to debug the problem. > + .stream_name = "HDMI Playback", > + .rates = SNDRV_PCM_RATE_48000 | > + SNDRV_PCM_RATE_96000 | > + SNDRV_PCM_RATE_192000, Indentation again. > +static int q6afe_of_xlate_dai_name(struct snd_soc_component *component, > + struct of_phandle_args *args, > + const char **dai_name) > +{ > + int id = args->args[0]; > + int i, ret = -EINVAL; Coding style, don't mix initialization in with other variable declarations on the same line like this. > +int q6afe_dai_dev_remove(struct device *dev) > +{ > + return 0; > +} Remove empty functions, if they can't be removed it's probably not OK for them to be empty either. _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Thanks for the review comments, On 02/03/18 12:50, Mark Brown wrote: > On Tue, Feb 13, 2018 at 04:58:26PM +0000, srinivas.kandagatla@linaro.org wrote: > >> +static int q6hdmi_format_put(struct snd_kcontrol *kcontrol, >> + struct snd_ctl_elem_value *ucontrol) >> +{ >> + struct q6afe_dai_data *dai_data = kcontrol->private_data; >> + int value = ucontrol->value.integer.value[0]; >> + >> + dai_data->port_config[AFE_PORT_HDMI_RX].hdmi.datatype = value; >> + >> + return 0; >> +} > > No validation, and do we not need to tell a currently running stream if > the format changed on it (or block such changes if they're not going to > work, which seems more likely)? Yes, It would not work if the stream is running. This mixer has to be setup before the stream/port is prepared/started. TBH, I have no means to test Compr format, I should probably remove this control until am able to test this format. > >> +static int q6hdmi_hw_params(struct snd_pcm_substream *substream, >> + struct snd_pcm_hw_params *params, >> + struct snd_soc_dai *dai) >> +{ >> + struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev); >> + int channels = params_channels(params); >> + struct q6afe_hdmi_cfg *hdmi = &dai_data->port_config[dai->id].hdmi; >> + >> + hdmi->sample_rate = params_rate(params); >> + switch (params_format(params)) { >> + case SNDRV_PCM_FORMAT_S16_LE: >> + hdmi->bit_width = 16; >> + break; >> + case SNDRV_PCM_FORMAT_S24_LE: >> + hdmi->bit_width = 24; >> + break; >> + } > > This silently accepts invalid values. > Yep, I will fix this in next version. >> + /*refer to HDMI spec CEA-861-E: Table 28 Audio InfoFrame Data Byte 4*/ > > Coding style, spaces around the /* */. Agreed! Will fix it in next version. > >> +static int q6afe_dai_startup(struct snd_pcm_substream *substream, >> + struct snd_soc_dai *dai) >> +{ >> + struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev); >> + >> + dai_data->is_port_started[dai->id] = false; >> + >> + return 0; >> +} > > If this is needed it makes me a bit worried that we've got some kind of > bug with not shutting things down properly somewhere - what's going on > here? This looks over done, we do not need to set this flag in startup, as it would be properly reset in shutdown. Will remove this function totally as its not required. > >> +static void q6afe_dai_shutdown(struct snd_pcm_substream *substream, >> + struct snd_soc_dai *dai) >> +{ >> + struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev); >> + int rc; >> + >> + rc = q6afe_port_stop(dai_data->port[dai->id]); >> + if (rc < 0) >> + dev_err(dai->dev, "fail to close AFE port\n"); > > Better to print the error code so users have more information to debug > the problem. Yep. > >> + .stream_name = "HDMI Playback", >> + .rates = SNDRV_PCM_RATE_48000 | >> + SNDRV_PCM_RATE_96000 | >> + SNDRV_PCM_RATE_192000, > > Indentation again. Will sort it out in next version. > >> +static int q6afe_of_xlate_dai_name(struct snd_soc_component *component, >> + struct of_phandle_args *args, >> + const char **dai_name) >> +{ >> + int id = args->args[0]; >> + int i, ret = -EINVAL; > > Coding style, don't mix initialization in with other variable > declarations on the same line like this. Will fix all such instances in next version. > >> +int q6afe_dai_dev_remove(struct device *dev) >> +{ >> + return 0; >> +} > > Remove empty functions, if they can't be removed it's probably not OK > for them to be empty either. Sure will do that. > thanks, srini _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Fri, Mar 02, 2018 at 01:13:17PM +0000, Srinivas Kandagatla wrote: > On 01/03/18 20:59, Mark Brown wrote: > > On Tue, Feb 13, 2018 at 04:58:17PM +0000, srinivas.kandagatla@linaro.org wrote: > > > +static struct afe_port_map port_maps[AFE_PORT_MAX] = { > > > + [AFE_PORT_HDMI_RX] = { AFE_PORT_ID_MULTICHAN_HDMI_RX, > > > + AFE_PORT_HDMI_RX, 1, 1}, > > > +}; > > Is this not device specific in any way? It looks likely to be. > It is specific to Audio firmware build. > AFAIK, DSP port IDs are consistent across a given range of AVS firmware > builds. So far I have seen them not change in any of the B family SoCs. > However on older A family SOCs these are very different numbers. Which is > where ADSP version info would help select correct map. Can we have some documentation of this in the code please? > > > +static struct q6afe_port *afe_find_port(struct q6afe *afe, int token) > > > +{ > > > + struct q6afe_port *p = NULL; > > > + > > > + spin_lock(&afe->port_list_lock); > > > + list_for_each_entry(p, &afe->port_list, node) > > > + if (p->token == token) > > > + break; > > > + > > > + spin_unlock(&afe->port_list_lock); > > Why do we need to lock the port list, what are we protecting it against? > This is just to protect the list from anyone deleting this. > Its very rare but the use case could be somelike the adsp is up and we are > in the middle of finding a port and then adsp went down or crashed we would > delete an entry in the list. If that's what we're protecting against then this also needs to do something to ensure that the port we looked up doesn't get deallocated before or while we look at it. > > > +int q6afe_port_start(struct q6afe_port *port) > > > +{ > > > + return afe_port_start(port, &port->port_cfg); > > > +} > > > +EXPORT_SYMBOL_GPL(q6afe_port_start); > > This is the third level of wrapper for the port start command in this > > file. Do we *really* need all these wrappers? > Intention here is that we have plans to support different version of ADSP, > on A family this command is different, so having this wrapper would help > tackle this use-case. Why not just take out the level of wrapper for now then add it in when there's actually an abstraction in there? The code might end up looking different anyway. _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Thanks for the review comments, On 02/03/18 17:54, Mark Brown wrote: > On Fri, Mar 02, 2018 at 01:13:17PM +0000, Srinivas Kandagatla wrote: >> On 01/03/18 20:59, Mark Brown wrote: >>> On Tue, Feb 13, 2018 at 04:58:17PM +0000, srinivas.kandagatla@linaro.org wrote: > >>>> +static struct afe_port_map port_maps[AFE_PORT_MAX] = { >>>> + [AFE_PORT_HDMI_RX] = { AFE_PORT_ID_MULTICHAN_HDMI_RX, >>>> + AFE_PORT_HDMI_RX, 1, 1}, >>>> +}; > >>> Is this not device specific in any way? It looks likely to be. > >> It is specific to Audio firmware build. >> AFAIK, DSP port IDs are consistent across a given range of AVS firmware >> builds. So far I have seen them not change in any of the B family SoCs. >> However on older A family SOCs these are very different numbers. Which is >> where ADSP version info would help select correct map. > > Can we have some documentation of this in the code please? > Sure, I will add documentation in next version. >>>> +static struct q6afe_port *afe_find_port(struct q6afe *afe, int token) >>>> +{ >>>> + struct q6afe_port *p = NULL; >>>> + >>>> + spin_lock(&afe->port_list_lock); >>>> + list_for_each_entry(p, &afe->port_list, node) >>>> + if (p->token == token) >>>> + break; >>>> + >>>> + spin_unlock(&afe->port_list_lock); > >>> Why do we need to lock the port list, what are we protecting it against? > >> This is just to protect the list from anyone deleting this. > >> Its very rare but the use case could be somelike the adsp is up and we are >> in the middle of finding a port and then adsp went down or crashed we would >> delete an entry in the list. > > If that's what we're protecting against then this also needs to do > something to ensure that the port we looked up doesn't get deallocated > before or while we look at it. Yes, I will take closer look at all possible paths before sending next version. > >>>> +int q6afe_port_start(struct q6afe_port *port) >>>> +{ >>>> + return afe_port_start(port, &port->port_cfg); >>>> +} >>>> +EXPORT_SYMBOL_GPL(q6afe_port_start); > >>> This is the third level of wrapper for the port start command in this >>> file. Do we *really* need all these wrappers? > >> Intention here is that we have plans to support different version of ADSP, >> on A family this command is different, so having this wrapper would help >> tackle this use-case. > > Why not just take out the level of wrapper for now then add it in when > there's actually an abstraction in there? The code might end up looking > different anyway. Okay, I can do that, will remove extra abstraction layer. thanks, srini > _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Thanks for the review, On 01/03/18 21:28, Mark Brown wrote: > On Tue, Feb 13, 2018 at 04:58:22PM +0000, srinivas.kandagatla@linaro.org wrote: > >> + num_regions = is_contiguous ? 1 : periods; >> + buf_sz = is_contiguous ? (period_sz * periods) : period_sz; > > Please write normal if statements, it's much easier to read. > Sure, will fix it in next version. >> + buf_sz = PAGE_ALIGN(buf_sz); > > I don't understand what this is doing, buf_sz is a length not an address > so why are we attempting to align it? Yes, this is a requirement form the DSP side that the size is multiple of 4KB. I will fix this properly in next version. thanks, srini > _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> This patchset aims to provide a basic version of QCOM DSP based audio support which is available in downstream andriod kernels. This patchset support audio playback on HDMI-RX, MI2S, SLIMBus and will add support to other features as we move on. QDSP has both static and dynamic modules. static modules like AFE (Audio FrontEnd), ADM (Audio Device Manager), ASM(Audio Stream Manager) and CORE to provide this audio services. All these services use APR (Asynchronous Packet Router) protocol via smd/glink transport to communicate with Application processor. More details on each module is availble in there respective patch. This patchset is tested on DB820c, with HDMI audio playback, MI2S on DB410c on top of mainline, Also tested SLIMBus analog audio using wcd9355 with an additional patches. Here is my test branch incase somone want to try these patches https://git.linaro.org/people/srinivas.kandagatla/linux.git/log/?h=v4.16-rc1-dsp-audio-v3 Here is block diagram to give a quick overview of the components +---------+ +---------+ +---------+ | q6asm | |q6routing| | q6afe | | fedai | <------> | mixers | <-----> | bedai | +---------+ +---------+ +---------+ ^ ^ ^ | | | | +------------------+----------------+ | | | | | | v v v v v +---------+ +---------+ +---------+ | q6ASM | | q6ADM | | q6AFE | +---------+ +---------+ +---------+ ^ ^ ^ ^ | | | CPU Side | ------+---------------------+-------------------+-------- | | | | |APR(smd/glink) | | | | | +------------------+----------------+ | | | | | | +-----+--+-----------------------------------+--+------- | | | | | QDSP Side | v v v v v v +---------+ +---------+ +---------+ | ASM | <------> | ADM | <-----> | AFE | +---------+ +---------+ +---------+ ^ | +-------------------+ | ---------------------------+-------------------------- | Audio I/O | v v +--------------------------------------------------+ | Audio devices | | CODEC | HDMI-TX | PCM | SLIMBUS | I2S |MI2S |...| | | +--------------------------------------------------+ Changes since v2 (https://lwn.net/Articles/741472/) - fixed locking issues with ASM and APR. - Added support to MI2S and SLIMBus ports - moved dma device to apr. - added dt bindings for ASM, ADM, AFE. - Fixed dt bindings for msm8996 sound card. - added compatible strings to ASM, ADM, AFE modules. - Cleaned up common code as suggested in review. - converted sound card driver to proper apr client driver. - Fixed various issues spotted by Banajit and Kasam. - Tested on DB410c, DB820c for both analog and digital playbacks. - Exported q6core functions as suggested by Rohit. - Fixed various issues with dsp carsh/recover usecase. - Added multiple ASM streams as requested in review. Srinivas Kandagatla (25): dt-bindings: soc: qcom: Add bindings for APR bus soc: qcom: add support to APR bus driver ASoC: qcom: qdsp6: Add common qdsp6 helper functions dt-bindings: sound: qcom: Add bindings for q6afe ASoC: qcom: qdsp6: Add support to Q6AFE dt-bindings: sound: qcom: Add bindings for q6adm ASoC: qcom: qdsp6: Add support to Q6ADM dt-bindings: sound: qcom: Add bindings for q6asm ASoC: qcom: qdsp6: Add support to Q6ASM ASoC: qcom: q6asm: Add support to memory map and unmap ASoC: qcom: q6asm: add support to audio stream apis ASoC: qcom: qdsp6: Add support to Q6CORE ASoC: qcom: qdsp6: Add support to q6routing driver ASoC: qcom: qdsp6: Add support to q6afe dai driver ASoC: qcom: qdsp6: Add support to q6asm dai driver ASoC: qcom: q6afe: add SLIMBus port Support ASoC: qcom: q6afe-dai: add support to slim afe dais ASoC: qcom: q6routing: add support to all SLIMBus Mixers ASoC: qcom: q6afe: add support to MI2S ports ASoC: qcom: q6afe: add support to MI2S sysclks ASoC: qcom: q6afe-dai: add support to 4 MI2S ports ASoC: qcom: q6routing: add support to MI2S Mixers dt-bindings: sound: qcom: Add devicetree bindings for apq8096 ASoC: qcom: apq8096: Add db820c machine driver arm64: dts: msm8996: db820c: Add sound card support .../devicetree/bindings/soc/qcom/qcom,apr.txt | 83 ++ .../devicetree/bindings/sound/qcom,apq8096.txt | 89 ++ .../devicetree/bindings/sound/qcom,q6adm.txt | 31 + .../devicetree/bindings/sound/qcom,q6afe.txt | 38 + .../devicetree/bindings/sound/qcom,q6asm.txt | 38 + arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 44 +- arch/arm64/boot/dts/qcom/msm8996.dtsi | 62 ++ drivers/soc/qcom/Kconfig | 9 + drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/apr.c | 381 +++++++ include/dt-bindings/soc/qcom,apr.h | 27 + include/dt-bindings/sound/qcom,q6afe.h | 33 + include/dt-bindings/sound/qcom,q6asm.h | 22 + include/linux/mod_devicetable.h | 11 + include/linux/soc/qcom/apr.h | 131 +++ sound/soc/qcom/Kconfig | 42 + sound/soc/qcom/Makefile | 5 + sound/soc/qcom/apq8096.c | 173 ++++ sound/soc/qcom/qdsp6/Makefile | 5 + sound/soc/qcom/qdsp6/q6adm.c | 634 ++++++++++++ sound/soc/qcom/qdsp6/q6adm.h | 29 + sound/soc/qcom/qdsp6/q6afe-dai.c | 645 ++++++++++++ sound/soc/qcom/qdsp6/q6afe.c | 875 ++++++++++++++++ sound/soc/qcom/qdsp6/q6afe.h | 74 ++ sound/soc/qcom/qdsp6/q6asm-dai.c | 621 ++++++++++++ sound/soc/qcom/qdsp6/q6asm.c | 1063 ++++++++++++++++++++ sound/soc/qcom/qdsp6/q6asm.h | 66 ++ sound/soc/qcom/qdsp6/q6core.c | 235 +++++ sound/soc/qcom/qdsp6/q6core.h | 9 + sound/soc/qcom/qdsp6/q6dsp-common.c | 67 ++ sound/soc/qcom/qdsp6/q6dsp-common.h | 24 + sound/soc/qcom/qdsp6/q6dsp-errno.h | 97 ++ sound/soc/qcom/qdsp6/q6routing.c | 758 ++++++++++++++ sound/soc/qcom/qdsp6/q6routing.h | 9 + 34 files changed, 6430 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,apr.txt create mode 100644 Documentation/devicetree/bindings/sound/qcom,apq8096.txt create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6adm.txt create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6afe.txt create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6asm.txt create mode 100644 drivers/soc/qcom/apr.c create mode 100644 include/dt-bindings/soc/qcom,apr.h create mode 100644 include/dt-bindings/sound/qcom,q6afe.h create mode 100644 include/dt-bindings/sound/qcom,q6asm.h create mode 100644 include/linux/soc/qcom/apr.h create mode 100644 sound/soc/qcom/apq8096.c create mode 100644 sound/soc/qcom/qdsp6/Makefile create mode 100644 sound/soc/qcom/qdsp6/q6adm.c create mode 100644 sound/soc/qcom/qdsp6/q6adm.h create mode 100644 sound/soc/qcom/qdsp6/q6afe-dai.c create mode 100644 sound/soc/qcom/qdsp6/q6afe.c create mode 100644 sound/soc/qcom/qdsp6/q6afe.h create mode 100644 sound/soc/qcom/qdsp6/q6asm-dai.c create mode 100644 sound/soc/qcom/qdsp6/q6asm.c create mode 100644 sound/soc/qcom/qdsp6/q6asm.h create mode 100644 sound/soc/qcom/qdsp6/q6core.c create mode 100644 sound/soc/qcom/qdsp6/q6core.h create mode 100644 sound/soc/qcom/qdsp6/q6dsp-common.c create mode 100644 sound/soc/qcom/qdsp6/q6dsp-common.h create mode 100644 sound/soc/qcom/qdsp6/q6dsp-errno.h create mode 100644 sound/soc/qcom/qdsp6/q6routing.c create mode 100644 sound/soc/qcom/qdsp6/q6routing.h -- 2.15.1 _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel