[PATCH 0/2] FastRPC reserved memory assignment for SDM845 SLPI

[PATCH 0/2] FastRPC reserved memory assignment for SDM845 SLPI

[Dylan Van Assche]

* About *

The Qualcomm SDM845 SoC has a separate SLPI (Sensor Low Power Island)
DSP for sensors connected to the SoC which is responsible for exposing
sensors to userspace, power saving, and other features. 
While sensors are connected to GPIOs of the SoC, they cannot be used
because the hypervisor blocks direct access to the sensors, thus the 
DSP must be used to access any sensor on this SoC. The SLPI DSP uses a
GLink edge (dsps) to communicate with the host and has a FastRPC interface
to load files from the host filesystem such as sensor configuration files.
The FastRPC interface does not use regular FastRPC Compute Banks
but instead uses an allocated CMA region through which communication happens.

* Changes *

This patchseries add support to the FastRPC for assigning a coherent memory
region to a DSP via the hypervisor with the correct permissions.
This is necessary to support the SLPI found in the Qualcomm SDM845 SoC which
does not have dedicated FastRPC Compute Banks, in contrast to newer SoCs,
but uses a memory region instead.

* Related patches *

1. Remoteproc changes to support the SLPI DSP in SDM845:
https://lore.kernel.org/linux-remoteproc/20230325132117.19733-1-me@dylanvanassche.be/
2. DTS changes will be submitted after this serie.

This serie does not depend on any serie, but all of them are necessary
to enable the feature in the end.

Kind regards,
Dylan Van Assche

Dylan Van Assche (2):
  dt-bindings: misc: qcom,fastrpc: add qcom,assign-all-memory property
  misc: fastrpc: support complete DMA pool access to the DSP

 .../bindings/misc/qcom,fastrpc.yaml           |  6 ++++++
 drivers/misc/fastrpc.c                        | 19 ++++++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)

-- 
2.39.2

[PATCH 1/2] dt-bindings: misc: qcom,fastrpc: add qcom,assign-all-memory property

[Dylan Van Assche]

Document the added qcom,assign-all-memory in devicetree bindings.

Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
---
 Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
index 1ab9588cdd89..fa5b00534b30 100644
--- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
+++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
@@ -57,6 +57,12 @@ properties:
       Virtual machine IDs for remote processor.
     $ref: "/schemas/types.yaml#/definitions/uint32-array"
 
+  qcom,assign-all-mem:
+    description:
+      Assign memory to all Virtual machines defined by qcom,vmids.
+    type: boolean
+
+
   "#address-cells":
     const: 1
 
-- 
2.39.2

[PATCH 2/2] misc: fastrpc: support complete DMA pool access to the DSP

[Dylan Van Assche]

To support FastRPC Context Banks which aren't mapped via the SMMU,
make the whole reserved memory region available to the DSP to allow
access to coherent buffers.

This is performed by assigning the memory to the DSP via a hypervisor
call to set the correct permissions for the Virtual Machines on the DSP.
Only perform this operation when at least one VM is enabled
and 'qcom,assign-all-mem' property is present in DTS.

Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
---
 drivers/misc/fastrpc.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index f48466960f1b..ecfd0a91113c 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -2230,7 +2230,9 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	struct fastrpc_channel_ctx *data;
 	int i, err, domain_id = -1, vmcount;
 	const char *domain;
-	bool secure_dsp;
+	bool secure_dsp, assign_all_mem;
+	struct device_node *rmem_node;
+	struct reserved_mem *rmem;
 	unsigned int vmids[FASTRPC_MAX_VMIDS];
 
 	err = of_property_read_string(rdev->of_node, "label", &domain);
@@ -2265,6 +2267,11 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	if (!data)
 		return -ENOMEM;
 
+	assign_all_mem = of_property_read_bool(rdev->of_node, "qcom,assign-all-mem");
+
+	if (assign_all_mem && !vmcount)
+		return -EINVAL;
+
 	if (vmcount) {
 		data->vmcount = vmcount;
 		data->perms = BIT(QCOM_SCM_VMID_HLOS);
@@ -2274,6 +2281,16 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 		}
 	}
 
