On Tue, Feb 20, 2018 at 3:33 AM, Srinivas Kandagatla
<srinivas.kandagatla@xxxxxxxxxx> 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@xxxxxxxxxx
>>>> wrote:
>>>>>
>>>>> From: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
>>>>>
>>>>> 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@xxxxxxxxxx>
>>>>> ---
>>>>> .../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.
>>>>> +
>>>>> +- compatible:
>>>>> + Usage: required
>>>>> + Value type: <stringlist>
>>>>> + Definition: must be "qcom,apr-v<VERSION-NUMBER>", example
>>>>> "qcom,apr-v2"
>>>>> +
>>>>> +- qcom,apr-dest-domain-id
>>>>> + Usage: required
>>>>> + Value type: <prop-encoded-array>
>>>>> + Definition: Destination processor ID.
>>>>> + Possible values are :
>>>>> + 1 - APR simulator
>>>>> + 2 - PC
>>>>> + 3 - MODEM
>>>>> + 4 - ADSP
>>>>> + 5 - APPS
>>>>> + 6 - MODEM2
>>>>> + 7 - APPS2
>>>>> +
>>>>> += APR SERVICES
>>>>> +Each subnode of the APR node can represent service tied to this apr.
>>>>> The name
>>>>> +of the nodes are not important. The properties of these nodes are
>>>>> defined
>>>>> +by the individual bindings for the specific service
>>>>> +- but must contain the following property:
>>>>> +
>>>
>>> ...
>>>>>
>>>>> += APR DEVICES:
>>>>> +Each subnode of the APR node can represent devices tied to this apr,
>>>>> like
>>>>> +sound-card. The properties of these nodes are defined by the
>>>>> individual
>>>>> +bindings for the specific device.
>>>>
>>>>
>>>> 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?
I don't see any versioning of services here.
>
>>>
>>> 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?
>
> 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.
>
>>
>>> };
>>>
>>> apr-devices {
>>> audio {
>>> compatible = "qcom,msm8996-snd-card";
>>
>>
>> This is a pseudo device. Why not put it at the top level like other
>> sound cards?
>
>
> APR bus depends on the state of DSP services, which can go off if the DSP
> crashes or DSP is stopped. If we remove this sound card out of apr bus then
> the sound card dependency on apr bus is totally lost.
>
> Main purpose of having sound card under this bus is that the sound card
> should register/unregister depending up the apr channel presence/absence
> respectively.
Okay, that seems sensible.
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
