[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <625410299fd45188759d3b1fe205e6c42af0bbda.camel@mediatek.com>
Date: Sun, 6 Apr 2025 06:46:23 +0000
From: Xiangzhi Tang (唐相志)
<Xiangzhi.Tang@...iatek.com>
To: "krzk@...nel.org" <krzk@...nel.org>, "conor+dt@...nel.org"
<conor+dt@...nel.org>, "robh@...nel.org" <robh@...nel.org>,
"matthias.bgg@...il.com" <matthias.bgg@...il.com>, "krzk+dt@...nel.org"
<krzk+dt@...nel.org>, "andersson@...nel.org" <andersson@...nel.org>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
"mathieu.poirier@...aro.org" <mathieu.poirier@...aro.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
"linux-remoteproc@...r.kernel.org" <linux-remoteproc@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Hailong Fan (范海龙) <Hailong.Fan@...iatek.com>,
Project_Global_Chrome_Upstream_Group
<Project_Global_Chrome_Upstream_Group@...iatek.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
Jjian Zhou (周建) <Jjian.Zhou@...iatek.com>
Subject: Re: [PATCH 1/2] dt-bindings: remoteproc: Add VCP support for mt8196
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@...00000 {
> > + 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
Powered by blists - more mailing lists