[PATCH v3 1/3] dt-bindings: reserved-memory: rmtfs: Make qcom,vmid an array

[PATCH v3 1/3] dt-bindings: reserved-memory: rmtfs: Make qcom,vmid an array

[Konrad Dybcio]

Some SoCs mandate that the RMTFS is also assigned to the NAV VM, while
others really don't want that. Since it has to be conditional, turn
qcom,vmid into an u32 array so that we can handle the NAV case, as
well as other similar ones if they pop up in the future.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
v2 -> v3:
Rewrite to accomodate for changes, don't pick up tags

 .../devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml   | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
index 2998f1c8f0db..cfc2fda30eba 100644
--- a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
+++ b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
@@ -27,9 +27,9 @@ properties:
       identifier of the client to use this region for buffers
 
   qcom,vmid:
-    $ref: /schemas/types.yaml#/definitions/uint32
+    $ref: /schemas/types.yaml#/definitions/uint32-array
     description: >
-      vmid of the remote processor, to set up memory protection
+      Array of vmids of the remote processors, to set up memory protection
 
 required:
   - qcom,client-id
-- 
2.39.0

[PATCH v3 2/3] soc: qcom: rmtfs: Optionally map RMTFS to more VMs

[Konrad Dybcio]

From: Loic Poulain <loic.poulain@linaro.org>

Some SoCs require that RMTFS is also mapped to the NAV VM. Trying to
power on the modem without that results in the whole platform
crashing and forces a hard reboot within about 2 seconds. Add support
for mapping the region to additional VMs, such as NAV to open a path
towards enabling modem on such platforms.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
[Konrad: reword, make conditional and flexible, add a define for NAV VMID]
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
v2 -> v3:
Rewrite, make it accept more than just NAV through qcom,vmid

 drivers/soc/qcom/rmtfs_mem.c | 29 ++++++++++++++++++++++-------
 include/linux/qcom_scm.h     |  1 +
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
index 0feaae357821..9d59ad509a5c 100644
--- a/drivers/soc/qcom/rmtfs_mem.c
+++ b/drivers/soc/qcom/rmtfs_mem.c
@@ -17,6 +17,7 @@
 #include <linux/qcom_scm.h>
 
 #define QCOM_RMTFS_MEM_DEV_MAX	(MINORMASK + 1)
+#define NUM_MAX_VMIDS		2
 
 static dev_t qcom_rmtfs_mem_major;
 
