Re: [PATCH v3 01/25] dt-bindings: soc: qcom: Add bindings for APR bus

Srinivas Kandagatla
2018-02-22T10:03:49Z

On 22/02/18 00:14, Rob Herring wrote:
> On Tue, Feb 20, 2018 at 3:33 AM, Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>> Thanks for your review comments,
>>
>>
>> On 18/02/18 23:04, Rob Herring wrote:
>>>
>>> On Wed, Feb 14, 2018 at 09:13:23AM +0000, Srinivas Kandagatla wrote:
>>>>
>>>> Thanks for the review,
>>>>
>>>> On 13/02/18 23:12, Rob Herring wrote:
>>>>>
>>>>> On Tue, Feb 13, 2018 at 04:58:13PM +0000, srinivas.kandagatla@linaro.org
>>>>> wrote:
>>>>>>
>>>>>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>>>>>
>>>>>> This patch add dt bindings for Qualcomm APR (Asynchronous Packet
>>>>>> Router)
>>>>>> bus driver. This bus is used for communicating with DSP which provides
>>>>>> audio and various other services to cpu.
>>>>>>
>>>>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>>>>> ---
>>>>>> .../devicetree/bindings/soc/qcom/qcom,apr.txt | 83
>>>>>> ++++++++++++++++++++++
>>>>>> 1 file changed, 83 insertions(+)
>>>>>> create mode 100644
>>>>>> Documentation/devicetree/bindings/soc/qcom/qcom,apr.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,apr.txt
>>>>>> b/Documentation/devicetree/bindings/soc/qcom/qcom,apr.txt
>>>>>> new file mode 100644
>>>>>> index 000000000000..1b95fbfed348
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,apr.txt
>>>>>> @@ -0,0 +1,83 @@
>>>>>> +Qualcomm APR (Asynchronous Packet Router) binding
>>>>>> +
>>>>>> +This binding describes the Qualcomm APR. APR is a IPC protocol for
>>>>>> +communication between Application processor and QDSP. APR is mainly
>>>>>> +used for audio/voice services on the QDSP.
>>>>>> +
...
>>>>>
>>>>>
>>>>> It's not a good design generally to mix different types of nodes at one
>>>>> level.
>>>>
>>>>
>>>> I agree, may be I can split the services and devices into different
>>>> subnodes
>>>> like below, which should avoid mixing different types of nodes.
>>>>
>>>> Does this sound good to you?
>>>
>>>
>>> Seems your original example wasn't so complete...
>>>
>> Yep, I will fix it in next version.
>>>
>>> I don't see why you need all these nodes in the first place. For a
>>> single SoC, how much does all this vary?
>>>
>> It might not vary for a given SoC, but It does vary across the SoCs.
>> Also the versions of each service are independent to each other.
>
> Not sure I follow the last statement. Meaning firmware updates change
> the services?
Sorry for not being clear, so the services like AFE, ASM, ADM have
different version numbers for a given SoC/firmware.

Not 100% sure if firmware updates would change the service version
number, even if it does, it can be queried dynamically on new B family
SoCs, and on older A Family SoCs I have not seen the firmware updates in
last 4 years.

> I don't see any versioning of services here.

Yes, Plan is that it will be part of the compatible string in cases
where version query is not supported on older QCOM A family SoCs.
On B Family SoCs we can query the version dynamically.

>
>>
>>>>
>>>> apr {
>>>> compatible = "qcom,apr-v2";
>>>> qcom,smd-channels = "apr_audio_svc";
>>>> qcom,apr-dest-domain-id = <APR_DOMAIN_ADSP>;
>>>>
>>>> apr-services {
>>>> q6core {
>>>> qcom,apr-svc-name = "CORE";
>>>> qcom,apr-svc-id = <APR_SVC_ADSP_CORE>;
>>>> compatible = "qcom,q6core";
>>>> };
>>>>
>>>> q6afe: q6afe {
>>>> compatible = "qcom,q6afe";
>>>> qcom,apr-svc-name = "AFE";
>>>> qcom,apr-svc-id = <APR_SVC_AFE>;
>>>> #sound-dai-cells = <1>;
>>>> };
>>>>
>>>> q6asm: q6asm {
>>>> compatible = "qcom,q6asm";
>>>> qcom,apr-svc-name = "ASM";
>>>> qcom,apr-svc-id = <APR_SVC_ASM>;
>>>> #sound-dai-cells = <1>;
>>>> };
>>>>
>>>> q6adm: q6adm {
>>>> compatible = "qcom,q6adm";
>>>> qcom,apr-svc-name = "ADM";
>>>> qcom,apr-svc-id = <APR_SVC_ADM>;
>>>> #sound-dai-cells = <0>;
>>>> };
>>>
>>>
>>> All these DAI nodes could be a single node and the cell value be the
>>> svc-id?
>>
So we will have 2 cell values, one representing the apr service and
other the dai.

>> No, DAI's here are both backends and frontends, and some of the services
>> like core, USM are not DAI's
>>
>> Are you also saying that we should have a single driver entity for all these
>> services?
>
> DT nodes do not equate driver entities. A driver can instantiate other drivers.
>
> Am I saying a single DT node for this? Yes, perhaps.
Yes, I will give that a go.

So we will endup having something like this in DT for one frontend and
backend dailink:

apr {
compatible = "qcom,apr-v2";
qcom,smd-channels = "apr_audio_svc";
qcom,apr-dest-domain-id = <APR_DOMAIN_ADSP>

q6audio: audiosvc {
compatible = "qcom,q6audio-svc";
qcom,apr-svcs = <APR_SVC_CORE APR_SVC_AFE APR_SVC_ASM>;
qcom,apr-svc-names = "CORE", "AFE", "ASM";
};

sndcard {
compatible = "qcom,msm8996-snd-card"
qcom,model = "DB820c";
qcom,audio-routing =
"RX_BIAS", "MCLK";
fe@1 {
is-fe;
link-name = "MultiMedia1";
cpu {
sound-dai = <&q6audio APR_SVC_ASM MM1>
};
platform {
sound-dai = <&q6audio APR_SVC_ASM MM1>;
};
};

be@1 {
link-name = "HDMI Playback";
cpu {
sound-dai = <&q6audio APR_SVC_AFE HDMI>;
};

platform {
sound-dai = <&q6audio ARP_SVC_ADM>;
};

codec {
sound-dai = <&hdmi 0>;
};
};
};
};

Thanks,
Srini