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.0On 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
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
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
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
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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.