mbox series

[v2,00/16] ASoC: qcom: Add AudioReach support

Message ID 20210714153039.28373-1-srinivas.kandagatla@linaro.org
Headers show
Series ASoC: qcom: Add AudioReach support | expand

Message

Srinivas Kandagatla July 14, 2021, 3:30 p.m. UTC
This patchset adds ASoC driver support to configure signal processing
framework ("AudioReach") which is integral part of Qualcomm next
generation audio SDK and will be deployed on upcoming Qualcomm chipsets.
It makes use of ASoC Topology to load graphs on to the DSP which is then
managed by APM (Audio Processing Manager) service to prepare/start/stop.

Here is simpified high-level block diagram of AudioReach:

 ___________________________________________________________
|                 CPU (Application Processor)               |
|  +---------+          +---------+         +---------+     |
|  |  q6apm  |          |  q6apm  |         | q6afe   |     |
|  |   dais  | <------> |         | <-----> | bedais  |     |
|  +---------+          +---------+         +---------+     |
|                            ^  ^                           |
|                            |  |           +---------+     |
|  +---------+               v  +---------->|topology |     |
|  | q6prm   |          +---------+         |         |     |
|  |         |<-------->|   GPR   |         +---------+     |
|  +---------+          +---------+                         |
|                            ^                              |
|____________________________|______________________________|
                             |  
                             | RPMSG (IPC over GLINK)              
 ____________________________|______________________________
|                            |                              |
|    +-----------------------+                              |
|    |                       |                              |
|    v                       v              q6 (Audio DSP)  |
|+-----+    +----------------------------------+            |
|| PRM |    | APM (Audio Processing Manager)   |            |
|+-----+    |  . Graph Management              |            |  
|           |  . Command Handing               |            |  
|           |  . Event Management              |            |  
|           |  ...                             |            |  
|           +----------------------------------+            |  
|                            ^                              |
|____________________________|______________________________|
                             |  
                             |   LPASS AIF
 ____________________________|______________________________
|                            |            Audio I/O         |
|                            v                              |
|   +--------------------------------------------------+    |
|    |                Audio devices                     |   |
|    | CODEC | HDMI-TX | PCM  | SLIMBUS | I2S |MI2S |...|   |
|    |                                                  |   |
|    +--------------------------------------------------+   |
|___________________________________________________________|

AudioReach has constructs of sub-graph, container and modules.
Each sub-graph can have N containers and each Container can have N Modules
and connections between them can be linear or non-linear.
An audio function can be realized with one or many connected
sub-graphs. There are also control/event paths between modules that can
be wired up while building graph to achieve various control mechanism
between modules. These concepts of Sub-Graph, Containers and Modules
are represented in ASoC topology.

Here is simple I2S graph with a Write Shared Memory and a
Volume control module within a single Subgraph (1) with one Container (1)
and 5 modules.

  ____________________________________________________________
 |                        Sub-Graph [1]                       |
 |  _______________________________________________________   |
 | |                       Container [1]                   |  |
 | | [WR_SH] -> [PCM DEC] -> [PCM CONV] -> [VOL]-> [I2S-EP]|  |
 | |_______________________________________________________|  |
 |____________________________________________________________|

For now this graph is split into two subgraphs to achieve dpcm like below:
 ________________________________________________    _________________
|                Sub-Graph [1]                   |  |  Sub-Graph [2]  |
|  ____________________________________________  |  |  _____________  |
| |              Container [1]                 | |  | |Container [2]| |
| | [WR_SH] -> [PCM DEC] -> [PCM CONV] -> [VOL]| |  | |   [I2S-EP]  | |
| |____________________________________________| |  | |_____________| |
|________________________________________________|  |_________________|

                                                      _________________
                                                    |  Sub-Graph [3]  |
                                                    |  _____________  |
                                                    | |Container [3]| |
                                                    | |  [DMA-EP]   | |
                                                    | |_____________| |
                                                    |_________________|


This patchset adds very minimal support for AudioReach which includes
supporting sub-graphs containing CODEC DMA ports and simple PCM
Decoder/Encoder and Logger Modules. Additional capabilities will
be built over time to expose features offered by AudioReach. 

This patchset is Tested on SM8250 SoC based Qualcomm Robotics Platform RB5
and SM9250 MTP with WSA881X Smart Speaker Amplifiers, DMICs connected via
VA Macro and WCD938x Codec connected via TX and RX Macro.

Sample WIP ASoC graphs are available at 
https://git.linaro.org/people/srinivas.kandagatla/audioreach-topology.git/

Thanks,
srini

Changes since RFC:
	- moved GPR support into APR driver to avoid code duplication.
	- Fixed various dt-bindings-check warnings and errors.
	- Moved include/dt-bindings headers to dt-bindings patch.
	- added PGA widget support
	- Fixed few ordering issues while testing with ASoC Topology 2.0
	- Removed duplicate defines in various headers

Srinivas Kandagatla (16):
  soc: dt-bindings: qcom: add gpr bindings
  soc: qcom: apr: make code more reuseable
  soc: qcom: apr: Add GPR support
  ASoC: qcom: dt-bindings: add bindings Audio Processing manager
  ASoC: qcom: audioreach: add basic pkt alloc support
  ASoC: qcom: audioreach: add q6apm support
  ASoC: qcom: audioreach: add module configuration command helpers
  ASoC: qcom: audioreach: add topology support
  ASoC: qcom: audioreach: add q6apm-dai support
  ASoC: qcom: audioreach: add bedai support
  ASoC: qcom: dt-bindings: add bindings for Proxy Resource Manager
  ASoC: qcom: audioreach: add q6prm support
  ASoC: qcom: dt-bindings: add audioreach soundcard compatibles
  ASoC: qcom: audioreach: add volume ctrl module support
  ASoC: qcom: audioreach: topology add dapm pga support
  ASoC: qcom: sm8250: Add audioreach support

 .../bindings/soc/qcom/qcom,gpr.yaml           |   83 ++
 .../devicetree/bindings/sound/qcom,q6apm.yaml |   87 ++
 .../devicetree/bindings/sound/qcom,q6prm.yaml |   43 +
 .../bindings/sound/qcom,sm8250.yaml           |   43 +
 drivers/soc/qcom/Kconfig                      |    8 +
 drivers/soc/qcom/apr.c                        |  270 +++-
 include/dt-bindings/soc/qcom,gpr.h            |   18 +
 include/dt-bindings/sound/qcom,q6apm.h        |    8 +
 include/linux/soc/qcom/apr.h                  |   69 +-
 include/uapi/sound/snd_ar_tokens.h            |  198 +++
 sound/soc/qcom/Kconfig                        |   19 +
 sound/soc/qcom/Makefile                       |    1 +
 sound/soc/qcom/audioreach/Makefile            |   12 +
 sound/soc/qcom/audioreach/audioreach.c        | 1162 +++++++++++++++++
 sound/soc/qcom/audioreach/audioreach.h        |  676 ++++++++++
 sound/soc/qcom/audioreach/q6apm-bedai.c       |  323 +++++
 sound/soc/qcom/audioreach/q6apm-dai.c         |  494 +++++++
 sound/soc/qcom/audioreach/q6apm.c             |  963 ++++++++++++++
 sound/soc/qcom/audioreach/q6apm.h             |  174 +++
 sound/soc/qcom/audioreach/q6prm.c             |  414 ++++++
 sound/soc/qcom/audioreach/topology.c          | 1075 +++++++++++++++
 sound/soc/qcom/sm8250.c                       |  144 +-
 22 files changed, 6236 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,gpr.yaml
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6apm.yaml
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6prm.yaml
 create mode 100644 include/dt-bindings/soc/qcom,gpr.h
 create mode 100644 include/dt-bindings/sound/qcom,q6apm.h
 create mode 100644 include/uapi/sound/snd_ar_tokens.h
 create mode 100644 sound/soc/qcom/audioreach/Makefile
 create mode 100644 sound/soc/qcom/audioreach/audioreach.c
 create mode 100644 sound/soc/qcom/audioreach/audioreach.h
 create mode 100644 sound/soc/qcom/audioreach/q6apm-bedai.c
 create mode 100644 sound/soc/qcom/audioreach/q6apm-dai.c
 create mode 100644 sound/soc/qcom/audioreach/q6apm.c
 create mode 100644 sound/soc/qcom/audioreach/q6apm.h
 create mode 100644 sound/soc/qcom/audioreach/q6prm.c
 create mode 100644 sound/soc/qcom/audioreach/topology.c

Comments

Pierre-Louis Bossart July 14, 2021, 4:20 p.m. UTC | #1
> +void gpr_free_port(gpr_port_t *port)
> +{
> +	struct packet_router *gpr = port->pr;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&gpr->svcs_lock, flags);
> +	idr_remove(&gpr->svcs_idr, port->id);
> +	spin_unlock_irqrestore(&gpr->svcs_lock, flags);

maybe add a comment somewhere on why you need the irqsave/irqrestore version of spin_lock/unlock?

It's not very clear by looking at this patch only that those locks are handled in interrupt context.

