| Message ID | 20171214173402.19074-1-srinivas.kandagatla@linaro.org |
|---|---|
| Headers | show |
| Series | ASoC: qcom: Add support to QDSP6 based audio | expand |
On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla@linaro.org wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > This patch adds support to memory map and unmap regions commands in > q6asm module. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > sound/soc/qcom/qdsp6/q6asm.c | 343 ++++++++++++++++++++++++++++++++++++++++++- > sound/soc/qcom/qdsp6/q6asm.h | 5 + > 2 files changed, 347 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/qcom/qdsp6/q6asm.c b/sound/soc/qcom/qdsp6/q6asm.c > index 9cc583afef4d..4be92441f524 100644 > --- a/sound/soc/qcom/qdsp6/q6asm.c > +++ b/sound/soc/qcom/qdsp6/q6asm.c > @@ -14,9 +14,46 @@ > #include "q6asm.h" > #include "common.h" > > +#define ASM_CMD_SHARED_MEM_MAP_REGIONS 0x00010D92 > +#define ASM_CMDRSP_SHARED_MEM_MAP_REGIONS 0x00010D93 > +#define ASM_CMD_SHARED_MEM_UNMAP_REGIONS 0x00010D94 > + > #define TUN_READ_IO_MODE 0x0004 /* tunnel read write mode */ > #define SYNC_IO_MODE 0x0001 > #define ASYNC_IO_MODE 0x0002 > +#define ASM_SHIFT_GAPLESS_MODE_FLAG 31 > +#define ADSP_MEMORY_MAP_SHMEM8_4K_POOL 3 > + > +struct avs_cmd_shared_mem_map_regions { > + struct apr_hdr hdr; > + u16 mem_pool_id; > + u16 num_regions; > + u32 property_flag; > +} __packed; > + > +struct avs_shared_map_region_payload { > + u32 shm_addr_lsw; > + u32 shm_addr_msw; > + u32 mem_size_bytes; > +} __packed; > + > +struct avs_cmd_shared_mem_unmap_regions { > + struct apr_hdr hdr; > + u32 mem_map_handle; > +} __packed; > + > +struct audio_buffer { > + dma_addr_t phys; > + uint32_t used; > + uint32_t size; /* size of buffer */ > +}; > + > +struct audio_port_data { > + struct audio_buffer *buf; > + uint32_t max_buf_cnt; This seems to denote the number of audio_buffers in the buf array, so I'm not sure about the meaning of "max_". > + uint32_t dsp_buf; > + uint32_t mem_map_handle; > +}; > > struct audio_client { > int session; > @@ -27,6 +64,8 @@ struct audio_client { > uint64_t time_stamp; > struct apr_device *adev; > struct mutex cmd_lock; > + /* idx:1 out port, 0: in port */ > + struct audio_port_data port[2]; > wait_queue_head_t cmd_wait; > int perf_mode; > int stream_id; > @@ -86,6 +125,260 @@ static void q6asm_session_free(struct audio_client *ac) > mutex_unlock(&a->session_lock); > } > > +static inline void q6asm_add_mmaphdr(struct audio_client *ac, > + struct apr_hdr *hdr, u32 pkt_size, > + bool cmd_flg, u32 token) cmd_flg is true in both callers, so this function ends up simply assigning hdr_field, pkt_size and token. Inlining these assignments would make for cleaner call sites as well. > +{ > + hdr->hdr_field = APR_SEQ_CMD_HDR_FIELD; > + hdr->src_port = 0; > + hdr->dest_port = 0; > + hdr->pkt_size = pkt_size; > + if (cmd_flg) > + hdr->token = token; > +} > + > +static inline void q6asm_add_hdr(struct audio_client *ac, struct apr_hdr *hdr, This is unused. > + uint32_t pkt_size, bool cmd_flg, > + uint32_t stream_id) > +{ > + hdr->hdr_field = APR_SEQ_CMD_HDR_FIELD; > + hdr->src_svc = ac->adev->svc_id; > + hdr->src_domain = APR_DOMAIN_APPS; > + hdr->dest_svc = APR_SVC_ASM; > + hdr->dest_domain = APR_DOMAIN_ADSP; > + hdr->src_port = ((ac->session << 8) & 0xFF00) | (stream_id); > + hdr->dest_port = ((ac->session << 8) & 0xFF00) | (stream_id); > + hdr->pkt_size = pkt_size; > + if (cmd_flg) > + hdr->token = ac->session; > +} > + > +static int __q6asm_memory_unmap(struct audio_client *ac, > + phys_addr_t buf_add, int dir) > +{ > + struct avs_cmd_shared_mem_unmap_regions mem_unmap; If you name this "cmd" you will declutter below code a bit. > + struct q6asm *a = dev_get_drvdata(ac->dev->parent); > + int rc; > + > + if (!a) > + return -ENODEV; Does this NULL check add any real value? > + > + q6asm_add_mmaphdr(ac, &mem_unmap.hdr, sizeof(mem_unmap), true, > + ((ac->session << 8) | dir)); > + a->mem_state = -1; > + > + mem_unmap.hdr.opcode = ASM_CMD_SHARED_MEM_UNMAP_REGIONS; > + mem_unmap.mem_map_handle = ac->port[dir].mem_map_handle; > + > + if (mem_unmap.mem_map_handle == 0) { Start the function by checking for !ac->port[dir].mem_map_handle > + dev_err(ac->dev, "invalid mem handle\n"); > + return -EINVAL; > + } > + > + rc = apr_send_pkt(a->adev, (uint32_t *) &mem_unmap); > + if (rc < 0) > + return rc; > + > + rc = wait_event_timeout(a->mem_wait, (a->mem_state >= 0), > + 5 * HZ); > + if (!rc) { > + dev_err(ac->dev, "CMD timeout for memory_unmap 0x%x\n", > + mem_unmap.mem_map_handle); > + return -ETIMEDOUT; > + } else if (a->mem_state > 0) { > + return adsp_err_get_lnx_err_code(a->mem_state); > + } > + ac->port[dir].mem_map_handle = 0; Does all errors indicate that the region is left mapped? > + > + return 0; > +} > + > +/** > + * q6asm_unmap_memory_regions() - unmap memory regions in the dsp. > + * > + * @dir: direction of audio stream > + * @ac: audio client instanace > + * > + * Return: Will be an negative value on failure or zero on success > + */ > +int q6asm_unmap_memory_regions(unsigned int dir, struct audio_client *ac) > +{ > + struct audio_port_data *port; > + int cnt = 0; > + int rc = 0; > + > + mutex_lock(&ac->cmd_lock); > + port = &ac->port[dir]; > + if (!port->buf) { > + mutex_unlock(&ac->cmd_lock); > + return 0; Put a label right before the mutex_unlock below and return rc instead of 0, then you can replace these two lines with "goto unlock" > + } > + cnt = port->max_buf_cnt - 1; What if we mapped 1 period? Why shouldn't we enter the unmap path? > + if (cnt >= 0) { > + rc = __q6asm_memory_unmap(ac, port->buf[dir].phys, dir); > + if (rc < 0) { > + dev_err(ac->dev, "%s: Memory_unmap_regions failed %d\n", > + __func__, rc); Most of the code paths through __q6asm_memory_unmap() will print an error, make this consistent and print the warning once. > + mutex_unlock(&ac->cmd_lock); > + return rc; Same here. > + } > + } > + > + port->max_buf_cnt = 0; > + kfree(port->buf); > + port->buf = NULL; > + mutex_unlock(&ac->cmd_lock); I think however that if you rearrange this function somewhat you can start off by doing: mutex_lock(&ac->cmd_lock); port = &ac->port[dir]; bufs = port->buf; cnt = port->max_buf_cnt; port->max_buf_cnt = 0; port->buf = NULL; mutex_unlock(&ac->cmd_lock); And then you can do the rest without locks. > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(q6asm_unmap_memory_regions); > + > +static int __q6asm_memory_map_regions(struct audio_client *ac, int dir, > + uint32_t period_sz, uint32_t periods, period_sz is typical size_t material. > + bool is_contiguous) > +{ > + struct avs_cmd_shared_mem_map_regions *mmap_regions = NULL; Calling this "cmd" would declutter the function. > + struct avs_shared_map_region_payload *mregions = NULL; > + struct q6asm *a = dev_get_drvdata(ac->dev->parent); > + struct audio_port_data *port = NULL; > + struct audio_buffer *ab = NULL; > + void *mmap_region_cmd = NULL; No need to initialize this. Also, this really is a avs_cmd_shared_mem_map_regions with some extra data at the end, not a void *. > + void *payload = NULL; > + int rc = 0; > + int i = 0; > + int cmd_size = 0; Most of these can be left uninitialized. > + uint32_t num_regions; > + uint32_t buf_sz; > + > + if (!a) > + return -ENODEV; > + num_regions = is_contiguous ? 1 : periods; > + buf_sz = is_contiguous ? (period_sz * periods) : period_sz; > + buf_sz = PAGE_ALIGN(buf_sz); > + > + cmd_size = sizeof(*mmap_regions) + (sizeof(*mregions) * num_regions); > + > + mmap_region_cmd = kzalloc(cmd_size, GFP_KERNEL); > + if (!mmap_region_cmd) > + return -ENOMEM; > + > + mmap_regions = (struct avs_cmd_shared_mem_map_regions *)mmap_region_cmd; > + q6asm_add_mmaphdr(ac, &mmap_regions->hdr, cmd_size, true, > + ((ac->session << 8) | dir)); > + a->mem_state = -1; > + > + mmap_regions->hdr.opcode = ASM_CMD_SHARED_MEM_MAP_REGIONS; > + mmap_regions->mem_pool_id = ADSP_MEMORY_MAP_SHMEM8_4K_POOL; > + mmap_regions->num_regions = num_regions; > + mmap_regions->property_flag = 0x00; > + > + payload = ((u8 *) mmap_region_cmd + > + sizeof(struct avs_cmd_shared_mem_map_regions)); mmap_region_cmd is void *, so no need to type cast. > + > + mregions = (struct avs_shared_map_region_payload *)payload; Payload is void *, so no need to type cast. But there's also no benefit of having "payload", as this line can be written as: mregions = mmap_region_cmd + sizeof(*mmap_regions); But adding a flexible array member to the avs_cmd_shared_mem_map_regions struct would make things even clearer, in particular you would be able to read the struct definition and see that there's a payload following the command. > + > + ac->port[dir].mem_map_handle = 0; Isn't this already 0? > + port = &ac->port[dir]; > + > + for (i = 0; i < num_regions; i++) { > + ab = &port->buf[i]; > + mregions->shm_addr_lsw = lower_32_bits(ab->phys); > + mregions->shm_addr_msw = upper_32_bits(ab->phys); > + mregions->mem_size_bytes = buf_sz; Here you're dereferencing port->buf without holding cmd_lock. > + ++mregions; > + } > + > + rc = apr_send_pkt(a->adev, (uint32_t *) mmap_region_cmd); > + if (rc < 0) > + goto fail_cmd; > + > + rc = wait_event_timeout(a->mem_wait, (a->mem_state >= 0), > + 5 * HZ); > + if (!rc) { > + dev_err(ac->dev, "timeout. waited for memory_map\n"); > + rc = -ETIMEDOUT; > + goto fail_cmd; > + } > + > + if (a->mem_state > 0) { > + rc = adsp_err_get_lnx_err_code(a->mem_state); > + goto fail_cmd; > + } > + rc = 0; > +fail_cmd: > + kfree(mmap_region_cmd); > + return rc; > +} > + > +/** > + * q6asm_map_memory_regions() - map memory regions in the dsp. > + * > + * @dir: direction of audio stream This sounds boolean, perhaps worth mentioning here if true means rx or tx. And it's idiomatic to have such a parameter later in the list, it would probably be more natural to read the call sight if the order was: q6asm_map_memory_regions(ac, phys, periods, size, true); > + * @ac: audio client instanace > + * @phys: physcial address that needs mapping. > + * @period_sz: audio period size > + * @periods: number of periods > + * > + * Return: Will be an negative value on failure or zero on success > + */ > +int q6asm_map_memory_regions(unsigned int dir, struct audio_client *ac, > + dma_addr_t phys, > + unsigned int period_sz, unsigned int periods) period_sz could with benefit be of type size_t. > +{ > + struct audio_buffer *buf; > + int cnt; > + int rc; > + > + if (ac->port[dir].buf) { > + dev_err(ac->dev, "Buffer already allocated\n"); > + return 0; > + } > + > + mutex_lock(&ac->cmd_lock); I believe this lock should cover above check. > + > + buf = kzalloc(((sizeof(struct audio_buffer)) * periods), GFP_KERNEL); Loose a few of those parenthesis and use *buf in the sizeof. > + if (!buf) { > + mutex_unlock(&ac->cmd_lock); > + return -ENOMEM; > + } > + > + > + ac->port[dir].buf = buf; > + > + buf[0].phys = phys; > + buf[0].used = dir ^ 1; Why would this be called "used", and it's probably cleaner to just use !!dir. > + buf[0].size = period_sz; > + cnt = 1; > + while (cnt < periods) { cnt goes from 1 to periods and is incremented 1 each step, this would be more succinct as a for loop. > + if (period_sz > 0) { > + buf[cnt].phys = buf[0].phys + (cnt * period_sz); > + buf[cnt].used = dir ^ 1; > + buf[cnt].size = period_sz; > + } > + cnt++; > + } > + > + ac->port[dir].max_buf_cnt = periods; > + mutex_unlock(&ac->cmd_lock); The only possible purpose of taking cmd_lock here is to protect ac->port[dir].buf, but > + > + rc = __q6asm_memory_map_regions(ac, dir, period_sz, periods, 1); The last parameter should be "true". > + if (rc < 0) { > + dev_err(ac->dev, > + "CMD Memory_map_regions failed %d for size %d\n", rc, > + period_sz); > + > + > + ac->port[dir].max_buf_cnt = 0; > + kfree(buf); > + ac->port[dir].buf = NULL; These operations are done without holding cmd_lock. > + > + return rc; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(q6asm_map_memory_regions); > + > /** > * q6asm_audio_client_free() - Freee allocated audio client > * > @@ -117,8 +410,10 @@ static struct audio_client *q6asm_get_audio_client(struct q6asm *a, > > static int q6asm_srvc_callback(struct apr_device *adev, struct apr_client_data *data) > { > - struct q6asm *q6asm = dev_get_drvdata(&adev->dev); > + struct q6asm *a, *q6asm = dev_get_drvdata(&adev->dev); > struct audio_client *ac = NULL; > + struct audio_port_data *port; > + uint32_t dir = 0; > uint32_t sid = 0; > uint32_t *payload; > > @@ -135,6 +430,52 @@ static int q6asm_srvc_callback(struct apr_device *adev, struct apr_client_data * > return 0; > } > > + a = dev_get_drvdata(ac->dev->parent); > + if (data->opcode == APR_BASIC_RSP_RESULT) { This is a case in below switch statement. > + switch (payload[0]) { > + case ASM_CMD_SHARED_MEM_MAP_REGIONS: > + case ASM_CMD_SHARED_MEM_UNMAP_REGIONS: > + if (payload[1] != 0) { > + dev_err(ac->dev, > + "cmd = 0x%x returned error = 0x%x sid:%d\n", > + payload[0], payload[1], sid); > + a->mem_state = payload[1]; > + } else { > + a->mem_state = 0; Just assign a->mem_state = payload[1] outside the conditional, as it will be the same value. > + } > + > + wake_up(&a->mem_wait); > + break; > + default: > + dev_err(&adev->dev, "command[0x%x] not expecting rsp\n", > + payload[0]); > + break; > + } > + return 0; > + } Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla@linaro.org wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > This patch adds support to core apr service, which is used to query > status of other static and dynamic services on the dsp. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > sound/soc/qcom/Kconfig | 5 + > sound/soc/qcom/qdsp6/Makefile | 1 + > sound/soc/qcom/qdsp6/q6core.c | 227 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 233 insertions(+) > create mode 100644 sound/soc/qcom/qdsp6/q6core.c > > diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig > index 7ebdb879a8a3..121b9c957024 100644 > --- a/sound/soc/qcom/Kconfig > +++ b/sound/soc/qcom/Kconfig > @@ -56,11 +56,16 @@ config SND_SOC_QDSP6_ASM > tristate > default n > > +config SND_SOC_QDSP6_CORE > + tristate > + default n > + > config SND_SOC_QDSP6 > tristate "SoC ALSA audio driver for QDSP6" > select SND_SOC_QDSP6_AFE > select SND_SOC_QDSP6_ADM > select SND_SOC_QDSP6_ASM > + select SND_SOC_QDSP6_CORE > help > To add support for MSM QDSP6 Soc Audio. > This will enable sound soc platform specific > diff --git a/sound/soc/qcom/qdsp6/Makefile b/sound/soc/qcom/qdsp6/Makefile > index 49dd3ccab27b..ad7f10691e54 100644 > --- a/sound/soc/qcom/qdsp6/Makefile > +++ b/sound/soc/qcom/qdsp6/Makefile > @@ -1,3 +1,4 @@ > obj-$(CONFIG_SND_SOC_QDSP6_AFE) += q6afe.o > obj-$(CONFIG_SND_SOC_QDSP6_ADM) += q6adm.o > obj-$(CONFIG_SND_SOC_QDSP6_ASM) += q6asm.o > +obj-$(CONFIG_SND_SOC_QDSP6_CORE) += q6core.o > diff --git a/sound/soc/qcom/qdsp6/q6core.c b/sound/soc/qcom/qdsp6/q6core.c > new file mode 100644 > index 000000000000..d4e2dbc62489 > --- /dev/null > +++ b/sound/soc/qcom/qdsp6/q6core.c > @@ -0,0 +1,227 @@ > +/* SPDX-License-Identifier: GPL-2.0 > +* Copyright (c) 2017, Linaro Limited > +*/ > +#include <linux/slab.h> > +#include <linux/wait.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/sched.h> > +#include <linux/jiffies.h> > +#include <linux/wait.h> > +#include <linux/soc/qcom/apr.h> > +#include <linux/platform_device.h> > +#include <sound/asound.h> > +#include "common.h" > + > +#define ADSP_STATE_READY_TIMEOUT_MS 3000 > +#define Q6_READY_TIMEOUT_MS 100 > +#define AVCS_CMD_ADSP_EVENT_GET_STATE 0x0001290C > +#define AVCS_CMDRSP_ADSP_EVENT_GET_STATE 0x0001290D > +#define AVCS_GET_VERSIONS 0x00012905 > +#define AVCS_GET_VERSIONS_RSP 0x00012906 > + > +struct avcs_svc_info { > + uint32_t service_id; > + uint32_t version; > +} __packed; > + > +struct q6core { > + struct apr_device *adev; > + wait_queue_head_t wait; > + uint32_t avcs_state; > + int resp_received; bool > + uint32_t num_services; > + struct avcs_svc_info *svcs_info; > +}; > + > +static struct apr_device_id static_services[] = { > + ADSP_AUDIO_APR_DEV("AFE", APR_SVC_AFE), > + ADSP_AUDIO_APR_DEV("ASM", APR_SVC_ASM), > + ADSP_AUDIO_APR_DEV("ADM", APR_SVC_ADM), > + ADSP_AUDIO_APR_DEV("TEST", APR_SVC_TEST_CLIENT), > + ADSP_AUDIO_APR_DEV("MVM", APR_SVC_ADSP_MVM), > + ADSP_AUDIO_APR_DEV("CVS", APR_SVC_ADSP_CVS), > + ADSP_AUDIO_APR_DEV("CVP", APR_SVC_ADSP_CVP), > + ADSP_AUDIO_APR_DEV("USM", APR_SVC_USM), > + ADSP_AUDIO_APR_DEV("VIDC", APR_SVC_VIDC), > + ADSP_AUDIO_APR_DEV("LSM", APR_SVC_LSM), > +}; > + > +static int core_callback(struct apr_device *adev, struct apr_client_data *data) > +{ > + struct q6core *core = dev_get_drvdata(&adev->dev); > + uint32_t *payload; > + > + switch (data->opcode) { > + case AVCS_GET_VERSIONS_RSP: > + payload = data->payload; > + core->num_services = payload[1]; Describe the payload in a struct (with flexible array member for the svcs_info list). > + > + if (!core->svcs_info) > + core->svcs_info = kcalloc(core->num_services, > + sizeof(*core->svcs_info), > + GFP_ATOMIC); > + if (!core->svcs_info) > + return -ENOMEM; > + If we receive this twice with different num_services for some reason the memcpy might trash the heap. But as this is the get_version response and we're only going to issue that once you should remove the check for !core->svcs_info above. And don't forget to free svcs_info once you have added your services. > + /* svc info is after 8 bytes */ > + memcpy(core->svcs_info, payload + 2, > + core->num_services * sizeof(*core->svcs_info)); > + > + core->resp_received = 1; > + wake_up(&core->wait); > + > + break; > + case AVCS_CMDRSP_ADSP_EVENT_GET_STATE: > + payload = data->payload; > + core->avcs_state = payload[0]; > + > + core->resp_received = 1; > + wake_up(&core->wait); > + break; > + default: > + dev_err(&adev->dev, "Message id from adsp core svc: 0x%x\n", > + data->opcode); > + break; > + } > + > + return 0; > +} > + > +void q6core_add_service(struct device *dev, uint32_t svc_id, uint32_t version) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(static_services); i++) { > + if (static_services[i].svc_id == svc_id) { > + static_services[i].svc_version = version; > + apr_add_device(dev->parent, &static_services[i]); > + dev_info(dev, Please don't spam the logs, dev_dbg() should be enough. And as apr_add_device() returns you can find the devices registered in /sys > + "Adding SVC: %s: id 0x%x API Ver 0x%x:0x%x\n", > + static_services[i].name, svc_id, > + APR_SVC_MAJOR_VERSION(version), > + APR_SVC_MINOR_VERSION(version)); > + } > + } > +} > + > +static void q6core_add_static_services(struct q6core *core) The name of this function is deceiving, it doesn't really add the static services. It adds devices for the services that we've been informed exists, by the other side - using the static list of services. Per the comment on a previous patch I don't think the "name" in apr_device_id provides any real value and in this case if forces you to perform a lookup using this table. If you drop the name, you can loop over the list of service ids returned from the remote and just register them with a hard coded domain id (based on apr instance?) and client_id. You don't need the lookup table. > +{ > + int i; > + struct apr_device *adev = core->adev; > + struct avcs_svc_info *svcs_info = core->svcs_info; > + > + for (i = 0; i < core->num_services; i++) > + q6core_add_service(&adev->dev, svcs_info[i].service_id, > + svcs_info[i].version); > +} > + > +static int q6core_get_svc_versions(struct q6core *core) > +{ > + struct apr_device *adev = core->adev; > + struct apr_hdr hdr = {0}; > + int rc; > + > + hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD, > + APR_HDR_LEN(APR_HDR_SIZE), APR_PKT_VER); > + hdr.pkt_size = APR_PKT_SIZE(APR_HDR_SIZE, 0); > + hdr.opcode = AVCS_GET_VERSIONS; > + > + rc = apr_send_pkt(adev, (uint32_t *)&hdr); > + if (rc < 0) > + return rc; > + > + rc = wait_event_timeout(core->wait, (core->resp_received == 1), > + msecs_to_jiffies(Q6_READY_TIMEOUT_MS)); The wait and resp_received could favourably be expressed as a completion instead, as all we care about is that this happened once. > + if (rc > 0 && core->resp_received) { > + core->resp_received = 0; > + return 0; > + } It wasn't obvious at first sight that this is the success case and the return rc below was the error case... > + > + return rc; And this will actually be 0 if core->resp_received has not become 1 at the timeout. > +} > + > +static bool q6core_is_adsp_ready(struct q6core *core) > +{ > + struct apr_device *adev = core->adev; > + struct apr_hdr hdr = {0}; > + int rc; > + > + hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD, > + APR_HDR_LEN(APR_HDR_SIZE), APR_PKT_VER); > + hdr.pkt_size = APR_PKT_SIZE(APR_HDR_SIZE, 0); > + hdr.opcode = AVCS_CMD_ADSP_EVENT_GET_STATE; > + > + rc = apr_send_pkt(adev, (uint32_t *)&hdr); > + if (rc < 0) > + return false; > + > + rc = wait_event_timeout(core->wait, (core->resp_received == 1), > + msecs_to_jiffies(Q6_READY_TIMEOUT_MS)); I think this too would be nicer to describe with a completion. Currently it's possible to ask is the dsp is ready, time out and ask again, just to receive the first ack and continue. The service request sleep might then wake up on this previous ack. If you describe this by two different completions for the two waits you avoid any such race conditions occurring. > + if (rc > 0 && core->resp_received) { > + core->resp_received = 0; > + if (core->avcs_state == 0x1) > + return true; > + } > + > + return false; > +} > + > +static int q6core_probe(struct apr_device *adev) > +{ > + struct q6core *core; > + unsigned long timeout = jiffies + > + msecs_to_jiffies(ADSP_STATE_READY_TIMEOUT_MS); > + int ret = 0; > + > + core = devm_kzalloc(&adev->dev, sizeof(*core), GFP_KERNEL); > + if (!core) > + return -ENOMEM; > + > + dev_set_drvdata(&adev->dev, core); > + > + core->adev = adev; > + init_waitqueue_head(&core->wait); > + > + do { > + if (!q6core_is_adsp_ready(core)) { > + dev_info(&adev->dev, "ADSP Audio isn't ready\n"); > + } else { > + dev_info(&adev->dev, "ADSP Audio is ready\n"); > + > + ret = q6core_get_svc_versions(core); > + if (!ret) > + q6core_add_static_services(core); > + > + break; > + } > + } while (time_after(timeout, jiffies)); This would be much better rewritten as: for (;;) { if (q6core_is_adsp_ready(core)) break; if (time_after(timeout, jiffies)) return -ETIMEDOUT; } ret = q6core_get_svc_versions(core); if (ret) return ret; q6core_add_static_services(core); > + > + return ret; > +} > + > +static int q6core_exit(struct apr_device *adev) > +{ > + return 0; > +} > + > +static const struct apr_device_id core_id[] = { > + {"Q6CORE", APR_DOMAIN_ADSP, APR_SVC_ADSP_CORE, APR_CLIENT_AUDIO}, > + { }, > +}; > + > +static struct apr_driver qcom_q6core_driver = { > + .probe = q6core_probe, > + .remove = q6core_exit, > + .callback = core_callback, > + .id_table = core_id, > + .driver = { > + .name = "qcom-q6core", > + }, Indentation. > +}; > + > +module_apr_driver(qcom_q6core_driver); > + > +MODULE_AUTHOR("Srinivas Kandagatla <srinivas.kandagatla@linaro.org"); > +MODULE_DESCRIPTION("q6 core"); > +MODULE_LICENSE("GPL v2"); > -- > 2.15.0 > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 14 Dec 09:34 PST 2017, srinivas.kandagatla@linaro.org wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > uThis patch adds support to DB820c machine driver. Drop 'u' and expand the message to claim that this is the machine driver for 8996, used by the db820c. [..] > +static struct snd_soc_dai_link msm8996_dai_links[] = { Are there any differences between the DAI links of apq8096 and msm8996? > + /* FrontEnd DAI Links */ > + { > + .name = "MultiMedia1 Playback", > + .stream_name = "MultiMedia1", > + .cpu_dai_name = "MM_DL1", > + .platform_name = "q6asm_dai", > + .dynamic = 1, > + .dpcm_playback = 1, > + > + .codec_dai_name = "snd-soc-dummy-dai", > + .codec_name = "snd-soc-dummy", > + }, > + /* Backend DAI Links */ > + { > + .name = "HDMI Playback", > + .stream_name = "q6afe_dai", > + .cpu_dai_name = "HDMI", > + .platform_name = "q6routing", > + .no_pcm = 1, > + .dpcm_playback = 1, > + .be_hw_params_fixup = msm8996_be_hw_params_fixup, > + .codec_dai_name = "i2s-hifi", > + .codec_name = "hdmi-audio-codec.0.auto", > + }, > +}; > + > +static int apq8096_sbc_parse_of(struct snd_soc_card *card) msm8996_parse_of() > +{ > + struct device *dev = card->dev; > + int ret; > + > + ret = snd_soc_of_parse_card_name(card, "qcom,model"); > + if (ret) > + dev_err(dev, "Error parsing card name: %d\n", ret); > + > + return ret; > +} > + > +static int msm_snd_apq8096_probe(struct platform_device *pdev) msm_snd_msm8996_probe()? > +{ > + int ret; > + struct snd_soc_card *card; > + > + card = devm_kzalloc(&pdev->dev, sizeof(*card), GFP_KERNEL); > + if (!card) > + return -ENOMEM; > + > + card->dev = &pdev->dev; > + > + ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); > + if (ret) > + return ret; > + > + card->dai_link = msm8996_dai_links; > + card->num_links = ARRAY_SIZE(msm8996_dai_links); > + > + ret = apq8096_sbc_parse_of(card); > + if (ret) { > + dev_err(&pdev->dev, "Error parsing OF data\n"); No need to print in both parse_of() and here. > + return ret; > + } > + > + ret = devm_snd_soc_register_card(&pdev->dev, card); > + if (ret) > + dev_err(&pdev->dev, "sound card register failed (%d)!\n", ret); > + else > + dev_err(&pdev->dev, "sound card register Sucessfull\n"); This isn't an error, skip the print here. > + > + return ret; > +} > + > +static const struct of_device_id msm_snd_apq8096_dt_match[] = { > + {.compatible = "qcom,apq8096-sndcard"}, > + {} > +}; > + > +static struct platform_driver msm_snd_apq8096_driver = { > + .probe = msm_snd_apq8096_probe, > + .driver = { > + .name = "msm-snd-apq8096", > + .owner = THIS_MODULE, Drop the .owner Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the review comments, On 02/01/18 00:19, Bjorn Andersson wrote: > On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla@linaro.org wrote: >> +static inline int q6dsp_map_channels(u8 *ch_map, int ch) >> +{ >> + memset(ch_map, 0, PCM_FORMAT_MAX_NUM_CHANNEL); > > This implies that ch_map is always an array of > PCM_FORMAT_MAX_NUM_CHANNEL elements. As such it would be better to > express this in the prototype; i.e u8 ch_map[PCM_FORMAT_MAX_NUM_CHANNEL] > Yep, Will do that. >> + >> + if (ch == 1) { > > This is a switch statement. > Yes, makes more sense. >> + ch_map[0] = PCM_CHANNEL_FC; >> + } else if (ch == 2) { > [..] >> +struct adsp_err_code { >> + int lnx_err_code; > > Indentation, and these could be given more succinct names. > >> + char *adsp_err_str; >> +}; >> + >> +static struct adsp_err_code adsp_err_code_info[ADSP_ERR_MAX+1] = { >> + { 0, ADSP_EOK_STR}, >> + { -ENOTRECOVERABLE, ADSP_EFAILED_STR}, >> + { -EINVAL, ADSP_EBADPARAM_STR}, >> + { -ENOSYS, ADSP_EUNSUPPORTED_STR}, >> + { -ENOPROTOOPT, ADSP_EVERSION_STR}, >> + { -ENOTRECOVERABLE, ADSP_EUNEXPECTED_STR}, >> + { -ENOTRECOVERABLE, ADSP_EPANIC_STR}, >> + { -ENOSPC, ADSP_ENORESOURCE_STR}, >> + { -EBADR, ADSP_EHANDLE_STR}, >> + { -EALREADY, ADSP_EALREADY_STR}, >> + { -EPERM, ADSP_ENOTREADY_STR}, >> + { -EINPROGRESS, ADSP_EPENDING_STR}, >> + { -EBUSY, ADSP_EBUSY_STR}, >> + { -ECANCELED, ADSP_EABORTED_STR}, >> + { -EAGAIN, ADSP_EPREEMPTED_STR}, >> + { -EAGAIN, ADSP_ECONTINUE_STR}, >> + { -EAGAIN, ADSP_EIMMEDIATE_STR}, >> + { -EAGAIN, ADSP_ENOTIMPL_STR}, >> + { -ENODATA, ADSP_ENEEDMORE_STR}, >> + { -EADV, ADSP_ERR_MAX_STR}, > > This, element 0x13, is not listed among the defined errors. Is this a > placeholder? > > How about making this even more descriptive by using the format > > [ADSP_EBADPARAM] = { -EINVAL, ADSP_EBADPARAM_STR }, > > That way the mapping table is self-describing. > > And you can use ARRAY_SIZE() instead of specifying the fixed size of > ADSP_ERR_MAX + 1... > Will give that a try! >> + { -ENOMEM, ADSP_ENOMEMORY_STR}, >> + { -ENODEV, ADSP_ENOTEXIST_STR}, >> + { -EADV, ADSP_ERR_MAX_STR}, > > "Advertise error"? No, downstream seems to define any unexpected error as -EADV, am not sure if this correct, probably we should change this to be more sensible one. > >> +}; >> + >> +static inline int adsp_err_get_lnx_err_code(u32 adsp_error) > > Can this be made internal to some c-file? So that any third party deals > only with linux error codes? > > > How about renaming this q6dsp_errno()? > yep will do that. >> +{ >> + if (adsp_error > ADSP_ERR_MAX) >> + return adsp_err_code_info[ADSP_ERR_MAX].lnx_err_code; >> + else >> + return adsp_err_code_info[adsp_error].lnx_err_code; > > I think this would look better if you assign a local variable and have a > single return. And just hard code the "invalid error code" errno, rather > than looking up ADSP_ERR_MAX in the list. > >> +} >> + >> +static inline char *adsp_err_get_err_str(u32 adsp_error) > > q6dsp_strerror(), to match strerror(3)? yep! > >> +{ >> + if (adsp_error > ADSP_ERR_MAX) >> + return adsp_err_code_info[ADSP_ERR_MAX].adsp_err_str; >> + else >> + return adsp_err_code_info[adsp_error].adsp_err_str; > > And I do think that, as with strerror, this should return a human > readable error, not the stringified define. okay! > >> +} > > > I'm puzzled to why these helper functions lives in a header file, at > least some aspects of this would better be hidden... Will try to improve on this in next version. > > Regards, > Bjorn > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the comments! On 02/01/18 00:45, Bjorn Andersson wrote: > On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla@linaro.org wrote: > > [..] >> + >> +config SND_SOC_QDSP6_AFE >> + tristate >> + default n > > Do you see a particular benefit of having one kernel module per > function? Why not just compile them all into the same q6dsp.ko? > No, I do not see any benefit in doing so, I can try to make it as single module in next version. >> + >> +config SND_SOC_QDSP6 >> + tristate "SoC ALSA audio driver for QDSP6" >> + select SND_SOC_QDSP6_AFE >> + help >> + To add support for MSM QDSP6 Soc Audio. >> + This will enable sound soc platform specific >> + audio drivers. This includes q6asm, q6adm, >> + q6afe interfaces to DSP using apr. > [..] >> +struct q6afev2 { >> + void *apr; > > apr has a type, even if it's definition is hidden you should use the > proper type here. > I agree. >> + struct device *dev; >> + int state; >> + int status; >> + struct platform_device *daidev; >> + >> + struct mutex afe_cmd_lock; > > You shouldn't need to include afe_ in this name. > make sense! >> + struct list_head port_list; >> + spinlock_t port_list_lock; >> + struct list_head node; >> +}; >> + > [..] >> +/* 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, > > Looks like you have an extra tab here, consider breaking the 80 char > "rule" to not have to wrap these. yep! > >> + AFE_PORT_HDMI_RX, 1}, >> +}; >> + >> +static struct q6afe_port *afe_find_port(struct q6afev2 *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); >> + return p; >> +} > > Make port_list an idr and you can just use idr_find() instead of rolling > your own search function. > Will give that a go. >> + >> +static int afe_callback(struct apr_device *adev, struct apr_client_data *data) >> +{ >> + struct q6afev2 *afe = dev_get_drvdata(&adev->dev);//priv; > > This is perfectly fine, no need to extend the interface with a priv (so > drop the comment). I think it was a leftover, will clean such instances. > >> + struct q6afe_port *port; >> + >> + if (!data) { >> + dev_err(afe->dev, "%s: Invalid param data\n", __func__); >> + return -EINVAL; >> + } > > Just define on in the apr layer that data will never be NULL, that will > save you 4 lines of code in every apr client. > I agree! >> + >> + if (data->payload_size) { >> + uint32_t *payload = data->payload; > > So the payload is 2 ints, where the first is a command and the second is > the status of it. This you can express in a simple struct to make the > code even easier on the eye. > Will do that, if it make code more readable. >> + >> + if (data->opcode == APR_BASIC_RSP_RESULT) { >> + if (payload[1] != 0) { >> + afe->status = payload[1]; >> + dev_err(afe->dev, >> + "cmd = 0x%x returned error = 0x%x\n", >> + payload[0], payload[1]); >> + } >> + switch (payload[0]) { >> + 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); >> + >> + break; >> + default: >> + dev_err(afe->dev, "Unknown cmd 0x%x\n", >> + payload[0]); > > If you flip the check for payload_size to return early if there isn't a > payload then you can reduce the indentation level one step and probably > doesn't have to wrap this line. yep make sense! > >> + break; >> + } >> + } >> + } >> + return 0; >> +} >> +/** >> + * q6afe_get_port_id() - Get port id from a given port index >> + * >> + * @index: port index >> + * >> + * Return: Will be an negative on error or valid port_id on success >> + */ >> +int q6afe_get_port_id(int index) >> +{ >> + if (index < 0 || index > AFE_PORT_MAX) >> + return -EINVAL; >> + >> + return port_maps[index].port_id; >> +} >> +EXPORT_SYMBOL_GPL(q6afe_get_port_id); >> + >> +static int afe_apr_send_pkt(struct q6afev2 *afe, void *data, >> + wait_queue_head_t *wait) > > Rather than conditionally passing the wait, split this function in > afe_send_sync(*afe, *data, wait) and afe_send_async(*afe, *data). > Will do that across other modules too. >> +{ >> + int ret; >> + >> + if (wait) >> + afe->state = AFE_CMD_RESP_NONE; >> + >> + afe->status = 0; >> + ret = apr_send_pkt(afe->apr, data); >> + if (ret > 0) { > > Check ret < 0 and return here, this saves you one indentation level in > the following chunk. > > If you then check !wait and return early you can reduce another level. > okay! >> + if (wait) { >> + 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", >> + adsp_err_get_err_str(afe->status)); >> + ret = adsp_err_get_lnx_err_code(afe->status); >> + } else { >> + ret = 0; >> + } >> + } else { >> + ret = 0; >> + } >> + } else { >> + dev_err(afe->dev, "packet not transmitted\n"); >> + ret = -EINVAL; >> + } >> + >> + return ret; >> +} >> + >> +static int afe_send_cmd_port_start(struct q6afe_port *port) >> +{ >> + u16 port_id = port->id; >> + struct afe_port_cmd_device_start start; >> + struct q6afev2 *afe = port->afe.v2; >> + int ret, index; >> + >> + index = port->token; >> + start.hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD, >> + APR_HDR_LEN(APR_HDR_SIZE), >> + APR_PKT_VER); >> + start.hdr.pkt_size = sizeof(start); >> + start.hdr.src_port = 0; >> + start.hdr.dest_port = 0; >> + start.hdr.token = index; > > Just put port->token here, saves you a local variable. > yep! >> + start.hdr.opcode = AFE_PORT_CMD_DEVICE_START; >> + start.port_id = port_id; >> + >> + ret = afe_apr_send_pkt(afe, &start, &port->wait); >> + if (ret) >> + dev_err(afe->dev, "AFE enable for port 0x%x failed %d\n", >> + port_id, ret); >> + >> + return ret; >> +} >> + >> +static int afe_port_start(struct q6afe_port *port, >> + union afe_port_config *afe_config) >> +{ >> + struct afe_audioif_config_command config; >> + struct q6afev2 *afe = port->afe.v2; >> + int ret = 0; >> + int port_id = port->id; >> + int cfg_type; >> + int index = 0; >> + >> + if (!afe_config) { > > Looking at the one caller of this function, afe_config can't be NULL, so > no need for this error handling. > okay. >> + dev_err(afe->dev, "Error, no configuration data\n"); >> + ret = -EINVAL; >> + return ret; >> + } >> + >> + index = port->token; >> + >> + mutex_lock(&afe->afe_cmd_lock); >> + /* Also send the topology id here: */ >> + config.hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD, >> + APR_HDR_LEN(APR_HDR_SIZE), >> + APR_PKT_VER); >> + config.hdr.pkt_size = sizeof(config); >> + config.hdr.src_port = 0; >> + config.hdr.dest_port = 0; >> + config.hdr.token = index; >> + >> + cfg_type = port->cfg_type; >> + config.hdr.opcode = AFE_PORT_CMD_SET_PARAM_V2; >> + config.param.port_id = port_id; >> + config.param.payload_size = sizeof(config) - sizeof(struct apr_hdr) - >> + sizeof(config.param); >> + config.param.payload_address_lsw = 0x00; >> + config.param.payload_address_msw = 0x00; >> + config.param.mem_map_handle = 0x00; >> + config.pdata.module_id = AFE_MODULE_AUDIO_DEV_INTERFACE; >> + config.pdata.param_id = cfg_type; >> + config.pdata.param_size = sizeof(config.port); > > This looks like a good candidate for a afe_port_set_param() function. > makes sense. >> + >> + config.port = *afe_config; >> + >> + ret = afe_apr_send_pkt(afe, &config, &port->wait); >> + if (ret) { >> + dev_err(afe->dev, "AFE enable for port 0x%x failed %d\n", >> + port_id, ret); >> + goto fail_cmd; >> + } >> + >> + ret = afe_send_cmd_port_start(port); >> + >> +fail_cmd: >> + mutex_unlock(&afe->afe_cmd_lock); >> + return ret; >> +} > [..] >> +/** >> + * q6afe_port_get_from_id() - Get port instance from a port id >> + * >> + * @dev: Pointer to afe child device. >> + * @id: port id >> + * >> + * Return: Will be an error pointer on error or a valid afe port >> + * on success. >> + */ >> +struct q6afe_port *q6afe_port_get_from_id(struct device *dev, int id) > > Will there be any other getter? Otherwise you can just call this > q6afe_port_get(). There is one more get, which is basically lookup from index to port number. > >> +{ >> + int port_id; >> + struct q6afev2 *afe = dev_get_drvdata(dev->parent); >> + struct q6afe_port *port; >> + int token; >> + int cfg_type; >> + >> + if (!afe) { >> + dev_err(dev, "Unable to find instance of afe service\n"); >> + return ERR_PTR(-ENOENT); >> + } > > As this comes from the passed dev this check will catch bugs withing > this driver, but if the client accidentally passes the wrong dev it's > likely that you don't catch it here anyways. Consider dropping the > check. yes! > >> + >> + token = id; >> + if (token < 0 || token > AFE_PORT_MAX) { >> + dev_err(dev, "AFE port token[%d] invalid!\n", token); >> + return ERR_PTR(-EINVAL); >> + } >> + >> + port_id = port_maps[id].port_id; >> + >> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL); >> + if (!port) >> + return ERR_PTR(-ENOMEM); >> + >> + init_waitqueue_head(&port->wait); >> + >> + port->token = token; >> + port->id = port_id; >> + >> + port->afe.v2 = afe; >> + switch (port_id) { > > How about moving this switch statement above the allocation? > Yes, this can be done! >> + case AFE_PORT_ID_MULTICHAN_HDMI_RX: >> + cfg_type = AFE_PARAM_ID_HDMI_CONFIG; >> + break; >> + default: >> + dev_err(dev, "Invalid port id 0x%x\n", port_id); >> + return ERR_PTR(-EINVAL); >> + } >> + >> + port->cfg_type = cfg_type; >> + >> + spin_lock(&afe->port_list_lock); >> + list_add_tail(&port->node, &afe->port_list); >> + spin_unlock(&afe->port_list_lock); >> + >> + return port; >> + >> +} >> +EXPORT_SYMBOL_GPL(q6afe_port_get_from_id); > > Regards, > Bjorn > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/01/18 23:28, Bjorn Andersson wrote: > On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla@linaro.org wrote: > >> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> >> This patch adds support to q6afe backend dais driver. >> > > Isn't the list of backend DAIs platform-dependent? dai links and connections between backend and front ends are platform dependent. > > [..] >> +static const struct snd_soc_dapm_widget hdmi_dapm_widgets[] = { >> + SND_SOC_DAPM_AIF_OUT("HDMI", "HDMI Playback", 0, 0, 0, 0), >> + SND_SOC_DAPM_OUTPUT("HDMI-RX"), >> +}; >> + >> +static const struct snd_soc_component_driver msm_dai_hdmi_q6_component = { > > How will this look beyond HDMI? I'm having issues mapping this to > downstream. ex: For slimbus dais, we would have more entries of "struct snd_soc_dai_driver" in this file. Basically these are the dais that are exposed by the dsp firmware. Depending on the actually platform some of these dais would be setup accordingly. > >> + .name = "msm-dai-q6-hdmi", >> + .dapm_widgets = hdmi_dapm_widgets, >> + .num_dapm_widgets = ARRAY_SIZE(hdmi_dapm_widgets), >> + .controls = hdmi_config_controls, >> + .num_controls = ARRAY_SIZE(hdmi_config_controls), >> + .dapm_routes = hdmi_dapm_routes, >> + .num_dapm_routes = ARRAY_SIZE(hdmi_dapm_routes), >> +}; >> + > > Regards, > Bjorn > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/14/2017 09:34 AM, srinivas.kandagatla@linaro.org wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > uThis patch adds support to DB820c machine driver. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > sound/soc/qcom/Kconfig | 9 +++- > sound/soc/qcom/apq8096.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 132 insertions(+), 1 deletion(-) > create mode 100644 sound/soc/qcom/apq8096.c > > diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig > index ecd1e4ba834d..748ccc3edefa 100644 > --- a/sound/soc/qcom/Kconfig > +++ b/sound/soc/qcom/Kconfig > @@ -72,7 +72,6 @@ config SND_SOC_QDSP6_ASM_DAI > tristate > default n > > - > config SND_SOC_QDSP6 > tristate "SoC ALSA audio driver for QDSP6" > select SND_SOC_QDSP6_AFE > @@ -87,3 +86,11 @@ config SND_SOC_QDSP6 > This will enable sound soc platform specific > audio drivers. This includes q6asm, q6adm, > q6afe interfaces to DSP using apr. > + > +config SND_SOC_MSM8996 > + tristate "SoC Machine driver for MSM8996 and APQ8096 boards" > + select SND_SOC_QDSP6V2 > + default n > + help > + To add support for SoC audio on MSM8996 and APQ8096 boards > + > diff --git a/sound/soc/qcom/apq8096.c b/sound/soc/qcom/apq8096.c > new file mode 100644 > index 000000000000..5b2036317f71 > --- /dev/null > +++ b/sound/soc/qcom/apq8096.c > @@ -0,0 +1,124 @@ > +/* > + * Copyright (c) 2017 The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > +#include <linux/clk.h> No clk usage though? > +#include <linux/platform_device.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <sound/soc.h> [...] > +static struct snd_soc_dai_link msm8996_dai_links[] = { > + /* FrontEnd DAI Links */ > + { > + .name = "MultiMedia1 Playback", > + .stream_name = "MultiMedia1", > + .cpu_dai_name = "MM_DL1", > + .platform_name = "q6asm_dai", > + .dynamic = 1, > + .dpcm_playback = 1, > + > + .codec_dai_name = "snd-soc-dummy-dai", > + .codec_name = "snd-soc-dummy", > + }, > + /* Backend DAI Links */ > + { > + .name = "HDMI Playback", > + .stream_name = "q6afe_dai", > + .cpu_dai_name = "HDMI", > + .platform_name = "q6routing", > + .no_pcm = 1, > + .dpcm_playback = 1, > + .be_hw_params_fixup = msm8996_be_hw_params_fixup, > + .codec_dai_name = "i2s-hifi", > + .codec_name = "hdmi-audio-codec.0.auto", > + }, > +}; > + > +static int apq8096_sbc_parse_of(struct snd_soc_card *card) > +{ > + struct device *dev = card->dev; > + int ret; > + > + ret = snd_soc_of_parse_card_name(card, "qcom,model"); > + if (ret) > + dev_err(dev, "Error parsing card name: %d\n", ret); So this prints an error, and the caller also prints an error when it fails. Double error messages? > + > + return ret; > +} > + > +static int msm_snd_apq8096_probe(struct platform_device *pdev) > +{ > + int ret; > + struct snd_soc_card *card; > + > + card = devm_kzalloc(&pdev->dev, sizeof(*card), GFP_KERNEL); > + if (!card) > + return -ENOMEM; > + > + card->dev = &pdev->dev; > + > + ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); Why do we need to do this? Can you add some sort of comment in the code about why? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for your review comments, On 03/01/18 17:20, Stephen Boyd wrote: > On 12/14/2017 09:34 AM, srinivas.kandagatla@linaro.org wrote: >> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> >> diff --git a/sound/soc/qcom/apq8096.c b/sound/soc/qcom/apq8096.c >> new file mode 100644 >> index 000000000000..5b2036317f71 >> --- /dev/null >> +++ b/sound/soc/qcom/apq8096.c >> @@ -0,0 +1,124 @@ >> + */ >> +#include <linux/clk.h> > > No clk usage though? Will remove it in next version. > >> +#include <linux/platform_device.h> >> +#include <linux/module.h> >> +#include <linux/of_device.h> >> +#include <sound/soc.h> > [...] >> +static int apq8096_sbc_parse_of(struct snd_soc_card *card) >> +{ >> + struct device *dev = card->dev; >> + int ret; >> + >> + ret = snd_soc_of_parse_card_name(card, "qcom,model"); >> + if (ret) >> + dev_err(dev, "Error parsing card name: %d\n", ret); > > So this prints an error, and the caller also prints an error when it > fails. Double error messages? > looks redundant, will remove it. >> + >> + return ret; >> +} >> + >> +static int msm_snd_apq8096_probe(struct platform_device *pdev) >> +{ >> + int ret; >> + struct snd_soc_card *card; >> + >> + card = devm_kzalloc(&pdev->dev, sizeof(*card), GFP_KERNEL); >> + if (!card) >> + return -ENOMEM; >> + >> + card->dev = &pdev->dev; >> + >> + ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); > > Why do we need to do this? Can you add some sort of comment in the code > about why? Even though dsp supports 64 bit addresses, but the sid sits at offset of 32, which brings this restriction of supporting only 32 bit iova. thanks, srini > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/01/18 12:02, Mark Brown wrote: > On Wed, Jan 03, 2018 at 09:20:45AM -0800, Stephen Boyd wrote: >> On 12/14/2017 09:34 AM, srinivas.kandagatla@linaro.org wrote: > >>> uThis patch adds support to DB820c machine driver. > >>> + ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); > >> Why do we need to do this? Can you add some sort of comment in the code >> about why? > > And why are we applying DMA restrictions in a machine driver? Initially I had this in pcm driver, but looking at example usage of snd_dma_alloc_pages, most of them use card->dev and some of them use pcm device for allocating dma memory. Also, as I moved most dsp static services and dais out of DT, except codec and sound card, sound card device was the only choice I had for binding with iommu and enforcing iova range restrictions. This call will be replaced by dma-ranges property in DT either way. --srini > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/14/2017 11:03 PM, srinivas.kandagatla@linaro.org wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > This patch adds support to q6asm dai driver which configures Q6ASM streams > to pass pcm data. > Currently the driver only exposes 2 playback streams for hdmi playback > support, it can be easily extended to add all 8 streams. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > sound/soc/qcom/Kconfig | 6 + > sound/soc/qcom/qdsp6/Makefile | 1 + > sound/soc/qcom/qdsp6/q6asm-dai.c | 534 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 541 insertions(+) > create mode 100644 sound/soc/qcom/qdsp6/q6asm-dai.c > > diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig > index 003ce182691c..ecd1e4ba834d 100644 > --- a/sound/soc/qcom/Kconfig [..] > +static void event_handler(uint32_t opcode, uint32_t token, > + uint32_t *payload, void *priv) > +{ > + struct q6asm_dai_rtd *prtd = priv; > + struct snd_pcm_substream *substream = prtd->substream; > + > + switch (opcode) { > + case ASM_CLIENT_EVENT_CMD_RUN_DONE: > + q6asm_write_nolock(prtd->audio_client, > + prtd->pcm_count, 0, 0, NO_TIMESTAMP); > + break; > + case ASM_CLIENT_EVENT_CMD_EOS_DONE: > + prtd->state = STOPPED; > + break; Add support for V2 version of opcode to support latest adsp > + case ASM_CLIENT_EVENT_DATA_WRITE_DONE: { > + prtd->pcm_irq_pos += prtd->pcm_count; > + snd_pcm_period_elapsed(substream); > + if (prtd->state == RUNNING) > + q6asm_write_nolock(prtd->audio_client, > + prtd->pcm_count, 0, 0, NO_TIMESTAMP); > + > + break; > + } > + default: > + break; > + } > +} > + > +static int q6asm_dai_prepare(struct snd_pcm_substream *substream) > +{ > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct snd_soc_pcm_runtime *soc_prtd = substream->private_data; > + struct q6asm_dai_rtd *prtd = runtime->private_data; > + struct q6asm_dai_data *pdata; > + int ret; > + > + pdata = dev_get_drvdata(soc_prtd->platform->dev); > + if (!pdata) > + return -EINVAL; > + > + if (!prtd || !prtd->audio_client) { > + pr_err("%s: private data null or audio client freed\n", > + __func__); > + return -EINVAL; > + } > + > + prtd->pcm_count = snd_pcm_lib_period_bytes(substream); > + prtd->pcm_irq_pos = 0; > + /* rate and channels are sent to audio driver */ > + if (prtd->state) { > + /* clear the previous setup if any */ > + q6asm_cmd(prtd->audio_client, CMD_CLOSE); > + q6asm_unmap_memory_regions(substream->stream, > + prtd->audio_client); > + q6routing_dereg_phy_stream(soc_prtd->dai_link->id, > + SNDRV_PCM_STREAM_PLAYBACK); > + } > + > + ret = q6asm_map_memory_regions(substream->stream, prtd->audio_client, > + prtd->phys, > + (prtd->pcm_size / prtd->periods), > + prtd->periods); > + > + if (ret < 0) { > + pr_err("Audio Start: Buffer Allocation failed rc = %d\n", > + ret); > + return -ENOMEM; > + } > + > + ret = q6asm_open_write(prtd->audio_client, FORMAT_LINEAR_PCM, > + prtd->bits_per_sample); > + if (ret < 0) { > + pr_err("%s: q6asm_open_write failed\n", __func__); > + q6asm_audio_client_free(prtd->audio_client); > + prtd->audio_client = NULL; > + return -ENOMEM; > + } > + > + prtd->session_id = q6asm_get_session_id(prtd->audio_client); > + ret = q6routing_reg_phy_stream(soc_prtd->dai_link->id, LEGACY_PCM_MODE, > + prtd->session_id, substream->stream); > + if (ret) { > + pr_err("%s: stream reg failed ret:%d\n", __func__, ret); > + return ret; > + } > + > + ret = q6asm_media_format_block_multi_ch_pcm( > + prtd->audio_client, runtime->rate, > + runtime->channels, !prtd->set_channel_map, > + prtd->channel_map, prtd->bits_per_sample); > + if (ret < 0) > + pr_info("%s: CMD Format block failed\n", __func__); > + > + prtd->state = RUNNING; > + > + return 0; > +} > + > +static int q6asm_dai_trigger(struct snd_pcm_substream *substream, int cmd) > +{ > + int ret = 0; > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct q6asm_dai_rtd *prtd = runtime->private_data; > + > + switch (cmd) { > + case SNDRV_PCM_TRIGGER_START: > + ret = q6asm_run_nowait(prtd->audio_client, 0, 0, 0); > + break; > + case SNDRV_PCM_TRIGGER_RESUME: > + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > + ret = q6asm_run_nowait(prtd->audio_client, 0, 0, 0); > + break; > + case SNDRV_PCM_TRIGGER_STOP: > + prtd->state = STOPPED; > + ret = q6asm_cmd_nowait(prtd->audio_client, CMD_EOS); > + break; > + case SNDRV_PCM_TRIGGER_SUSPEND: > + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: > + ret = q6asm_cmd_nowait(prtd->audio_client, CMD_PAUSE); > + break; > + default: > + ret = -EINVAL; > + break; > + } > + > + return ret; > +} > + > +static int q6asm_dai_open(struct snd_pcm_substream *substream) > +{ > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct snd_soc_pcm_runtime *soc_prtd = substream->private_data; > + struct q6asm_dai_rtd *prtd; > + struct q6asm_dai_data *pdata; > + struct device *dev = soc_prtd->platform->dev; > + int ret = 0; > + > + pdata = dev_get_drvdata(dev); > + if (!pdata) { > + pr_err("Platform data not found ..\n"); > + return -EINVAL; > + } > + > + prtd = kzalloc(sizeof(struct q6asm_dai_rtd), GFP_KERNEL); > + if (prtd == NULL) > + return -ENOMEM; > + > + prtd->substream = substream; > + prtd->audio_client = q6asm_audio_client_alloc(dev, > + (app_cb)event_handler, prtd); > + if (!prtd->audio_client) { > + pr_info("%s: Could not allocate memory\n", __func__); > + kfree(prtd); > + return -ENOMEM; > + } > + > +// prtd->audio_client->dev = dev; > + > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) > + runtime->hw = q6asm_dai_hardware_playback; > + > + ret = snd_pcm_hw_constraint_list(runtime, 0, > + SNDRV_PCM_HW_PARAM_RATE, > + &constraints_sample_rates); > + if (ret < 0) > + pr_info("snd_pcm_hw_constraint_list failed\n"); > + /* Ensure that buffer size is a multiple of period size */ > + ret = snd_pcm_hw_constraint_integer(runtime, > + SNDRV_PCM_HW_PARAM_PERIODS); > + if (ret < 0) > + pr_info("snd_pcm_hw_constraint_integer failed\n"); > + > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { > + ret = snd_pcm_hw_constraint_minmax(runtime, > + SNDRV_PCM_HW_PARAM_BUFFER_BYTES, > + PLAYBACK_MIN_NUM_PERIODS * PLAYBACK_MIN_PERIOD_SIZE, > + PLAYBACK_MAX_NUM_PERIODS * PLAYBACK_MAX_PERIOD_SIZE); > + if (ret < 0) { > + pr_err("constraint for buffer bytes min max ret = %d\n", > + ret); > + } > + } > + > + ret = snd_pcm_hw_constraint_step(runtime, 0, > + SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 32); > + if (ret < 0) { > + pr_err("constraint for period bytes step ret = %d\n", > + ret); > + } > + ret = snd_pcm_hw_constraint_step(runtime, 0, > + SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 32); > + if (ret < 0) { > + pr_err("constraint for buffer bytes step ret = %d\n", > + ret); > + } > + > + prtd->set_channel_map = false; > + runtime->private_data = prtd; > + > + snd_soc_set_runtime_hwparams(substream, &q6asm_dai_hardware_playback); > + > + runtime->dma_bytes = q6asm_dai_hardware_playback.buffer_bytes_max; > + > + > + if (pdata->sid < 0) > + prtd->phys = substream->dma_buffer.addr; > + else > + prtd->phys = substream->dma_buffer.addr | (pdata->sid << 32); > + > + snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer); > + > + return 0; > +} > + > +static int q6asm_dai_close(struct snd_pcm_substream *substream) > +{ > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct snd_soc_pcm_runtime *soc_prtd = substream->private_data; > + struct q6asm_dai_rtd *prtd = runtime->private_data; > + > + if (prtd->audio_client) { > + q6asm_cmd(prtd->audio_client, CMD_CLOSE); > + q6asm_unmap_memory_regions(substream->stream, > + prtd->audio_client); > + q6asm_audio_client_free(prtd->audio_client); > + } > + q6routing_dereg_phy_stream(soc_prtd->dai_link->id, > + SNDRV_PCM_STREAM_PLAYBACK); > + kfree(prtd); > + return 0; > +} > + > +static snd_pcm_uframes_t q6asm_dai_pointer(struct snd_pcm_substream *substream) > +{ > + > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct q6asm_dai_rtd *prtd = runtime->private_data; > + > + if (prtd->pcm_irq_pos >= prtd->pcm_size) > + prtd->pcm_irq_pos = 0; > + > + return bytes_to_frames(runtime, (prtd->pcm_irq_pos)); > +} > + > +static int q6asm_dai_mmap(struct snd_pcm_substream *substream, > + struct vm_area_struct *vma) > +{ > + > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct snd_soc_pcm_runtime *soc_prtd = substream->private_data; > + struct snd_card *card = soc_prtd->card->snd_card; > + > + return dma_mmap_coherent(card->dev, vma, > + runtime->dma_area, runtime->dma_addr, > + runtime->dma_bytes); > +} > + > +static int q6asm_dai_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params) > +{ > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct q6asm_dai_rtd *prtd = runtime->private_data; > + > + prtd->pcm_size = params_buffer_bytes(params); > + prtd->periods = params_periods(params); > + > + switch (params_format(params)) { > + case SNDRV_PCM_FORMAT_S16_LE: > + prtd->bits_per_sample = 16; > + break; > + case SNDRV_PCM_FORMAT_S24_LE: > + prtd->bits_per_sample = 24; > + break; > + } > + > + return 0; > +} > + > +static struct snd_pcm_ops q6asm_dai_ops = { > + .open = q6asm_dai_open, > + .hw_params = q6asm_dai_hw_params, > + .close = q6asm_dai_close, > + .ioctl = snd_pcm_lib_ioctl, > + .prepare = q6asm_dai_prepare, > + .trigger = q6asm_dai_trigger, > + .pointer = q6asm_dai_pointer, > + .mmap = q6asm_dai_mmap, > +}; > + > +static int q6asm_dai_pcm_new(struct snd_soc_pcm_runtime *rtd) > +{ > + struct snd_pcm *pcm = rtd->pcm; > + struct snd_pcm_substream *substream; > + struct snd_card *card = rtd->card->snd_card; > + struct device *dev = card->dev; > + struct device_node *node = dev->of_node; > + struct q6asm_dai_data *pdata = dev_get_drvdata(rtd->platform->dev); > + struct of_phandle_args args; > + > + int size, ret = 0; > + > + ret = of_parse_phandle_with_fixed_args(node, "iommus", 1, 0, &args); > + if (ret < 0) > + pdata->sid = -1; > + else > + pdata->sid = args.args[0]; > + > + > + > + substream = pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream; > + size = q6asm_dai_hardware_playback.buffer_bytes_max; > + ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, dev, size, > + &substream->dma_buffer); > + if (ret) { > + dev_err(dev, "Cannot allocate buffer(s)\n"); > + return ret; > + } > + > + return ret; > +} > + > +static void q6asm_dai_pcm_free(struct snd_pcm *pcm) > +{ > + struct snd_pcm_substream *substream; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(pcm->streams); i++) { > + substream = pcm->streams[i].substream; > + if (substream) { > + snd_dma_free_pages(&substream->dma_buffer); > + substream->dma_buffer.area = NULL; > + substream->dma_buffer.addr = 0; > + } > + } > +} > + > +static struct snd_soc_platform_driver q6asm_soc_platform = { > + .ops = &q6asm_dai_ops, > + .pcm_new = q6asm_dai_pcm_new, > + .pcm_free = q6asm_dai_pcm_free, > + > +}; > + > +static const struct snd_soc_dapm_route afe_pcm_routes[] = { > + {"MM_DL1", NULL, "MultiMedia1 Playback" }, > + {"MM_DL2", NULL, "MultiMedia2 Playback" }, > + > +}; > + > +static int fe_dai_probe(struct snd_soc_dai *dai) > +{ > + struct snd_soc_dapm_context *dapm; > + > + dapm = snd_soc_component_get_dapm(dai->component); > + snd_soc_dapm_add_routes(dapm, afe_pcm_routes, > + ARRAY_SIZE(afe_pcm_routes)); > + > + return 0; > +} > + > +static const struct snd_soc_component_driver q6asm_fe_dai_component = { > + .name = "q6asm-fe-dai", > +}; > + > +static struct snd_soc_dai_driver q6asm_fe_dais[] = { > + { > + .playback = { > + .stream_name = "MultiMedia1 Playback", > + .rates = (SNDRV_PCM_RATE_8000_192000| > + SNDRV_PCM_RATE_KNOT), > + .formats = (SNDRV_PCM_FMTBIT_S16_LE | > + SNDRV_PCM_FMTBIT_S24_LE), > + .channels_min = 1, > + .channels_max = 8, > + .rate_min = 8000, > + .rate_max = 192000, > + }, > + .name = "MM_DL1", > + .probe = fe_dai_probe, > + .id = MSM_FRONTEND_DAI_MULTIMEDIA1, > + }, > + { > + .playback = { > + .stream_name = "MultiMedia2 Playback", > + .rates = (SNDRV_PCM_RATE_8000_192000| > + SNDRV_PCM_RATE_KNOT), > + .formats = (SNDRV_PCM_FMTBIT_S16_LE | > + SNDRV_PCM_FMTBIT_S24_LE), > + .channels_min = 1, > + .channels_max = 8, > + .rate_min = 8000, > + .rate_max = 192000, > + }, > + .name = "MM_DL2", > + .probe = fe_dai_probe, > + .id = MSM_FRONTEND_DAI_MULTIMEDIA2, > + }, > +}; > + > +static int q6asm_dai_probe(struct platform_device *pdev) > +{ > + struct q6asm_dai_data *pdata; > + struct device *dev = &pdev->dev; > + int rc; > + > + pdata = devm_kzalloc(dev, sizeof(struct q6asm_dai_data), GFP_KERNEL); > + if (!pdata) > + return -ENOMEM; > + > + > + dev_set_drvdata(dev, pdata); > + > + rc = snd_soc_register_platform(dev, &q6asm_soc_platform); > + if (rc) { > + dev_err(&pdev->dev, "err_dai_platform\n"); > + return rc; > + } > + > + rc = snd_soc_register_component(dev, &q6asm_fe_dai_component, > + q6asm_fe_dais, > + ARRAY_SIZE(q6asm_fe_dais)); > + if (rc) > + dev_err(dev, "err_dai_component\n"); > + > + return rc; > + > +} > + > +static int q6asm_dai_remove(struct platform_device *pdev) > +{ > + snd_soc_unregister_platform(&pdev->dev); > + return 0; > +} > + > +static struct platform_driver q6asm_dai_driver = { > + .driver = { > + .name = "q6asm_dai", > + .owner = THIS_MODULE, > + }, > + .probe = q6asm_dai_probe, > + .remove = q6asm_dai_remove, > +}; > + > +module_platform_driver(q6asm_dai_driver); > + > +MODULE_DESCRIPTION("PCM module platform driver"); > +MODULE_LICENSE("GPL v2"); -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/02/18 11:34, Rohit Kumar wrote: >> + >> +module_platform_driver(q6afe_dai_driver); > MODULE_LICENSE missing. Thanks Rohit, I have included this in v3. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/14/2017 11:03 PM, srinivas.kandagatla@linaro.org wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > This patch adds support to core apr service, which is used to query > status of other static and dynamic services on the dsp. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > sound/soc/qcom/Kconfig | 5 + > sound/soc/qcom/qdsp6/Makefile | 1 + > sound/soc/qcom/qdsp6/q6core.c | 227 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 233 insertions(+) > create mode 100644 sound/soc/qcom/qdsp6/q6core.c > > diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig > index 7ebdb879a8a3..121b9c957024 100644 > --- a/sound/soc/qcom/Kconfig > +++ b/sound/soc/qcom/Kconfig > @@ -56,11 +56,16 @@ config SND_SOC_QDSP6_ASM > tristate > default n > > +config SND_SOC_QDSP6_CORE > + tristate > + default n > + > config SND_SOC_QDSP6 > tristate "SoC ALSA audio driver for QDSP6" > select SND_SOC_QDSP6_AFE > select SND_SOC_QDSP6_ADM > select SND_SOC_QDSP6_ASM > + select SND_SOC_QDSP6_CORE > help > To add support for MSM QDSP6 Soc Audio. > This will enable sound soc platform specific > diff --git a/sound/soc/qcom/qdsp6/Makefile b/sound/soc/qcom/qdsp6/Makefile > index 49dd3ccab27b..ad7f10691e54 100644 > --- a/sound/soc/qcom/qdsp6/Makefile > +++ b/sound/soc/qcom/qdsp6/Makefile > @@ -1,3 +1,4 @@ > obj-$(CONFIG_SND_SOC_QDSP6_AFE) += q6afe.o > obj-$(CONFIG_SND_SOC_QDSP6_ADM) += q6adm.o > obj-$(CONFIG_SND_SOC_QDSP6_ASM) += q6asm.o > +obj-$(CONFIG_SND_SOC_QDSP6_CORE) += q6core.o > diff --git a/sound/soc/qcom/qdsp6/q6core.c b/sound/soc/qcom/qdsp6/q6core.c > new file mode 100644 > index 000000000000..d4e2dbc62489 > --- /dev/null > +++ b/sound/soc/qcom/qdsp6/q6core.c > @@ -0,0 +1,227 @@ > +/* SPDX-License-Identifier: GPL-2.0 > +* Copyright (c) 2017, Linaro Limited > +*/ > +#include <linux/slab.h> > +#include <linux/wait.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/sched.h> > +#include <linux/jiffies.h> [..] > + > + dev_set_drvdata(&adev->dev, core); > + > + core->adev = adev; > + init_waitqueue_head(&core->wait); > + > + do { > + if (!q6core_is_adsp_ready(core)) { > + dev_info(&adev->dev, "ADSP Audio isn't ready\n"); > + } else { > + dev_info(&adev->dev, "ADSP Audio is ready\n"); > + If q6core_is_adsp_ready() return failure, then we should not call and ADSP API. > + ret = q6core_get_svc_versions(core); > + if (!ret) > + q6core_add_static_services(core); > + > + break; > + } > + } while (time_after(timeout, jiffies)); > + I think we should defer probe if q6core_is_adsp_ready() returns failure and timeouts. > + return ret; > +} > + > +static int q6core_exit(struct apr_device *adev) > +{ > + return 0; > +} > + > +static const struct apr_device_id core_id[] = { > + {"Q6CORE", APR_DOMAIN_ADSP, APR_SVC_ADSP_CORE, APR_CLIENT_AUDIO}, > + { }, > +}; > + > +static struct apr_driver qcom_q6core_driver = { > + .probe = q6core_probe, > + .remove = q6core_exit, > + .callback = core_callback, > + .id_table = core_id, > + .driver = { > + .name = "qcom-q6core", > + }, > +}; > + > +module_apr_driver(qcom_q6core_driver); > + > +MODULE_AUTHOR("Srinivas Kandagatla <srinivas.kandagatla@linaro.org"); > +MODULE_DESCRIPTION("q6 core"); > +MODULE_LICENSE("GPL v2"); -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> This patchset aims to provide a very basic version of QCOM DSP based audio support which is available in downstream andriod kernels. This patchset only support Digital audio based for HDMI-RX 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 on top of mainline, also tested slimbus analog audio using wcd9355 with an additional small patch. 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 RFC: - Converted APR driver to proper bus driver - Added API version info to apr devices - Added q6core driver. - Fixed various issues spotted by Banajit and Kasam. - Added kernel doc to exported symbols. - renamed qdsp6v2 to qdsp6 as api version info can be used from apr device. - removed unnecessary dt bindings for apr devices. Srinivas Kandagatla (15): 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 ASoC: qcom: qdsp6: Add support to Q6AFE ASoC: qcom: qdsp6: Add support to Q6ADM 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 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 | 28 + .../devicetree/bindings/sound/qcom,apq8096.txt | 22 + arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 5 + arch/arm64/boot/dts/qcom/msm8996.dtsi | 33 + drivers/soc/qcom/Kconfig | 8 + drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/apr.c | 395 +++++++ include/linux/mod_devicetable.h | 13 + include/linux/soc/qcom/apr.h | 174 +++ sound/soc/qcom/Kconfig | 51 + sound/soc/qcom/Makefile | 5 + sound/soc/qcom/apq8096.c | 124 +++ sound/soc/qcom/qdsp6/Makefile | 7 + sound/soc/qcom/qdsp6/common.h | 225 ++++ sound/soc/qcom/qdsp6/q6adm.c | 602 +++++++++++ sound/soc/qcom/qdsp6/q6adm.h | 24 + sound/soc/qcom/qdsp6/q6afe-dai.c | 241 +++++ sound/soc/qcom/qdsp6/q6afe.c | 503 +++++++++ sound/soc/qcom/qdsp6/q6afe.h | 30 + sound/soc/qcom/qdsp6/q6asm-dai.c | 534 ++++++++++ sound/soc/qcom/qdsp6/q6asm.c | 1119 ++++++++++++++++++++ sound/soc/qcom/qdsp6/q6asm.h | 61 ++ sound/soc/qcom/qdsp6/q6core.c | 227 ++++ sound/soc/qcom/qdsp6/q6routing.c | 386 +++++++ sound/soc/qcom/qdsp6/q6routing.h | 9 + 25 files changed, 4827 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,apr.txt create mode 100644 Documentation/devicetree/bindings/sound/qcom,apq8096.txt create mode 100644 drivers/soc/qcom/apr.c 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/common.h 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/q6routing.c create mode 100644 sound/soc/qcom/qdsp6/q6routing.h -- 2.15.0 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html