Re: [PATCH v2 1/2] dt-bindings: mailbox: add cix,sky1-mbox

[Guomin chen]

On Tue, Mar 25, 2025 at 11:36:18AM +0100, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL
> 
> On 25/03/2025 11:18, Guomin Chen wrote:
> > +
> > +description:
> > +  The Cixtech mailbox controller, used in the Cixtech Sky1 SoC,
> > +  is used for message transmission between multiple processors
> > +  within the SoC, such as the AP, PM, audio DSP, SensorHub MCU,
> > +  and others
> > +
> > +  Each Cixtech mailbox controller is unidirectional, so they are
> > +  typically used in pairs-one for receiving and one for transmitting.
> > +
> > +  Each Cixtech mailbox supports 11 channels with different transmission modes
> > +    channel 0-7 - Fast channel with 32bit transmit register and IRQ support
> > +    channel 8   - Doorbell mode,using the mailbox as an interrupt-generating
> > +                   mechanism.
> > +    channel 9   - Fifo based channel with 32*32bit depth fifo and IRQ support
> > +    channel 10  - Reg based channel with 32*32bit transmit register and
> > +                   Doorbell+transmit acknowledgment IRQ support
> 
> Maybe you split one device into two and that's why you have
> unidirectional mailboxes?
Not like this. Each Cixtech mailbox controller is unidirectional, so two mailboxs
are required to achieve bidirectional communication between two internal
coprocessors—one for receiving and one for transmitting.

> > +
> > +properties:
> > +  compatible:
> > +    const: cix,sky1-mbox
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  "#mbox-cells":
> > +    description: Common mailbox binding property to identify the number
> 
> Drop
> 
> > +        of cells required for the mailbox specifier. Should be 1
> 
> Drop
> 
> Don't repeat constraints in free form text. Missing const.
Okay, I will remove this duplicate description and add const.

> You kept here pieces I asked to drop. You dropped pieces I did not
> comment under. Please observe where the comments appear.
> 
> So again: explain what each cell argument means.
> 
> 
> > +> +  cix,mbox-dir:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: Direction of the mailbox (0:TX or 1:RX)
> > +    enum: [0, 1]
> 
> Respond to comments or implement them. Previous discussion was not
> resolved - you did not respond. Sending the same with unfinished
> discussion is not way to convince maintainer. Opposite: you will get
> NAKed because you try to finish discussion and push your patch over the
> wall.
> 
Sorry, about your previous comments.

As previously mentioned, each Cixtech mailbox controller is unidirectional.
Each mailbox supports 11 channels, which differ only in their transmission
methods (e.g., FIFO, REG, doorbell). However, the transmission direction of
all channels within the same mailbox is fixed and determined by the mailbox
controller itself.

The Sky1 SoC contains 4 pairs (8 total) mailbox controllers. To identify
whether a controller is TX or RX relative to the AP, the driver introduces
the 'mbox-dir' property in DTS.

Alternatively, the direction could be specified directly in the compatible string
without using 'mbox-dir'. However, I think these two approaches are the same.

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - "#mbox-cells"
> > +  - cix,mbox-dir
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        mbox_ap2pm: mailbox@30000000 {
> > +            compatible = "cix,sky1-mbox";
> > +            reg = <0 0x30000000 0 0x10000>;
> > +            interrupts = <GIC_SPI 136 IRQ_TYPE_LEVEL_HIGH 0>;
> > +            #mbox-cells = <1>;
> > +            cix,mbox-dir = <0>; /* 0:tx; 1:rx */
> 
> Drop comment. If you need comment, then this should be string.
> 
> > +        };
> > +
> > +        mbox_pm2ap: mailbox@30010000 {
> > +            compatible = "cix,sky1-mbox";
> > +            reg = <0 0x30010000 0 0x10000>;
> > +            interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH 0>;
> > +            #mbox-cells = <1>;
> > +            cix,mbox-dir = <1>; /* 0:tx; 1:rx */
> 
> Drop second example. Wasn't here and is not needed. Same properties,
> difference in one value is not that much.
> 
Okay, I will remove the comments.

Using two examples is merely to illustrate that the Cixtech mailbox
appears in pairs to achieve bidirectional communication.
If you think it's unnecessary, I will remove the second example. thanks.

Best regards,
Guomin Chen