> +
> +	kfree(port);
> +}
> +EXPORT_SYMBOL_GPL(gpr_free_port);
> +
> +gpr_port_t *gpr_alloc_port(struct apr_device *gdev, struct device *dev,
> +				gpr_port_cb cb,	void *priv)
> +{
> +	struct packet_router *pr = dev_get_drvdata(gdev->dev.parent);
> +	gpr_port_t *port;
> +	struct pkt_router_svc *svc;
> +	int id;
> +
> +	port = kzalloc(sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return ERR_PTR(-ENOMEM);
> +
> +	svc = port;
> +	svc->callback = cb;
> +	svc->pr = pr;
> +	svc->priv = priv;
> +	svc->dev = dev;
> +	spin_lock_init(&svc->lock);
> +
> +	spin_lock(&pr->svcs_lock);
> +	id = idr_alloc_cyclic(&pr->svcs_idr, svc, GPR_DYNAMIC_PORT_START,
> +			      GPR_DYNAMIC_PORT_END, GFP_ATOMIC);
> +	if (id < 0) {
> +		dev_err(dev, "Unable to allocate dynamic GPR src port\n");
> +		kfree(port);
> +		spin_unlock(&pr->svcs_lock);

nit-pick: more this before the dev_err?

> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	svc->id = id;
> +	spin_unlock(&pr->svcs_lock);
> +
> +	dev_info(dev, "Adding GPR src port (%x)\n", svc->id);
> +
> +	return port;
> +}

> +static int gpr_do_rx_callback(struct packet_router *gpr, struct apr_rx_buf *abuf)
> +{
> +	uint16_t hdr_size, ver;
> +	struct pkt_router_svc *svc = NULL;

unnecessary init? it's overritten...

> +	struct gpr_resp_pkt resp;
> +	struct gpr_hdr *hdr;
> +	unsigned long flags;
> +	void *buf = abuf->buf;
> +	int len = abuf->len;
> +
> +	hdr = buf;
> +	ver = hdr->version;
> +	if (ver > GPR_PKT_VER + 1)
> +		return -EINVAL;
> +
> +	hdr_size = hdr->hdr_size;
> +	if (hdr_size < GPR_PKT_HEADER_WORD_SIZE) {
> +		dev_err(gpr->dev, "GPR: Wrong hdr size:%d\n", hdr_size);
> +		return -EINVAL;
> +	}
> +
> +	if (hdr->pkt_size < GPR_PKT_HEADER_BYTE_SIZE || hdr->pkt_size != len) {
> +		dev_err(gpr->dev, "GPR: Wrong packet size\n");
> +		return -EINVAL;
> +	}
> +
> +	resp.hdr = *hdr;
> +	resp.payload_size = hdr->pkt_size - (hdr_size * 4);
> +
> +	/*
> +	 * NOTE: hdr_size is not same as GPR_HDR_SIZE as remote can include
> +	 * optional headers in to gpr_hdr which should be ignored
> +	 */
> +	if (resp.payload_size > 0)
> +		resp.payload = buf + (hdr_size *  4);
> +
> +
> +	spin_lock_irqsave(&gpr->svcs_lock, flags);
> +	svc = idr_find(&gpr->svcs_idr, hdr->dest_port);

... here 

> +	spin_unlock_irqrestore(&gpr->svcs_lock, flags);
> +
> +	if (!svc) {
> +		dev_err(gpr->dev, "GPR: Port(%x) is not registered\n",
> +			hdr->dest_port);
> +		return -EINVAL;
> +	}
> +
> +	if (svc->callback)
> +		svc->callback(&resp, svc->priv, 0);
> +
> +	return 0;
> +}
> +
Pierre-Louis Bossart July 14, 2021, 4:30 p.m. UTC | #2
> diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
> index cc7c1de2f1d9..721ea56b2cb5 100644
> --- a/sound/soc/qcom/Kconfig
> +++ b/sound/soc/qcom/Kconfig
> @@ -103,6 +103,12 @@ config SND_SOC_QDSP6
>  	 audio drivers. This includes q6asm, q6adm,
>  	 q6afe interfaces to DSP using apr.
>  
> +config SND_SOC_QCOM_AUDIOREACH
> +	tristate "SoC ALSA audio drives for Qualcomm AUDIOREACH"

typo: drivers?


> +/* container config */
> +struct apm_container_obj  {
> +	struct apm_container_cfg container_cfg;
> +	/* Capablity ID list */

typo: Capability

> +	struct apm_prop_data cap_data;
> +	uint32_t num_capablity_id;


> +static void *__audioreach_alloc_pkt(int payload_size, uint32_t opcode,
> +				     uint32_t token, uint32_t src_port,
> +				     uint32_t dest_port, bool has_cmd_hdr)
> +{
> +	struct apm_cmd_header *cmd_header;
> +	struct gpr_pkt *pkt;
> +	void *p;
> +	int pkt_size = GPR_HDR_SIZE + payload_size;
> +
> +	if (has_cmd_hdr)
> +		pkt_size += APM_CMD_HDR_SIZE;
> +
> +	p = kzalloc(pkt_size, GFP_ATOMIC);

is GFP_ATOMIC required? it's the same question really than my earlier one on spinlock_irqsave, it's rather hard to figure out in what context this code will run.

> +	if (!p)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pkt = p;
> +	pkt->hdr.version = GPR_PKT_VER;
> +	pkt->hdr.hdr_size = 6;

magic number. looks like a missing macro...

> +	pkt->hdr.pkt_size = pkt_size;
> +	pkt->hdr.dest_port = dest_port;
> +	pkt->hdr.src_port = src_port;
> +
> +	pkt->hdr.dest_domain = GPR_DOMAIN_ID_ADSP;
> +	pkt->hdr.src_domain = GPR_DOMAIN_ID_APPS;
> +	pkt->hdr.token = token;
> +	pkt->hdr.opcode = opcode;
> +
> +	if (has_cmd_hdr) {
> +		p = p + GPR_HDR_SIZE;
> +		cmd_header = p;
> +		cmd_header->payload_size = payload_size;
> +	}
> +
> +	return pkt;
> +}
Pierre-Louis Bossart July 14, 2021, 4:40 p.m. UTC | #3
>  /* SubGraph Config */
> @@ -32,7 +33,7 @@ struct apm_sub_graph_params  {
>  /* container config */
>  struct apm_container_obj  {
>  	struct apm_container_cfg container_cfg;
> -	/* Capablity ID list */
> +	/* Capability ID list */

squash in wrong patch, this should have been included in the previous patch.

>  	struct apm_prop_data cap_data;
>  	uint32_t num_capablity_id;

and this is still wrong...

>  	uint32_t capability_id;
> @@ -270,3 +271,308 @@ void *audioreach_alloc_apm_cmd_pkt(int pkt_size, uint32_t opcode,
>  				       APM_MODULE_INSTANCE_ID,
>  				       true);
>  }
> +
> +static void apm_populate_container_config(
> +			struct apm_container_obj *cfg,
> +			struct audioreach_container *cont)
> +{
> +
> +	/* Container Config */
> +	cfg->container_cfg.container_id = cont->container_id;
> +	cfg->container_cfg.num_prop = 4;
> +
> +	/* Capablity list */

Man, again ...

> +	cfg->cap_data.prop_id = APM_CONTAINER_PROP_ID_CAPABILITY_LIST;
> +	cfg->cap_data.prop_size = APM_CONTAINER_PROP_ID_CAPABILITY_SIZE;
> +	cfg->num_capablity_id = 1;
> +	cfg->capability_id = cont->capability_id;
> +
> +	/* Graph Position */
> +	cfg->pos_data.prop_id = APM_CONTAINER_PROP_ID_GRAPH_POS;
> +	cfg->pos_data.prop_size = sizeof(struct apm_cont_prop_id_graph_pos);
> +	cfg->pos.graph_pos = cont->graph_pos;
> +
> +	/* Stack size */
> +	cfg->stack_data.prop_id = APM_CONTAINER_PROP_ID_STACK_SIZE;
> +	cfg->stack_data.prop_size = sizeof(struct
> +					       apm_cont_prop_id_stack_size);
> +	cfg->stack.stack_size = cont->stack_size;
> +
> +	/* Proc domain */
> +	cfg->domain_data.prop_id = APM_CONTAINER_PROP_ID_PROC_DOMAIN;
> +	cfg->domain_data.prop_size = sizeof(struct
> +					       apm_cont_prop_id_domain);
> +	cfg->domain.proc_domain = cont->proc_domain;
> +}

> +static struct audioreach_graph *q6apm_get_audioreach_graph(struct q6apm *apm,
> +						      uint32_t graph_id)
> +{
> +	struct audioreach_graph *graph = NULL;

useless init

> +	struct audioreach_graph_info *info;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&apm->lock, flags);
> +	graph = idr_find(&apm->graph_idr, graph_id);
> +	spin_unlock_irqrestore(&apm->lock, flags);
> +
> +	if (graph) {
> +		kref_get(&graph->refcount);
> +		return graph;
> +	}
> +
> +	info = idr_find(&apm->graph_info_idr, graph_id);
> +
> +	if (!info)
> +		return ERR_PTR(-ENODEV);
> +
> +	graph = kzalloc(sizeof(*graph), GFP_KERNEL);
> +	if (!graph)
> +		return ERR_PTR(-ENOMEM);
> +
> +	graph->apm = apm;
> +	graph->info = info;
> +	graph->id = graph_id;
> +
> +	/* Assuming Linear Graphs only for now! */
> +	graph->graph = audioreach_alloc_graph_pkt(apm, &info->sg_list, graph_id);
> +	if (IS_ERR(graph->graph))
> +		return ERR_PTR(-ENOMEM);
> +
> +	spin_lock(&apm->lock);
> +	idr_alloc(&apm->graph_idr, graph, graph_id,
> +		  graph_id + 1, GFP_ATOMIC);

ATOMIC?

> +	spin_unlock(&apm->lock);
> +
> +	kref_init(&graph->refcount);
> +
> +	q6apm_send_cmd_sync(apm, graph->graph, 0);
> +
> +	return graph;
> +}
> +
> +static int audioreach_graph_mgmt_cmd(struct audioreach_graph *graph,
> +				      uint32_t opcode)
> +{
> +	struct gpr_pkt *pkt;
> +	void *p;
> +	int i = 0, rc, payload_size;
> +	struct q6apm *apm = graph->apm;
> +	struct audioreach_graph_info *info = graph->info;
> +	int num_sub_graphs = info->num_sub_graphs;
> +	struct apm_graph_mgmt_cmd *mgmt_cmd;
> +	struct apm_module_param_data *param_data;
> +	struct audioreach_sub_graph *sg;
> +
> +	payload_size = APM_GRAPH_MGMT_PSIZE(num_sub_graphs);
> +
> +	p = audioreach_alloc_apm_cmd_pkt(payload_size, opcode, 0);
> +	if (IS_ERR(p))
> +		return -ENOMEM;
> +
> +	pkt = p;
> +	p = p + GPR_HDR_SIZE + APM_CMD_HDR_SIZE;
> +
> +	mgmt_cmd = p;
> +	mgmt_cmd->num_sub_graphs = num_sub_graphs;
> +
> +	param_data = &mgmt_cmd->param_data;
> +	param_data->module_instance_id = APM_MODULE_INSTANCE_ID;
> +	param_data->param_id = APM_PARAM_ID_SUB_GRAPH_LIST;
> +	param_data->param_size = payload_size - APM_MODULE_PARAM_DATA_SIZE;
> +
> +	list_for_each_entry(sg, &info->sg_list, node) {
> +		mgmt_cmd->sub_graph_id_list[i++] = sg->sub_graph_id;
> +	}
> +
> +	rc = q6apm_send_cmd_sync(apm, pkt, 0);
> +
> +	kfree(pkt);
> +
> +	return rc;
> +}
> +
> +static void q6apm_put_audioreach_graph(struct kref *ref)
> +{
> +	struct audioreach_graph *graph;
> +	struct q6apm *apm;
> +	unsigned long flags;
> +
> +	graph = container_of(ref, struct audioreach_graph, refcount);
> +	apm = graph->apm;
> +
> +	audioreach_graph_mgmt_cmd(graph, APM_CMD_GRAPH_CLOSE);
> +
> +	spin_lock_irqsave(&apm->lock, flags);
> +	graph = idr_remove(&apm->graph_idr, graph->id);
> +	spin_unlock_irqrestore(&apm->lock, flags);
> +
> +	kfree(graph->graph);
> +	kfree(graph);

earlier in the _get routine, you had a kref_get(&graph->refcount)

is it intentional that there's kref_put?

adding a comment on the refcounts might be useful...
Pierre-Louis Bossart July 14, 2021, 4:48 p.m. UTC | #4
> +static int audioreach_shmem_set_media_format(struct q6apm_graph *graph,
> +				       struct audioreach_module *module,
> +				       int direction, uint32_t rate,
> +				       uint32_t num_channels,
> +				       u8 channel_map[PCM_MAX_NUM_CHANNEL],
> +				       uint16_t bits_per_sample)
> +{
> +	struct apm_module_param_data *param_data;
> +	struct payload_media_fmt_pcm *cfg;
> +	struct media_format *header;
> +	int rc, payload_size;
> +	struct gpr_pkt *pkt;
> +	void *p;
> +
> +	if (num_channels < 0 || num_channels > 2)
> +		dev_err(graph->dev, "Error: Invalid channels (%d)!\n", num_channels);

that doesn't sound good, you flag num_channels as an invalid value but still continue using it.

> +
> +	payload_size = APM_SHMEM_FMT_CFG_PSIZE(num_channels) + APM_MODULE_PARAM_DATA_SIZE;
> +
> +	p = audioreach_alloc_cmd_pkt(payload_size, APM_CMD_SET_CFG, 0,
> +				     graph->port->id, module->instance_id);
> +	if (IS_ERR(p))
> +		return -ENOMEM;
> +
> +	pkt = p;
> +	p = p + GPR_HDR_SIZE + APM_CMD_HDR_SIZE;
> +
> +	param_data = p;
> +	param_data->module_instance_id = module->instance_id;
> +	param_data->error_code = 0;
> +	param_data->param_id = PARAM_ID_MEDIA_FORMAT;
> +	param_data->param_size = payload_size - APM_MODULE_PARAM_DATA_SIZE;
> +	p = p + APM_MODULE_PARAM_DATA_SIZE;
> +
> +	header = p;
> +	header->data_format = DATA_FORMAT_FIXED_POINT;
> +	header->fmt_id = MEDIA_FMT_ID_PCM;
> +	header->payload_size = payload_size - sizeof(*header);
> +
> +	p = p + sizeof(*header);
> +	cfg = p;
> +	cfg->sample_rate = rate;
> +	cfg->bit_width = bits_per_sample;
> +	cfg->alignment = PCM_LSB_ALIGNED;
> +	cfg->bits_per_sample = bits_per_sample;
> +	cfg->q_factor = bits_per_sample - 1;
> +	cfg->endianness = PCM_LITTLE_ENDIAN;
> +	cfg->num_channels = num_channels;
> +
> +	if (num_channels == 1) {
> +		cfg->channel_mapping[0] =  PCM_CHANNEL_L;
> +	} else if (num_channels == 2) {
> +		cfg->channel_mapping[0] =  PCM_CHANNEL_L;
> +		cfg->channel_mapping[1] =  PCM_CHANNEL_R;
> +	} else {
> +		dev_err(graph->dev, "Error: Invalid channels (%d)!\n", num_channels);

already tested above.

> +	}
> +
> +	rc = audioreach_graph_send_cmd_sync(graph, pkt, 0);
> +
> +	kfree(pkt);
> +
> +	return rc;
> +}

> +int audioreach_shared_memory_send_eos(struct q6apm_graph *graph)
> +{
> +	struct data_cmd_wr_sh_mem_ep_eos *eos;
> +	struct gpr_pkt *pkt;
> +	int rc = 0, iid;

useless init

> +	void *p;
> +
> +	iid = q6apm_graph_get_rx_shmem_module_iid(graph);
> +	p = audioreach_alloc_cmd_pkt(sizeof(*eos),
> +				      DATA_CMD_WR_SH_MEM_EP_EOS,
> +				      0,
> +				      graph->port->id, iid);
> +	if (IS_ERR(p))
> +		return -ENOMEM;
> +
> +	pkt = p;
> +	eos = p + GPR_HDR_SIZE + APM_CMD_HDR_SIZE;
> +
> +	eos->policy = WR_SH_MEM_EP_EOS_POLICY_LAST;
> +
> +	rc = gpr_send_port_pkt(graph->port, pkt);
> +	kfree(pkt);
> +
> +	return rc;
> +}

> +int q6apm_unmap_memory_regions(struct q6apm_graph *graph,
> +			       unsigned int dir)
> +{
> +	struct audioreach_graph_data *data;
> +	struct apm_cmd_shared_mem_unmap_regions *cmd = NULL;

useless init

> +	struct gpr_pkt *pkt;
> +	void *p;
> +	int rc;
> +
> +	if (dir == SNDRV_PCM_STREAM_PLAYBACK)
> +		data = &graph->rx_data;
> +	else
> +		data = &graph->tx_data;
> +
> +	if (!data->mem_map_handle)
> +		return 0;
> +
> +	p = audioreach_alloc_apm_pkt(sizeof(*cmd),
> +				      APM_CMD_SHARED_MEM_UNMAP_REGIONS, dir,
> +				      graph->port->id);
> +	if (IS_ERR(p))
> +		return -ENOMEM;
> +
> +	pkt = p;
> +	cmd = p + GPR_HDR_SIZE;
> +	cmd->mem_map_handle = data->mem_map_handle;
> +
> +	rc = audioreach_graph_send_cmd_sync(graph, pkt, APM_CMD_SHARED_MEM_UNMAP_REGIONS);
> +	kfree(pkt);
> +
> +	audioreach_graph_free_buf(graph);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(q6apm_unmap_memory_regions);
Pierre-Louis Bossart July 14, 2021, 4:59 p.m. UTC | #5
> +static void event_handler(uint32_t opcode, uint32_t token,
> +			  uint32_t *payload, void *priv)
> +{
> +	struct q6apm_dai_rtd *prtd = priv;
> +	struct snd_pcm_substream *substream = prtd->substream;
> +
> +	switch (opcode) {
> +	case APM_CLIENT_EVENT_CMD_EOS_DONE:
> +		prtd->state = Q6APM_STREAM_STOPPED;
> +		break;
> +	case APM_CLIENT_EVENT_DATA_WRITE_DONE: {
> +		prtd->pcm_irq_pos += prtd->pcm_count;
> +		snd_pcm_period_elapsed(substream);

A comment on the relationship between data writes and periods would be nice. It looks like the code assumes the period and data transfers are identical, but periods can be chosen by applications, can't they?

> +		if (prtd->state == Q6APM_STREAM_RUNNING) {
> +			q6apm_write_async(prtd->graph,
> +					   prtd->pcm_count, 0, 0, NO_TIMESTAMP);
> +		}
> +
> +		break;
> +		}
> +	case APM_CLIENT_EVENT_DATA_READ_DONE:
> +		prtd->pcm_irq_pos += prtd->pcm_count;
> +		snd_pcm_period_elapsed(substream);
> +		if (prtd->state == Q6APM_STREAM_RUNNING)
> +			q6apm_read(prtd->graph);
> +
> +		break;
> +	default:
> +		break;
> +	}
> +}

> +static int q6apm_dai_trigger(struct snd_soc_component *component,
> +			     struct snd_pcm_substream *substream, int cmd)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct q6apm_dai_rtd *prtd = runtime->private_data;
> +	int ret = 0;
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +			ret = q6apm_write_async(prtd->graph, prtd->pcm_count, 0, 0, NO_TIMESTAMP);
> +		break;
> +	case SNDRV_PCM_TRIGGER_STOP:
> +		prtd->state = Q6APM_STREAM_STOPPED;
> +		//ret = q6apm_cmd_nowait(prtd->graph, CMD_EOS);
> +		break;
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		//ret = q6apm_cmd_nowait(prtd->graph, CMD_PAUSE);

these // comments are odd, with a clear imbalance between suspend/resume and pause_push/pause_release... 
is this intentional or a left-over from an experiment?

> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int q6apm_dai_open(struct snd_soc_component *component,
> +			  struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct snd_soc_pcm_runtime *soc_prtd = substream->private_data;
> +	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_prtd, 0);
> +	struct q6apm_dai_rtd *prtd;
> +	struct q6apm_dai_data *pdata;
> +	struct device *dev = component->dev;
> +	int ret;
> +	int graph_id;
> +
> +	graph_id = cpu_dai->driver->id;
> +
> +	pdata = snd_soc_component_get_drvdata(component);
> +	if (!pdata) {
> +		dev_err(component->dev, "Drv data not found ..\n");
> +		return -EINVAL;
> +	}
> +
> +	prtd = kzalloc(sizeof(*prtd), GFP_KERNEL);
> +	if (prtd == NULL)
> +		return -ENOMEM;
> +
> +	prtd->substream = substream;
> +
> +	prtd->graph = q6apm_graph_open(dev, (q6apm_cb)event_handler,
> +				       prtd, graph_id);
> +	if (IS_ERR(prtd->graph)) {
> +		pr_info("%s: Could not allocate memory\n", __func__);
> +		ret = PTR_ERR(prtd->graph);
> +		kfree(prtd);
> +		return ret;
> +	}
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		runtime->hw = q6apm_dai_hardware_playback;
> +	else if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> +		runtime->hw = q6apm_dai_hardware_capture;
> +
> +	/* 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)
> +		dev_err(dev, "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,
> +			BUFFER_BYTES_MIN, BUFFER_BYTES_MAX);
> +		if (ret < 0) {
> +			dev_err(dev, "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) {
> +		dev_err(dev, "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) {
> +		dev_err(dev, "constraint for buffer bytes step ret = %d\n",
> +								ret);
> +	}

dev_err() used but no return code sent?

> +
> +	runtime->private_data = prtd;
> +	runtime->dma_bytes = 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 q6apm_dai_hw_params(struct snd_soc_component *component,
> +			       struct snd_pcm_substream *substream,
> +			       struct snd_pcm_hw_params *params)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct q6apm_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;

default:
    return -EINVAL
?

> +	}
> +
> +	return 0;
> +}
> +
Pierre-Louis Bossart July 14, 2021, 5:03 p.m. UTC | #6
> +static int q6dma_hw_params(struct snd_pcm_substream *substream,
> +			   struct snd_pcm_hw_params *params,
> +			   struct snd_soc_dai *dai)
> +{
> +	struct q6apm_bedai_data *dai_data = dev_get_drvdata(dai->dev);
> +	struct q6apm_cdc_dma_cfg *cfg = &dai_data->port_config[dai->id].dma_cfg;
> +
> +	cfg->bit_width = params_width(params);
> +	cfg->sample_rate = params_rate(params);
> +	cfg->num_channels = params_channels(params);
> +
> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_S16_LE:
> +		dai_data->bits_per_sample[dai->id] = 16;
> +		break;
> +	case SNDRV_PCM_FORMAT_S24_LE:
> +		dai_data->bits_per_sample[dai->id] = 24;
> +		break;

default:
    return -EINVAL;

?

> +	}
> +
> +	return 0;
> +}
> +
> +static void q6apm_bedai_shutdown(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai)
> +{
> +	struct q6apm_bedai_data *dai_data = dev_get_drvdata(dai->dev);
> +	int rc;
> +
> +	if (!dai_data->is_port_started[dai->id])
> +		return;
> +	rc = q6apm_graph_stop(dai_data->graph[dai->id]);
> +	if (rc < 0)
> +		dev_err(dai->dev, "fail to close APM port (%d)\n", rc);
> +
> +	q6apm_graph_close(dai_data->graph[dai->id]);
> +	dai_data->is_port_started[dai->id] = false;

difficult to follow, this mixes '!start', 'stop', 'close'


> +
> +}
> +
> +static int q6apm_bedai_prepare(struct snd_pcm_substream *substream,
> +			       struct snd_soc_dai *dai)
> +{
> +	struct q6apm_bedai_data *dai_data = dev_get_drvdata(dai->dev);
> +	struct q6apm_cdc_dma_cfg *cfg = &dai_data->port_config[dai->id].dma_cfg;
> +	int graph_id = dai->id;
> +	int rc;
> +	int ret;
> +	struct q6apm_graph *graph;
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		graph = q6apm_graph_open(dai->dev, NULL, dai->dev, graph_id);
> +		if (IS_ERR(graph)) {
> +			dev_err(dai->dev, "Failed to open graph (%d)\n",
> +				graph_id);
> +			ret = PTR_ERR(graph);
> +			return ret;
> +		}
> +		dai_data->graph[graph_id] = graph;
> +	}
> +
> +	rc = q6apm_graph_media_format_pcm(dai_data->graph[dai->id],
> +					  substream->stream, cfg->sample_rate,
> +					  cfg->num_channels, NULL, cfg->bit_width);
> +
> +	rc = q6apm_graph_prepare(dai_data->graph[dai->id]);

any good static analyzer would tell you you didn't test rc before overriding it..

this looks rather questionable.

> +	rc = q6apm_graph_start(dai_data->graph[dai->id]);
> +	if (rc < 0) {
> +		dev_err(dai->dev, "fail to start APM port %x\n", dai->id);
> +		return rc;
> +	}
> +	dai_data->is_port_started[dai->id] = true;
> +
> +	return 0;
> +}
Pierre-Louis Bossart July 14, 2021, 5:09 p.m. UTC | #7
> +static int q6prm_send_cmd_sync(struct q6prm *prm, struct gpr_pkt *pkt,
> +			uint32_t rsp_opcode)
> +{
> +	gpr_device_t *gdev = prm->gdev;
> +	struct gpr_hdr *hdr = &pkt->hdr;
> +	int rc;
> +
> +	mutex_lock(&prm->lock);
> +	prm->result.opcode = 0;
> +	prm->result.status = 0;
> +
> +	rc = gpr_send_pkt(prm->gdev, pkt);
> +	if (rc < 0)
> +		goto err;
> +
> +	if (rsp_opcode)
> +		rc = wait_event_timeout(prm->wait,
> +					(prm->result.opcode == hdr->opcode) ||
> +					(prm->result.opcode == rsp_opcode),
> +					5 * HZ);
> +	else
> +		rc = wait_event_timeout(prm->wait,
> +					(prm->result.opcode == hdr->opcode),
> +					5 * HZ);
> +
> +	if (!rc) {
> +		dev_err(&gdev->dev, "CMD timeout for [%x] opcode\n",
> +			hdr->opcode);
> +		rc = -ETIMEDOUT;
> +	} else if (prm->result.status > 0) {
> +		dev_err(&gdev->dev, "DSP returned error[%x] %x\n", hdr->opcode,
> +			prm->result.status);
> +		rc = -EINVAL;
> +	} else {
> +		dev_err(&gdev->dev, "DSP returned [%x]\n",
> +			prm->result.status);
> +		rc = 0;
> +	}
> +
> +err:
> +	mutex_unlock(&prm->lock);
> +	return rc;
> +}

In patch7 you had more or less the same code. can a helper be used?

+int audioreach_graph_send_cmd_sync(struct q6apm_graph *graph,
+				   struct gpr_pkt *pkt, uint32_t rsp_opcode)
+{
+
+	struct device *dev = graph->dev;
+	struct gpr_hdr *hdr = &pkt->hdr;
+	int rc;
+
+	mutex_lock(&graph->cmd_lock);
+	graph->result.opcode = 0;
+	graph->result.status = 0;
+
+	rc = gpr_send_port_pkt(graph->port, pkt);
+	if (rc < 0)
+		goto err;
+
+	if (rsp_opcode)
+		rc = wait_event_timeout(graph->cmd_wait,
+					(graph->result.opcode == hdr->opcode) ||
+					(graph->result.opcode == rsp_opcode),
+					5 * HZ);
+	else
+		rc = wait_event_timeout(graph->cmd_wait,
+					(graph->result.opcode == hdr->opcode),
+					5 * HZ);
+
+	if (!rc) {
+		dev_err(dev, "CMD timeout for [%x] opcode\n", hdr->opcode);
+		rc = -ETIMEDOUT;
+	} else if (graph->result.status > 0) {
+		dev_err(dev, "DSP returned error[%x] %x\n", hdr->opcode,
+			graph->result.status);
+		rc = -EINVAL;
+	} else {
+		dev_err(dev, "DSP returned [%x]\n", graph->result.status);
+		rc = 0;
+	}
+
+err:
+	mutex_unlock(&graph->cmd_lock);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(audioreach_graph_send_cmd_sync);


> +static int q6prm_set_hw_core_req(struct device *dev, uint32_t hw_block_id,   bool enable)
> +{
> +	struct prm_cmd_request_hw_core *req;
> +	struct apm_module_param_data *param_data;
> +	struct gpr_pkt *pkt;
> +	struct q6prm *prm = dev_get_drvdata(dev);
> +	gpr_device_t *gdev  = prm->gdev;
> +	void *p;
> +	int rc = 0;
> +	uint32_t opcode, rsp_opcode;
> +
> +	if (enable) {
> +		opcode = PRM_CMD_REQUEST_HW_RSC;
> +		rsp_opcode = PRM_CMD_RSP_REQUEST_HW_RSC;
> +	} else {
> +		opcode = PRM_CMD_RELEASE_HW_RSC;
> +		rsp_opcode = PRM_CMD_RSP_RELEASE_HW_RSC;
> +	}
> +
> +	p = audioreach_alloc_cmd_pkt(sizeof(*req), opcode, 0, gdev->svc.id,
> +				     GPR_PRM_MODULE_IID);
> +	if (IS_ERR(p))
> +		return -ENOMEM;
> +
> +	pkt = p;
> +	req = p + GPR_HDR_SIZE + APM_CMD_HDR_SIZE;
> +
> +	param_data = &req->param_data;
> +
> +	param_data->module_instance_id = GPR_PRM_MODULE_IID;
> +	param_data->error_code = 0;
> +	param_data->param_id = PARAM_ID_RSC_HW_CORE;
> +	param_data->param_size = sizeof(*req) - APM_MODULE_PARAM_DATA_SIZE;
> +
> +	req->hw_clk_id = hw_block_id;
> +
> +	q6prm_send_cmd_sync(prm, pkt, rsp_opcode);
> +
> +	kfree(pkt);
> +
> +	return rc;

rc is not assigned, should this not trap the result of sending a command?

> +}
> +
> +static int q6prm_set_lpass_clock(struct device *dev, int clk_id, int clk_attr,
> +				 int clk_root, unsigned int freq)
> +{
> +	struct prm_cmd_request_rsc *req;
> +	struct apm_module_param_data *param_data;
> +	struct gpr_pkt *pkt;
> +	struct q6prm *prm = dev_get_drvdata(dev);
> +	gpr_device_t *gdev  = prm->gdev;
> +	void *p;
> +	int rc = 0;
> +
> +	p = audioreach_alloc_cmd_pkt(sizeof(*req), PRM_CMD_REQUEST_HW_RSC,
> +				     0, gdev->svc.id, GPR_PRM_MODULE_IID);
> +	if (IS_ERR(p))
> +		return -ENOMEM;
> +
> +	pkt = p;
> +	req = p + GPR_HDR_SIZE + APM_CMD_HDR_SIZE;
> +
> +	param_data = &req->param_data;
> +
> +	param_data->module_instance_id = GPR_PRM_MODULE_IID;
> +	param_data->error_code = 0;
> +	param_data->param_id = PARAM_ID_RSC_AUDIO_HW_CLK;
> +	param_data->param_size = sizeof(*req) - APM_MODULE_PARAM_DATA_SIZE;
> +
> +	req->num_clk_id = 1;
> +	req->clock_ids[0].clock_id = clk_id;
> +	req->clock_ids[0].clock_freq = freq;
> +	req->clock_ids[0].clock_attri = clk_attr;
> +	req->clock_ids[0].clock_root = clk_root;
> +
> +	q6prm_send_cmd_sync(prm, pkt, PRM_CMD_RSP_REQUEST_HW_RSC);

rc = q6prm_send_cmd_sync(prm, pkt, PRM_CMD_RSP_REQUEST_HW_RSC);

?

> +
> +	kfree(pkt);
> +
> +	return rc;
> +}
> +
Pierre-Louis Bossart July 14, 2021, 5:12 p.m. UTC | #8
On 7/14/21 10:30 AM, Srinivas Kandagatla wrote:
> This patch adds support for parsing dt for AudioReach based soundcards
> which only have backend DAI links in DT.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  sound/soc/qcom/sm8250.c | 144 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 143 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/qcom/sm8250.c b/sound/soc/qcom/sm8250.c
> index fe8fd7367e21..421f9d1d2bed 100644
> --- a/sound/soc/qcom/sm8250.c
> +++ b/sound/soc/qcom/sm8250.c
> @@ -20,6 +20,141 @@ struct sm8250_snd_data {
>  	struct sdw_stream_runtime *sruntime[AFE_PORT_MAX];
>  };
>  
> +static int qcom_audioreach_snd_parse_of(struct snd_soc_card *card)
> +{
> +	struct device_node *np;
> +	struct device_node *codec = NULL;
> +	struct device_node *platform = NULL;
> +	struct device_node *cpu = NULL;
> +	struct device *dev = card->dev;
> +	struct snd_soc_dai_link *link;
> +	struct of_phandle_args args;
> +	struct snd_soc_dai_link_component *dlc;
> +	int ret, num_links;
> +
> +	ret = snd_soc_of_parse_card_name(card, "model");
> +	if (ret) {
> +		dev_err(dev, "Error parsing card name: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* DAPM routes */
> +	if (of_property_read_bool(dev->of_node, "audio-routing")) {
> +		ret = snd_soc_of_parse_audio_routing(card, "audio-routing");
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Populate links */
> +	num_links = of_get_child_count(dev->of_node);
> +
> +	/* Allocate the DAI link array */
> +	card->dai_link = devm_kcalloc(dev, num_links, sizeof(*link), GFP_KERNEL);
> +	if (!card->dai_link)
> +		return -ENOMEM;
> +
> +	card->num_links = num_links;
> +	link = card->dai_link;
> +
> +	for_each_child_of_node(dev->of_node, np) {
> +
> +		dlc = devm_kzalloc(dev, 2 * sizeof(*dlc), GFP_KERNEL);
> +		if (!dlc) {
> +			ret = -ENOMEM;
> +			goto err_put_np;
> +		}
> +
> +		link->cpus	= &dlc[0];
> +		link->platforms	= &dlc[1];
> +
> +		link->num_cpus		= 1;
> +		link->num_platforms	= 1;
> +
> +
> +		ret = of_property_read_string(np, "link-name", &link->name);
> +		if (ret) {
> +			dev_err(card->dev, "error getting codec dai_link name\n");
> +			goto err_put_np;
> +		}
> +
> +		cpu = of_get_child_by_name(np, "cpu");
> +		platform = of_get_child_by_name(np, "platform");
> +		codec = of_get_child_by_name(np, "codec");
> +		if (!cpu) {
> +			dev_err(dev, "%s: Can't find cpu DT node\n", link->name);
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +
> +		if (!platform) {
> +			dev_err(dev, "%s: Can't find platform DT node\n", link->name);
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +
> +		if (!codec) {
> +			dev_err(dev, "%s: Can't find codec DT node\n", link->name);
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +
> +		ret = of_parse_phandle_with_args(cpu, "sound-dai", "#sound-dai-cells", 0, &args);
> +		if (ret) {
> +			dev_err(card->dev, "%s: error getting cpu phandle\n", link->name);
> +			goto err;
> +		}
> +
> +		link->cpus->of_node = args.np;
> +		link->id = args.args[0];
> +
> +		ret = snd_soc_of_get_dai_name(cpu, &link->cpus->dai_name);
> +		if (ret) {
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(card->dev, "%s: error getting cpu dai name: %d\n",
> +					link->name, ret);
> +			goto err;
> +		}
> +
> +		link->platforms->of_node = of_parse_phandle(platform, "sound-dai", 0);
> +		if (!link->platforms->of_node) {
> +			dev_err(card->dev, "%s: platform dai not found\n", link->name);
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +
> +		ret = snd_soc_of_get_dai_link_codecs(dev, codec, link);
> +		if (ret < 0) {
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(card->dev, "%s: codec dai not found: %d\n",
> +					link->name, ret);
> +			goto err;
> +		}
> +
> +		/* DPCM backend */
> +		link->no_pcm = 1;
> +		link->ignore_pmdown_time = 1;
> +		link->ignore_suspend = 1;

why are those two fields set unconditionally?

If you parse information from DT shouldn't those links be explicitly tagged as requiring those fields to be set?

It's a recurring battle for me to ask why people set them in Intel machine drivers, I find it really odd that you would set them since they aren't without side effect on clocks and suspend.

> +
> +		link->stream_name = link->name;
> +		snd_soc_dai_link_set_capabilities(link);
> +		link++;
> +
> +		of_node_put(cpu);
> +		of_node_put(codec);
> +		of_node_put(platform);
> +
> +	}
> +
> +	return 0;
> +err:
> +	of_node_put(cpu);
> +	of_node_put(codec);
> +	of_node_put(platform);
> +err_put_np:
> +	of_node_put(np);
> +	return ret;
> +}
> +
Srinivas Kandagatla July 15, 2021, 10:31 a.m. UTC | #9
Many thanks Pierre for the review,

On 14/07/2021 17:20, Pierre-Louis Bossart wrote:
> 
>> +void gpr_free_port(gpr_port_t *port)
>> +{
>> +	struct packet_router *gpr = port->pr;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&gpr->svcs_lock, flags);
>> +	idr_remove(&gpr->svcs_idr, port->id);
>> +	spin_unlock_irqrestore(&gpr->svcs_lock, flags);
> 
> maybe add a comment somewhere on why you need the irqsave/irqrestore version of spin_lock/unlock?

All the responses to the messages arrive in interrupt context which 
could try to find the matching svc idr, so we needed this irq version of 
spinlock here. I did move this handling to a work queue in the past, so 
we should be able to relax this lock to non-irq version.

> 
> It's not very clear by looking at this patch only that those locks are handled in interrupt context.
> 
>> +
>> +	kfree(port);
>> +}
>> +EXPORT_SYMBOL_GPL(gpr_free_port);
>> +
>> +gpr_port_t *gpr_alloc_port(struct apr_device *gdev, struct device *dev,
>> +				gpr_port_cb cb,	void *priv)
>> +{
>> +	struct packet_router *pr = dev_get_drvdata(gdev->dev.parent);
>> +	gpr_port_t *port;
>> +	struct pkt_router_svc *svc;
>> +	int id;
>> +
>> +	port = kzalloc(sizeof(*port), GFP_KERNEL);
>> +	if (!port)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	svc = port;
>> +	svc->callback = cb;
>> +	svc->pr = pr;
>> +	svc->priv = priv;
>> +	svc->dev = dev;
>> +	spin_lock_init(&svc->lock);
>> +
>> +	spin_lock(&pr->svcs_lock);
>> +	id = idr_alloc_cyclic(&pr->svcs_idr, svc, GPR_DYNAMIC_PORT_START,
>> +			      GPR_DYNAMIC_PORT_END, GFP_ATOMIC);
>> +	if (id < 0) {
>> +		dev_err(dev, "Unable to allocate dynamic GPR src port\n");
>> +		kfree(port);
>> +		spin_unlock(&pr->svcs_lock);
> 
> nit-pick: more this before the dev_err?

I agree, will do that in next spin.
> 
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	svc->id = id;
>> +	spin_unlock(&pr->svcs_lock);
>> +
>> +	dev_info(dev, "Adding GPR src port (%x)\n", svc->id);
>> +
>> +	return port;
>> +}
> 
>> +static int gpr_do_rx_callback(struct packet_router *gpr, struct apr_rx_buf *abuf)
>> +{
>> +	uint16_t hdr_size, ver;
>> +	struct pkt_router_svc *svc = NULL;
> 
> unnecessary init? it's overritten...
> 
Yep.


--srini

>> +	struct gpr_resp_pkt resp;
>> +	struct gpr_hdr *hdr;
>> +	unsigned long flags;
>> +	void *buf = abuf->buf;
>> +	int len = abuf->len;
>> +
>> +	hdr = buf;
>> +	ver = hdr->version;
>> +	if (ver > GPR_PKT_VER + 1)
>> +		return -EINVAL;
>> +
>> +	hdr_size = hdr->hdr_size;
>> +	if (hdr_size < GPR_PKT_HEADER_WORD_SIZE) {
>> +		dev_err(gpr->dev, "GPR: Wrong hdr size:%d\n", hdr_size);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (hdr->pkt_size < GPR_PKT_HEADER_BYTE_SIZE || hdr->pkt_size != len) {
>> +		dev_err(gpr->dev, "GPR: Wrong packet size\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	resp.hdr = *hdr;
>> +	resp.payload_size = hdr->pkt_size - (hdr_size * 4);
>> +
>> +	/*
>> +	 * NOTE: hdr_size is not same as GPR_HDR_SIZE as remote can include
>> +	 * optional headers in to gpr_hdr which should be ignored
>> +	 */
>> +	if (resp.payload_size > 0)
>> +		resp.payload = buf + (hdr_size *  4);
>> +
>> +
>> +	spin_lock_irqsave(&gpr->svcs_lock, flags);
>> +	svc = idr_find(&gpr->svcs_idr, hdr->dest_port);
> 
> ... here
> 
>> +	spin_unlock_irqrestore(&gpr->svcs_lock, flags);
>> +
>> +	if (!svc) {
>> +		dev_err(gpr->dev, "GPR: Port(%x) is not registered\n",
>> +			hdr->dest_port);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (svc->callback)
>> +		svc->callback(&resp, svc->priv, 0);
>> +
>> +	return 0;
>> +}
>> +
>
Srinivas Kandagatla July 15, 2021, 10:31 a.m. UTC | #10
Thanks for review Pierre,

On 14/07/2021 17:30, Pierre-Louis Bossart wrote:
> 
> 
> 
>> diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
>> index cc7c1de2f1d9..721ea56b2cb5 100644
>> --- a/sound/soc/qcom/Kconfig
>> +++ b/sound/soc/qcom/Kconfig
>> @@ -103,6 +103,12 @@ config SND_SOC_QDSP6
>>   	 audio drivers. This includes q6asm, q6adm,
>>   	 q6afe interfaces to DSP using apr.
>>   
>> +config SND_SOC_QCOM_AUDIOREACH
>> +	tristate "SoC ALSA audio drives for Qualcomm AUDIOREACH"
> 
> typo: drivers?
> 
Will fix all the typos in next spin.


>> +static void *__audioreach_alloc_pkt(int payload_size, uint32_t opcode,
>> +				     uint32_t token, uint32_t src_port,
>> +				     uint32_t dest_port, bool has_cmd_hdr)
>> +{
>> +	struct apm_cmd_header *cmd_header;
>> +	struct gpr_pkt *pkt;
>> +	void *p;
>> +	int pkt_size = GPR_HDR_SIZE + payload_size;
>> +
>> +	if (has_cmd_hdr)
>> +		pkt_size += APM_CMD_HDR_SIZE;
>> +
>> +	p = kzalloc(pkt_size, GFP_ATOMIC);
> 
> is GFP_ATOMIC required? it's the same question really than my earlier one on spinlock_irqsave, it's rather hard to figure out in what context this code will run.

I had some spinlocks in this patch for compress offload cases, so had to 
make it ATOMIC. having said that we could avoid ATOMIC here.

> 
>> +	if (!p)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	pkt = p;
>> +	pkt->hdr.version = GPR_PKT_VER;
>> +	pkt->hdr.hdr_size = 6;
> 
> magic number. looks like a missing macro...

There is already a macro for this, GPR_PKT_HEADER_WORD_SIZE I should 
have used it.
> 
>> +	pkt->hdr.pkt_size = pkt_size;
>> +	pkt->hdr.dest_port = dest_port;
>> +	pkt->hdr.src_port = src_port;
>> +
>> +	pkt->hdr.dest_domain = GPR_DOMAIN_ID_ADSP;
>> +	pkt->hdr.src_domain = GPR_DOMAIN_ID_APPS;
>> +	pkt->hdr.token = token;
>> +	pkt->hdr.opcode = opcode;
>> +
>> +	if (has_cmd_hdr) {
>> +		p = p + GPR_HDR_SIZE;
>> +		cmd_header = p;
>> +		cmd_header->payload_size = payload_size;
>> +	}
>> +
>> +	return pkt;
>> +}
>
Srinivas Kandagatla July 15, 2021, 10:31 a.m. UTC | #11
Thanks Pierre for review,

On 14/07/2021 17:40, Pierre-Louis Bossart wrote:
> 
>>   /* SubGraph Config */
>> @@ -32,7 +33,7 @@ struct apm_sub_graph_params  {
>>   /* container config */
>>   struct apm_container_obj  {
>>   	struct apm_container_cfg container_cfg;
>> -	/* Capablity ID list */
>> +	/* Capability ID list */
> 
> squash in wrong patch, this should have been included in the previous patch.

My bad.. will fix such instances in next spin.


> 
>>   	struct apm_prop_data cap_data;
>>   	uint32_t num_capablity_id;

...

> 
>> +static struct audioreach_graph *q6apm_get_audioreach_graph(struct q6apm *apm,
>> +						      uint32_t graph_id)
>> +{
>> +	struct audioreach_graph *graph = NULL;
> 
> useless init
> 
>> +	struct audioreach_graph_info *info;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&apm->lock, flags);
>> +	graph = idr_find(&apm->graph_idr, graph_id);
>> +	spin_unlock_irqrestore(&apm->lock, flags);
>> +
>> +	if (graph) {
>> +		kref_get(&graph->refcount);
>> +		return graph;
>> +	}
>> +
>> +	info = idr_find(&apm->graph_info_idr, graph_id);
>> +
>> +	if (!info)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	graph = kzalloc(sizeof(*graph), GFP_KERNEL);
>> +	if (!graph)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	graph->apm = apm;
>> +	graph->info = info;
>> +	graph->id = graph_id;
>> +
>> +	/* Assuming Linear Graphs only for now! */
>> +	graph->graph = audioreach_alloc_graph_pkt(apm, &info->sg_list, graph_id);
>> +	if (IS_ERR(graph->graph))
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	spin_lock(&apm->lock);
>> +	idr_alloc(&apm->graph_idr, graph, graph_id,
>> +		  graph_id + 1, GFP_ATOMIC);
> 
> ATOMIC?
we are under spinlock here, so we need this.

> 
>> +	spin_unlock(&apm->lock);
>> +
>> +	kref_init(&graph->refcount);
>> +
>> +	q6apm_send_cmd_sync(apm, graph->graph, 0);
>> +
>> +	return graph;
>> +}
>> +
>> +static int audioreach_graph_mgmt_cmd(struct audioreach_graph *graph,
>> +				      uint32_t opcode)
>> +{
>> +	struct gpr_pkt *pkt;
>> +	void *p;
>> +	int i = 0, rc, payload_size;
>> +	struct q6apm *apm = graph->apm;
>> +	struct audioreach_graph_info *info = graph->info;
>> +	int num_sub_graphs = info->num_sub_graphs;
>> +	struct apm_graph_mgmt_cmd *mgmt_cmd;
>> +	struct apm_module_param_data *param_data;
>> +	struct audioreach_sub_graph *sg;
>> +
>> +	payload_size = APM_GRAPH_MGMT_PSIZE(num_sub_graphs);
>> +
>> +	p = audioreach_alloc_apm_cmd_pkt(payload_size, opcode, 0);
>> +	if (IS_ERR(p))
>> +		return -ENOMEM;
>> +
>> +	pkt = p;
>> +	p = p + GPR_HDR_SIZE + APM_CMD_HDR_SIZE;
>> +
>> +	mgmt_cmd = p;
>> +	mgmt_cmd->num_sub_graphs = num_sub_graphs;
>> +
>> +	param_data = &mgmt_cmd->param_data;
>> +	param_data->module_instance_id = APM_MODULE_INSTANCE_ID;
>> +	param_data->param_id = APM_PARAM_ID_SUB_GRAPH_LIST;
>> +	param_data->param_size = payload_size - APM_MODULE_PARAM_DATA_SIZE;
>> +
>> +	list_for_each_entry(sg, &info->sg_list, node) {
>> +		mgmt_cmd->sub_graph_id_list[i++] = sg->sub_graph_id;
>> +	}
>> +
>> +	rc = q6apm_send_cmd_sync(apm, pkt, 0);
>> +
>> +	kfree(pkt);
>> +
>> +	return rc;
>> +}
>> +
>> +static void q6apm_put_audioreach_graph(struct kref *ref)
>> +{
>> +	struct audioreach_graph *graph;
>> +	struct q6apm *apm;
>> +	unsigned long flags;
>> +
>> +	graph = container_of(ref, struct audioreach_graph, refcount);
>> +	apm = graph->apm;
>> +
>> +	audioreach_graph_mgmt_cmd(graph, APM_CMD_GRAPH_CLOSE);
>> +
>> +	spin_lock_irqsave(&apm->lock, flags);
>> +	graph = idr_remove(&apm->graph_idr, graph->id);
>> +	spin_unlock_irqrestore(&apm->lock, flags);
>> +
>> +	kfree(graph->graph);
>> +	kfree(graph);
> 
> earlier in the _get routine, you had a kref_get(&graph->refcount)
> 
This is a release callback for kref. Probably I should rename this to 
q6apm_release_audioreach_graph().


> is it intentional that there's kref_put?

kref_put is called in q6apm_graph_close()

q6apm_graph_open() calls q6apm_get_audioreach_graph() which will do 
kref_get.

q6apm_graph_close() will call kref_put().

when refcount is zero q6apm_put_audioreach_graph() callback will be invoked.

--srini
> 
> adding a comment on the refcounts might be useful...
>
Srinivas Kandagatla July 15, 2021, 10:32 a.m. UTC | #12
Thanks Pierre,

On 14/07/2021 17:48, Pierre-Louis Bossart wrote:
> 
>> +static int audioreach_shmem_set_media_format(struct q6apm_graph *graph,
>> +				       struct audioreach_module *module,
>> +				       int direction, uint32_t rate,
>> +				       uint32_t num_channels,
>> +				       u8 channel_map[PCM_MAX_NUM_CHANNEL],
>> +				       uint16_t bits_per_sample)
>> +{
>> +	struct apm_module_param_data *param_data;
>> +	struct payload_media_fmt_pcm *cfg;
>> +	struct media_format *header;
>> +	int rc, payload_size;
>> +	struct gpr_pkt *pkt;
>> +	void *p;
>> +
>> +	if (num_channels < 0 || num_channels > 2)
>> +		dev_err(graph->dev, "Error: Invalid channels (%d)!\n", num_channels);
> 
> that doesn't sound good, you flag num_channels as an invalid value but still continue using it.

I forgot to run cppcheck after doing make W=1 C=1.

It did catch these, will fix all such instances before I send out next 
version.



--srini
Srinivas Kandagatla July 15, 2021, 10:32 a.m. UTC | #13
On 14/07/2021 18:03, Pierre-Louis Bossart wrote:
>> +	rc = q6apm_graph_media_format_pcm(dai_data->graph[dai->id],
>> +					  substream->stream, cfg->sample_rate,
>> +					  cfg->num_channels, NULL, cfg->bit_width);
>> +
>> +	rc = q6apm_graph_prepare(dai_data->graph[dai->id]);
> any good static analyzer would tell you you didn't test rc before overriding it..

yes you are correct, I did forgot to do cppcheck after make W=1 C=1

These are now fixed in new version.

--srini
Srinivas Kandagatla July 15, 2021, 10:32 a.m. UTC | #14
On 14/07/2021 17:59, Pierre-Louis Bossart wrote:
> 
> 
>> +static void event_handler(uint32_t opcode, uint32_t token,
>> +			  uint32_t *payload, void *priv)
>> +{
>> +	struct q6apm_dai_rtd *prtd = priv;
>> +	struct snd_pcm_substream *substream = prtd->substream;
>> +
>> +	switch (opcode) {
>> +	case APM_CLIENT_EVENT_CMD_EOS_DONE:
>> +		prtd->state = Q6APM_STREAM_STOPPED;
>> +		break;
>> +	case APM_CLIENT_EVENT_DATA_WRITE_DONE: {
>> +		prtd->pcm_irq_pos += prtd->pcm_count;
>> +		snd_pcm_period_elapsed(substream);
> 
> A comment on the relationship between data writes and periods would be nice. It looks like the code assumes the period and data transfers are identical, but periods can be chosen by applications, can't they?

Yes, pcm_count is set to period size, so we get ack for each period.


> 
>> +		if (prtd->state == Q6APM_STREAM_RUNNING) {
>> +			q6apm_write_async(prtd->graph,
>> +					   prtd->pcm_count, 0, 0, NO_TIMESTAMP);
>> +		}
>> +
>> +		break;
>> +		}
>> +	case APM_CLIENT_EVENT_DATA_READ_DONE:
>> +		prtd->pcm_irq_pos += prtd->pcm_count;
>> +		snd_pcm_period_elapsed(substream);
>> +		if (prtd->state == Q6APM_STREAM_RUNNING)
>> +			q6apm_read(prtd->graph);
>> +
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +}
> 
>> +static int q6apm_dai_trigger(struct snd_soc_component *component,
>> +			     struct snd_pcm_substream *substream, int cmd)
>> +{
>> +	struct snd_pcm_runtime *runtime = substream->runtime;
>> +	struct q6apm_dai_rtd *prtd = runtime->private_data;
>> +	int ret = 0;
>> +
>> +	switch (cmd) {
>> +	case SNDRV_PCM_TRIGGER_START:
>> +	case SNDRV_PCM_TRIGGER_RESUME:
>> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> +			ret = q6apm_write_async(prtd->graph, prtd->pcm_count, 0, 0, NO_TIMESTAMP);
>> +		break;
>> +	case SNDRV_PCM_TRIGGER_STOP:
>> +		prtd->state = Q6APM_STREAM_STOPPED;
>> +		//ret = q6apm_cmd_nowait(prtd->graph, CMD_EOS);
>> +		break;
>> +	case SNDRV_PCM_TRIGGER_SUSPEND:
>> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>> +		//ret = q6apm_cmd_nowait(prtd->graph, CMD_PAUSE);
> 
> these // comments are odd, with a clear imbalance between suspend/resume and pause_push/pause_release...
> is this intentional or a left-over from an experiment?

Yes, I forgot to clean this up before sending out.

> 
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int q6apm_dai_open(struct snd_soc_component *component,
>> +			  struct snd_pcm_substream *substream)
>> +{
>> +	struct snd_pcm_runtime *runtime = substream->runtime;
>> +	struct snd_soc_pcm_runtime *soc_prtd = substream->private_data;
>> +	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_prtd, 0);
>> +	struct q6apm_dai_rtd *prtd;
>> +	struct q6apm_dai_data *pdata;
>> +	struct device *dev = component->dev;
>> +	int ret;
>> +	int graph_id;
>> +
>> +	graph_id = cpu_dai->driver->id;
>> +
>> +	pdata = snd_soc_component_get_drvdata(component);
>> +	if (!pdata) {
>> +		dev_err(component->dev, "Drv data not found ..\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	prtd = kzalloc(sizeof(*prtd), GFP_KERNEL);
>> +	if (prtd == NULL)
>> +		return -ENOMEM;
>> +
>> +	prtd->substream = substream;
>> +
>> +	prtd->graph = q6apm_graph_open(dev, (q6apm_cb)event_handler,
>> +				       prtd, graph_id);
>> +	if (IS_ERR(prtd->graph)) {
>> +		pr_info("%s: Could not allocate memory\n", __func__);
>> +		ret = PTR_ERR(prtd->graph);
>> +		kfree(prtd);
>> +		return ret;
>> +	}
>> +
>> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> +		runtime->hw = q6apm_dai_hardware_playback;
>> +	else if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
>> +		runtime->hw = q6apm_dai_hardware_capture;
>> +
>> +	/* 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)
>> +		dev_err(dev, "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,
>> +			BUFFER_BYTES_MIN, BUFFER_BYTES_MAX);
>> +		if (ret < 0) {
>> +			dev_err(dev, "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) {
>> +		dev_err(dev, "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) {
>> +		dev_err(dev, "constraint for buffer bytes step ret = %d\n",
>> +								ret);
>> +	}
> 
> dev_err() used but no return code sent?

Ack.

> 
>> +
>> +	runtime->private_data = prtd;
>> +	runtime->dma_bytes = 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 q6apm_dai_hw_params(struct snd_soc_component *component,
>> +			       struct snd_pcm_substream *substream,
>> +			       struct snd_pcm_hw_params *params)
>> +{
>> +	struct snd_pcm_runtime *runtime = substream->runtime;
>> +	struct q6apm_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;
> 
> default:
>      return -EINVAL
> ?
Ack, will do that in next spin.

--srini
> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>
Srinivas Kandagatla July 15, 2021, 10:32 a.m. UTC | #15
On 14/07/2021 18:09, Pierre-Louis Bossart wrote:
> 
> 
> 
>> +static int q6prm_send_cmd_sync(struct q6prm *prm, struct gpr_pkt *pkt,
>> +			uint32_t rsp_opcode)
>> +{
>> +	gpr_device_t *gdev = prm->gdev;
>> +	struct gpr_hdr *hdr = &pkt->hdr;
>> +	int rc;
>> +
>> +	mutex_lock(&prm->lock);
>> +	prm->result.opcode = 0;
>> +	prm->result.status = 0;
>> +
>> +	rc = gpr_send_pkt(prm->gdev, pkt);
>> +	if (rc < 0)
>> +		goto err;
>> +
>> +	if (rsp_opcode)
>> +		rc = wait_event_timeout(prm->wait,
>> +					(prm->result.opcode == hdr->opcode) ||
>> +					(prm->result.opcode == rsp_opcode),
>> +					5 * HZ);
>> +	else
>> +		rc = wait_event_timeout(prm->wait,
>> +					(prm->result.opcode == hdr->opcode),
>> +					5 * HZ);
>> +
>> +	if (!rc) {
>> +		dev_err(&gdev->dev, "CMD timeout for [%x] opcode\n",
>> +			hdr->opcode);
>> +		rc = -ETIMEDOUT;
>> +	} else if (prm->result.status > 0) {
>> +		dev_err(&gdev->dev, "DSP returned error[%x] %x\n", hdr->opcode,
>> +			prm->result.status);
>> +		rc = -EINVAL;
>> +	} else {
>> +		dev_err(&gdev->dev, "DSP returned [%x]\n",
>> +			prm->result.status);
>> +		rc = 0;
>> +	}
>> +
>> +err:
>> +	mutex_unlock(&prm->lock);
>> +	return rc;
>> +}
> 
> In patch7 you had more or less the same code. can a helper be used?

Its possible, will abstract this out in audioreach.c.


> 
> +int audioreach_graph_send_cmd_sync(struct q6apm_graph *graph,
> +				   struct gpr_pkt *pkt, uint32_t rsp_opcode)
> +{
> +
> +	struct device *dev = graph->dev;
> +	struct gpr_hdr *hdr = &pkt->hdr;
> +	int rc;
> +
> +	mutex_lock(&graph->cmd_lock);
> +	graph->result.opcode = 0;
> +	graph->result.status = 0;
> +
> +	rc = gpr_send_port_pkt(graph->port, pkt);
> +	if (rc < 0)
> +		goto err;
> +
> +	if (rsp_opcode)
> +		rc = wait_event_timeout(graph->cmd_wait,
> +					(graph->result.opcode == hdr->opcode) ||
> +					(graph->result.opcode == rsp_opcode),
> +					5 * HZ);
> +	else
> +		rc = wait_event_timeout(graph->cmd_wait,
> +					(graph->result.opcode == hdr->opcode),
> +					5 * HZ);
> +
> +	if (!rc) {
> +		dev_err(dev, "CMD timeout for [%x] opcode\n", hdr->opcode);
> +		rc = -ETIMEDOUT;
> +	} else if (graph->result.status > 0) {
> +		dev_err(dev, "DSP returned error[%x] %x\n", hdr->opcode,
> +			graph->result.status);
> +		rc = -EINVAL;
> +	} else {
> +		dev_err(dev, "DSP returned [%x]\n", graph->result.status);
> +		rc = 0;
> +	}
> +
> +err:
> +	mutex_unlock(&graph->cmd_lock);
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(audioreach_graph_send_cmd_sync);
> 
> 
>> +static int q6prm_set_hw_core_req(struct device *dev, uint32_t hw_block_id,   bool enable)
>> +{
>> +	struct prm_cmd_request_hw_core *req;
>> +	struct apm_module_param_data *param_data;
>> +	struct gpr_pkt *pkt;
>> +	struct q6prm *prm = dev_get_drvdata(dev);
>> +	gpr_device_t *gdev  = prm->gdev;
>> +	void *p;
>> +	int rc = 0;
>> +	uint32_t opcode, rsp_opcode;
>> +
>> +	if (enable) {
>> +		opcode = PRM_CMD_REQUEST_HW_RSC;
>> +		rsp_opcode = PRM_CMD_RSP_REQUEST_HW_RSC;
>> +	} else {
>> +		opcode = PRM_CMD_RELEASE_HW_RSC;
>> +		rsp_opcode = PRM_CMD_RSP_RELEASE_HW_RSC;
>> +	}
>> +
>> +	p = audioreach_alloc_cmd_pkt(sizeof(*req), opcode, 0, gdev->svc.id,
>> +				     GPR_PRM_MODULE_IID);
>> +	if (IS_ERR(p))
>> +		return -ENOMEM;
>> +
>> +	pkt = p;
>> +	req = p + GPR_HDR_SIZE + APM_CMD_HDR_SIZE;
>> +
>> +	param_data = &req->param_data;
>> +
>> +	param_data->module_instance_id = GPR_PRM_MODULE_IID;
>> +	param_data->error_code = 0;
>> +	param_data->param_id = PARAM_ID_RSC_HW_CORE;
>> +	param_data->param_size = sizeof(*req) - APM_MODULE_PARAM_DATA_SIZE;
>> +
>> +	req->hw_clk_id = hw_block_id;
>> +
>> +	q6prm_send_cmd_sync(prm, pkt, rsp_opcode);
>> +
>> +	kfree(pkt);
>> +
>> +	return rc;
> 
> rc is not assigned, should this not trap the result of sending a command?
My bad! will fix such instances.


--srini
Srinivas Kandagatla July 15, 2021, 10:32 a.m. UTC | #16
On 14/07/2021 18:12, Pierre-Louis Bossart wrote:
> 
> 
> On 7/14/21 10:30 AM, Srinivas Kandagatla wrote:
>> This patch adds support for parsing dt for AudioReach based soundcards
>> which only have backend DAI links in DT.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   sound/soc/qcom/sm8250.c | 144 +++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 143 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/qcom/sm8250.c b/sound/soc/qcom/sm8250.c
>> index fe8fd7367e21..421f9d1d2bed 100644
>> --- a/sound/soc/qcom/sm8250.c
>> +++ b/sound/soc/qcom/sm8250.c
>> @@ -20,6 +20,141 @@ struct sm8250_snd_data {
>>   	struct sdw_stream_runtime *sruntime[AFE_PORT_MAX];
>>   };
>>   
>> +static int qcom_audioreach_snd_parse_of(struct snd_soc_card *card)
>> +{
>> +	struct device_node *np;
>> +	struct device_node *codec = NULL;
>> +	struct device_node *platform = NULL;
>> +	struct device_node *cpu = NULL;
>> +	struct device *dev = card->dev;
>> +	struct snd_soc_dai_link *link;
>> +	struct of_phandle_args args;
>> +	struct snd_soc_dai_link_component *dlc;
>> +	int ret, num_links;
>> +
>> +	ret = snd_soc_of_parse_card_name(card, "model");
>> +	if (ret) {
>> +		dev_err(dev, "Error parsing card name: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* DAPM routes */
>> +	if (of_property_read_bool(dev->of_node, "audio-routing")) {
>> +		ret = snd_soc_of_parse_audio_routing(card, "audio-routing");
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	/* Populate links */
>> +	num_links = of_get_child_count(dev->of_node);
>> +
>> +	/* Allocate the DAI link array */
>> +	card->dai_link = devm_kcalloc(dev, num_links, sizeof(*link), GFP_KERNEL);
>> +	if (!card->dai_link)
>> +		return -ENOMEM;
>> +
>> +	card->num_links = num_links;
>> +	link = card->dai_link;
>> +
>> +	for_each_child_of_node(dev->of_node, np) {
>> +
>> +		dlc = devm_kzalloc(dev, 2 * sizeof(*dlc), GFP_KERNEL);
>> +		if (!dlc) {
>> +			ret = -ENOMEM;
>> +			goto err_put_np;
>> +		}
>> +
>> +		link->cpus	= &dlc[0];
>> +		link->platforms	= &dlc[1];
>> +
>> +		link->num_cpus		= 1;
>> +		link->num_platforms	= 1;
>> +
>> +
>> +		ret = of_property_read_string(np, "link-name", &link->name);
>> +		if (ret) {
>> +			dev_err(card->dev, "error getting codec dai_link name\n");
>> +			goto err_put_np;
>> +		}
>> +
>> +		cpu = of_get_child_by_name(np, "cpu");
>> +		platform = of_get_child_by_name(np, "platform");
>> +		codec = of_get_child_by_name(np, "codec");
>> +		if (!cpu) {
>> +			dev_err(dev, "%s: Can't find cpu DT node\n", link->name);
>> +			ret = -EINVAL;
>> +			goto err;
>> +		}
>> +
>> +		if (!platform) {
>> +			dev_err(dev, "%s: Can't find platform DT node\n", link->name);
>> +			ret = -EINVAL;
>> +			goto err;
>> +		}
>> +
>> +		if (!codec) {
>> +			dev_err(dev, "%s: Can't find codec DT node\n", link->name);
>> +			ret = -EINVAL;
>> +			goto err;
>> +		}
>> +
>> +		ret = of_parse_phandle_with_args(cpu, "sound-dai", "#sound-dai-cells", 0, &args);
>> +		if (ret) {
>> +			dev_err(card->dev, "%s: error getting cpu phandle\n", link->name);
>> +			goto err;
>> +		}
>> +
>> +		link->cpus->of_node = args.np;
>> +		link->id = args.args[0];
>> +
>> +		ret = snd_soc_of_get_dai_name(cpu, &link->cpus->dai_name);
>> +		if (ret) {
>> +			if (ret != -EPROBE_DEFER)
>> +				dev_err(card->dev, "%s: error getting cpu dai name: %d\n",
>> +					link->name, ret);
>> +			goto err;
>> +		}
>> +
>> +		link->platforms->of_node = of_parse_phandle(platform, "sound-dai", 0);
>> +		if (!link->platforms->of_node) {
>> +			dev_err(card->dev, "%s: platform dai not found\n", link->name);
>> +			ret = -EINVAL;
>> +			goto err;
>> +		}
>> +
>> +		ret = snd_soc_of_get_dai_link_codecs(dev, codec, link);
>> +		if (ret < 0) {
>> +			if (ret != -EPROBE_DEFER)
>> +				dev_err(card->dev, "%s: codec dai not found: %d\n",
>> +					link->name, ret);
>> +			goto err;
>> +		}
>> +
>> +		/* DPCM backend */
>> +		link->no_pcm = 1;

Currently we only allow backend dailinks from DT, as the rest of pcm dai 
links come from topology.

>> +		link->ignore_pmdown_time = 1;
>> +		link->ignore_suspend = 1;
> 
> why are those two fields set unconditionally?

These two flags have been inherited from ./sound/soc/qcom/common.c

> 
> If you parse information from DT shouldn't those links be explicitly tagged as requiring those fields to be set?

Currently we do not have any DT bindings for these generic flags. May be 
I/we should think about adding binding for this soon.

> 
> It's a recurring battle for me to ask why people set them in Intel machine drivers, I find it really odd that you would set them since they aren't without side effect on clocks and suspend.

Noted, Thanks for hints, I think we are yet to hit these issues for 
wrong reasons, As most of the qcom audio drivers are not tested with PM. 
Some of the Google hw did test full PM though.
Currently am working on getting PM support for various qcom codecs, and 
soundwire controller.

--srini

> 
>> +
>> +		link->stream_name = link->name;
>> +		snd_soc_dai_link_set_capabilities(link);
>> +		link++;
>> +
>> +		of_node_put(cpu);
>> +		of_node_put(codec);
>> +		of_node_put(platform);
>> +
>> +	}
>> +
>> +	return 0;
>> +err:
>> +	of_node_put(cpu);
>> +	of_node_put(codec);
>> +	of_node_put(platform);
>> +err_put_np:
>> +	of_node_put(np);
>> +	return ret;
>> +}
>> +
>