+	if (assign_all_mem) {
+		rmem_node = of_parse_phandle(rdev->of_node, "memory-region", 0);
+		if (rmem_node) {
+			rmem = of_reserved_mem_lookup(rmem_node);
+			if (rmem)
+				qcom_scm_assign_mem(rmem->base, rmem->size, &data->perms,
+						    data->vmperms, data->vmcount);
+		}
+	}
+
 	secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
 	data->secure = secure_dsp;
 
-- 
2.39.2

Re: [PATCH 1/2] dt-bindings: misc: qcom,fastrpc: add qcom,assign-all-memory property

[Krzysztof Kozlowski]

On 25/03/2023 14:44, Dylan Van Assche wrote:
> Document the added qcom,assign-all-memory in devicetree bindings.
> 
> Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
> ---
>  Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> index 1ab9588cdd89..fa5b00534b30 100644
> --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> @@ -57,6 +57,12 @@ properties:
>        Virtual machine IDs for remote processor.
>      $ref: "/schemas/types.yaml#/definitions/uint32-array"
>  
> +  qcom,assign-all-mem:
> +    description:
> +      Assign memory to all Virtual machines defined by qcom,vmids.

This (neither commit msg) does not explain why this is needed and
actually does not sound like hardware-related property.

> +    type: boolean
> +
> +

Do not add double blank lines.

>    "#address-cells":
>      const: 1
>  

Best regards,
Krzysztof

Re: [PATCH 2/2] misc: fastrpc: support complete DMA pool access to the DSP

[Dan Carpenter]

