Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c237958c-c274-73b4-f844-b2e29d905447@linaro.org>
Date:   Thu, 14 Jul 2022 14:33:51 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH 1/2] dt-bindings: mailbox : marvell,mbox: Add bindings
Content-Language: en-US
To:     Wojciech Bartczak <wbartczak@marvell.com>,
        linux-kernel@vger.kernel.org
Cc:     wbartczak@gmail.com, Piyush Malgujar <pmalgujar@marvell.com>,
        Sunil Goutham <sgoutham@marvell.com>,
        Jassi Brar <jassisinghbrar@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        devicetree@vger.kernel.org
References: <20220714121215.22931-1-wbartczak@marvell.com>
 <20220714121215.22931-2-wbartczak@marvell.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20220714121215.22931-2-wbartczak@marvell.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 14/07/2022 14:12, Wojciech Bartczak wrote:
> This patch adds device tree binding for Marvell Message Handling Unit
> (MHU) controller.
> 
> Signed-off-by: Wojciech Bartczak <wbartczak@marvell.com>
> ---
>  .../mailbox/marvell,mbox-mailbox.yaml         | 112 ++++++++++++++++++
>  1 file changed, 112 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/marvell,mbox-mailbox.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/marvell,mbox-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/marvell,mbox-mailbox.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..d9a6e14dcb12da6c3a9db2dfafc075ccefa8711c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/marvell,mbox-mailbox.yaml
> @@ -0,0 +1,112 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/marvell,mbox-mailbox.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell Message Handling Unit
> +
> +maintainers:
> +  - Wojciech Bartczak <wbartczak@marvell.com>
> +  - Piyush Malgujar <pmalgujar@marvell.com>
> +
> +description:
> +  The Marvell's Message Handling Unit is a mailbox controller
> +  with a single channel used to communicate with System Control Processor.
> +  Driver supports series of cn9x and cn10x SoC.


Driver is not related to bindings, remove implementation details.

> +  Sole purpose of the link is to exchange SCMI related data with SCP.
> +  The link has hardwired configuration, it uses simple notification scheme
> +  over shared memory block to push data back and forth.
> +  Interrupts used by mailbox may be configured in two ways,
> +  as SPI interrupts, then driver uses platform device forntend.
> +  Other way is to use PCI bus frontend with LPI interrupts.
> +
> +properties:
> +  compatible:
> +    items:

No "items", you have just one here, but see comment below.

> +      - const: marvell,mbox

Need SoC or model specific compatible. Generic fallbacks are accepted if
always prepended with specific compatible.

Are you sure that all, absolutely all, including ones made in 20 years,
of Marvell mboxes will be 100% compatible with "marvell,mbox"?

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    items:
> +      - description: interrupt type
> +        const: 0
> +      - description: interrupt number
> +      - description: interrupt trigger type
> +        const: 1

This looks wrong. You just specified here three interrupts, which are
fixed... It does not make really sense.


> +
> +  '#mbox-cells':
> +    description: Index of the channel

Skip description, obvious.

> +    const: 1
> +
> +  shmem:
> +    description:
> +      List of phandle pointing to the shared memory area between SCP and AP.
> +      The area is used to exchange additional information not covered by SCMI.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#mbox-cells'
> +  - shmem
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    soc@0 {
> +      reg = <0 0>;
> +       #address-cells = <2>;

Messed up indentation. While fixing it, convert to 4 space for DTS
example, as recommended.

> +       #size-cells = <2>;
> +
> +      sram@36,0 {
> +        compatible = "cpc-shmem";
> +        reg = <0x86d0 0xdd400 0 0x200>;
> +        #address-cells = <2>;
> +        #size-cells = <1>;
> +        ranges = <0 0x0 0x86d0 0xdd400 0x200>;
> +
> +        scp_to_cpu0: scp-shmame@0 {
> +          compatible = "cpc-shmem";
> +          reg = <0x0 0x0 0x200>;
> +        };
> +      };

Isn't smem a generic property and generic use case? If so, then drop
this part, unless it brings anything specific to your mailbox example.

> +
> +      mailbox0: mailbox@28,0 {
> +        #mbox-cells = <1>;

First compatible, then reg then the rest of properties.

> +        compatible = "marvell,mbox";
> +        reg = <0xe000 0 0 0>;
> +        shmem = <&scp_to_cpu0>;
> +      };
> +    };
> +  - |
> +    soc@1 {
> +      reg = <1 0>;
> +       #address-cells = <2>;
> +       #size-cells = <2>;
> +
> +      sram@36,0 {
> +        compatible = "cpc-shmem";
> +        reg = <0x86d0 0xdd400 0 0x200>;
> +        #address-cells = <2>;
> +        #size-cells = <1>;
> +        ranges = <0 0x0 0x86d0 0xdd400 0x200>;
> +
> +        scp_to_cpu1: scp-shmame@1 {
> +          compatible = "cpc-shmem";
> +          reg = <0x0 0x0 0x200>;
> +        };
> +      };
> +
> +      mailbox1: mailbox@82c000000000 {
> +        #mbox-cells = <1>;
> +        compatible = "marvell,mbox";
> +        reg = <0x82c0 0x00000000 0x0 0x100000>;
> +        interrupt-parent = <&gic0>;
> +        interrupts = <0 59 1>;

These look like GIC and interrupt flags, so definitely not three
interrupts. Use proper defines.


> +        shmem = <&scp_to_cpu1>;

This is the same example as before, drop it or merge with previous.

> +      };
> +    };


Best regards,
Krzysztof