[net-next PATCH v2 0/3] Grab IPA IMEM slice through DT

[net-next PATCH v2 0/3] Grab IPA IMEM slice through DT

[Konrad Dybcio]

This adds the necessary driver change to migrate over from
hardcoded-per-IPA-version-but-varying-per-implementation numbers, while
unfortunately keeping them in there for backwards compatibility.

The DT changes will be submitted in a separate series, this one is OK
to merge independently.

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
Changes in v2:
- Actually pass the retrieved data to the target function
- Re-wrap comments to match net/ style
- Mention next-next in the mail subjects
- Pick up tags
- Link to v1: https://lore.kernel.org/r/20250523-topic-ipa_imem-v1-0-b5d536291c7f@oss.qualcomm.com

---
Konrad Dybcio (3):
      dt-bindings: sram: qcom,imem: Allow modem-tables
      dt-bindings: net: qcom,ipa: Add sram property for describing IMEM slice
      net: ipa: Grab IMEM slice base/size from DTS

 Documentation/devicetree/bindings/net/qcom,ipa.yaml |  7 +++++++
 .../devicetree/bindings/sram/qcom,imem.yaml         |  3 +++
 drivers/net/ipa/ipa_data.h                          |  4 ++++
 drivers/net/ipa/ipa_mem.c                           | 21 ++++++++++++++++++++-
 4 files changed, 34 insertions(+), 1 deletion(-)
---
base-commit: 460178e842c7a1e48a06df684c66eb5fd630bcf7
change-id: 20250523-topic-ipa_imem-def66cca88e5

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

[PATCH net-next v2 1/3] dt-bindings: sram: qcom,imem: Allow modem-tables

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

The IP Accelerator hardware/firmware owns a sizeable region within the
IMEM, ominously named 'modem-tables', presumably having to do with some
internal IPA-modem specifics.

It's not actually accessed by the OS, although we have to IOMMU-map it
with the IPA device, so that presumably the firmware can act upon it.

Allow it as a subnode of IMEM.

Reviewed-by: Alex Elder <elder@riscstar.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 Documentation/devicetree/bindings/sram/qcom,imem.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
index 2711f90d9664b70fcd1e2f7e2dfd3386ed5c1952..7c882819222dc04190db357ac6f9a3a35137cc9e 100644
--- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
+++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
@@ -51,6 +51,9 @@ properties:
     $ref: /schemas/power/reset/syscon-reboot-mode.yaml#
 
 patternProperties:
+  "^modem-tables@[0-9a-f]+$":
+    description: Region reserved for the IP Accelerator
+
   "^pil-reloc@[0-9a-f]+$":
     $ref: /schemas/remoteproc/qcom,pil-info.yaml#
     description: Peripheral image loader relocation region

-- 
2.49.0

[PATCH net-next v2 2/3] dt-bindings: net: qcom,ipa: Add sram property for describing IMEM slice

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

The IPA driver currently grabs a slice of IMEM through hardcoded
addresses. Not only is that ugly and against the principles of DT,
but it also creates a situation where two distinct platforms
implementing the same version of IPA would need to be hardcoded
together and matched at runtime.

Instead, do the sane thing and accept a handle to said region directly.

Don't make it required on purpose, as a) it's not there on ancient
implementations (currently unsupported) and we're not yet done with
filling the data across al DTs.

Reviewed-by: Alex Elder <elder@riscstar.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 Documentation/devicetree/bindings/net/qcom,ipa.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/qcom,ipa.yaml b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
index b4a79912d4739bec33933cdd7bb5e720eb41c814..1109f4d170af7178b998c6b7d415cc60de1c58c5 100644
--- a/Documentation/devicetree/bindings/net/qcom,ipa.yaml
+++ b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
@@ -166,6 +166,13 @@ properties:
       initializing IPA hardware.  Optional, and only used when
       Trust Zone performs early initialization.
 
+  sram:
+    maxItems: 1
+    description:
+      A reference to an additional region residing in IMEM (special
+      on-chip SRAM), which is accessed by the IPA firmware and needs
+      to be IOMMU-mapped from the OS.
+
 required:
   - compatible
   - iommus

-- 
2.49.0

[PATCH net-next v2 3/3] net: ipa: Grab IMEM slice base/size from DTS

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

This is a detail that differ per chip, and not per IPA version (and
there are cases of the same IPA versions being implemented across very
very very different SoCs).

This region isn't actually used by the driver, but we most definitely
want to iommu-map it, so that IPA can poke at the data within.

