Re: [PATCH v1 4/5] dt-bindings: remoteproc: add binding for Microchip IPC remoteproc

[Conor Dooley]

On Tue, Oct 15, 2024 at 03:35:46PM +0200, Krzysztof Kozlowski wrote:
> On 15/10/2024 14:09, Valentina.FernandezAlanis@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx>
> >>> ---
> >>>   .../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@xxxxxxxxxxxxx>
> >>> +
> >>> +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.

Attachment: signature.asc
Description: PGP signature