Hi Dylan,

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dylan-Van-Assche/dt-bindings-misc-qcom-fastrpc-add-qcom-assign-all-memory-property/20230325-214518
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230325134410.21092-3-me%40dylanvanassche.be
patch subject: [PATCH 2/2] misc: fastrpc: support complete DMA pool access to the DSP
config: microblaze-randconfig-m041-20230326 (https://download.01.org/0day-ci/archive/20230327/202303270739.ODb2LA29-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Link: https://lore.kernel.org/r/202303270739.ODb2LA29-lkp@intel.com/

New smatch warnings:
drivers/misc/fastrpc.c:2273 fastrpc_rpmsg_probe() warn: possible memory leak of 'data'

vim +/data +2273 drivers/misc/fastrpc.c

f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2227  static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2228  {
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2229  	struct device *rdev = &rpdev->dev;
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2230  	struct fastrpc_channel_ctx *data;
e90d911906196b Vamsi Krishna Gattupalli 2022-02-14  2231  	int i, err, domain_id = -1, vmcount;
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2232  	const char *domain;
99edd50174e519 Dylan Van Assche         2023-03-25  2233  	bool secure_dsp, assign_all_mem;
99edd50174e519 Dylan Van Assche         2023-03-25  2234  	struct device_node *rmem_node;
99edd50174e519 Dylan Van Assche         2023-03-25  2235  	struct reserved_mem *rmem;
e90d911906196b Vamsi Krishna Gattupalli 2022-02-14  2236  	unsigned int vmids[FASTRPC_MAX_VMIDS];
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2237  
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2238  	err = of_property_read_string(rdev->of_node, "label", &domain);
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2239  	if (err) {
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2240  		dev_info(rdev, "FastRPC Domain not specified in DT\n");
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2241  		return err;
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2242  	}
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2243  
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2244  	for (i = 0; i <= CDSP_DOMAIN_ID; i++) {
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2245  		if (!strcmp(domains[i], domain)) {
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2246  			domain_id = i;
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2247  			break;
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2248  		}
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2249  	}
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2250  
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2251  	if (domain_id < 0) {
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2252  		dev_info(rdev, "FastRPC Invalid Domain ID %d\n", domain_id);
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2253  		return -EINVAL;
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2254  	}
f6f9279f2bf0e3 Srinivas Kandagatla      2019-02-08  2255  
1ce91d45ba77a4 Abel Vesa                2022-11-25  2256  	if (of_reserved_mem_device_init_by_idx(rdev, rdev->of_node, 0))
1ce91d45ba77a4 Abel Vesa                2022-11-25  2257  		dev_info(rdev, "no reserved DMA memory for FASTRPC\n");
1ce91d45ba77a4 Abel Vesa                2022-11-25  2258  
e90d911906196b Vamsi Krishna Gattupalli 2022-02-14  2259  	vmcount = of_property_read_variable_u32_array(rdev->of_node,
e90d911906196b Vamsi Krishna Gattupalli 2022-02-14  2260  				"qcom,vmids", &vmids[0], 0, FASTRPC_MAX_VMIDS);
e90d911906196b Vamsi Krishna Gattupalli 2022-02-14  2261  	if (vmcount < 0)
e90d911906196b Vamsi Krishna Gattupalli 2022-02-14  2262  		vmcount = 0;
e90d911906196b Vamsi Krishna Gattupalli 2022-02-14  2263  	else if (!qcom_scm_is_available())
e90d911906196b Vamsi Krishna Gattupalli 2022-02-14  2264  		return -EPROBE_DEFER;
e90d911906196b Vamsi Krishna Gattupalli 2022-02-14  2265  
278d56f970ae6e Bjorn Andersson          2019-08-29  2266  	data = kzalloc(sizeof(*data), GFP_KERNEL);
278d56f970ae6e Bjorn Andersson          2019-08-29  2267  	if (!data)
278d56f970ae6e Bjorn Andersson          2019-08-29  2268  		return -ENOMEM;
278d56f970ae6e Bjorn Andersson          2019-08-29  2269  
99edd50174e519 Dylan Van Assche         2023-03-25  2270  	assign_all_mem = of_property_read_bool(rdev->of_node, "qcom,assign-all-mem");
99edd50174e519 Dylan Van Assche         2023-03-25  2271  
99edd50174e519 Dylan Van Assche         2023-03-25  2272  	if (assign_all_mem && !vmcount)
99edd50174e519 Dylan Van Assche         2023-03-25 @2273  		return -EINVAL;

Move this code before the data = kzalloc() allocation to avoid a memory
leak.

99edd50174e519 Dylan Van Assche         2023-03-25  2274  
e90d911906196b Vamsi Krishna Gattupalli 2022-02-14  2275  	if (vmcount) {
e90d911906196b Vamsi Krishna Gattupalli 2022-02-14  2276  		data->vmcount = vmcount;
e90d911906196b Vamsi Krishna Gattupalli 2022-02-14  2277  		data->perms = BIT(QCOM_SCM_VMID_HLOS);
e90d911906196b Vamsi Krishna Gattupalli 2022-02-14  2278  		for (i = 0; i < data->vmcount; i++) {
e90d911906196b Vamsi Krishna Gattupalli 2022-02-14  2279  			data->vmperms[i].vmid = vmids[i];
e90d911906196b Vamsi Krishna Gattupalli 2022-02-14  2280  			data->vmperms[i].perm = QCOM_SCM_PERM_RWX;

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 1/2] dt-bindings: misc: qcom,fastrpc: add qcom,assign-all-memory property

[Dylan Van Assche]

Hi Krzysztof,

On Sun, 2023-03-26 at 10:55 +0200, Krzysztof Kozlowski wrote:
> On 25/03/2023 14:44, Dylan Van Assche wrote:
> > Document the added qcom,assign-all-memory in devicetree bindings.
> > 
> > Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
> > ---
> >  Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6
> > ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > index 1ab9588cdd89..fa5b00534b30 100644
> > --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > @@ -57,6 +57,12 @@ properties:
> >        Virtual machine IDs for remote processor.
> >      $ref: "/schemas/types.yaml#/definitions/uint32-array"
> >  
> > +  qcom,assign-all-mem:
> > +    description:
> > +      Assign memory to all Virtual machines defined by qcom,vmids.
> 
> This (neither commit msg) does not explain why this is needed and
> actually does not sound like hardware-related property.

This is made a separate property to toggle different behavior in the
driver if it is needed for some FastRPC nodes. Downstream does guard
this with a property 'restrict-access' as well, see [1] for a random
SDM845 downstream kernel. On SDM845, this property is not present, thus
the IF block runs. On SDM670, this property is present, then the IF
block is skipped. That's why I opt for this property to have this
behaviour conditionally. I'm not sure how to explain it better though.

Any feedback is appreciated, thanks!

Kind regards,
Dylan Van Assche

[1]
https://github.com/SHIFTPHONES/android_kernel_shift_sdm845/blob/sos-3.x/drivers/char/adsprpc.c#L4615-L4631

> 
> > +    type: boolean
> > +
> > +
> 
> Do not add double blank lines.
> 
> >    "#address-cells":
> >      const: 1
> >  
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/2] dt-bindings: misc: qcom,fastrpc: add qcom,assign-all-memory property

[Krzysztof Kozlowski]

On 27/03/2023 13:37, Dylan Van Assche wrote:
> Hi Krzysztof,
> 
> On Sun, 2023-03-26 at 10:55 +0200, Krzysztof Kozlowski wrote:
>> On 25/03/2023 14:44, Dylan Van Assche wrote:
>>> Document the added qcom,assign-all-memory in devicetree bindings.
>>>
>>> Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
>>> ---
>>>  Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6
>>> ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>>> b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>>> index 1ab9588cdd89..fa5b00534b30 100644
>>> --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>>> +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>>> @@ -57,6 +57,12 @@ properties:
>>>        Virtual machine IDs for remote processor.
>>>      $ref: "/schemas/types.yaml#/definitions/uint32-array"
>>>  
>>> +  qcom,assign-all-mem:
>>> +    description:
>>> +      Assign memory to all Virtual machines defined by qcom,vmids.
>>
>> This (neither commit msg) does not explain why this is needed and
>> actually does not sound like hardware-related property.
> 
> This is made a separate property to toggle different behavior in the
> driver if it is needed for some FastRPC nodes. 

Bindings are not for driver behavior.

> Downstream does guard
> this with a property 'restrict-access' as well, see [1] for a random
> SDM845 downstream kernel. On SDM845, this property is not present, thus
> the IF block runs. On SDM670, this property is present, then the IF
> block is skipped. That's why I opt for this property to have this
> behaviour conditionally. I'm not sure how to explain it better though.

Still you described driver... Please come with something more hardware
related.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: misc: qcom,fastrpc: add qcom,assign-all-memory property

[Dylan Van Assche]

Hi Krzysztof,

On Mon, 2023-03-27 at 14:22 +0200, Krzysztof Kozlowski wrote:
> On 27/03/2023 13:37, Dylan Van Assche wrote:
> > Hi Krzysztof,
> > 
> > On Sun, 2023-03-26 at 10:55 +0200, Krzysztof Kozlowski wrote:
> > > On 25/03/2023 14:44, Dylan Van Assche wrote:
> > > > Document the added qcom,assign-all-memory in devicetree
> > > > bindings.
> > > > 
> > > > Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
> > > > ---
> > > >  Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6
> > > > ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > > > b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > > > index 1ab9588cdd89..fa5b00534b30 100644
> > > > --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > > > +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > > > @@ -57,6 +57,12 @@ properties:
> > > >        Virtual machine IDs for remote processor.
> > > >      $ref: "/schemas/types.yaml#/definitions/uint32-array"
> > > >  
> > > > +  qcom,assign-all-mem:
> > > > +    description:
> > > > +      Assign memory to all Virtual machines defined by
> > > > qcom,vmids.
> > > 
> > > This (neither commit msg) does not explain why this is needed and
> > > actually does not sound like hardware-related property.
> > 
> > This is made a separate property to toggle different behavior in
> > the
> > driver if it is needed for some FastRPC nodes. 
> 
> Bindings are not for driver behavior.
> 
> > Downstream does guard
> > this with a property 'restrict-access' as well, see [1] for a
> > random
> > SDM845 downstream kernel. On SDM845, this property is not present,
> > thus
> > the IF block runs. On SDM670, this property is present, then the IF
> > block is skipped. That's why I opt for this property to have this
> > behaviour conditionally. I'm not sure how to explain it better
> > though.
> 
> Still you described driver... Please come with something more
> hardware
> related.

So just updating the description is enough then?

As this is all reverse engineered, I have no access to the
documentation of FastRPC, so best effort:

"""
Mark allocated memory region accessible to remote processor.
This memory region is used by remote processor to communicate
when no dedicated Fastrpc context bank hardware is available 
for remote processor.
"""

Is this the description that is 'more hardware related'?

Kind regards,
Dylan Van Assche

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/2] dt-bindings: misc: qcom,fastrpc: add qcom,assign-all-memory property

[Dmitry Baryshkov]

On Mon, 27 Mar 2023 at 17:27, Dylan Van Assche <me@dylanvanassche.be> wrote:
>
> Hi Krzysztof,
>
> On Mon, 2023-03-27 at 14:22 +0200, Krzysztof Kozlowski wrote:
> > On 27/03/2023 13:37, Dylan Van Assche wrote:
> > > Hi Krzysztof,
> > >
> > > On Sun, 2023-03-26 at 10:55 +0200, Krzysztof Kozlowski wrote:
> > > > On 25/03/2023 14:44, Dylan Van Assche wrote:
> > > > > Document the added qcom,assign-all-memory in devicetree
> > > > > bindings.
> > > > >
> > > > > Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6
> > > > > ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > > > > b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > > > > index 1ab9588cdd89..fa5b00534b30 100644
> > > > > --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > > > > +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > > > > @@ -57,6 +57,12 @@ properties:
> > > > >        Virtual machine IDs for remote processor.
> > > > >      $ref: "/schemas/types.yaml#/definitions/uint32-array"
> > > > >
> > > > > +  qcom,assign-all-mem:
> > > > > +    description:
> > > > > +      Assign memory to all Virtual machines defined by
> > > > > qcom,vmids.
> > > >
> > > > This (neither commit msg) does not explain why this is needed and
> > > > actually does not sound like hardware-related property.
> > >
> > > This is made a separate property to toggle different behavior in
> > > the
> > > driver if it is needed for some FastRPC nodes.
> >
> > Bindings are not for driver behavior.
> >
> > > Downstream does guard
> > > this with a property 'restrict-access' as well, see [1] for a
> > > random
> > > SDM845 downstream kernel. On SDM845, this property is not present,
> > > thus
> > > the IF block runs. On SDM670, this property is present, then the IF
> > > block is skipped. That's why I opt for this property to have this
> > > behaviour conditionally. I'm not sure how to explain it better
> > > though.
> >
> > Still you described driver... Please come with something more
> > hardware
> > related.
>
> So just updating the description is enough then?
>
> As this is all reverse engineered, I have no access to the
> documentation of FastRPC, so best effort:

Vendor kernels put a lot of controls into DT, despite some of these
controls being related to software or being a platform constant.
Upstream tends to push some of the constraints into the driver data,
leaving only variadic parts in DT.
Could you please check if the property you are proposing is constant
among the devices on a platform or not. If it is a platform
peculiarity, a usual way is to add it to driver data rather than the
DT.

>
> """
> Mark allocated memory region accessible to remote processor.
> This memory region is used by remote processor to communicate
> when no dedicated Fastrpc context bank hardware is available
> for remote processor.
> """
>
> Is this the description that is 'more hardware related'?

-- 
With best wishes
Dmitry

Re: [PATCH 1/2] dt-bindings: misc: qcom,fastrpc: add qcom,assign-all-memory property

[Krzysztof Kozlowski]

On 27/03/2023 16:26, Dylan Van Assche wrote:
>> Bindings are not for driver behavior.
>>
>>> Downstream does guard
>>> this with a property 'restrict-access' as well, see [1] for a
>>> random
>>> SDM845 downstream kernel. On SDM845, this property is not present,
>>> thus
>>> the IF block runs. On SDM670, this property is present, then the IF
>>> block is skipped. That's why I opt for this property to have this
>>> behaviour conditionally. I'm not sure how to explain it better
>>> though.
>>
>> Still you described driver... Please come with something more
>> hardware
>> related.
> 
> So just updating the description is enough then?
> 
> As this is all reverse engineered, I have no access to the
> documentation of FastRPC, so best effort:
> 
> """
> Mark allocated memory region accessible to remote processor.
> This memory region is used by remote processor to communicate
> when no dedicated Fastrpc context bank hardware is available 
> for remote processor.

This description does not explain here anything. The memory region is
already accessible without this property.

You described the desired Linux feature or behavior, not the actual
hardware. The bindings are about the latter, so instead you need to
rephrase the property and its description to match actual hardware
capabilities/features/configuration etc.

Remember that any arguments to downstream are not really good arguments.
Their design choices and bindings are usually totally not acceptable.
They simply embed whatever driver needs in DT - policies, system
configuration, driver behavior...

Also, Dmitry made here good point.



Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: misc: qcom,fastrpc: add qcom,assign-all-memory property

[Dylan Van Assche]

Hi Dmitry & Krzysztof,

On Mon, 2023-03-27 at 16:36 +0200, Krzysztof Kozlowski wrote:
> On 27/03/2023 16:26, Dylan Van Assche wrote:
> > > Bindings are not for driver behavior.
> > > 
> > > > Downstream does guard
> > > > this with a property 'restrict-access' as well, see [1] for a
> > > > random
> > > > SDM845 downstream kernel. On SDM845, this property is not
> > > > present,
> > > > thus
> > > > the IF block runs. On SDM670, this property is present, then
> > > > the IF
> > > > block is skipped. That's why I opt for this property to have
> > > > this
> > > > behaviour conditionally. I'm not sure how to explain it better
> > > > though.
> > > 
> > > Still you described driver... Please come with something more
> > > hardware
> > > related.
> > 
> > So just updating the description is enough then?
> > 
> > As this is all reverse engineered, I have no access to the
> > documentation of FastRPC, so best effort:
> > 
> > """
> > Mark allocated memory region accessible to remote processor.
> > This memory region is used by remote processor to communicate
> > when no dedicated Fastrpc context bank hardware is available 
> > for remote processor.
> 
> This description does not explain here anything. The memory region is
> already accessible without this property.
> 
> You described the desired Linux feature or behavior, not the actual
> hardware. The bindings are about the latter, so instead you need to
> rephrase the property and its description to match actual hardware
> capabilities/features/configuration etc.
> 
> Remember that any arguments to downstream are not really good
> arguments.
> Their design choices and bindings are usually totally not acceptable.
> They simply embed whatever driver needs in DT - policies, system
> configuration, driver behavior...
> 
> Also, Dmitry made here good point.
> 
> 

I agree, downstream is not doing great on being upstreamable.
Thanks Dmitry, your explanation makes it pretty clear what I should
figure out. This helps a lot! As far as I know, this assignment is only
skipped when the sensors are not on the SLPI but on the ADSP e.g.
SDM670, thus mid range SoCs. So reading these comments, this looks more
like 'driver behavior' which should not end up in bindings as mentioned
above. As I now understand the problem with this property, I will
rework it for v2 and drop it. This is only done for the SLPI so by
guarding it with a domain ID check we should be able to avoid the
property in the bindings.

Thanks for the feedback & patience! I really learned a lot!

Kind regards,
Dylan Van Assche

> 
> Best regards,
> Krzysztof
>