| Message ID | 20240912170025.455167-5-valentina.fernandezalanis@microchip.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add Microchip IPC mailbox and remoteproc support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 12/09/2024 19:00, Valentina Fernandez wrote: > Microchip family of RISC-V SoCs typically has or more clusters. These > clusters can be configured to run in Asymmetric Multi Processing (AMP) > mode A nit, subject: drop second/last, redundant "binding for". The "dt-bindings" prefix is already stating that these are bindings. See also: https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > > Add a dt-binding for the Microchip IPC Remoteproc platform driver. > Binding is for hardware, not driver. Please rephrase it to describe hardware. > Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com> > --- > .../remoteproc/microchip,ipc-remoteproc.yaml | 84 +++++++++++++++++++ > 1 file changed, 84 insertions(+) > create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml > > diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml > new file mode 100644 > index 000000000000..1765c68d22cf > --- /dev/null > +++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml > @@ -0,0 +1,84 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-remoteproc.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Microchip IPC Remote Processor > + > +description: > + Microchip family of RISC-V SoCs typically have one or more > + clusters. These clusters can be configured to run in an Asymmetric > + Multi Processing (AMP) mode where clusters are split in independent > + software contexts. > + > + This document defines the binding for the remoteproc component that > + loads and boots firmwares on remote clusters. Don't say that binding is a binding for. Say what this hardware piece is. > + > + This SBI interface is compatible with the Mi-V Inter-hart > + Communication (IHC) IP. > + > +maintainers: > + - Valentina Fernandez <valentina.fernandezalanis@microchip.com> > + > +properties: > + compatible: > + const: microchip,ipc-remoteproc That's quite generic. Basically this says it will handle IPC of all possible Microchip SoCs, not only RISC-V but also ARM and whatever you come up with. > + > + mboxes: > + description: > + This property is required only if the rpmsg/virtio functionality is used. > + Microchip IPC mailbox specifier. To be used for communication with a > + remote cluster. The specifier format is as per the bindings, > + Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml > + maxItems: 1 > + > + microchip,auto-boot: > + $ref: /schemas/types.yaml#/definitions/flag > + description: > + If defined, when remoteproc is probed, it loads the default firmware and > + starts the remote processor. You described the desired Linux feature or behavior, not the actual hardware. The bindings are about the latter, so instead you need to rephrase the property and its description to match actual hardware capabilities/features/configuration etc. > + > + microchip,skip-ready-wait: > + $ref: /schemas/types.yaml#/definitions/flag > + description: > + If defined, the master processor will not expect a ready signal from the > + remote processor indicating it has booted successfully. This allows the > + master processor to proceed with its operations without waiting for > + confirmation from the remote processor. Same problem. > + > + memory-region: > + description: > + If present, a phandle for a reserved memory area that used for vdev buffer, > + resource table, vring region and others used by remote cluster. missing constraints > + > +required: > + - compatible > + > +additionalProperties: false > + > +examples: > + - | > + Drop blank line > + reserved-memory { > + #address-cells = <1>; > + #size-cells = <1>; > + > + contextb: contextb_reserved@81000000 { > + reg = <0x81000000 0x400000>; > + no-map; > + }; > + }; Drop entire reserved-node. Obvious. > + > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + > + rproc-contextb { Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation usually remoteproc > + compatible = "microchip,ipc-remoteproc"; > + memory-region = <&contextb>; > + mboxes= <&ihc 8>; Make the binding complete. Fix the white-space issues. > + }; > + }; > + > +... Best regards, Krzysztof
On 16/09/2024 21:14, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 12/09/2024 19:00, Valentina Fernandez wrote: >> Microchip family of RISC-V SoCs typically has or more clusters. These >> clusters can be configured to run in Asymmetric Multi Processing (AMP) >> mode > > A nit, subject: drop second/last, redundant "binding for". The > "dt-bindings" prefix is already stating that these are bindings. > See also: > https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > >> >> Add a dt-binding for the Microchip IPC Remoteproc platform driver. >> > > Binding is for hardware, not driver. Please rephrase it to describe > hardware. > > >> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com> >> --- >> .../remoteproc/microchip,ipc-remoteproc.yaml | 84 +++++++++++++++++++ >> 1 file changed, 84 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml >> >> diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml >> new file mode 100644 >> index 000000000000..1765c68d22cf >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml >> @@ -0,0 +1,84 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-remoteproc.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Microchip IPC Remote Processor >> + >> +description: >> + Microchip family of RISC-V SoCs typically have one or more >> + clusters. These clusters can be configured to run in an Asymmetric >> + Multi Processing (AMP) mode where clusters are split in independent >> + software contexts. >> + >> + This document defines the binding for the remoteproc component that >> + loads and boots firmwares on remote clusters. > > Don't say that binding is a binding for. Say what this hardware piece is. > >> + >> + This SBI interface is compatible with the Mi-V Inter-hart >> + Communication (IHC) IP. >> + >> +maintainers: >> + - Valentina Fernandez <valentina.fernandezalanis@microchip.com> >> + >> +properties: >> + compatible: >> + const: microchip,ipc-remoteproc > > That's quite generic. Basically this says it will handle IPC of all > possible Microchip SoCs, not only RISC-V but also ARM and whatever you > come up with. IPC is the actual name of the hardware block described in this binding. I'll update the description of the binding in v2 to mention this. Additionally, I'll rename the compatible to microchip,ipc-sbi-remoteproc to further clarify that this binding is intended for devices using the Microchip IPC hardware block and for devices with an SBI interface (RISC-V). Thanks, Valentina > > > >> + >> + mboxes: >> + description: >> + This property is required only if the rpmsg/virtio functionality is used. >> + Microchip IPC mailbox specifier. To be used for communication with a >> + remote cluster. The specifier format is as per the bindings, >> + Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml >> + maxItems: 1 >> + >> + microchip,auto-boot: >> + $ref: /schemas/types.yaml#/definitions/flag >> + description: >> + If defined, when remoteproc is probed, it loads the default firmware and >> + starts the remote processor. > > You described the desired Linux feature or behavior, not the actual > hardware. The bindings are about the latter, so instead you need to > rephrase the property and its description to match actual hardware > capabilities/features/configuration etc. > >> + >> + microchip,skip-ready-wait: >> + $ref: /schemas/types.yaml#/definitions/flag >> + description: >> + If defined, the master processor will not expect a ready signal from the >> + remote processor indicating it has booted successfully. This allows the >> + master processor to proceed with its operations without waiting for >> + confirmation from the remote processor. > Same problem. > > >> + >> + memory-region: >> + description: >> + If present, a phandle for a reserved memory area that used for vdev buffer, >> + resource table, vring region and others used by remote cluster. > > missing constraints > >> + >> +required: >> + - compatible >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + > > Drop blank line > >> + reserved-memory { >> + #address-cells = <1>; >> + #size-cells = <1>; >> + >> + contextb: contextb_reserved@81000000 { >> + reg = <0x81000000 0x400000>; >> + no-map; >> + }; >> + }; > > Drop entire reserved-node. Obvious. > >> + >> + soc { >> + #address-cells = <2>; >> + #size-cells = <2>; >> + >> + rproc-contextb { > > Node names should be generic. See also an explanation and list of > examples (not exhaustive) in DT specification: > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > > usually remoteproc > >> + compatible = "microchip,ipc-remoteproc"; >> + memory-region = <&contextb>; >> + mboxes= <&ihc 8>; > > Make the binding complete. Fix the white-space issues. > >> + }; >> + }; >> + >> +... > > Best regards, > Krzysztof >
On 15/10/2024 14:09, Valentina.FernandezAlanis@microchip.com wrote: > On 16/09/2024 21:14, Krzysztof Kozlowski wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On 12/09/2024 19:00, Valentina Fernandez wrote: >>> Microchip family of RISC-V SoCs typically has or more clusters. These >>> clusters can be configured to run in Asymmetric Multi Processing (AMP) >>> mode >> >> A nit, subject: drop second/last, redundant "binding for". The >> "dt-bindings" prefix is already stating that these are bindings. >> See also: >> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 >> >>> >>> Add a dt-binding for the Microchip IPC Remoteproc platform driver. >>> >> >> Binding is for hardware, not driver. Please rephrase it to describe >> hardware. >> >> >>> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com> >>> --- >>> .../remoteproc/microchip,ipc-remoteproc.yaml | 84 +++++++++++++++++++ >>> 1 file changed, 84 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml >>> new file mode 100644 >>> index 000000000000..1765c68d22cf >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml >>> @@ -0,0 +1,84 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-remoteproc.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Microchip IPC Remote Processor >>> + >>> +description: >>> + Microchip family of RISC-V SoCs typically have one or more >>> + clusters. These clusters can be configured to run in an Asymmetric >>> + Multi Processing (AMP) mode where clusters are split in independent >>> + software contexts. >>> + >>> + This document defines the binding for the remoteproc component that >>> + loads and boots firmwares on remote clusters. >> >> Don't say that binding is a binding for. Say what this hardware piece is. >> >>> + >>> + This SBI interface is compatible with the Mi-V Inter-hart >>> + Communication (IHC) IP. >>> + >>> +maintainers: >>> + - Valentina Fernandez <valentina.fernandezalanis@microchip.com> >>> + >>> +properties: >>> + compatible: >>> + const: microchip,ipc-remoteproc >> >> That's quite generic. Basically this says it will handle IPC of all >> possible Microchip SoCs, not only RISC-V but also ARM and whatever you >> come up with. > IPC is the actual name of the hardware block described in this binding. > I'll update the description of the binding in v2 to mention this. > > Additionally, I'll rename the compatible to microchip,ipc-sbi-remoteproc > to further clarify that this binding is intended for devices using the > Microchip IPC hardware block and for devices with an SBI interface (RISC-V). Well, still generic. Explain why this deserves exception from specific SoC compatibles. Best regards, Krzysztof
On Tue, Oct 15, 2024 at 03:35:46PM +0200, Krzysztof Kozlowski wrote: > On 15/10/2024 14:09, Valentina.FernandezAlanis@microchip.com wrote: > > On 16/09/2024 21:14, Krzysztof Kozlowski wrote: > >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> > >> On 12/09/2024 19:00, Valentina Fernandez wrote: > >>> Microchip family of RISC-V SoCs typically has or more clusters. These > >>> clusters can be configured to run in Asymmetric Multi Processing (AMP) > >>> mode > >> > >> A nit, subject: drop second/last, redundant "binding for". The > >> "dt-bindings" prefix is already stating that these are bindings. > >> See also: > >> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > >> > >>> > >>> Add a dt-binding for the Microchip IPC Remoteproc platform driver. > >>> > >> > >> Binding is for hardware, not driver. Please rephrase it to describe > >> hardware. > >> > >> > >>> Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com> > >>> --- > >>> .../remoteproc/microchip,ipc-remoteproc.yaml | 84 +++++++++++++++++++ > >>> 1 file changed, 84 insertions(+) > >>> create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml > >>> > >>> diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml > >>> new file mode 100644 > >>> index 000000000000..1765c68d22cf > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml > >>> @@ -0,0 +1,84 @@ > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>> +%YAML 1.2 > >>> +--- > >>> +$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-remoteproc.yaml# > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>> + > >>> +title: Microchip IPC Remote Processor > >>> + > >>> +description: > >>> + Microchip family of RISC-V SoCs typically have one or more > >>> + clusters. These clusters can be configured to run in an Asymmetric > >>> + Multi Processing (AMP) mode where clusters are split in independent > >>> + software contexts. > >>> + > >>> + This document defines the binding for the remoteproc component that > >>> + loads and boots firmwares on remote clusters. > >> > >> Don't say that binding is a binding for. Say what this hardware piece is. > >> > >>> + > >>> + This SBI interface is compatible with the Mi-V Inter-hart > >>> + Communication (IHC) IP. > >>> + > >>> +maintainers: > >>> + - Valentina Fernandez <valentina.fernandezalanis@microchip.com> > >>> + > >>> +properties: > >>> + compatible: > >>> + const: microchip,ipc-remoteproc > >> > >> That's quite generic. Basically this says it will handle IPC of all > >> possible Microchip SoCs, not only RISC-V but also ARM and whatever you > >> come up with. > > IPC is the actual name of the hardware block described in this binding. > > I'll update the description of the binding in v2 to mention this. > > > > Additionally, I'll rename the compatible to microchip,ipc-sbi-remoteproc > > to further clarify that this binding is intended for devices using the > > Microchip IPC hardware block and for devices with an SBI interface (RISC-V). > > Well, still generic. Explain why this deserves exception from specific > SoC compatibles. If I understand this correctly, some degree of generic-ness is actually intended here. The IPC/IHC (the name depends on whether or not it is RTL for the FPGA fabric or a hardened version) isn't quite like some of the other remoteproc things here, that are intended for programming a DSP or similar with some firmware - it's intended for asymmetric multiprocessing stuff, where some of the CPU cores run Linux and the others are running something like freertos or zephyr, with an abstracted interface provided by the firmware/SBI implementation. The mailbox side of this is also implemented using an SBI abstraction (similar to PSCI or SCMI, I never recall which) and also has a compatible that isn't tied to specific soc. Granted, the mailbox side does also have a IP core specific version, but that is not intended to be consumed by the OS, but rather by the SBI implementation (e.g. OpenSBI). What this binding is supposed to be describing is the "generic" ecall interface to a set of remote processors provided by the SBI implementation. The platform you're running on is meant to be abstracted away by use of ecalls etc, just as the mailbox is - which is why Valentina went something not soc-specific here. I can see much more of an argument for encoding the version of the protocol that is implemented by the SBI firmware than for having a soc-specific set of compatibles here.
diff --git a/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml new file mode 100644 index 000000000000..1765c68d22cf --- /dev/null +++ b/Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml @@ -0,0 +1,84 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/remoteproc/microchip,ipc-remoteproc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Microchip IPC Remote Processor + +description: + Microchip family of RISC-V SoCs typically have one or more + clusters. These clusters can be configured to run in an Asymmetric + Multi Processing (AMP) mode where clusters are split in independent + software contexts. + + This document defines the binding for the remoteproc component that + loads and boots firmwares on remote clusters. + + This SBI interface is compatible with the Mi-V Inter-hart + Communication (IHC) IP. + +maintainers: + - Valentina Fernandez <valentina.fernandezalanis@microchip.com> + +properties: + compatible: + const: microchip,ipc-remoteproc + + mboxes: + description: + This property is required only if the rpmsg/virtio functionality is used. + Microchip IPC mailbox specifier. To be used for communication with a + remote cluster. The specifier format is as per the bindings, + Documentation/devicetree/bindings/mailbox/microchip,sbi-ipc.yaml + maxItems: 1 + + microchip,auto-boot: + $ref: /schemas/types.yaml#/definitions/flag + description: + If defined, when remoteproc is probed, it loads the default firmware and + starts the remote processor. + + microchip,skip-ready-wait: + $ref: /schemas/types.yaml#/definitions/flag + description: + If defined, the master processor will not expect a ready signal from the + remote processor indicating it has booted successfully. This allows the + master processor to proceed with its operations without waiting for + confirmation from the remote processor. + + memory-region: + description: + If present, a phandle for a reserved memory area that used for vdev buffer, + resource table, vring region and others used by remote cluster. + +required: + - compatible + +additionalProperties: false + +examples: + - | + + reserved-memory { + #address-cells = <1>; + #size-cells = <1>; + + contextb: contextb_reserved@81000000 { + reg = <0x81000000 0x400000>; + no-map; + }; + }; + + soc { + #address-cells = <2>; + #size-cells = <2>; + + rproc-contextb { + compatible = "microchip,ipc-remoteproc"; + memory-region = <&contextb>; + mboxes= <&ihc 8>; + }; + }; + +...
Microchip family of RISC-V SoCs typically has or more clusters. These clusters can be configured to run in Asymmetric Multi Processing (AMP) mode. Add a dt-binding for the Microchip IPC Remoteproc platform driver. Signed-off-by: Valentina Fernandez <valentina.fernandezalanis@microchip.com> --- .../remoteproc/microchip,ipc-remoteproc.yaml | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 Documentation/devicetree/bindings/remoteproc/microchip,ipc-remoteproc.yaml