Reviewed-by: Alex Elder <elder@riscstar.com>
Acked-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 drivers/net/ipa/ipa_data.h |  4 ++++
 drivers/net/ipa/ipa_mem.c  | 21 ++++++++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ipa/ipa_data.h b/drivers/net/ipa/ipa_data.h
index 2fd03f0799b207833f9f2b421ce043534720d718..5fe164981083674a08ba0b69e18140bbcb46053d 100644
--- a/drivers/net/ipa/ipa_data.h
+++ b/drivers/net/ipa/ipa_data.h
@@ -185,8 +185,12 @@ struct ipa_resource_data {
 struct ipa_mem_data {
 	u32 local_count;
 	const struct ipa_mem *local;
+
+	/* DEPRECATED (now passed via DT) fallback data,
+	 * varies per chip and not per IPA version */
 	u32 imem_addr;
 	u32 imem_size;
+
 	u32 smem_size;
 };
 
diff --git a/drivers/net/ipa/ipa_mem.c b/drivers/net/ipa/ipa_mem.c
index 835a3c9c1fd47167da3396424a1653ebcae81d40..583aea6257096b73aa60ff6cada1f0be478846a4 100644
--- a/drivers/net/ipa/ipa_mem.c
+++ b/drivers/net/ipa/ipa_mem.c
@@ -7,6 +7,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
 #include <linux/iommu.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
 
@@ -617,7 +618,9 @@ static void ipa_smem_exit(struct ipa *ipa)
 int ipa_mem_init(struct ipa *ipa, struct platform_device *pdev,
 		 const struct ipa_mem_data *mem_data)
 {
+	struct device_node *ipa_slice_np;
 	struct device *dev = &pdev->dev;
+	u32 imem_base, imem_size;
 	struct resource *res;
 	int ret;
 
@@ -656,7 +659,23 @@ int ipa_mem_init(struct ipa *ipa, struct platform_device *pdev,
 	ipa->mem_addr = res->start;
 	ipa->mem_size = resource_size(res);
 
-	ret = ipa_imem_init(ipa, mem_data->imem_addr, mem_data->imem_size);
+	ipa_slice_np = of_parse_phandle(dev->of_node, "sram", 0);
+	if (ipa_slice_np) {
+		ret = of_address_to_resource(ipa_slice_np, 0, res);
+		of_node_put(ipa_slice_np);
+		if (ret)
+			return ret;
+
+		imem_base = res->start;
+		imem_size = resource_size(res);
+	} else {
+		/* Backwards compatibility for DTs lacking
+		 * an explicit reference */
+		imem_base = mem_data->imem_addr;
+		imem_size = mem_data->imem_size;
+	}
+
+	ret = ipa_imem_init(ipa, imem_base, imem_size);
 	if (ret)
 		goto err_unmap;
 

-- 
2.49.0

Re: [PATCH net-next v2 1/3] dt-bindings: sram: qcom,imem: Allow modem-tables

[Krzysztof Kozlowski]

On 27/05/2025 13:26, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> The IP Accelerator hardware/firmware owns a sizeable region within the
> IMEM, ominously named 'modem-tables', presumably having to do with some
> internal IPA-modem specifics.
> 
> It's not actually accessed by the OS, although we have to IOMMU-map it
> with the IPA device, so that presumably the firmware can act upon it.
> 
> Allow it as a subnode of IMEM.
> 
> Reviewed-by: Alex Elder <elder@riscstar.com>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
>  Documentation/devicetree/bindings/sram/qcom,imem.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> index 2711f90d9664b70fcd1e2f7e2dfd3386ed5c1952..7c882819222dc04190db357ac6f9a3a35137cc9e 100644
> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> @@ -51,6 +51,9 @@ properties:
>      $ref: /schemas/power/reset/syscon-reboot-mode.yaml#
>  
>  patternProperties:
> +  "^modem-tables@[0-9a-f]+$":
> +    description: Region reserved for the IP Accelerator

Missing additionalProperties: false, which would point you that this is
incomplete (or useless because empty).

Best regards,
Krzysztof

Re: [PATCH net-next v2 2/3] dt-bindings: net: qcom,ipa: Add sram property for describing IMEM slice

[Krzysztof Kozlowski]

On 27/05/2025 13:26, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> The IPA driver currently grabs a slice of IMEM through hardcoded
> addresses. Not only is that ugly and against the principles of DT,
> but it also creates a situation where two distinct platforms
> implementing the same version of IPA would need to be hardcoded
> together and matched at runtime.
> 
> Instead, do the sane thing and accept a handle to said region directly.
> 
> Don't make it required on purpose, as a) it's not there on ancient
> implementations (currently unsupported) and we're not yet done with
> filling the data across al DTs.
> 
> Reviewed-by: Alex Elder <elder@riscstar.com>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH net-next v2 1/3] dt-bindings: sram: qcom,imem: Allow modem-tables

[Konrad Dybcio]

On 5/27/25 1:35 PM, Krzysztof Kozlowski wrote:
> On 27/05/2025 13:26, Konrad Dybcio wrote:
>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>
>> The IP Accelerator hardware/firmware owns a sizeable region within the
>> IMEM, ominously named 'modem-tables', presumably having to do with some
>> internal IPA-modem specifics.
>>
>> It's not actually accessed by the OS, although we have to IOMMU-map it
>> with the IPA device, so that presumably the firmware can act upon it.
>>
>> Allow it as a subnode of IMEM.
>>
>> Reviewed-by: Alex Elder <elder@riscstar.com>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>> ---
>>  Documentation/devicetree/bindings/sram/qcom,imem.yaml | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> index 2711f90d9664b70fcd1e2f7e2dfd3386ed5c1952..7c882819222dc04190db357ac6f9a3a35137cc9e 100644
>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>> @@ -51,6 +51,9 @@ properties:
>>      $ref: /schemas/power/reset/syscon-reboot-mode.yaml#
>>  
>>  patternProperties:
>> +  "^modem-tables@[0-9a-f]+$":
>> +    description: Region reserved for the IP Accelerator
> 
> Missing additionalProperties: false, which would point you that this is
> incomplete (or useless because empty).

How do I describe a 'stupid' node that is just a reg?

Konrad

Re: [PATCH net-next v2 1/3] dt-bindings: sram: qcom,imem: Allow modem-tables

[Krzysztof Kozlowski]

On 27/05/2025 13:36, Konrad Dybcio wrote:
>>> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> index 2711f90d9664b70fcd1e2f7e2dfd3386ed5c1952..7c882819222dc04190db357ac6f9a3a35137cc9e 100644
>>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>> @@ -51,6 +51,9 @@ properties:
>>>      $ref: /schemas/power/reset/syscon-reboot-mode.yaml#
>>>  
>>>  patternProperties:
>>> +  "^modem-tables@[0-9a-f]+$":
>>> +    description: Region reserved for the IP Accelerator
>>
>> Missing additionalProperties: false, which would point you that this is
>> incomplete (or useless because empty).
> 
> How do I describe a 'stupid' node that is just a reg?
With "reg" - similarly to many syscon bindings.

Best regards,
Krzysztof

Re: [PATCH net-next v2 3/3] net: ipa: Grab IMEM slice base/size from DTS

[Simon Horman]

On Tue, May 27, 2025 at 01:26:43PM +0200, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> This is a detail that differ per chip, and not per IPA version (and
> there are cases of the same IPA versions being implemented across very
> very very different SoCs).
> 
> This region isn't actually used by the driver, but we most definitely
> want to iommu-map it, so that IPA can poke at the data within.
> 
> Reviewed-by: Alex Elder <elder@riscstar.com>
> Acked-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

...

> diff --git a/drivers/net/ipa/ipa_mem.c b/drivers/net/ipa/ipa_mem.c

...

> @@ -656,7 +659,23 @@ int ipa_mem_init(struct ipa *ipa, struct platform_device *pdev,
>  	ipa->mem_addr = res->start;
>  	ipa->mem_size = resource_size(res);
>  
> -	ret = ipa_imem_init(ipa, mem_data->imem_addr, mem_data->imem_size);
> +	ipa_slice_np = of_parse_phandle(dev->of_node, "sram", 0);
> +	if (ipa_slice_np) {
> +		ret = of_address_to_resource(ipa_slice_np, 0, res);
> +		of_node_put(ipa_slice_np);
> +		if (ret)
> +			return ret;
> +
> +		imem_base = res->start;
> +		imem_size = resource_size(res);
> +	} else {
> +		/* Backwards compatibility for DTs lacking
> +		 * an explicit reference */
> +		imem_base = mem_data->imem_addr;
> +		imem_size = mem_data->imem_size;
> +	}
> +
> +	ret = ipa_imem_init(ipa, imem_base, imem_size);

Thanks for the update to use imem_base and imem_size on the line above.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 1/3] dt-bindings: sram: qcom,imem: Allow modem-tables

[Konrad Dybcio]

On 5/27/25 1:42 PM, Krzysztof Kozlowski wrote:
> On 27/05/2025 13:36, Konrad Dybcio wrote:
>>>> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>>> index 2711f90d9664b70fcd1e2f7e2dfd3386ed5c1952..7c882819222dc04190db357ac6f9a3a35137cc9e 100644
>>>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>>> @@ -51,6 +51,9 @@ properties:
>>>>      $ref: /schemas/power/reset/syscon-reboot-mode.yaml#
>>>>  
>>>>  patternProperties:
>>>> +  "^modem-tables@[0-9a-f]+$":
>>>> +    description: Region reserved for the IP Accelerator
>>>
>>> Missing additionalProperties: false, which would point you that this is
>>> incomplete (or useless because empty).
>>
>> How do I describe a 'stupid' node that is just a reg?
> With "reg" - similarly to many syscon bindings.

Is this sort of inline style acceptable, or should I introduce
a separate file?

diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
index 7555947d7001..95fbb4ac9daa 100644
--- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
+++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
@@ -67,7 +67,13 @@ properties:
 
 patternProperties:
   "^modem-tables@[0-9a-f]+$":
+    type: object
+    properties:
+      reg:
+        maxItems: 1
+
     description: Region reserved for the IP Accelerator
+    additionalProperties: false
 
   "^pil-reloc@[0-9a-f]+$":
     $ref: /schemas/remoteproc/qcom,pil-info.yaml#

(fwiw checks are happy with the above)

Konrad

Re: [PATCH net-next v2 1/3] dt-bindings: sram: qcom,imem: Allow modem-tables

[Krzysztof Kozlowski]

On 14/07/2025 19:53, Konrad Dybcio wrote:
> On 5/27/25 1:42 PM, Krzysztof Kozlowski wrote:
>> On 27/05/2025 13:36, Konrad Dybcio wrote:
>>>>> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>>>> index 2711f90d9664b70fcd1e2f7e2dfd3386ed5c1952..7c882819222dc04190db357ac6f9a3a35137cc9e 100644
>>>>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>>>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>>>> @@ -51,6 +51,9 @@ properties:
>>>>>      $ref: /schemas/power/reset/syscon-reboot-mode.yaml#
>>>>>  
>>>>>  patternProperties:
>>>>> +  "^modem-tables@[0-9a-f]+$":
>>>>> +    description: Region reserved for the IP Accelerator
>>>>
>>>> Missing additionalProperties: false, which would point you that this is
>>>> incomplete (or useless because empty).
>>>
>>> How do I describe a 'stupid' node that is just a reg?
>> With "reg" - similarly to many syscon bindings.
> 
> Is this sort of inline style acceptable, or should I introduce
> a separate file?

It's fine, assuming that it is desired in general. We do not describe
individual memory regions of syscon nodes and this is a syscon.

If this is NVMEM (which it looks like), then could use NVMEM bindings to
describe its cells - individual regions. But otherwise we just don't.

There are many exceptions in other platforms, mostly old or even
unreviewed by DT maintainers, so they are not a recommended example.

This would need serious justification WHY you need to describe the
child. Why phandle to the main node is not enough for consumers.

If the reason is - to instantiate child device driver - then as well no.
This has been NAKed on the lists many times - you need resources if the
child should be a separate node. Address space is one resource but not
enough, because it can easily be obtained from the parent/main node.



Best regards,
Krzysztof

Re: [PATCH net-next v2 1/3] dt-bindings: sram: qcom,imem: Allow modem-tables

[Konrad Dybcio]

On 7/15/25 8:37 AM, Krzysztof Kozlowski wrote:
> On 14/07/2025 19:53, Konrad Dybcio wrote:
>> On 5/27/25 1:42 PM, Krzysztof Kozlowski wrote:
>>> On 27/05/2025 13:36, Konrad Dybcio wrote:
>>>>>> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>>>>> index 2711f90d9664b70fcd1e2f7e2dfd3386ed5c1952..7c882819222dc04190db357ac6f9a3a35137cc9e 100644
>>>>>> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
>>>>>> @@ -51,6 +51,9 @@ properties:
>>>>>>      $ref: /schemas/power/reset/syscon-reboot-mode.yaml#
>>>>>>  
>>>>>>  patternProperties:
>>>>>> +  "^modem-tables@[0-9a-f]+$":
>>>>>> +    description: Region reserved for the IP Accelerator
>>>>>
>>>>> Missing additionalProperties: false, which would point you that this is
>>>>> incomplete (or useless because empty).
>>>>
>>>> How do I describe a 'stupid' node that is just a reg?
>>> With "reg" - similarly to many syscon bindings.
>>
>> Is this sort of inline style acceptable, or should I introduce
>> a separate file?
> 
> It's fine, assuming that it is desired in general. We do not describe
> individual memory regions of syscon nodes and this is a syscon.
> 
> If this is NVMEM (which it looks like), then could use NVMEM bindings to
> describe its cells - individual regions. But otherwise we just don't.

It's volatile on-chip memory

> There are many exceptions in other platforms, mostly old or even
> unreviewed by DT maintainers, so they are not a recommended example.
> 
> This would need serious justification WHY you need to describe the
> child. Why phandle to the main node is not enough for consumers.

It's simply a region of the SRAM, which needs to be IOMMU-mapped in a
specific manner (should IMEM move away from syscon+simple-mfd to
mmio-sram?). Describing slices is the DT way to pass them (like under
NVMEM providers).

> 
> If the reason is - to instantiate child device driver - then as well no.
> This has been NAKed on the lists many times - you need resources if the
> child should be a separate node. Address space is one resource but not
> enough, because it can easily be obtained from the parent/main node.

There is no additional driver for this

Konrad

Re: [PATCH net-next v2 1/3] dt-bindings: sram: qcom,imem: Allow modem-tables

[Krzysztof Kozlowski]

On 30/07/2025 14:07, Konrad Dybcio wrote:
>>>>>>
>>>>>> Missing additionalProperties: false, which would point you that this is
>>>>>> incomplete (or useless because empty).
>>>>>
>>>>> How do I describe a 'stupid' node that is just a reg?
>>>> With "reg" - similarly to many syscon bindings.
>>>
>>> Is this sort of inline style acceptable, or should I introduce
>>> a separate file?
>>
>> It's fine, assuming that it is desired in general. We do not describe
>> individual memory regions of syscon nodes and this is a syscon.
>>
>> If this is NVMEM (which it looks like), then could use NVMEM bindings to
>> describe its cells - individual regions. But otherwise we just don't.
> 
> It's volatile on-chip memory
> 
>> There are many exceptions in other platforms, mostly old or even
>> unreviewed by DT maintainers, so they are not a recommended example.
>>
>> This would need serious justification WHY you need to describe the
>> child. Why phandle to the main node is not enough for consumers.
> 
> It's simply a region of the SRAM, which needs to be IOMMU-mapped in a
> specific manner (should IMEM move away from syscon+simple-mfd to
> mmio-sram?). Describing slices is the DT way to pass them (like under
> NVMEM providers).


Then this might be not a syscon, IMO. I don't think mixing syscon and
SRAM is appropriate, even though Linux could treat it very similar.

syscon is for registers. mmio-sram is for SRAM or other parts of
non-volatile RAM.

Indeed you might need to move towards mmio-sram.

> 
>>
>> If the reason is - to instantiate child device driver - then as well no.
>> This has been NAKed on the lists many times - you need resources if the
>> child should be a separate node. Address space is one resource but not
>> enough, because it can easily be obtained from the parent/main node.
> 
> There is no additional driver for this

Then it is not a simple-mfd...

Best regards,
Krzysztof

Re: [PATCH net-next v2 1/3] dt-bindings: sram: qcom,imem: Allow modem-tables

[Konrad Dybcio]

On 7/30/25 3:14 PM, Krzysztof Kozlowski wrote:
> On 30/07/2025 14:07, Konrad Dybcio wrote:
>>>>>>>
>>>>>>> Missing additionalProperties: false, which would point you that this is
>>>>>>> incomplete (or useless because empty).
>>>>>>
>>>>>> How do I describe a 'stupid' node that is just a reg?
>>>>> With "reg" - similarly to many syscon bindings.
>>>>
>>>> Is this sort of inline style acceptable, or should I introduce
>>>> a separate file?
>>>
>>> It's fine, assuming that it is desired in general. We do not describe
>>> individual memory regions of syscon nodes and this is a syscon.
>>>
>>> If this is NVMEM (which it looks like), then could use NVMEM bindings to
>>> describe its cells - individual regions. But otherwise we just don't.
>>
>> It's volatile on-chip memory
>>
>>> There are many exceptions in other platforms, mostly old or even
>>> unreviewed by DT maintainers, so they are not a recommended example.
>>>
>>> This would need serious justification WHY you need to describe the
>>> child. Why phandle to the main node is not enough for consumers.
>>
>> It's simply a region of the SRAM, which needs to be IOMMU-mapped in a
>> specific manner (should IMEM move away from syscon+simple-mfd to
>> mmio-sram?). Describing slices is the DT way to pass them (like under
>> NVMEM providers).
> 
> 
> Then this might be not a syscon, IMO. I don't think mixing syscon and
> SRAM is appropriate, even though Linux could treat it very similar.
> 
> syscon is for registers. mmio-sram is for SRAM or other parts of
> non-volatile RAM.
> 
> Indeed you might need to move towards mmio-sram.
> 
>>
>>>
>>> If the reason is - to instantiate child device driver - then as well no.
>>> This has been NAKed on the lists many times - you need resources if the
>>> child should be a separate node. Address space is one resource but not
>>> enough, because it can easily be obtained from the parent/main node.
>>
>> There is no additional driver for this
> 
> Then it is not a simple-mfd...

Indeed it's really not

I found out however that the computer history museum (i.e.
qcom-apq8064-asus-nexus7-flo.dts and qcom-msm8974.dtsi) seems to
have used simple-mfd, so that the subnode (syscon-reboot-mode) is
matched against a driver

The same can be achieved if we stick an of_platform_populate() at
the end of mmio-sram probe - thoughts?

Konrad

Re: [PATCH net-next v2 1/3] dt-bindings: sram: qcom,imem: Allow modem-tables

[Krzysztof Kozlowski]

On 31/07/2025 11:47, Konrad Dybcio wrote:
> On 7/30/25 3:14 PM, Krzysztof Kozlowski wrote:
>> On 30/07/2025 14:07, Konrad Dybcio wrote:
>>>>>>>>
>>>>>>>> Missing additionalProperties: false, which would point you that this is
>>>>>>>> incomplete (or useless because empty).
>>>>>>>
>>>>>>> How do I describe a 'stupid' node that is just a reg?
>>>>>> With "reg" - similarly to many syscon bindings.
>>>>>
>>>>> Is this sort of inline style acceptable, or should I introduce
>>>>> a separate file?
>>>>
>>>> It's fine, assuming that it is desired in general. We do not describe
>>>> individual memory regions of syscon nodes and this is a syscon.
>>>>
>>>> If this is NVMEM (which it looks like), then could use NVMEM bindings to
>>>> describe its cells - individual regions. But otherwise we just don't.
>>>
>>> It's volatile on-chip memory
>>>
>>>> There are many exceptions in other platforms, mostly old or even
>>>> unreviewed by DT maintainers, so they are not a recommended example.
>>>>
>>>> This would need serious justification WHY you need to describe the
>>>> child. Why phandle to the main node is not enough for consumers.
>>>
>>> It's simply a region of the SRAM, which needs to be IOMMU-mapped in a
>>> specific manner (should IMEM move away from syscon+simple-mfd to
>>> mmio-sram?). Describing slices is the DT way to pass them (like under
>>> NVMEM providers).
>>
>>
>> Then this might be not a syscon, IMO. I don't think mixing syscon and
>> SRAM is appropriate, even though Linux could treat it very similar.
>>
>> syscon is for registers. mmio-sram is for SRAM or other parts of
>> non-volatile RAM.
>>
>> Indeed you might need to move towards mmio-sram.
>>
>>>
>>>>
>>>> If the reason is - to instantiate child device driver - then as well no.
>>>> This has been NAKed on the lists many times - you need resources if the
>>>> child should be a separate node. Address space is one resource but not
>>>> enough, because it can easily be obtained from the parent/main node.
>>>
>>> There is no additional driver for this
>>
>> Then it is not a simple-mfd...
> 
> Indeed it's really not
> 
> I found out however that the computer history museum (i.e.
> qcom-apq8064-asus-nexus7-flo.dts and qcom-msm8974.dtsi) seems to
> have used simple-mfd, so that the subnode (syscon-reboot-mode) is
> matched against a driver
> 
> The same can be achieved if we stick an of_platform_populate() at
> the end of mmio-sram probe - thoughts?
You cannot (or should not) remove simple-mfd from existing binding. But
the point is that the list should not grow.

Maybe the binding should receive a comment next to compatible:
" # Do not grow this, if you add here new compatible, you agree to buy
round of drinks on next LPC to all upstream maintainers" ?

Best regards,
Krzysztof