diff mbox series

[3/5] dt-bindings: remoteproc: Add the documentation for Meson AO ARC rproc

Message ID 20201230012724.1326156-4-martin.blumenstingl@googlemail.com (mailing list archive)
State New, archived
Headers show
Series Amlogic Meson Always-On ARC remote-processor support | expand

Commit Message

Martin Blumenstingl Dec. 30, 2020, 1:27 a.m. UTC 
Amlogic Meson6, Meson8, Meson8b and Meson8m2 SoCs embed an ARC EM4
controller for always-on operations, typically used for managing system
suspend.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../remoteproc/amlogic,meson-mx-ao-arc.yaml   | 87 +++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/amlogic,meson-mx-ao-arc.yaml

Comments

Rob Herring Dec. 31, 2020, 3:34 p.m. UTC | #1 
On Wed, 30 Dec 2020 02:27:22 +0100, Martin Blumenstingl wrote:
> Amlogic Meson6, Meson8, Meson8b and Meson8m2 SoCs embed an ARC EM4
> controller for always-on operations, typically used for managing system
> suspend.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  .../remoteproc/amlogic,meson-mx-ao-arc.yaml   | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/amlogic,meson-mx-ao-arc.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/remoteproc/amlogic,meson-mx-ao-arc.yaml:23:9: [warning] wrong indentation: expected 10 but found 8 (indentation)
./Documentation/devicetree/bindings/remoteproc/amlogic,meson-mx-ao-arc.yaml:45:6: [warning] wrong indentation: expected 4 but found 5 (indentation)

dtschema/dtc warnings/errors:

See https://patchwork.ozlabs.org/patch/1421301

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
Bjorn Andersson March 18, 2021, 2:55 a.m. UTC | #2 
On Tue 29 Dec 19:27 CST 2020, Martin Blumenstingl wrote:

> Amlogic Meson6, Meson8, Meson8b and Meson8m2 SoCs embed an ARC EM4
> controller for always-on operations, typically used for managing system
> suspend.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  .../remoteproc/amlogic,meson-mx-ao-arc.yaml   | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/amlogic,meson-mx-ao-arc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/amlogic,meson-mx-ao-arc.yaml b/Documentation/devicetree/bindings/remoteproc/amlogic,meson-mx-ao-arc.yaml
> new file mode 100644
> index 000000000000..ba5deebaf7dc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/amlogic,meson-mx-ao-arc.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/remoteproc/amlogic,meson-mx-ao-arc.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Amlogic Meson AO ARC Remote Processor bindings
> +
> +description:
> +  Amlogic Meson6, Meson8, Meson8b and Meson8m2 SoCs embed an ARC core
> +  controller for always-on operations, typically used for managing
> +  system suspend. Meson6 and older use a ARC core based on the ARCv1
> +  ISA, while Meson8, Meson8b and Meson8m2 use an ARC EM4 (ARCv2 ISA)
> +  core.
> +
> +maintainers:
> +  - Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +        - amlogic,meson8-ao-arc
> +        - amlogic,meson8b-ao-arc
> +      - const: amlogic,meson-mx-ao-arc
> +
> +  firmware-name:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description:
> +      The name of the firmware which should be loaded for this remote
> +      processor.
> +
> +  reg:
> +    description:
> +      Address ranges of the remap and CPU control addresses for the
> +      remote processor.
> +    minItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: remap
> +      - const: cpu
> +
> +  resets:
> +     minItems: 1
> +
> +  clocks:
> +    minItems: 1
> +
> +  sram:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandles to a reserved SRAM region which is used as the memory of
> +      the ARC core. The region should be defined as child nodes of the
> +      AHB SRAM node as per the generic bindings in
> +      Documentation/devicetree/bindings/sram/sram.yaml
> +
> +  amlogic,secbus2:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      A phandle to the SECBUS2 region which contains some configuration
> +      bits of this remote processor
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - resets
> +  - clocks
> +  - sram
> +  - amlogic,secbus2
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    remoteproc@1c {
> +      compatible= "amlogic,meson8-ao-arc", "amlogic,meson-mx-ao-arc";
> +      reg = <0x1c 0x8>, <0x38 0x8>;

I'm generally not in favor of mapping "individual" registers, do you
know what hardware block this is part of? Can you express the whole
block as an single entity in your DT?

Regards,
Bjorn

> +      reg-names = "remap", "cpu";
> +      resets = <&media_cpu_reset>;
> +      clocks = <&media_cpu_clock>;
> +      sram = <&ahb_sram_ao_arc>;
> +      amlogic,secbus2 = <&secbus2>;
> +    };
> +
> +...
> -- 
> 2.30.0
>
Martin Blumenstingl March 23, 2021, 10:02 p.m. UTC | #3 
Hi Bjorn,

On Thu, Mar 18, 2021 at 3:55 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
[...]
> > +examples:
> > +  - |
> > +    remoteproc@1c {
> > +      compatible= "amlogic,meson8-ao-arc", "amlogic,meson-mx-ao-arc";
> > +      reg = <0x1c 0x8>, <0x38 0x8>;
>
> I'm generally not in favor of mapping "individual" registers, do you
> know what hardware block this is part of? Can you express the whole
> block as an single entity in your DT?
the answer is unfortunately not easy :-)

some background information:
Amlogic SoCs have two power domains:
- AO (Always-On)
- EE (Everything-Else)

AO includes (at least) one ARC core for which this remoteproc dt-binding is.
EE includes ARM Cortex-A7/15/... cores

The AO registers can be accessed from the EE power-domain and vice versa

Following is an extract (with comments added by me) for the AO
registers (taken from the GPL vendor kernel):
#define AO_RTI_STATUS_REG0 ((0x00 << 10) | (0x00 << 2))
#define AO_RTI_STATUS_REG1 ((0x00 << 10) | (0x01 << 2))
#define AO_RTI_STATUS_REG2 ((0x00 << 10) | (0x02 << 2))
these three are used for communication with the firmware on the AO ARC core
I am not sure into which Linux subsystem these would fit into best

#define AO_RTI_PWR_CNTL_REG1 ((0x00 << 10) | (0x03 << 2))
#define AO_RTI_PWR_CNTL_REG0 ((0x00 << 10) | (0x04 << 2))
this includes various power-domains for the following functionality
(and probably more):
- DDR PHY I/O
- AHB SRAM
- video encoder/decoders
- EE domain isolation

#define AO_RTI_PIN_MUX_REG ((0x00 << 10) | (0x05 << 2))
first part of the pin controller registers for the "AO" bank pads
this includes various GPIOs, UART, I2C for communication with a PMIC,
infrared remote decoder, two PWMs, etc.
all (known) functionality can be used by Linux as well.
especially the UART, I2C, IR decoder and GPIOs are functionality that
we use with Linux today - without involving the AO ARC
remote-processor.

#define AO_WD_GPIO_REG ((0x00 << 10) | (0x06 << 2))
(I think this is related to the watchdog being able to trigger the
SoC's reset line, but there's no documentation on this register)

#define AO_REMAP_REG0 ((0x00 << 10) | (0x07 << 2))
#define AO_REMAP_REG1 ((0x00 << 10) | (0x08 << 2))
remap registers for the AO ARC remote-processor as used in this binding

#define AO_GPIO_O_EN_N ((0x00 << 10) | (0x09 << 2))
#define AO_GPIO_I ((0x00 << 10) | (0x0A << 2))
GPIO controller registers for the "AO" bank pads

#define AO_RTI_PULL_UP_REG ((0x00 << 10) | (0x0B << 2))
second part of the pin controller registers for the "AO" bank pads

#define AO_RTI_WD_MARK ((0x00 << 10) | (0x0D << 2))
again, I think this is somehow related to the watchdog but there's no
documentation on this

#define AO_CPU_CNTL ((0x00 << 10) | (0x0E << 2))
#define AO_CPU_STAT ((0x00 << 10) | (0x0F << 2))
used for booting the AO ARC remote-processor

#define AO_RTI_GEN_CNTL_REG0 ((0x00 << 10) | (0x10 << 2))
seems to be a multi purpose register as it (seems to) contains some
reset bits (for the AO UART and RTC) - not documented

(more registers are following)

to summarize this: I think there's indeed three different sets of registers
having one big device-tree node spanning all of these registers seems
incorrect to me as the other IPs are independent of the AO ARC
remote-processor.
so the way I have done it in the original patch is the best I could
come up with.

Please let me know what you think!


Best regards,
Martin
Bjorn Andersson April 13, 2021, 8:59 p.m. UTC | #4 
On Tue 23 Mar 17:02 CDT 2021, Martin Blumenstingl wrote:

> Hi Bjorn,
> 
> On Thu, Mar 18, 2021 at 3:55 AM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> [...]
> > > +examples:
> > > +  - |
> > > +    remoteproc@1c {
> > > +      compatible= "amlogic,meson8-ao-arc", "amlogic,meson-mx-ao-arc";
> > > +      reg = <0x1c 0x8>, <0x38 0x8>;
> >
> > I'm generally not in favor of mapping "individual" registers, do you
> > know what hardware block this is part of? Can you express the whole
> > block as an single entity in your DT?
> the answer is unfortunately not easy :-)
> 
> some background information:
> Amlogic SoCs have two power domains:
> - AO (Always-On)
> - EE (Everything-Else)
> 
> AO includes (at least) one ARC core for which this remoteproc dt-binding is.
> EE includes ARM Cortex-A7/15/... cores
> 
> The AO registers can be accessed from the EE power-domain and vice versa
> 
> Following is an extract (with comments added by me) for the AO
> registers (taken from the GPL vendor kernel):
> #define AO_RTI_STATUS_REG0 ((0x00 << 10) | (0x00 << 2))
> #define AO_RTI_STATUS_REG1 ((0x00 << 10) | (0x01 << 2))
> #define AO_RTI_STATUS_REG2 ((0x00 << 10) | (0x02 << 2))
> these three are used for communication with the firmware on the AO ARC core
> I am not sure into which Linux subsystem these would fit into best
> 
> #define AO_RTI_PWR_CNTL_REG1 ((0x00 << 10) | (0x03 << 2))
> #define AO_RTI_PWR_CNTL_REG0 ((0x00 << 10) | (0x04 << 2))
> this includes various power-domains for the following functionality
> (and probably more):
> - DDR PHY I/O
> - AHB SRAM
> - video encoder/decoders
> - EE domain isolation
> 
> #define AO_RTI_PIN_MUX_REG ((0x00 << 10) | (0x05 << 2))
> first part of the pin controller registers for the "AO" bank pads
> this includes various GPIOs, UART, I2C for communication with a PMIC,
> infrared remote decoder, two PWMs, etc.
> all (known) functionality can be used by Linux as well.
> especially the UART, I2C, IR decoder and GPIOs are functionality that
> we use with Linux today - without involving the AO ARC
> remote-processor.
> 
> #define AO_WD_GPIO_REG ((0x00 << 10) | (0x06 << 2))
> (I think this is related to the watchdog being able to trigger the
> SoC's reset line, but there's no documentation on this register)
> 
> #define AO_REMAP_REG0 ((0x00 << 10) | (0x07 << 2))
> #define AO_REMAP_REG1 ((0x00 << 10) | (0x08 << 2))
> remap registers for the AO ARC remote-processor as used in this binding
> 
> #define AO_GPIO_O_EN_N ((0x00 << 10) | (0x09 << 2))
> #define AO_GPIO_I ((0x00 << 10) | (0x0A << 2))
> GPIO controller registers for the "AO" bank pads
> 
> #define AO_RTI_PULL_UP_REG ((0x00 << 10) | (0x0B << 2))
> second part of the pin controller registers for the "AO" bank pads
> 
> #define AO_RTI_WD_MARK ((0x00 << 10) | (0x0D << 2))
> again, I think this is somehow related to the watchdog but there's no
> documentation on this
> 
> #define AO_CPU_CNTL ((0x00 << 10) | (0x0E << 2))
> #define AO_CPU_STAT ((0x00 << 10) | (0x0F << 2))
> used for booting the AO ARC remote-processor
> 
> #define AO_RTI_GEN_CNTL_REG0 ((0x00 << 10) | (0x10 << 2))
> seems to be a multi purpose register as it (seems to) contains some
> reset bits (for the AO UART and RTC) - not documented
> 
> (more registers are following)
> 
> to summarize this: I think there's indeed three different sets of registers
> having one big device-tree node spanning all of these registers seems
> incorrect to me as the other IPs are independent of the AO ARC
> remote-processor.
> so the way I have done it in the original patch is the best I could
> come up with.
> 
> Please let me know what you think!
> 

I see.

Describing these kinds blocks in DT is indeed tricky, I've had
both cases where a block maps to multiple "functions" or where they
contain misc registers to be used in relation to some other block.

The prior typically lends itself to be modelled as a "simple-mfd" and
the latter as a "syscon".

So perhaps you could do a simple-mfd that spans the entire block and
then describe the remoteproc, watchdog?, pinctrl pieces as children
under that?

Regards,
Bjorn
Martin Blumenstingl April 27, 2021, 7:03 p.m. UTC | #5 
Hi Bjorn,

On Tue, Apr 13, 2021 at 10:59 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
[...]
> Describing these kinds blocks in DT is indeed tricky, I've had
> both cases where a block maps to multiple "functions" or where they
> contain misc registers to be used in relation to some other block.
>
> The prior typically lends itself to be modelled as a "simple-mfd" and
> the latter as a "syscon".
I think here the former description matches better
each set of registers has one very specific purpose (pinctrl, GPIO,
I2C, RTC, IR receiver, ...). there's only one exception inside the
whole AO region called "PMU" (which mostly contains power management
registers and a few clock control bits)

> So perhaps you could do a simple-mfd that spans the entire block and
> then describe the remoteproc, watchdog?, pinctrl pieces as children
> under that?
I can send patches for the simple-mfd conversion (syscon won't be
involved) so Rob can also give his feedback.

in my opinion this would not change the dt-bindings for the AO ARC
remote-processor.
Please let me know if there would be any dt-bindings changes so I can
also include this when updating this series.


Best regards,
Martin
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/remoteproc/amlogic,meson-mx-ao-arc.yaml b/Documentation/devicetree/bindings/remoteproc/amlogic,meson-mx-ao-arc.yaml
new file mode 100644
index 000000000000..ba5deebaf7dc
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/amlogic,meson-mx-ao-arc.yaml
@@ -0,0 +1,87 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/remoteproc/amlogic,meson-mx-ao-arc.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Amlogic Meson AO ARC Remote Processor bindings
+
+description:
+  Amlogic Meson6, Meson8, Meson8b and Meson8m2 SoCs embed an ARC core
+  controller for always-on operations, typically used for managing
+  system suspend. Meson6 and older use a ARC core based on the ARCv1
+  ISA, while Meson8, Meson8b and Meson8m2 use an ARC EM4 (ARCv2 ISA)
+  core.
+
+maintainers:
+  - Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+
+properties:
+  compatible:
+    items:
+      - enum:
+        - amlogic,meson8-ao-arc
+        - amlogic,meson8b-ao-arc
+      - const: amlogic,meson-mx-ao-arc
+
+  firmware-name:
+    $ref: /schemas/types.yaml#/definitions/string
+    description:
+      The name of the firmware which should be loaded for this remote
+      processor.
+
+  reg:
+    description:
+      Address ranges of the remap and CPU control addresses for the
+      remote processor.
+    minItems: 2
+
+  reg-names:
+    items:
+      - const: remap
+      - const: cpu
+
+  resets:
+     minItems: 1
+
+  clocks:
+    minItems: 1
+
+  sram:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      phandles to a reserved SRAM region which is used as the memory of
+      the ARC core. The region should be defined as child nodes of the
+      AHB SRAM node as per the generic bindings in
+      Documentation/devicetree/bindings/sram/sram.yaml
+
+  amlogic,secbus2:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      A phandle to the SECBUS2 region which contains some configuration
+      bits of this remote processor
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - resets
+  - clocks
+  - sram
+  - amlogic,secbus2
+
+additionalProperties: false
+
+examples:
+  - |
+    remoteproc@1c {
+      compatible= "amlogic,meson8-ao-arc", "amlogic,meson-mx-ao-arc";
+      reg = <0x1c 0x8>, <0x38 0x8>;
+      reg-names = "remap", "cpu";
+      resets = <&media_cpu_reset>;
+      clocks = <&media_cpu_clock>;
+      sram = <&ahb_sram_ao_arc>;
+      amlogic,secbus2 = <&secbus2>;
+    };
+
+...