[v6,00/24] ASoC: qcom: Add support to QDSP based Audio

Message ID 20180426094606.4775-1-srinivas.kandagatla@linaro.org
Headers show
Series
  • ASoC: qcom: Add support to QDSP based Audio
Related show

Message

Srinivas Kandagatla April 26, 2018, 9:45 a.m.
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>


Thankyou everyone for providing feedback and testing v5 patchset.
This patchset aims to provide a basic version of QCOM DSP based
audio support which is available in downstream andriod kernels.
This patchset support audio playback on HDMI-RX, MI2S, SLIMBus and
will add support to other features as we move on.

QDSP has both static and dynamic modules. static modules like AFE
(Audio FrontEnd), ADM (Audio Device Manager), ASM(Audio Stream Manager)
and CORE to provide this audio services.
All these services use APR (Asynchronous Packet Router) protocol
via smd/glink transport to communicate with Application processor.
More details on each module is availble in there respective patch.

This patchset is tested on DB820c, with HDMI audio playback, MI2S on
DB410c on top of mainline, Also tested SLIMBus analog audio using
wcd9355 with an additional patches. Patches are also tested on
SDM845 by Rohit.

Here is my test branch incase someone want to try these patches
https://git.linaro.org/people/srinivas.kandagatla/linux.git/log/?h=v4.17-qdsp6

