Re: [PATCH 1/2] dt-bindings: remoteproc: Add VCP support for mt8196

["Xiangzhi Tang (唐相志)" <Xiangzhi.Tang@mediatek.com>]

On Wed, 2025-04-02 at 13:11 +0200, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 02/04/2025 11:19, Xiangzhi Tang wrote:
> > +
> > +description:
> > +  The MediaTek VCP enables the SoC control the MediaTek Video
> > Companion Risc-V coprocessor.
> 
> Wrap at coding style.
> Thanks comment, I will fix it on v1 version.
> 
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - mediatek,mt8196-vcp
> > +
> > +  reg:
> > +    items:
> > +      - description: sram base
> > +      - description: cfg group IO
> > +      - description: cfg core group IO
> > +      - description: cfg sec group IO
> > +      - description: vcp rdy group IO
> > +
> > +  reg-names:
> > +    items:
> > +      - const: sram
> > +      - const: cfg
> > +      - const: cfg_core
> > +      - const: cfg_sec
> > +      - const: vcp_vlp_ao_rsvd7
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  mboxes:
> > +    description:
> > +      Using mailbox to communicate with VCP, it should have this
> > +      property and list of phandle, mailbox specifiers. See
> > +      Documentation/devicetree/bindings/mailbox/mediatek,mt8196-
> > vcp-mbox.yaml
> > +      for details.
> 
> Drop entire description, redundant.
> Thanks comment, I will fix it on v1 version.
> 
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +
> 
> No, you do not get your own type. Instead list items or just
> maxItems.
> Thanks comment, I will fix it on v1 version.
> 

> 
> > +  mbox-names:
> > +    maxItems: 5
> 
> No, you must list the items.
> Thanks comment, I will fix it on v1 version.
> 
> > +
> > +  power-domains:
> > +    description:
> > +      A phandle and PM domain specifier as defined by bindings
> > +      of the power controller specified by phandle. See
> > +      Documentation/devicetree/bindings/power/power-domain.yaml
> > for details.
> 
> Look how other bindings do it. Do not repeat obvious stuff, do not
> develop bindings entirely different than all others.
> Thanks comment, I will update it on v1 version.
> 
> > +    maxItems: 1
> > +
> > +  iommus:
> > +    description:
> > +      Using MediaTek iommu to apply larb ports for Multimedia
> > Memory
> > +      Management Unit and address translation
> > +      Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml
> 
> Really, look at other code first.
> Yes, phandle reference iommu node
> 
> > +
> > +  memory-region:
> > +    maxItems: 1
> > +
> > +  vcp-mem-tbl:
> > +    description:
> > +      Manage reserved memory for VCP RTOS FW and various features.
> 
> No, reserved memory is in memory-region. Drop property.
> Thanks comment, I will fix it on v1 version.
> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    minItems: 2
> > +    maxItems: 12
> > +
> > +patternProperties:
> > +  "^vcp_[a-f0-9]+$":
> 
> Follow DTS coding style.
> 
> Heh, nothing here was really tested and you have obvious bugs pointed
> out by simple testing of DTS.
> I run test "make dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/remoteproc/mediatek,m
t8196-vcp.yaml", And, I have to cherry-pick first "
https://lore.kernel.org/all/20250307032942.10447-1-guangjie.song@mediatek.com/
" and "
https://patchwork.kernel.org/project/linux-mediatek/patch/20250317110331.2776-4-jjian.zhou@mediatek.com/
"
> 
> Why these children are needed in the first place? Offsets are implied
> by
> parent compatible.
> Thanks comment, I will check it dtb style again.
> 
> > +    type: object
> > +    description:
> > +      The MediaTek VCP integrated to SoC might be a multi-core
> > version.
> > +      The other cores are represented as child nodes of the boot
> > core.
> > +      There are some integration differences for the IP like the
> > usage of
> > +      address translator for translating SoC bus addresses into
> > address
> > +      space for the processor.
> > +
> > +      The SRAM are shared by all cores, each VCP core only using a
> > piece
> > +      SRAM memory. The power of SRAM should be enabled before
> > booting VCP cores.
> > +      The size of SRAM are varied on differnt SoCs.
> > +
> > +      The VCP cores has differences on different SoCs to support
> > for
> > +      Hart.
> > +
> > +    properties:
> > +      compatible:
> > +        enum:
> > +          - mediatek,vcp-core
> > +          - mediatek,mmup-core
> > +
> > +      twohart:
> 
> Missing vendor prefix, look at writing bindings and other examples.
> Thanks comment, I will check it dtb style again.
> 
> > +        enum: [0, 1]
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +      sram-offset:
> > +        description:
> > +          Allocated SRAM memory for each VCP core used.
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +    required:
> > +      - compatible
> > +      - twohart
> > +      - sram-offset
> > +
> > +    additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - interrupts
> > +  - mboxes
> > +  - mbox-names
> > +  - power-domains
> > +  - iommus
> > +  - memory-region
> > +  - vcp-mem-tbl
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/power/mt8196-power.h>
> > +
> > +    vcp: vcp@31800000 {
> > +        compatible = "mediatek,mt8196-vcp";
> > +        reg = <0x31800000 0x60000>,
> > +              <0x31a04000 0xa000>,
> > +              <0x31bd0000 0x1000>,
> > +              <0x31a70020 0x100>,
> > +              <0x1c00091c 0x4>;
> 
> Quite different address,  are you sure this is still part of this
> device? Looks like on register.
> Yes, It is necessary register for this device
> 
> 
> Best regards,
> Krzyszto