Re: [PATCH v2 2/2] Documentation: add Marvell MHU driver bindings

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 03/01/2023 16:56, Wojciech Zmuda wrote:
> From: Wojciech Bartczak <wbartczak@marvell.com>
> 
> Marvell Message Handling Unit is a mailbox controller present in
> Marvell OcteonTx and OcteonTX2 SoC family.

1. Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

2. Subject: drop second, redundant "bindings".

3. Subject: drop "driver", unless your piece of hardware is called driver?


> 
> Signed-off-by: Wojciech Bartczak <wbartczak@marvell.com>
> Signed-off-by: Wojciech Zmuda <wzmuda@marvell.com>
> ---
>  .../bindings/mailbox/marvell,mvl-mhu.yml      | 67 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/marvell,mvl-mhu.yml
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/marvell,mvl-mhu.yml b/Documentation/devicetree/bindings/mailbox/marvell,mvl-mhu.yml
> new file mode 100644
> index 000000000000..e06a17eab0f9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/marvell,mvl-mhu.yml

Filenames should be based on compatibles, e.g. be the same.

> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/marvell,mvl-mhu.yml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell Message Handling Unit driver

Drop driver

> +
> +maintainers:
> +  - Sujeet Baranwal <sbaranwal@marvell.com>
> +  - Sunil Goutham <sgoutham@marvell.com>
> +  - Wojciech Bartczak <wbartczak@marvell.com>
> +
> +description:
> +  The Control-Processors Cluster (CPC) provides Arm-platform specification
> +  entities for managing the system. On of the CPC processors is the System

On->One?

> +  Control Processor (SCP). The SCP is responsible, among others, for booting
> +  the chip, clock and power initialization, controlling power consumption
> +  through DVFS, monitoring temperature sensors and controlling AVS. The SCP,
> +  as each XCP, contains mailboxes for software-to-software communications.
> +  Mailbox writes cause an interrupt to the local XCP core or to the AP.
> +  This driver exposes AP-SCP Message Handling Unit to the system, providing
> +  the mailbox communication mechanism to the system, with the intention
> +  of plugging into the SCMI framework. It is designed to work with Marvell
> +  OcteonTX and OcteonTX2-based platforms.
> +  Mailbox has no other usage than SCMI communication. In case of
> +  configurations running without SCMI support it should be disabled.
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: marvell,mbox

This is too generic. Are you sure that all Marvel mailboxes - past,
current and future - will be exactly same as this one. No differences
for next 100 years? IOW, compatible has to be specific to hardware (SoC,
device etc).

> +
> +  reg:
> +    maxItems: 1
> +
> +  "#mbox-cells":
> +    description: Index of the channel
> +    const: 1

No interrupts?

> +
> +required:
> +  - "#mbox-cells"
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +		/ {

Use 4 spaces for example indentation.

> +			mailbox: mailbox@28,0 {
> +				compatible = "marvell,mbox";
> +				#mbox-cells = <1>;
> +				reg = <0xE000 0 0 0 0>;/*  DEVFN = 0xE0 (1C:0) */

1. reg is a second property, after compatible.
2. unit address is wrong.
3. lowercase hex.
4. Two spaces after /* but missing space before.

> +			};
> +
> +			/* ... */
> +
> +			firmware {
> +				scmi {
> +					compatible = "arm,scmi";
> +					mboxes = <&mailbox 0>;

Drop entire firmware example, unrelated.

> +					mbox-names = "scp_ap";
> +					/* ... */

Best regards,
Krzysztof