@@ -171,12 +172,12 @@ static void qcom_rmtfs_mem_release_device(struct device *dev)
 static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
 {
 	struct device_node *node = pdev->dev.of_node;
-	struct qcom_scm_vmperm perms[2];
+	struct qcom_scm_vmperm perms[NUM_MAX_VMIDS + 1];
 	struct reserved_mem *rmem;
 	struct qcom_rmtfs_mem *rmtfs_mem;
 	u32 client_id;
-	u32 vmid;
-	int ret;
+	u32 num_vmids, vmid[NUM_MAX_VMIDS];
+	int ret, i;
 
 	rmem = of_reserved_mem_lookup(node);
 	if (!rmem) {
@@ -226,7 +227,18 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
 		goto put_device;
 	}
 
-	ret = of_property_read_u32(node, "qcom,vmid", &vmid);
+	num_vmids = of_property_count_u32_elems(node, "qcom,vmid");
+	if (num_vmids < 0) {
+		dev_err(&pdev->dev, "failed to count qcom,vmid elements: %d\n", ret);
+		goto remove_cdev;
+	} else if (num_vmids > NUM_MAX_VMIDS) {
+		dev_warn(&pdev->dev,
+			 "too many VMIDs (%d) specified! Only mapping first %d entries\n",
+			 num_vmids, NUM_MAX_VMIDS);
+		num_vmids = NUM_MAX_VMIDS;
+	}
+
+	ret = of_property_read_u32_array(node, "qcom,vmid", vmid, num_vmids);
 	if (ret < 0 && ret != -EINVAL) {
 		dev_err(&pdev->dev, "failed to parse qcom,vmid\n");
 		goto remove_cdev;
@@ -238,12 +250,15 @@ static int qcom_rmtfs_mem_probe(struct platform_device *pdev)
 
 		perms[0].vmid = QCOM_SCM_VMID_HLOS;
 		perms[0].perm = QCOM_SCM_PERM_RW;
-		perms[1].vmid = vmid;
-		perms[1].perm = QCOM_SCM_PERM_RW;
+
+		for (i = 0; i < num_vmids; i++) {
+			perms[i + 1].vmid = vmid[i];
+			perms[i + 1].perm = QCOM_SCM_PERM_RW;
+		}
 
 		rmtfs_mem->perms = BIT(QCOM_SCM_VMID_HLOS);
 		ret = qcom_scm_assign_mem(rmtfs_mem->addr, rmtfs_mem->size,
-					  &rmtfs_mem->perms, perms, 2);
+					  &rmtfs_mem->perms, perms, num_vmids + 1);
 		if (ret < 0) {
 			dev_err(&pdev->dev, "assign memory failed\n");
 			goto remove_cdev;
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index f8335644a01a..150b72edb879 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -55,6 +55,7 @@ enum qcom_scm_ice_cipher {
 #define QCOM_SCM_VMID_MSS_MSA    0xF
 #define QCOM_SCM_VMID_WLAN       0x18
 #define QCOM_SCM_VMID_WLAN_CE    0x19
+#define QCOM_SCM_VMID_NAV        0x2B
 #define QCOM_SCM_PERM_READ       0x4
 #define QCOM_SCM_PERM_WRITE      0x2
 #define QCOM_SCM_PERM_EXEC       0x1
-- 
2.39.0

[PATCH v3 3/3] dt-bindings: firmware: qcom: scm: Separate VMIDs from header to bindings

[Konrad Dybcio]

With changes to the rmtfs binding, secure VMIDs will become useful to
have in device trees. Separate them out and add to include/dt-bindings.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
v2 -> v3:
New patch

 include/dt-bindings/firmware/qcom/scm.h | 16 ++++++++++++++++
 include/linux/qcom_scm.h                |  7 ++-----
 2 files changed, 18 insertions(+), 5 deletions(-)
 create mode 100644 include/dt-bindings/firmware/qcom/scm.h

diff --git a/include/dt-bindings/firmware/qcom/scm.h b/include/dt-bindings/firmware/qcom/scm.h
new file mode 100644
index 000000000000..d66818cd57a8
--- /dev/null
+++ b/include/dt-bindings/firmware/qcom/scm.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2010-2015, 2018-2019 The Linux Foundation. All rights reserved.
+ * Copyright (C) 2015 Linaro Ltd.
+ */
+
+#ifndef _DT_BINDINGS_FIRMWARE_QCOM_SCM_H
+#define _DT_BINDINGS_FIRMWARE_QCOM_SCM_H
+
+#define QCOM_SCM_VMID_HLOS		0x3
+#define QCOM_SCM_VMID_MSS_MSA		0xF
+#define QCOM_SCM_VMID_WLAN		0x18
+#define QCOM_SCM_VMID_WLAN_CE		0x19
+#define QCOM_SCM_VMID_NAV		0x2B
+
+#endif
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index 150b72edb879..b0d285b08c9f 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -9,6 +9,8 @@
 #include <linux/types.h>
 #include <linux/cpumask.h>
 
+#include <dt-bindings/firmware/qcom/scm.h>
+
 #define QCOM_SCM_VERSION(major, minor)	(((major) << 16) | ((minor) & 0xFF))
 #define QCOM_SCM_CPU_PWR_DOWN_L2_ON	0x0
 #define QCOM_SCM_CPU_PWR_DOWN_L2_OFF	0x1
@@ -51,11 +53,6 @@ enum qcom_scm_ice_cipher {
 	QCOM_SCM_ICE_CIPHER_AES_256_CBC = 4,
 };
 
-#define QCOM_SCM_VMID_HLOS       0x3
-#define QCOM_SCM_VMID_MSS_MSA    0xF
-#define QCOM_SCM_VMID_WLAN       0x18
-#define QCOM_SCM_VMID_WLAN_CE    0x19
-#define QCOM_SCM_VMID_NAV        0x2B
 #define QCOM_SCM_PERM_READ       0x4
 #define QCOM_SCM_PERM_WRITE      0x2
 #define QCOM_SCM_PERM_EXEC       0x1
-- 
2.39.0

Re: [PATCH v3 1/3] dt-bindings: reserved-memory: rmtfs: Make qcom,vmid an array

[Krzysztof Kozlowski]

On 09/01/2023 10:39, Konrad Dybcio wrote:
> Some SoCs mandate that the RMTFS is also assigned to the NAV VM, while
> others really don't want that. Since it has to be conditional, turn
> qcom,vmid into an u32 array so that we can handle the NAV case, as
> well as other similar ones if they pop up in the future.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> v2 -> v3:
> Rewrite to accomodate for changes, don't pick up tags
> 
>  .../devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml   | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
> index 2998f1c8f0db..cfc2fda30eba 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
> +++ b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
> @@ -27,9 +27,9 @@ properties:
>        identifier of the client to use this region for buffers
>  
>    qcom,vmid:
> -    $ref: /schemas/types.yaml#/definitions/uint32
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>      description: >
> -      vmid of the remote processor, to set up memory protection
> +      Array of vmids of the remote processors, to set up memory protection

You need now min and maxItems.

Best regards,
Krzysztof

Re: [PATCH v3 3/3] dt-bindings: firmware: qcom: scm: Separate VMIDs from header to bindings

[Krzysztof Kozlowski]

On 09/01/2023 10:39, Konrad Dybcio wrote:
> With changes to the rmtfs binding, secure VMIDs will become useful to
> have in device trees. Separate them out and add to include/dt-bindings.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> v2 -> v3:
> New patch
> 
>  include/dt-bindings/firmware/qcom/scm.h | 16 ++++++++++++++++
>  include/linux/qcom_scm.h                |  7 ++-----
>  2 files changed, 18 insertions(+), 5 deletions(-)
>  create mode 100644 include/dt-bindings/firmware/qcom/scm.h
> 
> diff --git a/include/dt-bindings/firmware/qcom/scm.h b/include/dt-bindings/firmware/qcom/scm.h
> new file mode 100644
> index 000000000000..d66818cd57a8
> --- /dev/null
> +++ b/include/dt-bindings/firmware/qcom/scm.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */

Only Codeaurora folks contributed these numbers, thus we can relicense
it to dual-license, I believe.

The other topic is what do these numbers represent: hardware interface?
registers? offsets? firmware? IOW, why bindings is the place for them?
(usefulness for DTS is not the reason)


Best regards,
Krzysztof

Re: [PATCH v3 3/3] dt-bindings: firmware: qcom: scm: Separate VMIDs from header to bindings

[Krzysztof Kozlowski]

On 09/01/2023 10:39, Konrad Dybcio wrote:
> With changes to the rmtfs binding, secure VMIDs will become useful to
> have in device trees. Separate them out and add to include/dt-bindings.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> v2 -> v3:
> New patch
> 
>  include/dt-bindings/firmware/qcom/scm.h | 16 ++++++++++++++++

Ah, and filename matching bindings file or compatible, so "qcom,scm.yaml".

>  include/linux/qcom_scm.h                |  7 ++-----
>  2 files changed, 18 insertions(+), 5 deletions(-)
Best regards,
Krzysztof

Re: [PATCH v3 3/3] dt-bindings: firmware: qcom: scm: Separate VMIDs from header to bindings

[Konrad Dybcio]

On 9.01.2023 10:54, Krzysztof Kozlowski wrote:
> On 09/01/2023 10:39, Konrad Dybcio wrote:
>> With changes to the rmtfs binding, secure VMIDs will become useful to
>> have in device trees. Separate them out and add to include/dt-bindings.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>> v2 -> v3:
>> New patch
>>
>>  include/dt-bindings/firmware/qcom/scm.h | 16 ++++++++++++++++
>>  include/linux/qcom_scm.h                |  7 ++-----
>>  2 files changed, 18 insertions(+), 5 deletions(-)
>>  create mode 100644 include/dt-bindings/firmware/qcom/scm.h
>>
>> diff --git a/include/dt-bindings/firmware/qcom/scm.h b/include/dt-bindings/firmware/qcom/scm.h
>> new file mode 100644
>> index 000000000000..d66818cd57a8
>> --- /dev/null
>> +++ b/include/dt-bindings/firmware/qcom/scm.h
>> @@ -0,0 +1,16 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
> 
> Only Codeaurora folks contributed these numbers, thus we can relicense
> it to dual-license, I believe.
> 
> The other topic is what do these numbers represent: hardware interface?
> registers? offsets? firmware?
Arguments for a SCM call, so firmware interface.

IOW, why bindings is the place for them?
> (usefulness for DTS is not the reason)
These defines correspond to mappings in a hardcoded, irreplaceable
and un-omittable firmware which is (unless you steal engineering
samples from the factory) always shipped with these SoCs and they
help clarify some otherwise totally magic numbers.

Konrad

> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v3 3/3] dt-bindings: firmware: qcom: scm: Separate VMIDs from header to bindings

[Krzysztof Kozlowski]

On 09/01/2023 11:16, Konrad Dybcio wrote:
> 
> 
> On 9.01.2023 10:54, Krzysztof Kozlowski wrote:
>> On 09/01/2023 10:39, Konrad Dybcio wrote:
>>> With changes to the rmtfs binding, secure VMIDs will become useful to
>>> have in device trees. Separate them out and add to include/dt-bindings.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>> v2 -> v3:
>>> New patch
>>>
>>>  include/dt-bindings/firmware/qcom/scm.h | 16 ++++++++++++++++
>>>  include/linux/qcom_scm.h                |  7 ++-----
>>>  2 files changed, 18 insertions(+), 5 deletions(-)
>>>  create mode 100644 include/dt-bindings/firmware/qcom/scm.h
>>>
>>> diff --git a/include/dt-bindings/firmware/qcom/scm.h b/include/dt-bindings/firmware/qcom/scm.h
>>> new file mode 100644
>>> index 000000000000..d66818cd57a8
>>> --- /dev/null
>>> +++ b/include/dt-bindings/firmware/qcom/scm.h
>>> @@ -0,0 +1,16 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>
>> Only Codeaurora folks contributed these numbers, thus we can relicense
>> it to dual-license, I believe.
>>
>> The other topic is what do these numbers represent: hardware interface?
>> registers? offsets? firmware?
> Arguments for a SCM call, so firmware interface.
> 
> IOW, why bindings is the place for them?
>> (usefulness for DTS is not the reason)
> These defines correspond to mappings in a hardcoded, irreplaceable
> and un-omittable firmware which is (unless you steal engineering
> samples from the factory) always shipped with these SoCs and they
> help clarify some otherwise totally magic numbers.

OK, makes sense. Please mention this in commit msg to justify adding
them to bindings.

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: reserved-memory: rmtfs: Make qcom,vmid an array

[Konrad Dybcio]

On 9.01.2023 10:51, Krzysztof Kozlowski wrote:
> On 09/01/2023 10:39, Konrad Dybcio wrote:
>> Some SoCs mandate that the RMTFS is also assigned to the NAV VM, while
>> others really don't want that. Since it has to be conditional, turn
>> qcom,vmid into an u32 array so that we can handle the NAV case, as
>> well as other similar ones if they pop up in the future.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>> v2 -> v3:
>> Rewrite to accomodate for changes, don't pick up tags
>>
>>  .../devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml   | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
>> index 2998f1c8f0db..cfc2fda30eba 100644
>> --- a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
>> +++ b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
>> @@ -27,9 +27,9 @@ properties:
>>        identifier of the client to use this region for buffers
>>  
>>    qcom,vmid:
>> -    $ref: /schemas/types.yaml#/definitions/uint32
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>      description: >
>> -      vmid of the remote processor, to set up memory protection
>> +      Array of vmids of the remote processors, to set up memory protection
> 
> You need now min and maxItems.
Hm, I tested it with and without:

minItems: 1
maxItems: 2

on DTs with either one or two VMIDs defined and neither complains..

Konrad
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v3 1/3] dt-bindings: reserved-memory: rmtfs: Make qcom,vmid an array

[Krzysztof Kozlowski]

On 09/01/2023 12:41, Konrad Dybcio wrote:
> 
> 
> On 9.01.2023 10:51, Krzysztof Kozlowski wrote:
>> On 09/01/2023 10:39, Konrad Dybcio wrote:
>>> Some SoCs mandate that the RMTFS is also assigned to the NAV VM, while
>>> others really don't want that. Since it has to be conditional, turn
>>> qcom,vmid into an u32 array so that we can handle the NAV case, as
>>> well as other similar ones if they pop up in the future.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>> v2 -> v3:
>>> Rewrite to accomodate for changes, don't pick up tags
>>>
>>>  .../devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml   | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
>>> index 2998f1c8f0db..cfc2fda30eba 100644
>>> --- a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
>>> +++ b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
>>> @@ -27,9 +27,9 @@ properties:
>>>        identifier of the client to use this region for buffers
>>>  
>>>    qcom,vmid:
>>> -    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>>      description: >
>>> -      vmid of the remote processor, to set up memory protection
>>> +      Array of vmids of the remote processors, to set up memory protection
>>
>> You need now min and maxItems.
> Hm, I tested it with and without:
> 
> minItems: 1
> maxItems: 2
> 
> on DTs with either one or two VMIDs defined and neither complains..

You can also make a property like:

	qcom,vmid: true

which will accept anything but it won't be correct approach. Properties
should have some reasonable constraints. Otherwise binding is not really
specific and is describing the interface in a very relaxed way.

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: reserved-memory: rmtfs: Make qcom,vmid an array

[Marijn Suijten]

On 2023-01-09 12:41:00, Konrad Dybcio wrote:
> 
> 
> On 9.01.2023 10:51, Krzysztof Kozlowski wrote:
> > On 09/01/2023 10:39, Konrad Dybcio wrote:
> >> Some SoCs mandate that the RMTFS is also assigned to the NAV VM, while
> >> others really don't want that. Since it has to be conditional, turn
> >> qcom,vmid into an u32 array so that we can handle the NAV case, as
> >> well as other similar ones if they pop up in the future.
> >>
> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >> ---
> >> v2 -> v3:
> >> Rewrite to accomodate for changes, don't pick up tags
> >>
> >>  .../devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml   | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
> >> index 2998f1c8f0db..cfc2fda30eba 100644
> >> --- a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
> >> +++ b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.yaml
> >> @@ -27,9 +27,9 @@ properties:
> >>        identifier of the client to use this region for buffers
> >>  
> >>    qcom,vmid:
> >> -    $ref: /schemas/types.yaml#/definitions/uint32
> >> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> >>      description: >
> >> -      vmid of the remote processor, to set up memory protection
> >> +      Array of vmids of the remote processors, to set up memory protection
> > 
> > You need now min and maxItems.
> Hm, I tested it with and without:
> 
> minItems: 1
> maxItems: 2
> 
> on DTs with either one or two VMIDs defined and neither complains..

This sounds like a constraint, so it'd only fail on DTs with zero or
more than two VMIDs (when min/maxItems is present, no complaints
otherwise).

- Marijn