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

From: Krzysztof Kozlowski
Date: Tue Jan 03 2023 - 11:03:00 EST


On 03/01/2023 16:56, Wojciech Zmuda wrote:
> From: Wojciech Bartczak <wbartczak@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> Signed-off-by: Wojciech Zmuda <wzmuda@xxxxxxxxxxx>
> ---
> .../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@xxxxxxxxxxx>
> + - Sunil Goutham <sgoutham@xxxxxxxxxxx>
> + - Wojciech Bartczak <wbartczak@xxxxxxxxxxx>
> +
> +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