Here is block diagram to give a quick overview of the components


  +---------+          +---------+         +---------+   
  |  q6asm  |          |q6routing|         | q6afe   | 
  |   dais  | <------> |  mixers | <-----> |  dais   |  
  +---------+          +---------+         +---------+   
      ^                     ^                   ^
      |                     |                   |
      |  +------------------+----------------+  |       
      |  |                  |                |  |       
      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 v5 (https://lkml.org/lkml/2018/4/18/553)
- updated bindings as reviewed by Rob H.
- added missing binding for q6core
- fixed copp double count reported by Amit Pundir
- Updated proper SPDX licence on headers
- reorder patches to fix bisect errors reported by kbuild

Srinivas Kandagatla (24):
  soc: qcom dt-bindings: Add APR bus bindings
  soc: qcom: Add APR bus driver
  ASoC: qdsp6: dt-bindings: Add q6core dt bindings
  ASoC: qdsp6: dt-bindings: Add q6afe dt bindings
  ASoC: qdsp6: dt-bindings: Add q6adm dt bindings
  ASoC: qdsp6: dt-bindings: Add q6asm dt bindings
  ASoC: qdsp6: q6common: Add qdsp6 helper functions
  ASoC: qdsp6: q6core: Add q6core driver
  ASoC: qdsp6: q6afe: Add q6afe driver
  ASoC: qdsp6: qdafe: Add SLIMBus port Support
  ASoC: qdsp6: q6afe: Add support to MI2S ports
  ASoC: qdsp6: q6afe: Add support to MI2S sysclks
  ASoC: qdsp6: q6adm: Add q6adm driver
  ASoC: qdsp6: q6asm: Add q6asm driver
  ASoC: qdsp6: q6asm: Add support to memory map and unmap
  ASoC: qdsp6: q6asm: Add support to audio stream apis
  ASoC: qdsp6: q6routing: Add q6routing driver
  ASoC: qdsp6: q6routing: Add support to all SLIMBus Mixers
  ASoC: qdsp6: q6routing: Add support to MI2S Mixers
  ASoC: qdsp6: q6afe: Add q6afe dai driver
  ASoC: qdsp6: q6asm: Add q6asm dai driver
  ASoC: qdsp6: dt-bindings: Add apq8096 machine bindings
  ASoC: qcom: apq8096: Add db820c machine driver
  MAINTAINERS: Add myself as co-maintainer of qcom audio

 .../devicetree/bindings/soc/qcom/qcom,apr.txt      |   84 ++
 .../devicetree/bindings/sound/qcom,apq8096.txt     |  109 ++
 .../devicetree/bindings/sound/qcom,q6adm.txt       |   33 +
 .../devicetree/bindings/sound/qcom,q6afe.txt       |   88 ++
 .../devicetree/bindings/sound/qcom,q6asm.txt       |   33 +
 .../devicetree/bindings/sound/qcom,q6core.txt      |   21 +
 MAINTAINERS                                        |    1 +
 drivers/soc/qcom/Kconfig                           |    9 +
 drivers/soc/qcom/Makefile                          |    1 +
 drivers/soc/qcom/apr.c                             |  384 ++++++
 include/dt-bindings/soc/qcom,apr.h                 |   28 +
 include/dt-bindings/sound/qcom,q6afe.h             |   31 +
 include/dt-bindings/sound/qcom,q6asm.h             |   22 +
 include/linux/mod_devicetable.h                    |   11 +
 include/linux/soc/qcom/apr.h                       |  130 ++
 sound/soc/qcom/Kconfig                             |   50 +
 sound/soc/qcom/Makefile                            |    5 +
 sound/soc/qcom/apq8096.c                           |  238 ++++
 sound/soc/qcom/qdsp6/Makefile                      |    8 +
 sound/soc/qcom/qdsp6/q6adm.c                       |  635 ++++++++++
 sound/soc/qcom/qdsp6/q6adm.h                       |   25 +
 sound/soc/qcom/qdsp6/q6afe-dai.c                   |  752 +++++++++++
 sound/soc/qcom/qdsp6/q6afe.c                       | 1071 ++++++++++++++++
 sound/soc/qcom/qdsp6/q6afe.h                       |  193 +++
 sound/soc/qcom/qdsp6/q6asm-dai.c                   |  632 ++++++++++
 sound/soc/qcom/qdsp6/q6asm.c                       | 1312 ++++++++++++++++++++
 sound/soc/qcom/qdsp6/q6asm.h                       |   69 +
 sound/soc/qcom/qdsp6/q6core.c                      |  380 ++++++
 sound/soc/qcom/qdsp6/q6core.h                      |   15 +
 sound/soc/qcom/qdsp6/q6dsp-common.c                |   66 +
 sound/soc/qcom/qdsp6/q6dsp-common.h                |   24 +
 sound/soc/qcom/qdsp6/q6dsp-errno.h                 |   51 +
 sound/soc/qcom/qdsp6/q6routing.c                   |  982 +++++++++++++++
 sound/soc/qcom/qdsp6/q6routing.h                   |    9 +
 34 files changed, 7502 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 Documentation/devicetree/bindings/sound/qcom,q6adm.txt
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6afe.txt
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6asm.txt
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6core.txt
 create mode 100644 drivers/soc/qcom/apr.c
 create mode 100644 include/dt-bindings/soc/qcom,apr.h
 create mode 100644 include/dt-bindings/sound/qcom,q6afe.h
 create mode 100644 include/dt-bindings/sound/qcom,q6asm.h
 create mode 100644 include/linux/soc/qcom/apr.h
 create mode 100644 sound/soc/qcom/apq8096.c
 create mode 100644 sound/soc/qcom/qdsp6/Makefile
 create mode 100644 sound/soc/qcom/qdsp6/q6adm.c
 create mode 100644 sound/soc/qcom/qdsp6/q6adm.h
 create mode 100644 sound/soc/qcom/qdsp6/q6afe-dai.c
 create mode 100644 sound/soc/qcom/qdsp6/q6afe.c
 create mode 100644 sound/soc/qcom/qdsp6/q6afe.h
 create mode 100644 sound/soc/qcom/qdsp6/q6asm-dai.c
 create mode 100644 sound/soc/qcom/qdsp6/q6asm.c
 create mode 100644 sound/soc/qcom/qdsp6/q6asm.h
 create mode 100644 sound/soc/qcom/qdsp6/q6core.c
 create mode 100644 sound/soc/qcom/qdsp6/q6core.h
 create mode 100644 sound/soc/qcom/qdsp6/q6dsp-common.c
 create mode 100644 sound/soc/qcom/qdsp6/q6dsp-common.h
 create mode 100644 sound/soc/qcom/qdsp6/q6dsp-errno.h
 create mode 100644 sound/soc/qcom/qdsp6/q6routing.c
 create mode 100644 sound/soc/qcom/qdsp6/q6routing.h

-- 
2.16.2

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Comments

Mark Brown April 26, 2018, 11:39 a.m. | #1
On Thu, Apr 26, 2018 at 10:45:44AM +0100, srinivas.kandagatla@linaro.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> 

> This patch adds support toi APR bus (Asynchronous Packet Router) driver.

> ARP driver is made as a bus driver so that the apr devices can added removed


What is the plan for getting this patch merged?  I think the only review
I've seen from maintainers thus far has been from Rob on the DT binding.
It's obviously going to be a dependency for everything else.

Also just saw ARP->APR there.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Srinivas Kandagatla April 26, 2018, 12:05 p.m. | #2
On 26/04/18 12:39, Mark Brown wrote:
> On Thu, Apr 26, 2018 at 10:45:44AM +0100, srinivas.kandagatla@linaro.org wrote:

>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

>>

>> This patch adds support toi APR bus (Asynchronous Packet Router) driver.

>> ARP driver is made as a bus driver so that the apr devices can added removed

> 

> What is the plan for getting this patch merged?  I think the only review

> I've seen from maintainers thus far has been from Rob on the DT binding.

It was initially reviewed by Bjorn and Rohit. I requested Andy to have 
quick look at this.

I was hoping that APR driver would go via Andy's ARM patches. And the 
rest via your tree.

"depends on QCOM_APR" should prevent drivers from building without apr.

> It's obviously going to be a dependency for everything else.

> 

> Also just saw ARP->APR there.

I will fix that!
> 


--srini
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Mark Brown April 27, 2018, 11:06 a.m. | #3
On Thu, Apr 26, 2018 at 04:17:40PM -0500, Andy Gross wrote:

> I just acked this, so it'd be great if you took it along with the others.  If

> you don't want to do that, I can just pull it in with my pull requests.


> Up to you,


That's fine, I'm happy to take it - I just wanted to work out what was
going on.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Rob Herring April 27, 2018, 2:10 p.m. | #4
On Thu, Apr 26, 2018 at 10:45:45AM +0100, srinivas.kandagatla@linaro.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> 

> This patch add DT bindings for Q6CORE DSP module.

> 

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> Reviewed-and-tested-by: Rohit kumar <rohitkr@codeaurora.org>

> ---

>  .../devicetree/bindings/sound/qcom,q6core.txt       | 21 +++++++++++++++++++++

>  1 file changed, 21 insertions(+)

>  create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6core.txt


Reviewed-by: Rob Herring <robh@kernel.org>

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Rob Herring April 27, 2018, 2:18 p.m. | #5
On Thu, Apr 26, 2018 at 10:46:04AM +0100, srinivas.kandagatla@linaro.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> 

> Add devicetree bindings documentation file for Qualcomm apq8096 sound card.

> 

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> ---

>  .../devicetree/bindings/sound/qcom,apq8096.txt     | 109 +++++++++++++++++++++

>  1 file changed, 109 insertions(+)

>  create mode 100644 Documentation/devicetree/bindings/sound/qcom,apq8096.txt


Reviewed-by: Rob Herring <robh@kernel.org>


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Srinivas Kandagatla April 28, 2018, 11:12 a.m. | #6
Thanks Bjorn for the review comments.

On 28/04/18 05:51, Bjorn Andersson wrote:
> On Thu 26 Apr 02:45 PDT 2018, Srinivas Kandagatla wrote:

>> diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c

> [..]

>> +int apr_send_pkt(struct apr_device *adev, void *buf)

> 

> Sorry, but I think we have discussed this before?

> 

Yes, I did mention that I would give it a try and see, This change was 
pretty intrusive when I last looked at this.

I agree with you on asymmetry! I will change this and add struc apr_pkt 
which would apr_hdr followed by payload. This should also work for 
callback as well.

> "buf" isn't some random buffer to be sent, it is a apr_hdr followed by

> some data. As such I think you should make this type struct apr_hdr *,

> or if you think that doesn't imply there's payload make a type apr_pkt

> that has a payload[] at the end.

> 

> It will make it obvious for both future readers and the compiler what

> kind of data we're passing here.

> 

> 

> This comment also applies to functions calling functions that calls

> apr_send_pkt() as they too lug around a void *.

> 

>> +{

>> +	struct apr *apr = dev_get_drvdata(adev->dev.parent);

>> +	struct apr_hdr *hdr;

>> +	unsigned long flags;

>> +	int ret;

>> +

>> +	spin_lock_irqsave(&adev->lock, flags);

>> +

>> +	hdr = (struct apr_hdr *)buf;

>> +	hdr->src_domain = APR_DOMAIN_APPS;

>> +	hdr->src_svc = adev->svc_id;

>> +	hdr->dest_domain = adev->domain_id;

>> +	hdr->dest_svc = adev->svc_id;

>> +

>> +	ret = rpmsg_trysend(apr->ch, buf, hdr->pkt_size);

>> +	if (ret) {

>> +		dev_err(&adev->dev, "Unable to send APR pkt %d\n",

>> +			hdr->pkt_size);

> 

> Afaict all callers of this function will print an error message,

> sometimes on more than one level in the stack. And if some code path

> does retry sending you will get a printout for each attempt, even though

> the caller is fine with it.

> 

> I would recommend unlocking the spinlock and then do:


I can do that !!

> 

> 	return ret ? : hdr->pkt_size;

> 

>> +	} else {

>> +		ret = hdr->pkt_size;

>> +	}

>> +

>> +	spin_unlock_irqrestore(&adev->lock, flags);

>> +

>> +	return ret;

>> +}

>> +EXPORT_SYMBOL_GPL(apr_send_pkt);

>> +


>> +

>> +static int apr_callback(struct rpmsg_device *rpdev, void *buf,

>> +				  int len, void *priv, u32 addr)

>> +{

>> +	struct apr *apr = dev_get_drvdata(&rpdev->dev);

>> +	struct apr_client_message data;

>> +	struct apr_device *p, *c_svc = NULL;

>> +	struct apr_driver *adrv = NULL;

>> +	struct apr_hdr *hdr;

>> +	unsigned long flags;

>> +	uint16_t hdr_size;

>> +	uint16_t msg_type;

>> +	uint16_t ver;

>> +	uint16_t svc;

>> +

>> +	if (len <= APR_HDR_SIZE) {

>> +		dev_err(apr->dev, "APR: Improper apr pkt received:%p %d\n",

>> +			buf, len);

>> +		return -EINVAL;

>> +	}

>> +

>> +	hdr = buf;

>> +	ver = APR_HDR_FIELD_VER(hdr->hdr_field);

>> +	if (ver > APR_PKT_VER + 1)

>> +		return -EINVAL;

>> +

>> +	hdr_size = APR_HDR_FIELD_SIZE_BYTES(hdr->hdr_field);

>> +	if (hdr_size < APR_HDR_SIZE) {

>> +		dev_err(apr->dev, "APR: Wrong hdr size:%d\n", hdr_size);

>> +		return -EINVAL;

>> +	}

>> +

>> +	if (hdr->pkt_size < APR_HDR_SIZE) {

> 

> I think it would be nice to make sure that hdr->pkt_size is < len as

> well, to reject messages that larger than the incoming buffer.

> 

> The pkt_size should be in the ballpark of len, could this check be

> changed to hdr->pkt_size != len?


Yep, It makes sense, I can add that check here.

> 

>> +		dev_err(apr->dev, "APR: Wrong paket size\n");

>> +		return -EINVAL;

>> +	}

>> +

>> +	msg_type = APR_HDR_FIELD_MT(hdr->hdr_field);

>> +	if (msg_type >= APR_MSG_TYPE_MAX && msg_type != APR_BASIC_RSP_RESULT) {

>> +		dev_err(apr->dev, "APR: Wrong message type: %d\n", msg_type);

>> +		return -EINVAL;

>> +	}

>> +

>> +	if (hdr->src_domain >= APR_DOMAIN_MAX ||

>> +			hdr->dest_domain >= APR_DOMAIN_MAX ||

>> +			hdr->src_svc >= APR_SVC_MAX ||

>> +			hdr->dest_svc >= APR_SVC_MAX) {

>> +		dev_err(apr->dev, "APR: Wrong APR header\n");

>> +		return -EINVAL;

>> +	}

>> +

>> +	svc = hdr->dest_svc;

>> +	spin_lock_irqsave(&apr->svcs_lock, flags);

>> +	list_for_each_entry(p, &apr->svcs, node) {

> 

> Rather than doing a O(n) search for the svc with svc_id you could use a

> idr or a radix_tree to keep the id -> svc mapping.


Am not 100% sure idr is correct thing here, as the svc_ids are static 
and the list we are talking here in worst case would be max of 13 
entires, in audio case it is just 4 services.
I think using radix_tree would be over do.

> 

>> +		if (svc == p->svc_id) {

>> +			c_svc = p;

>> +			if (c_svc->dev.driver)

>> +				adrv = to_apr_driver(c_svc->dev.driver);

>> +			break;

>> +		}

>> +	}

>> +	spin_unlock_irqrestore(&apr->svcs_lock, flags);

>> +

>> +	if (!adrv) {

>> +		dev_err(apr->dev, "APR: service is not registered\n");

>> +		return -EINVAL;

>> +	}

>> +

>> +	data.payload_size = hdr->pkt_size - hdr_size;

>> +	data.opcode = hdr->opcode;

>> +	data.src_port = hdr->src_port;

>> +	data.dest_port = hdr->dest_port;

>> +	data.token = hdr->token;

>> +	data.msg_type = msg_type;

>> +

>> +	if (data.payload_size > 0)

>> +		data.payload = buf + hdr_size;

>> +

> 

> Making a verbatim copy of parts of the hdr and then passing that to the

> APR devices creates an asymmetry in the send/callback API, without a

> whole lot of benefits. I would prefer that you introduce the apr_pkt, as

> described above, and once you have validated the size et al and found

> the service you just pass it along.

Okay, this would be a big rework, I will do it in next version.

> 

>> +	adrv->callback(c_svc, &data);

>> +

>> +	return 0;

>> +}

> [..]

>> +static int apr_uevent(struct device *dev, struct kobj_uevent_env *env)

>> +{

>> +	struct apr_device *adev = to_apr_device(dev);

>> +	int ret;

>> +

>> +	ret = of_device_uevent_modalias(dev, env);

>> +	if (ret != -ENODEV)

>> +		return ret;

>> +

>> +	return add_uevent_var(env, "MODALIAS= apr:%s", adev->name);

> 

> No space between '=' and "apr".

>

Yep.

>> +}

>> +

>> +struct bus_type aprbus = {

>> +	.name		= "aprbus",

> 

> Most busses doesn't have "bus" in the name.

> 

Yep, just "apr" make sense.

>> +	.match		= apr_device_match,

>> +	.probe		= apr_device_probe,

>> +	.uevent		= apr_uevent,

>> +	.remove		= apr_device_remove,

>> +};

>> +EXPORT_SYMBOL_GPL(aprbus);

>> +

>> +static int apr_add_device(struct device *dev, struct device_node *np,

>> +			  const struct apr_device_id *id)

>> +{

>> +	struct apr *apr = dev_get_drvdata(dev);

>> +	struct apr_device *adev = NULL;

>> +

>> +	adev = kzalloc(sizeof(*adev), GFP_KERNEL);

>> +	if (!adev)

>> +		return -ENOMEM;

>> +

>> +	spin_lock_init(&adev->lock);

>> +

>> +	adev->svc_id = id->svc_id;

>> +	adev->domain_id = id->domain_id;

>> +	adev->version = id->svc_version;

>> +	if (np)

>> +		strncpy(adev->name, np->name, APR_NAME_SIZE);

>> +	else

>> +		strncpy(adev->name, id->name, APR_NAME_SIZE);

>> +

>> +	dev_set_name(&adev->dev, "aprsvc:%s:%x:%x", adev->name,

>> +		     id->domain_id, id->svc_id);

>> +

>> +	adev->dev.bus = &aprbus;

>> +	adev->dev.parent = dev;

>> +	adev->dev.of_node = np;

>> +	adev->dev.release = apr_dev_release;

>> +	adev->dev.driver = NULL;

>> +

>> +	spin_lock(&apr->svcs_lock);

>> +	list_add_tail(&adev->node, &apr->svcs);

>> +	spin_unlock(&apr->svcs_lock);

>> +

>> +	dev_info(dev, "Adding APR dev: %s\n", dev_name(&adev->dev));

>> +

>> +	return device_register(&adev->dev);

> 

> If this fails you must call put_device(&adev->dev);

> 

Agree!!
>> +}

>> +

>> +static void of_register_apr_devices(struct device *dev)

>> +{

>> +	struct apr *apr = dev_get_drvdata(dev);

>> +	struct device_node *node;

>> +

>> +	for_each_child_of_node(dev->of_node, node) {

>> +		struct apr_device_id id = { {0} };

>> +

>> +		if (of_property_read_u32(node, "reg", &id.svc_id))

>> +			continue;

>> +

>> +		id.domain_id = apr->dest_domain_id;

>> +

>> +		if (apr_add_device(dev, node, &id))

>> +			dev_err(dev, "Failed to add arp %d svc\n", id.svc_id);

>> +	}

>> +}

> [..]

>> +

>> +static int __init apr_init(void)

>> +{

>> +	int ret;

>> +

>> +	ret = bus_register(&aprbus);

>> +	if (!ret)

>> +		ret = register_rpmsg_driver(&apr_driver);

> 

> bus_unregister() if ret here.

> 

Yep.

Thanks,
srini
>> +

>> +	return ret;

>> +}

>> +

> 

> Regards,

> Bjorn

> 

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel