[RFC] dt-bindings: dma: apple,sio: Add schema
Commit Message
Describe the SIO coprocessor which serves as pretend DMA controller on
recent Apple platforms.
Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
---
Since the schema mentions a loader preparing the binding appropriately,
here's a PR with the relevant (WIP) loader code, if anyone wants to look:
https://github.com/AsahiLinux/m1n1/pull/286
.../devicetree/bindings/dma/apple,sio.yaml | 129 ++++++++++++++++++
1 file changed, 129 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/apple,sio.yaml
Comments
> On 14. 2. 2023, at 17:12, Rob Herring <robh@kernel.org> wrote:
> On Tue, 14 Feb 2023 15:10:53 +0100, Martin Povišer wrote:
>> Describe the SIO coprocessor which serves as pretend DMA controller on
>> recent Apple platforms.
>>
>> Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
>> ---
>>
>> Since the schema mentions a loader preparing the binding appropriately,
>> here's a PR with the relevant (WIP) loader code, if anyone wants to look:
>> https://github.com/AsahiLinux/m1n1/pull/286
>>
>> .../devicetree/bindings/dma/apple,sio.yaml | 129 ++++++++++++++++++
>> 1 file changed, 129 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/dma/apple,sio.yaml
>>
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/dma/apple,sio.example.dts:24.35-27.11: Warning (node_name_vs_property_name): /example-0/interrupt-controller: node name and property name conflict
I saw the warning, but since I had copied that part of the example from
apple,admac.yaml I didn’t make much of it. Now that I look at it again,
the example in apple,admac doesn’t raise the same warning, how come?
This is just a curiosity since we don’t need the aic node in the example
anyway (unlike in apple,admac where it’s needed because of empty entries
in interrupts-extended).
Martin
On Tue, Feb 14, 2023 at 10:12:34AM -0600, Rob Herring wrote:
>
> On Tue, 14 Feb 2023 15:10:53 +0100, Martin Povišer wrote:
> > Describe the SIO coprocessor which serves as pretend DMA controller on
> > recent Apple platforms.
> >
> > Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
> > ---
> >
> > Since the schema mentions a loader preparing the binding appropriately,
> > here's a PR with the relevant (WIP) loader code, if anyone wants to look:
> > https://github.com/AsahiLinux/m1n1/pull/286
> >
> > .../devicetree/bindings/dma/apple,sio.yaml | 129 ++++++++++++++++++
> > 1 file changed, 129 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/dma/apple,sio.yaml
> >
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/dma/apple,sio.example.dts:24.35-27.11: Warning (node_name_vs_property_name): /example-0/interrupt-controller: node name and property name conflict
This is a quirk with how the examples are constructed. In any case, just
drop the 'interrupt-controller' node. No need to show providers in an
example.
Rob
On Tue, Feb 14, 2023 at 03:10:53PM +0100, Martin Povišer wrote:
> Describe the SIO coprocessor which serves as pretend DMA controller on
> recent Apple platforms.
>
> Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
> ---
>
> Since the schema mentions a loader preparing the binding appropriately,
> here's a PR with the relevant (WIP) loader code, if anyone wants to look:
> https://github.com/AsahiLinux/m1n1/pull/286
>
> .../devicetree/bindings/dma/apple,sio.yaml | 129 ++++++++++++++++++
> 1 file changed, 129 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/dma/apple,sio.yaml
>
> diff --git a/Documentation/devicetree/bindings/dma/apple,sio.yaml b/Documentation/devicetree/bindings/dma/apple,sio.yaml
> new file mode 100644
> index 000000000000..a76cc8265e76
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/apple,sio.yaml
> @@ -0,0 +1,129 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/dma/apple,sio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple SIO Coprocessor
> +
> +description: |
Don't need '|'.
> + SIO is a coprocessor on Apple M1 and later chips (and maybe also on earlier
> + chips). Its role is to offload SPI, UART and DisplayPort audio transfers,
> + being a pretend DMA controller.
> +
> +maintainers:
> + - Martin Povišer <povik+lin@cutebit.org>
> +
> +allOf:
> + - $ref: "dma-controller.yaml#"
Drop quotes.
> +
> +properties:
> + compatible:
> + items:
> + - enum:
> + - apple,t6000-sio
> + - apple,t8103-sio
> + - const: apple,sio
> +
> + reg:
> + maxItems: 1
> +
> + '#dma-cells':
> + const: 1
> + description:
> + DMA clients specify a single cell that corresponds to the RTKit endpoint
> + number used for arranging the transfers in question
> +
> + dma-channels:
> + maximum: 128
> +
> + mboxes:
> + maxItems: 1
> +
> + iommus:
> + maxItems: 1
> +
> + power-domains:
> + maxItems: 1
> +
> + memory-region:
> + description:
> + A number of references to reserved memory regions among which are the DATA/TEXT
> + sections of coprocessor executable firmware and also auxiliary firmware data
> + describing the available DMA-enabled peripherals
Can you define some range of number of entries.
> +
> + apple,sio-firmware-params:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + description: |
> + Parameters in the form of opaque key/value pairs that are to be sent to the SIO
> + coprocesssor once it boots. These parameters can refer to the reserved memory
> + regions described in 'memory-region'.
> +
> + Note that unlike Apple's firmware, we treat the parameters, and the data they
> + refer to, as opaque. Apple embed short data blobs into their SIO devicetree node
> + that describe the DMA-enabled peripherals (presumably with defined semantics).
> + Their driver processes those blobs and sets up data structure in mapped device
> + memory, then references this memory in the parameters sent to the SIO. At the
> + level of description we are opting for in this binding, we assume the job of
> + constructing those data structures has been done in advance, leaving behind an
> + opaque list of key/value parameter pairs to be sent by a prospective driver.
> +
> + This approach is chosen for two reasons:
> +
> + - It means we don't need to try to understand the semantics of Apple's blobs
> + as long as we know the transformation we need to do from Apple's devicetree
> + data to SIO data (which can be shoved away into a loader). It also means the
> + semantics of Apple's blobs (or of something to replace them) need not be part
> + of the binding and be kept up with Apple's firmware changes in the future.
> +
> + - It leaves less work for the driver attaching on this binding. Instead the work
> + is done upfront in the loader which can be better suited for keeping up with
> + Apple's firmware changes.
> +
> +required:
> + - compatible
> + - reg
> + - '#dma-cells'
> + - dma-channels
> + - mboxes
> + - iommus
> + - power-domains
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/apple-aic.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + aic: interrupt-controller {
> + interrupt-controller;
> + #interrupt-cells = <3>;
> + };
> +
> + sio_mbox: mbox@36408000 {
> + compatible = "apple,t8103-asc-mailbox", "apple,asc-mailbox-v4";
> + reg = <0x36408000 0x4000>;
> + interrupt-parent = <&aic>;
> + interrupts = <AIC_IRQ 640 IRQ_TYPE_LEVEL_HIGH>,
> + <AIC_IRQ 641 IRQ_TYPE_LEVEL_HIGH>,
> + <AIC_IRQ 642 IRQ_TYPE_LEVEL_HIGH>,
> + <AIC_IRQ 643 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "send-empty", "send-not-empty",
> + "recv-empty", "recv-not-empty";
> + #mbox-cells = <0>;
> + power-domains = <&ps_sio>;
> + };
Drop. Don't need to show providers.
> +
> + sio: dma-controller@36400000 {
> + status = "disabled"; /* To be filled by loader */
Drop. Examples should be enabled so we can validate them.
> + compatible = "apple,t8103-sio", "apple,sio";
> + reg = <0x36400000 0x8000>;
> + dma-channels = <128>;
> + #dma-cells = <1>;
> + mboxes = <&sio_mbox>;
> + iommus = <&sio_dart 0>;
> + power-domains = <&ps_sio_cpu>;
> + memory-region = <0x0>; /* To be filled by loader */
> + apple,sio-firmware-params = <0x0>; /* To be filled by loader */
> + };
> --
> 2.33.0
Thanks for the review!
> On 14. 2. 2023, at 20:25, Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Feb 14, 2023 at 03:10:53PM +0100, Martin Povišer wrote:
>> Describe the SIO coprocessor which serves as pretend DMA controller on
>> recent Apple platforms.
>>
>> Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
>> ---
>>
>> Since the schema mentions a loader preparing the binding appropriately,
>> here's a PR with the relevant (WIP) loader code, if anyone wants to look:
>> https://github.com/AsahiLinux/m1n1/pull/286
>>
>> .../devicetree/bindings/dma/apple,sio.yaml | 129 ++++++++++++++++++
>> 1 file changed, 129 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/dma/apple,sio.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/dma/apple,sio.yaml b/Documentation/devicetree/bindings/dma/apple,sio.yaml
>> new file mode 100644
>> index 000000000000..a76cc8265e76
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dma/apple,sio.yaml
(...)
>> + memory-region:
>> + description:
>> + A number of references to reserved memory regions among which are the DATA/TEXT
>> + sections of coprocessor executable firmware and also auxiliary firmware data
>> + describing the available DMA-enabled peripherals
>
> Can you define some range of number of entries.
I think I can provide a reliable lower bound (at least one segment with
DATA/TEXT and one segment with auxiliary data), and a conservative upper bound
(separate DATA/TEXT and no more than 6 segments of auxiliary data if each kind
of data has a segment of its own). I will work it in into the next version.
Martin
On Tue, Feb 14, 2023 at 1:18 PM Martin Povišer <povik+lin@cutebit.org> wrote:
>
>
> > On 14. 2. 2023, at 17:12, Rob Herring <robh@kernel.org> wrote:
> > On Tue, 14 Feb 2023 15:10:53 +0100, Martin Povišer wrote:
> >> Describe the SIO coprocessor which serves as pretend DMA controller on
> >> recent Apple platforms.
> >>
> >> Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
> >> ---
> >>
> >> Since the schema mentions a loader preparing the binding appropriately,
> >> here's a PR with the relevant (WIP) loader code, if anyone wants to look:
> >> https://github.com/AsahiLinux/m1n1/pull/286
> >>
> >> .../devicetree/bindings/dma/apple,sio.yaml | 129 ++++++++++++++++++
> >> 1 file changed, 129 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/dma/apple,sio.yaml
> >>
> >
> > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> >
> > yamllint warnings/errors:
> >
> > dtschema/dtc warnings/errors:
> > Documentation/devicetree/bindings/dma/apple,sio.example.dts:24.35-27.11: Warning (node_name_vs_property_name): /example-0/interrupt-controller: node name and property name conflict
>
> I saw the warning, but since I had copied that part of the example from
> apple,admac.yaml I didn’t make much of it. Now that I look at it again,
> the example in apple,admac doesn’t raise the same warning, how come?
>
> This is just a curiosity since we don’t need the aic node in the example
> anyway (unlike in apple,admac where it’s needed because of empty entries
> in interrupts-extended).
'interrupts-extended' is why. If the example just has 'interrupts',
then a fake parent is constructed. It's pretty hacky, but necessary to
parse the interrupts later on for validation.
Rob
new file mode 100644
@@ -0,0 +1,129 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/apple,sio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple SIO Coprocessor
+
+description: |
+ SIO is a coprocessor on Apple M1 and later chips (and maybe also on earlier
+ chips). Its role is to offload SPI, UART and DisplayPort audio transfers,
+ being a pretend DMA controller.
+
+maintainers:
+ - Martin Povišer <povik+lin@cutebit.org>
+
+allOf:
+ - $ref: "dma-controller.yaml#"
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - apple,t6000-sio
+ - apple,t8103-sio
+ - const: apple,sio
+
+ reg:
+ maxItems: 1
+
+ '#dma-cells':
+ const: 1
+ description:
+ DMA clients specify a single cell that corresponds to the RTKit endpoint
+ number used for arranging the transfers in question
+
+ dma-channels:
+ maximum: 128
+
+ mboxes:
+ maxItems: 1
+
+ iommus:
+ maxItems: 1
+
+ power-domains:
+ maxItems: 1
+
+ memory-region:
+ description:
+ A number of references to reserved memory regions among which are the DATA/TEXT
+ sections of coprocessor executable firmware and also auxiliary firmware data
+ describing the available DMA-enabled peripherals
+
+ apple,sio-firmware-params:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ description: |
+ Parameters in the form of opaque key/value pairs that are to be sent to the SIO
+ coprocesssor once it boots. These parameters can refer to the reserved memory
+ regions described in 'memory-region'.
+
+ Note that unlike Apple's firmware, we treat the parameters, and the data they
+ refer to, as opaque. Apple embed short data blobs into their SIO devicetree node
+ that describe the DMA-enabled peripherals (presumably with defined semantics).
+ Their driver processes those blobs and sets up data structure in mapped device
+ memory, then references this memory in the parameters sent to the SIO. At the
+ level of description we are opting for in this binding, we assume the job of
+ constructing those data structures has been done in advance, leaving behind an
+ opaque list of key/value parameter pairs to be sent by a prospective driver.
+
+ This approach is chosen for two reasons:
+
+ - It means we don't need to try to understand the semantics of Apple's blobs
+ as long as we know the transformation we need to do from Apple's devicetree
+ data to SIO data (which can be shoved away into a loader). It also means the
+ semantics of Apple's blobs (or of something to replace them) need not be part
+ of the binding and be kept up with Apple's firmware changes in the future.
+
+ - It leaves less work for the driver attaching on this binding. Instead the work
+ is done upfront in the loader which can be better suited for keeping up with
+ Apple's firmware changes.
+
+required:
+ - compatible
+ - reg
+ - '#dma-cells'
+ - dma-channels
+ - mboxes
+ - iommus
+ - power-domains
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/apple-aic.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ aic: interrupt-controller {
+ interrupt-controller;
+ #interrupt-cells = <3>;
+ };
+
+ sio_mbox: mbox@36408000 {
+ compatible = "apple,t8103-asc-mailbox", "apple,asc-mailbox-v4";
+ reg = <0x36408000 0x4000>;
+ interrupt-parent = <&aic>;
+ interrupts = <AIC_IRQ 640 IRQ_TYPE_LEVEL_HIGH>,
+ <AIC_IRQ 641 IRQ_TYPE_LEVEL_HIGH>,
+ <AIC_IRQ 642 IRQ_TYPE_LEVEL_HIGH>,
+ <AIC_IRQ 643 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "send-empty", "send-not-empty",
+ "recv-empty", "recv-not-empty";
+ #mbox-cells = <0>;
+ power-domains = <&ps_sio>;
+ };
+
+ sio: dma-controller@36400000 {
+ status = "disabled"; /* To be filled by loader */
+ compatible = "apple,t8103-sio", "apple,sio";
+ reg = <0x36400000 0x8000>;
+ dma-channels = <128>;
+ #dma-cells = <1>;
+ mboxes = <&sio_mbox>;
+ iommus = <&sio_dart 0>;
+ power-domains = <&ps_sio_cpu>;
+ memory-region = <0x0>; /* To be filled by loader */
+ apple,sio-firmware-params = <0x0>; /* To be filled by loader */
+ };