Re: [PATCH v3 2/4] dts: arm64: qcom: sdm845: add SLPI FastRPC support

[Konrad Dybcio]

On 31.03.2023 11:36, Dylan Van Assche wrote:
> Hi Konrad,
> 
> On Fri, 2023-03-31 at 04:03 +0200, Konrad Dybcio wrote:
>>
>>
>> On 30.03.2023 18:53, Dylan Van Assche wrote:
>>> Qualcomm SDM845 SoC features a SLPI DSP which uses FastRPC through
>>> an allocated memory region to load files from the host filesystem
>>> such as sensor configuration files.
>>>
>>> Add a FastRPC node at /dev/fastrpc-sdsp and a DMA region, similar
>>> to
>>> downstream, to allow userspace to communicate with the SLPI via the
>>> FastRPC interface for initializing the sensors on the SLPI.
>>>
>>> Signed-off-by: Dylan Van Assche <me@xxxxxxxxxxxxxxxxx>
>>> ---
>>>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 24 ++++++++++++++++++++++++
>>>  1 file changed, 24 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>> b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>> index 3b547cb7aeb8..8ea4944f3ad6 100644
>>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>> @@ -878,6 +878,14 @@ mdata_mem: mpss-metadata {
>>>                         size = <0 0x4000>;
>>>                         no-map;
>>>                 };
>>> +
>>> +               fastrpc_mem: fastrpc {
>>> +                       compatible = "shared-dma-pool";
>>> +                       reusable;
>> Please move it last to get a nice reverse-Christmas-tree layout.
>>
> 
> Will fix in v4.
> 
>>> +                       alloc-ranges = <0 0x00000000 0 0xffffffff>;
>> Would there be any issues with it starting over (1<<31 - 1)?
>>
> 
> You mean a bigger region then, like the whole CMA region then? AFAIK,
> the SLPI always use the same region expecting it to be in this range.
> However, I cannot confirm more, as I have no insights in the firmware
> running on there, this all comes from finding out what it exactly does
> on downstream.
I was asking about the <.. 0 0xfff.f> part specifically, as that means
it can't be allocated above 4 gigs. But I guess it's just how qcom
envisioned it.

Also, please use 0x0 in alloc-ranges as well, this is all addresses/
reg sizes.

Konrad
> 
>>> +                       alignment = <0 0x400000>;
>> Please use 0x0 for the 0 here, as it's essentially reg.size with
>> size-cells = 2
> 
> Will fix in v4.
> 
>>
>>> +                       size = <0 0x1000000>;
>> Ditto
> 
> Will fix in v4.
> 
>>
>>> +               };
>>>         };
>>>  
>>>         adsp_pas: remoteproc-adsp {
>>> @@ -3344,6 +3352,22 @@ glink-edge {
>>>                                 label = "dsps";
>>>                                 qcom,remote-pid = <3>;
>>>                                 mboxes = <&apss_shared 24>;
>>> +
>>> +                               fastrpc {
>>> +                                       compatible =
>>> "qcom,fastrpc";
>>> +                                       qcom,glink-channels =
>>> "fastrpcglink-apps-dsp";
>>> +                                       label = "sdsp";
>>> +                                       qcom,non-secure-domain;
>>> +                                       qcom,vmids = <0x3 0xF 0x5
>>> 0x6>;
>> Please use the recently-introduced header and depend on (and
>> make a patch atop)
>>
>> https://lore.kernel.org/linux-arm-msm/8685b710-b74d-556a-815d-0ffef2b0eeff@xxxxxxxxxx/T/#t
>>
>> Konrad
>>
>>> +                                       memory-region =
>>> <&fastrpc_mem>;
>>> +                                       #address-cells = <1>;
>>> +                                       #size-cells = <0>;
>>> +
>>> +                                       compute-cb@0 {
>>> +                                               compatible =
>>> "qcom,fastrpc-compute-cb";
>>> +                                               reg = <0>;
>>> +                                       };
>>> +                               };
>>>                         };
>>>                 };
>>>  
> 
> Kind regards,
> Dylan