| Message ID | 20210811025801.21597-14-yunfei.dong@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Using component framework to support multi hardware decode | expand |
On Wed, 11 Aug 2021 10:57:59 +0800, Yunfei Dong wrote: > Adds decoder dt-bindings for mt8192. > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > --- > v5: no changes > > This patch depends on "Mediatek MT8192 clock support"[1]. > > The definition of decoder clocks are in mt8192-clk.h, need to include them in case of build fail [1]. > > [1]https://patchwork.kernel.org/project/linux-mediatek/list/?series=511175 > --- > .../media/mediatek,vcodec-comp-decoder.yaml | 172 ++++++++++++++++++ > 1 file changed, 172 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: ./Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml: $id: relative path/filename doesn't match actual path or filename expected: http://devicetree.org/schemas/media/mediatek,vcodec-comp-decoder.yaml# Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.example.dts:22:18: fatal error: dt-bindings/clock/mt8192-clk.h: No such file or directory 22 | #include <dt-bindings/clock/mt8192-clk.h> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. make[1]: *** [scripts/Makefile.lib:380: Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.example.dt.yaml] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1419: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1515556 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On Wed, Aug 11, 2021 at 10:57:59AM +0800, Yunfei Dong wrote: > Adds decoder dt-bindings for mt8192. > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > --- > v5: no changes > > This patch depends on "Mediatek MT8192 clock support"[1]. > > The definition of decoder clocks are in mt8192-clk.h, need to include them in case of build fail [1]. > > [1]https://patchwork.kernel.org/project/linux-mediatek/list/?series=511175 > --- > .../media/mediatek,vcodec-comp-decoder.yaml | 172 ++++++++++++++++++ > 1 file changed, 172 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml > > diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml > new file mode 100644 > index 000000000000..083c89933917 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml > @@ -0,0 +1,172 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iommu/mediatek,vcodec-comp-decoder.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Mediatek Video Decode Accelerator With Component > + > +maintainers: > + - Yunfei Dong <yunfei.dong@mediatek.com> > + > +description: |+ > + Mediatek Video Decode is the video decode hardware present in Mediatek > + SoCs which supports high resolution decoding functionalities. Required > + master and component node. > + > +properties: > + compatible: > + oneOf: Don't need 'oneOf' when only one entry. > + - enum: > + - mediatek,mt8192-vcodec-dec # for lat hardware > + - mediatek,mtk-vcodec-lat # for core hardware > + - mediatek,mtk-vcodec-core > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + clocks: > + maxItems: 5 > + > + clock-names: > + items: > + - const: vdec-sel > + - const: vdec-soc-vdec > + - const: vdec-soc-lat > + - const: vdec-vdec > + - const: vdec-top > + > + assigned-clocks: true > + > + assigned-clock-parents: true Don't need these 2. Always valid if 'clocks' is present. > + > + power-domains: > + maxItems: 1 > + > + iommus: > + minItems: 1 > + maxItems: 32 > + description: | > + List of the hardware port in respective IOMMU block for current Socs. > + Refer to bindings/iommu/mediatek,iommu.yaml. > + > + dma-ranges: > + maxItems: 1 > + description: | > + Describes the physical address space of IOMMU maps to memory. > + > + mediatek,scp: > + $ref: /schemas/types.yaml#/definitions/phandle > + maxItems: 1 > + description: > + Describes point to scp. > + > +required: > + - compatible > + - reg > + - iommus > + - dma-ranges > + > +allOf: > + - if: #master node > + properties: > + compatible: > + contains: > + enum: > + - mediatek,mt8192-vcodec-dec # for lat hardware > + > + then: > + required: > + - mediatek,scp > + > + - if: #component node > + properties: > + compatible: > + contains: > + enum: > + - mediatek,mtk-vcodec-lat # for core hardware > + - mediatek,mtk-vcodec-core > + > + then: > + required: > + - interrupts > + - clocks > + - clock-names > + - assigned-clocks > + - assigned-clock-parents > + - power-domains > + > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/memory/mt8192-larb-port.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/clock/mt8192-clk.h> > + #include <dt-bindings/power/mt8192-power.h> > + > + vcodec_dec: vcodec_dec@16000000 { > + compatible = "mediatek,mt8192-vcodec-dec"; > + reg = <0 0x16000000 0 0x1000>; /* VDEC_SYS */ > + mediatek,scp = <&scp>; > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > + }; > + > + vcodec_lat: vcodec_lat@0x16010000 { > + compatible = "mediatek,mtk-vcodec-lat"; > + reg = <0 0x16010000 0 0x800>; /* VDEC_MISC */ > + interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; > + iommus = <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD_EXT>, > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD2_EXT>, > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_AVC_MV_EXT>, > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_PRED_RD_EXT>, > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_TILE_EXT>, > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_WDMA_EXT>, > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_RG_CTRL_DMA_EXT>, > + <&iommu0 M4U_PORT_L5_VDEC_UFO_ENC_EXT>; > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > + <&vdecsys_soc CLK_VDEC_SOC_VDEC>, > + <&vdecsys_soc CLK_VDEC_SOC_LAT>, > + <&vdecsys_soc CLK_VDEC_SOC_LARB1>, > + <&topckgen CLK_TOP_MAINPLL_D4>; > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-lat", > + "vdec-vdec", "vdec-top"; > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > + assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>; > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC>; > + }; > + > + vcodec_core: vcodec_core@0x16025000 { > + compatible = "mediatek,mtk-vcodec-core"; > + reg = <0 0x16025000 0 0x1000>; /* VDEC_CORE_MISC */ > + interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_UFO_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_PP_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_PRED_RD_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_PRED_WR_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_PPWRAP_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_TILE_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_VLD_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_VLD2_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_AVC_MV_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_RG_CTRL_DMA_EXT>; > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > + <&vdecsys CLK_VDEC_VDEC>, > + <&vdecsys CLK_VDEC_LAT>, > + <&vdecsys CLK_VDEC_LARB1>, > + <&topckgen CLK_TOP_MAINPLL_D4>; > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-lat", > + "vdec-vdec", "vdec-top"; > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > + assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>; > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC2>; > + }; > -- > 2.25.1 > >
Hi Yunfei, Thank you for the patch. On Wed, Aug 11, 2021 at 10:57:59AM +0800, Yunfei Dong wrote: > Adds decoder dt-bindings for mt8192. > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > --- > v5: no changes > > This patch depends on "Mediatek MT8192 clock support"[1]. > > The definition of decoder clocks are in mt8192-clk.h, need to include them in case of build fail [1]. > > [1]https://patchwork.kernel.org/project/linux-mediatek/list/?series=511175 > --- > .../media/mediatek,vcodec-comp-decoder.yaml | 172 ++++++++++++++++++ > 1 file changed, 172 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml > > diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml > new file mode 100644 > index 000000000000..083c89933917 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml > @@ -0,0 +1,172 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iommu/mediatek,vcodec-comp-decoder.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Mediatek Video Decode Accelerator With Component > + > +maintainers: > + - Yunfei Dong <yunfei.dong@mediatek.com> > + > +description: |+ > + Mediatek Video Decode is the video decode hardware present in Mediatek > + SoCs which supports high resolution decoding functionalities. Required > + master and component node. This should explain how the three IP cores relate to each other. > + > +properties: > + compatible: > + oneOf: > + - enum: > + - mediatek,mt8192-vcodec-dec # for lat hardware > + - mediatek,mtk-vcodec-lat # for core hardware > + - mediatek,mtk-vcodec-core > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + clocks: > + maxItems: 5 > + > + clock-names: > + items: > + - const: vdec-sel > + - const: vdec-soc-vdec > + - const: vdec-soc-lat > + - const: vdec-vdec > + - const: vdec-top > + > + assigned-clocks: true > + > + assigned-clock-parents: true > + > + power-domains: > + maxItems: 1 > + > + iommus: > + minItems: 1 > + maxItems: 32 > + description: | > + List of the hardware port in respective IOMMU block for current Socs. > + Refer to bindings/iommu/mediatek,iommu.yaml. > + > + dma-ranges: > + maxItems: 1 > + description: | > + Describes the physical address space of IOMMU maps to memory. > + > + mediatek,scp: > + $ref: /schemas/types.yaml#/definitions/phandle > + maxItems: 1 > + description: > + Describes point to scp. > + > +required: > + - compatible > + - reg > + - iommus > + - dma-ranges > + > +allOf: > + - if: #master node > + properties: > + compatible: > + contains: > + enum: > + - mediatek,mt8192-vcodec-dec # for lat hardware > + > + then: > + required: > + - mediatek,scp > + > + - if: #component node > + properties: > + compatible: > + contains: > + enum: > + - mediatek,mtk-vcodec-lat # for core hardware > + - mediatek,mtk-vcodec-core > + > + then: > + required: > + - interrupts > + - clocks > + - clock-names > + - assigned-clocks > + - assigned-clock-parents > + - power-domains > + > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/memory/mt8192-larb-port.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/clock/mt8192-clk.h> > + #include <dt-bindings/power/mt8192-power.h> > + > + vcodec_dec: vcodec_dec@16000000 { > + compatible = "mediatek,mt8192-vcodec-dec"; > + reg = <0 0x16000000 0 0x1000>; /* VDEC_SYS */ > + mediatek,scp = <&scp>; > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > + }; > + > + vcodec_lat: vcodec_lat@0x16010000 { > + compatible = "mediatek,mtk-vcodec-lat"; > + reg = <0 0x16010000 0 0x800>; /* VDEC_MISC */ > + interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; > + iommus = <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD_EXT>, > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD2_EXT>, > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_AVC_MV_EXT>, > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_PRED_RD_EXT>, > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_TILE_EXT>, > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_WDMA_EXT>, > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_RG_CTRL_DMA_EXT>, > + <&iommu0 M4U_PORT_L5_VDEC_UFO_ENC_EXT>; > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > + <&vdecsys_soc CLK_VDEC_SOC_VDEC>, > + <&vdecsys_soc CLK_VDEC_SOC_LAT>, > + <&vdecsys_soc CLK_VDEC_SOC_LARB1>, > + <&topckgen CLK_TOP_MAINPLL_D4>; > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-lat", > + "vdec-vdec", "vdec-top"; > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > + assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>; > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC>; > + }; > + > + vcodec_core: vcodec_core@0x16025000 { > + compatible = "mediatek,mtk-vcodec-core"; > + reg = <0 0x16025000 0 0x1000>; /* VDEC_CORE_MISC */ > + interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_UFO_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_PP_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_PRED_RD_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_PRED_WR_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_PPWRAP_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_TILE_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_VLD_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_VLD2_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_AVC_MV_EXT>, > + <&iommu0 M4U_PORT_L4_VDEC_RG_CTRL_DMA_EXT>; > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > + <&vdecsys CLK_VDEC_VDEC>, > + <&vdecsys CLK_VDEC_LAT>, > + <&vdecsys CLK_VDEC_LARB1>, > + <&topckgen CLK_TOP_MAINPLL_D4>; > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-lat", > + "vdec-vdec", "vdec-top"; > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > + assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>; > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC2>; > + }; I'm a bit late in the game, reviewing v5 only, but I'm wondering if those IP cores need to be modelled in separate nodes. It would be much easier, from a software point of view, to have a single node, with multiple register ranges. Are some of those IP cores used in different SoCs, combined in different ways, that make a modular design better ?
Hi Rob, Thanks for your detail suggestion. For the header file(dt-bindings/clock/mt8192-clk.h) is included in: https://patchwork.kernel.org/project/linux-mediatek/list/?series=511175 Need to depend on this patch, do you have any suggestion about how to add it? Thanks. On Wed, 2021-08-11 at 11:24 -0600, Rob Herring wrote: > On Wed, 11 Aug 2021 10:57:59 +0800, Yunfei Dong wrote: > > Adds decoder dt-bindings for mt8192. > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > --- > > v5: no changes > > > > This patch depends on "Mediatek MT8192 clock support"[1]. > > > > The definition of decoder clocks are in mt8192-clk.h, need to > > include them in case of build fail [1]. > > > > [1] > > https://patchwork.kernel.org/project/linux-mediatek/list/?series=511175 > > --- > > .../media/mediatek,vcodec-comp-decoder.yaml | 172 > > ++++++++++++++++++ > > 1 file changed, 172 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > decoder.yaml > > > > My bot found errors running 'make DT_CHECKER_FLAGS=-m > dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > ./Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > decoder.yaml: $id: relative path/filename doesn't match actual path > or filename > expected: > http://devicetree.org/schemas/media/mediatek,vcodec-comp-decoder.yaml# > Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > decoder.example.dts:22:18: fatal error: dt-bindings/clock/mt8192- > clk.h: No such file or directory > 22 | #include <dt-bindings/clock/mt8192-clk.h> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > compilation terminated. > make[1]: *** [scripts/Makefile.lib:380: > Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > decoder.example.dt.yaml] Error 1 > make[1]: *** Waiting for unfinished jobs.... > make: *** [Makefile:1419: dt_binding_check] Error 2 > > doc reference errors (make refcheckdocs): > > See https://patchwork.ozlabs.org/patch/1515556 > > This check can fail if there are any dependencies. The base for a > patch > series is generally the most recent rc1. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up > to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit. >
Hi Laurent, Thanks for your detail suggestion. On Wed, 2021-08-11 at 20:59 +0300, Laurent Pinchart wrote: > Hi Yunfei, > > Thank you for the patch. > > On Wed, Aug 11, 2021 at 10:57:59AM +0800, Yunfei Dong wrote: > > Adds decoder dt-bindings for mt8192. > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > --- > > v5: no changes > > > > This patch depends on "Mediatek MT8192 clock support"[1]. > > > > The definition of decoder clocks are in mt8192-clk.h, need to > > include them in case of build fail [1]. > > > > [1] > > https://patchwork.kernel.org/project/linux-mediatek/list/?series=511175 > > --- > > .../media/mediatek,vcodec-comp-decoder.yaml | 172 > > ++++++++++++++++++ > > 1 file changed, 172 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > decoder.yaml > > > > diff --git > > a/Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > decoder.yaml > > b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > decoder.yaml > > new file mode 100644 > > index 000000000000..083c89933917 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > decoder.yaml > > @@ -0,0 +1,172 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: > > http://devicetree.org/schemas/iommu/mediatek,vcodec-comp-decoder.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Mediatek Video Decode Accelerator With Component > > + > > +maintainers: > > + - Yunfei Dong <yunfei.dong@mediatek.com> > > + > > +description: |+ > > + Mediatek Video Decode is the video decode hardware present in > > Mediatek > > + SoCs which supports high resolution decoding functionalities. > > Required > > + master and component node. > > This should explain how the three IP cores relate to each other. > I will explain it in next patch. > > + > > +properties: > > + compatible: > > + oneOf: > > + - enum: > > + - mediatek,mt8192-vcodec-dec # for lat hardware > > + - mediatek,mtk-vcodec-lat # for core hardware > > + - mediatek,mtk-vcodec-core > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 5 > > + > > + clock-names: > > + items: > > + - const: vdec-sel > > + - const: vdec-soc-vdec > > + - const: vdec-soc-lat > > + - const: vdec-vdec > > + - const: vdec-top > > + > > + assigned-clocks: true > > + > > + assigned-clock-parents: true > > + > > + power-domains: > > + maxItems: 1 > > + > > + iommus: > > + minItems: 1 > > + maxItems: 32 > > + description: | > > + List of the hardware port in respective IOMMU block for > > current Socs. > > + Refer to bindings/iommu/mediatek,iommu.yaml. > > + > > + dma-ranges: > > + maxItems: 1 > > + description: | > > + Describes the physical address space of IOMMU maps to > > memory. > > + > > + mediatek,scp: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + maxItems: 1 > > + description: > > + Describes point to scp. > > + > > +required: > > + - compatible > > + - reg > > + - iommus > > + - dma-ranges > > + > > +allOf: > > + - if: #master node > > + properties: > > + compatible: > > + contains: > > + enum: > > + - mediatek,mt8192-vcodec-dec # for lat hardware > > + > > + then: > > + required: > > + - mediatek,scp > > + > > + - if: #component node > > + properties: > > + compatible: > > + contains: > > + enum: > > + - mediatek,mtk-vcodec-lat # for core hardware > > + - mediatek,mtk-vcodec-core > > + > > + then: > > + required: > > + - interrupts > > + - clocks > > + - clock-names > > + - assigned-clocks > > + - assigned-clock-parents > > + - power-domains > > + > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/memory/mt8192-larb-port.h> > > + #include <dt-bindings/interrupt-controller/irq.h> > > + #include <dt-bindings/clock/mt8192-clk.h> > > + #include <dt-bindings/power/mt8192-power.h> > > + > > + vcodec_dec: vcodec_dec@16000000 { > > + compatible = "mediatek,mt8192-vcodec-dec"; > > + reg = <0 0x16000000 0 0x1000>; /* VDEC_SYS */ > > + mediatek,scp = <&scp>; > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > + }; > > + > > + vcodec_lat: vcodec_lat@0x16010000 { > > + compatible = "mediatek,mtk-vcodec-lat"; > > + reg = <0 0x16010000 0 0x800>; /* VDEC_MISC */ > > + interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; > > + iommus = <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD_EXT>, > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD2_EXT>, > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_AVC_MV_EXT>, > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_PRED_RD_EXT>, > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_TILE_EXT>, > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_WDMA_EXT>, > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_RG_CTRL_DMA_EXT>, > > + <&iommu0 M4U_PORT_L5_VDEC_UFO_ENC_EXT>; > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > + <&vdecsys_soc CLK_VDEC_SOC_VDEC>, > > + <&vdecsys_soc CLK_VDEC_SOC_LAT>, > > + <&vdecsys_soc CLK_VDEC_SOC_LARB1>, > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-lat", > > + "vdec-vdec", "vdec-top"; > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > + assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>; > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC>; > > + }; > > + > > + vcodec_core: vcodec_core@0x16025000 { > > + compatible = "mediatek,mtk-vcodec-core"; > > + reg = <0 0x16025000 0 0x1000>; /* > > VDEC_CORE_MISC */ > > + interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_UFO_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_PP_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_RD_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_WR_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_PPWRAP_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_TILE_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_VLD_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_VLD2_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_AVC_MV_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_RG_CTRL_DMA_EXT>; > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > + <&vdecsys CLK_VDEC_VDEC>, > > + <&vdecsys CLK_VDEC_LAT>, > > + <&vdecsys CLK_VDEC_LARB1>, > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-lat", > > + "vdec-vdec", "vdec-top"; > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > + assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>; > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC2>; > > + }; > > I'm a bit late in the game, reviewing v5 only, but I'm wondering if > those IP cores need to be modelled in separate nodes. It would be > much > easier, from a software point of view, to have a single node, with > multiple register ranges. > > Are some of those IP cores used in different SoCs, combined in > different > ways, that make a modular design better ? > Different platform has different hardware, for mt8192 only has three nodes. but mt8195 will has five nodes. and the clk/power/irq/iommu are different. It is not easy to manage all hardware at the same time in one node, need to enable different hardware at the same time, the logic will be very complex. It is much easier to handle different hardware using component, enable different hardware when we need it.
On Wed, 11 Aug 2021 at 14:59, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Yunfei, > > Thank you for the patch. > > On Wed, Aug 11, 2021 at 10:57:59AM +0800, Yunfei Dong wrote: > > Adds decoder dt-bindings for mt8192. > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > --- > > v5: no changes > > > > This patch depends on "Mediatek MT8192 clock support"[1]. > > > > The definition of decoder clocks are in mt8192-clk.h, need to include them in case of build fail [1]. > > > > [1]https://patchwork.kernel.org/project/linux-mediatek/list/?series=511175 > > --- > > .../media/mediatek,vcodec-comp-decoder.yaml | 172 ++++++++++++++++++ > > 1 file changed, 172 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml > > new file mode 100644 > > index 000000000000..083c89933917 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml > > @@ -0,0 +1,172 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/iommu/mediatek,vcodec-comp-decoder.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Mediatek Video Decode Accelerator With Component > > + > > +maintainers: > > + - Yunfei Dong <yunfei.dong@mediatek.com> > > + > > +description: |+ > > + Mediatek Video Decode is the video decode hardware present in Mediatek > > + SoCs which supports high resolution decoding functionalities. Required > > + master and component node. > > This should explain how the three IP cores relate to each other. > > > + > > +properties: > > + compatible: > > + oneOf: > > + - enum: > > + - mediatek,mt8192-vcodec-dec # for lat hardware > > + - mediatek,mtk-vcodec-lat # for core hardware > > + - mediatek,mtk-vcodec-core > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 5 > > + > > + clock-names: > > + items: > > + - const: vdec-sel > > + - const: vdec-soc-vdec > > + - const: vdec-soc-lat > > + - const: vdec-vdec > > + - const: vdec-top > > + > > + assigned-clocks: true > > + > > + assigned-clock-parents: true > > + > > + power-domains: > > + maxItems: 1 > > + > > + iommus: > > + minItems: 1 > > + maxItems: 32 > > + description: | > > + List of the hardware port in respective IOMMU block for current Socs. > > + Refer to bindings/iommu/mediatek,iommu.yaml. > > + > > + dma-ranges: > > + maxItems: 1 > > + description: | > > + Describes the physical address space of IOMMU maps to memory. > > + > > + mediatek,scp: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + maxItems: 1 > > + description: > > + Describes point to scp. > > + > > +required: > > + - compatible > > + - reg > > + - iommus > > + - dma-ranges > > + > > +allOf: > > + - if: #master node > > + properties: > > + compatible: > > + contains: > > + enum: > > + - mediatek,mt8192-vcodec-dec # for lat hardware > > + > > + then: > > + required: > > + - mediatek,scp > > + > > + - if: #component node > > + properties: > > + compatible: > > + contains: > > + enum: > > + - mediatek,mtk-vcodec-lat # for core hardware > > + - mediatek,mtk-vcodec-core > > + > > + then: > > + required: > > + - interrupts > > + - clocks > > + - clock-names > > + - assigned-clocks > > + - assigned-clock-parents > > + - power-domains > > + > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/memory/mt8192-larb-port.h> > > + #include <dt-bindings/interrupt-controller/irq.h> > > + #include <dt-bindings/clock/mt8192-clk.h> > > + #include <dt-bindings/power/mt8192-power.h> > > + > > + vcodec_dec: vcodec_dec@16000000 { > > + compatible = "mediatek,mt8192-vcodec-dec"; > > + reg = <0 0x16000000 0 0x1000>; /* VDEC_SYS */ > > + mediatek,scp = <&scp>; > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > + }; > > + > > + vcodec_lat: vcodec_lat@0x16010000 { > > + compatible = "mediatek,mtk-vcodec-lat"; > > + reg = <0 0x16010000 0 0x800>; /* VDEC_MISC */ > > + interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; > > + iommus = <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD_EXT>, > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD2_EXT>, > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_AVC_MV_EXT>, > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_PRED_RD_EXT>, > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_TILE_EXT>, > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_WDMA_EXT>, > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_RG_CTRL_DMA_EXT>, > > + <&iommu0 M4U_PORT_L5_VDEC_UFO_ENC_EXT>; > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > + <&vdecsys_soc CLK_VDEC_SOC_VDEC>, > > + <&vdecsys_soc CLK_VDEC_SOC_LAT>, > > + <&vdecsys_soc CLK_VDEC_SOC_LARB1>, > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-lat", > > + "vdec-vdec", "vdec-top"; > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > + assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>; > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC>; > > + }; > > + > > + vcodec_core: vcodec_core@0x16025000 { > > + compatible = "mediatek,mtk-vcodec-core"; > > + reg = <0 0x16025000 0 0x1000>; /* VDEC_CORE_MISC */ > > + interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_UFO_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_PP_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_RD_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_WR_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_PPWRAP_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_TILE_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_VLD_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_VLD2_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_AVC_MV_EXT>, > > + <&iommu0 M4U_PORT_L4_VDEC_RG_CTRL_DMA_EXT>; > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > + <&vdecsys CLK_VDEC_VDEC>, > > + <&vdecsys CLK_VDEC_LAT>, > > + <&vdecsys CLK_VDEC_LARB1>, > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-lat", > > + "vdec-vdec", "vdec-top"; > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > + assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>; > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC2>; > > + }; > > I'm a bit late in the game, reviewing v5 only, but I'm wondering if > those IP cores need to be modelled in separate nodes. It would be much > easier, from a software point of view, to have a single node, with > multiple register ranges. > > Are some of those IP cores used in different SoCs, combined in different > ways, that make a modular design better ? > Yeah, I agree with Laurent here. This way of modelling the different parts of the CODEC as different pieces is the reason why you need a framework to pull them together, such as the component API (I guess v4l2-async could have been used as well). I normally don't bother with driver internals, as its up to each driver author to decide what is best. However, I believe this design is too convoluted, and it may lead other developers to follow the same pattern, so please avoid it. There are several ways of modelling this and initializing or probing the sub-devices, without using any async framework. Thanks, Ezequiel
On Tue, 17 Aug 2021 at 00:52, yunfei.dong@mediatek.com <yunfei.dong@mediatek.com> wrote: > > Hi Laurent, > > Thanks for your detail suggestion. > > On Wed, 2021-08-11 at 20:59 +0300, Laurent Pinchart wrote: > > Hi Yunfei, > > > > Thank you for the patch. > > > > On Wed, Aug 11, 2021 at 10:57:59AM +0800, Yunfei Dong wrote: > > > Adds decoder dt-bindings for mt8192. > > > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > > --- > > > v5: no changes > > > > > > This patch depends on "Mediatek MT8192 clock support"[1]. > > > > > > The definition of decoder clocks are in mt8192-clk.h, need to > > > include them in case of build fail [1]. > > > > > > [1] > > > https://patchwork.kernel.org/project/linux-mediatek/list/?series=511175 > > > --- > > > .../media/mediatek,vcodec-comp-decoder.yaml | 172 > > > ++++++++++++++++++ > > > 1 file changed, 172 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > > decoder.yaml > > > > > > diff --git > > > a/Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > > decoder.yaml > > > b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > > decoder.yaml > > > new file mode 100644 > > > index 000000000000..083c89933917 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > > decoder.yaml > > > @@ -0,0 +1,172 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: > > > http://devicetree.org/schemas/iommu/mediatek,vcodec-comp-decoder.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Mediatek Video Decode Accelerator With Component > > > + > > > +maintainers: > > > + - Yunfei Dong <yunfei.dong@mediatek.com> > > > + > > > +description: |+ > > > + Mediatek Video Decode is the video decode hardware present in > > > Mediatek > > > + SoCs which supports high resolution decoding functionalities. > > > Required > > > + master and component node. > > > > This should explain how the three IP cores relate to each other. > > > I will explain it in next patch. > > > + > > > +properties: > > > + compatible: > > > + oneOf: > > > + - enum: > > > + - mediatek,mt8192-vcodec-dec # for lat hardware > > > + - mediatek,mtk-vcodec-lat # for core hardware > > > + - mediatek,mtk-vcodec-core > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + interrupts: > > > + maxItems: 1 > > > + > > > + clocks: > > > + maxItems: 5 > > > + > > > + clock-names: > > > + items: > > > + - const: vdec-sel > > > + - const: vdec-soc-vdec > > > + - const: vdec-soc-lat > > > + - const: vdec-vdec > > > + - const: vdec-top > > > + > > > + assigned-clocks: true > > > + > > > + assigned-clock-parents: true > > > + > > > + power-domains: > > > + maxItems: 1 > > > + > > > + iommus: > > > + minItems: 1 > > > + maxItems: 32 > > > + description: | > > > + List of the hardware port in respective IOMMU block for > > > current Socs. > > > + Refer to bindings/iommu/mediatek,iommu.yaml. > > > + > > > + dma-ranges: > > > + maxItems: 1 > > > + description: | > > > + Describes the physical address space of IOMMU maps to > > > memory. > > > + > > > + mediatek,scp: > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > + maxItems: 1 > > > + description: > > > + Describes point to scp. > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - iommus > > > + - dma-ranges > > > + > > > +allOf: > > > + - if: #master node > > > + properties: > > > + compatible: > > > + contains: > > > + enum: > > > + - mediatek,mt8192-vcodec-dec # for lat hardware > > > + > > > + then: > > > + required: > > > + - mediatek,scp > > > + > > > + - if: #component node > > > + properties: > > > + compatible: > > > + contains: > > > + enum: > > > + - mediatek,mtk-vcodec-lat # for core hardware > > > + - mediatek,mtk-vcodec-core > > > + > > > + then: > > > + required: > > > + - interrupts > > > + - clocks > > > + - clock-names > > > + - assigned-clocks > > > + - assigned-clock-parents > > > + - power-domains > > > + > > > + > > > +additionalProperties: false > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > > + #include <dt-bindings/memory/mt8192-larb-port.h> > > > + #include <dt-bindings/interrupt-controller/irq.h> > > > + #include <dt-bindings/clock/mt8192-clk.h> > > > + #include <dt-bindings/power/mt8192-power.h> > > > + > > > + vcodec_dec: vcodec_dec@16000000 { > > > + compatible = "mediatek,mt8192-vcodec-dec"; > > > + reg = <0 0x16000000 0 0x1000>; /* VDEC_SYS */ > > > + mediatek,scp = <&scp>; > > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > + }; > > > + > > > + vcodec_lat: vcodec_lat@0x16010000 { > > > + compatible = "mediatek,mtk-vcodec-lat"; > > > + reg = <0 0x16010000 0 0x800>; /* VDEC_MISC */ > > > + interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; > > > + iommus = <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD_EXT>, > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD2_EXT>, > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_AVC_MV_EXT>, > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_PRED_RD_EXT>, > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_TILE_EXT>, > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_WDMA_EXT>, > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_RG_CTRL_DMA_EXT>, > > > + <&iommu0 M4U_PORT_L5_VDEC_UFO_ENC_EXT>; > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > > + <&vdecsys_soc CLK_VDEC_SOC_VDEC>, > > > + <&vdecsys_soc CLK_VDEC_SOC_LAT>, > > > + <&vdecsys_soc CLK_VDEC_SOC_LARB1>, > > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-lat", > > > + "vdec-vdec", "vdec-top"; > > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > > + assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>; > > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC>; > > > + }; > > > + > > > + vcodec_core: vcodec_core@0x16025000 { > > > + compatible = "mediatek,mtk-vcodec-core"; > > > + reg = <0 0x16025000 0 0x1000>; /* > > > VDEC_CORE_MISC */ > > > + interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; > > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_UFO_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_PP_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_RD_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_WR_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_PPWRAP_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_TILE_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_VLD_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_VLD2_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_AVC_MV_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_RG_CTRL_DMA_EXT>; > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > > + <&vdecsys CLK_VDEC_VDEC>, > > > + <&vdecsys CLK_VDEC_LAT>, > > > + <&vdecsys CLK_VDEC_LARB1>, > > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-lat", > > > + "vdec-vdec", "vdec-top"; > > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > > + assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>; > > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC2>; > > > + }; > > > > I'm a bit late in the game, reviewing v5 only, but I'm wondering if > > those IP cores need to be modelled in separate nodes. It would be > > much > > easier, from a software point of view, to have a single node, with > > multiple register ranges. > > > > Are some of those IP cores used in different SoCs, combined in > > different > > ways, that make a modular design better ? > > > Different platform has different hardware, for mt8192 only has three > nodes. but mt8195 will has five nodes. and the clk/power/irq/iommu are > different. It is not easy to manage all hardware at the same time in > one node, need to enable different hardware at the same time, the logic > will be very complex. > It is much easier to handle different hardware using component, enable > different hardware when we need it. > > You can still have one device-tree node for each device, which means you can still manage your resources (clk/power/irq/iommu) easily, but doing this so that it avoids an async framework to pull the parts together. I gave you this feedback several times, and you have been objecting it every time without even trying to consider a different approach. Thanks, Ezequiel
On Sun, 2021-08-29 at 17:50 -0300, Ezequiel Garcia wrote: > On Wed, 11 Aug 2021 at 14:59, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Yunfei, > > > > Thank you for the patch. > > > > On Wed, Aug 11, 2021 at 10:57:59AM +0800, Yunfei Dong wrote: > > > Adds decoder dt-bindings for mt8192. > > > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > > --- > > > v5: no changes > > > > > > This patch depends on "Mediatek MT8192 clock support"[1]. > > > > > > The definition of decoder clocks are in mt8192-clk.h, need to > > > include them in case of build fail [1]. > > > > > > [1] > > > https://patchwork.kernel.org/project/linux-mediatek/list/?series=511175 > > > --- > > > .../media/mediatek,vcodec-comp-decoder.yaml | 172 > > > ++++++++++++++++++ > > > 1 file changed, 172 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > > decoder.yaml > > > > > > diff --git > > > a/Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > > decoder.yaml > > > b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > > decoder.yaml > > > new file mode 100644 > > > index 000000000000..083c89933917 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec- > > > comp-decoder.yaml > > > @@ -0,0 +1,172 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: > > > http://devicetree.org/schemas/iommu/mediatek,vcodec-comp-decoder.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Mediatek Video Decode Accelerator With Component > > > + > > > +maintainers: > > > + - Yunfei Dong <yunfei.dong@mediatek.com> > > > + > > > +description: |+ > > > + Mediatek Video Decode is the video decode hardware present in > > > Mediatek > > > + SoCs which supports high resolution decoding functionalities. > > > Required > > > + master and component node. > > > > This should explain how the three IP cores relate to each other. > > > > > + > > > +properties: > > > + compatible: > > > + oneOf: > > > + - enum: > > > + - mediatek,mt8192-vcodec-dec # for lat hardware > > > + - mediatek,mtk-vcodec-lat # for core hardware > > > + - mediatek,mtk-vcodec-core > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + interrupts: > > > + maxItems: 1 > > > + > > > + clocks: > > > + maxItems: 5 > > > + > > > + clock-names: > > > + items: > > > + - const: vdec-sel > > > + - const: vdec-soc-vdec > > > + - const: vdec-soc-lat > > > + - const: vdec-vdec > > > + - const: vdec-top > > > + > > > + assigned-clocks: true > > > + > > > + assigned-clock-parents: true > > > + > > > + power-domains: > > > + maxItems: 1 > > > + > > > + iommus: > > > + minItems: 1 > > > + maxItems: 32 > > > + description: | > > > + List of the hardware port in respective IOMMU block for > > > current Socs. > > > + Refer to bindings/iommu/mediatek,iommu.yaml. > > > + > > > + dma-ranges: > > > + maxItems: 1 > > > + description: | > > > + Describes the physical address space of IOMMU maps to > > > memory. > > > + > > > + mediatek,scp: > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > + maxItems: 1 > > > + description: > > > + Describes point to scp. > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - iommus > > > + - dma-ranges > > > + > > > +allOf: > > > + - if: #master node > > > + properties: > > > + compatible: > > > + contains: > > > + enum: > > > + - mediatek,mt8192-vcodec-dec # for lat hardware > > > + > > > + then: > > > + required: > > > + - mediatek,scp > > > + > > > + - if: #component node > > > + properties: > > > + compatible: > > > + contains: > > > + enum: > > > + - mediatek,mtk-vcodec-lat # for core hardware > > > + - mediatek,mtk-vcodec-core > > > + > > > + then: > > > + required: > > > + - interrupts > > > + - clocks > > > + - clock-names > > > + - assigned-clocks > > > + - assigned-clock-parents > > > + - power-domains > > > + > > > + > > > +additionalProperties: false > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > > + #include <dt-bindings/memory/mt8192-larb-port.h> > > > + #include <dt-bindings/interrupt-controller/irq.h> > > > + #include <dt-bindings/clock/mt8192-clk.h> > > > + #include <dt-bindings/power/mt8192-power.h> > > > + > > > + vcodec_dec: vcodec_dec@16000000 { > > > + compatible = "mediatek,mt8192-vcodec-dec"; > > > + reg = <0 0x16000000 0 0x1000>; /* VDEC_SYS > > > */ > > > + mediatek,scp = <&scp>; > > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > + }; > > > + > > > + vcodec_lat: vcodec_lat@0x16010000 { > > > + compatible = "mediatek,mtk-vcodec-lat"; > > > + reg = <0 0x16010000 0 0x800>; /* > > > VDEC_MISC */ > > > + interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; > > > + iommus = <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD_EXT>, > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD2_EXT>, > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_AVC_MV_EXT>, > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_PRED_RD_EXT>, > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_TILE_EXT>, > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_WDMA_EXT>, > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_RG_CTRL_DMA_EXT>, > > > + <&iommu0 M4U_PORT_L5_VDEC_UFO_ENC_EXT>; > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > > + <&vdecsys_soc CLK_VDEC_SOC_VDEC>, > > > + <&vdecsys_soc CLK_VDEC_SOC_LAT>, > > > + <&vdecsys_soc CLK_VDEC_SOC_LARB1>, > > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc- > > > lat", > > > + "vdec-vdec", "vdec-top"; > > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > > + assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>; > > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC>; > > > + }; > > > + > > > + vcodec_core: vcodec_core@0x16025000 { > > > + compatible = "mediatek,mtk-vcodec-core"; > > > + reg = <0 0x16025000 0 0x1000>; /* > > > VDEC_CORE_MISC */ > > > + interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; > > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_UFO_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_PP_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_RD_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_WR_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_PPWRAP_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_TILE_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_VLD_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_VLD2_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_AVC_MV_EXT>, > > > + <&iommu0 M4U_PORT_L4_VDEC_RG_CTRL_DMA_EXT>; > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > > + <&vdecsys CLK_VDEC_VDEC>, > > > + <&vdecsys CLK_VDEC_LAT>, > > > + <&vdecsys CLK_VDEC_LARB1>, > > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc- > > > lat", > > > + "vdec-vdec", "vdec-top"; > > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > > + assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>; > > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC2>; > > > + }; > > > > I'm a bit late in the game, reviewing v5 only, but I'm wondering if > > those IP cores need to be modelled in separate nodes. It would be > > much > > easier, from a software point of view, to have a single node, with > > multiple register ranges. > > > > Are some of those IP cores used in different SoCs, combined in > > different > > ways, that make a modular design better ? > > > 1. Each a node should respect a HW node. Defining a complex node that contain multiple register is not better, for they belong to different hardware. Different platforms has different hardware count, mt8195 has five hardwares. 2. Another reason is from the IOMMU point, the vcodec HW include core and lat hardwares, each of them connect to the independent IOMMU hardware for mt8195, can't write all iommu ports together, or hardware can't access dram data, so we must separate them. > Yeah, I agree with Laurent here. This way of modelling the different > parts > of the CODEC as different pieces is the reason why you need a > framework > to pull them together, such as the component API (I guess v4l2-async > could have been used as well). > We must separate each hardware node, only master need to register video and media device, component just used for store clk/power/irq/register information for current hardware. > I normally don't bother with driver internals, as its up to each > driver > author to decide what is best. However, I believe this design is too > convoluted, and it may lead other developers to follow the same > pattern, > so please avoid it. > According to your information, it looks that component and v4l2 async are ok for this architecture, only component is too convoluted(iommu driver also use component). For we are not very familiar with v4l2 async, out architecture is designed for component include next forty patches. It is a very big change for out driver. I will try to spend a lot of time to change it. > There are several ways of modelling this and initializing or probing > the sub-devices, > without using any async framework. > You may misunderstand, I'm not objecting your suggestion every time, for our driver is designed for component api, need to spend a lot of time to change it in many platforms. I will change it if v4l2 async has many advantages then component as you said. I will try to use v4l2 async as compared. > Thanks, > Ezequiel Thanks for your suggestion. Yunfei Dong
Hi Yunfei, On Mon, Aug 30, 2021 at 02:07:44PM +0800, yunfei.dong@mediatek.com wrote: > On Sun, 2021-08-29 at 17:50 -0300, Ezequiel Garcia wrote: > > On Wed, 11 Aug 2021 at 14:59, Laurent Pinchart wrote: > > > On Wed, Aug 11, 2021 at 10:57:59AM +0800, Yunfei Dong wrote: > > > > Adds decoder dt-bindings for mt8192. > > > > > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > > > --- > > > > v5: no changes > > > > > > > > This patch depends on "Mediatek MT8192 clock support"[1]. > > > > > > > > The definition of decoder clocks are in mt8192-clk.h, need to > > > > include them in case of build fail [1]. > > > > > > > > [1] > > > > https://patchwork.kernel.org/project/linux-mediatek/list/?series=511175 > > > > --- > > > > .../media/mediatek,vcodec-comp-decoder.yaml | 172 ++++++++++++++++++ > > > > 1 file changed, 172 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml > > > > new file mode 100644 > > > > index 000000000000..083c89933917 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml > > > > @@ -0,0 +1,172 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/iommu/mediatek,vcodec-comp-decoder.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Mediatek Video Decode Accelerator With Component > > > > + > > > > +maintainers: > > > > + - Yunfei Dong <yunfei.dong@mediatek.com> > > > > + > > > > +description: |+ > > > > + Mediatek Video Decode is the video decode hardware present in Mediatek > > > > + SoCs which supports high resolution decoding functionalities. Required > > > > + master and component node. > > > > > > This should explain how the three IP cores relate to each other. > > > > > > > + > > > > +properties: > > > > + compatible: > > > > + oneOf: > > > > + - enum: > > > > + - mediatek,mt8192-vcodec-dec # for lat hardware > > > > + - mediatek,mtk-vcodec-lat # for core hardware > > > > + - mediatek,mtk-vcodec-core > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + interrupts: > > > > + maxItems: 1 > > > > + > > > > + clocks: > > > > + maxItems: 5 > > > > + > > > > + clock-names: > > > > + items: > > > > + - const: vdec-sel > > > > + - const: vdec-soc-vdec > > > > + - const: vdec-soc-lat > > > > + - const: vdec-vdec > > > > + - const: vdec-top > > > > + > > > > + assigned-clocks: true > > > > + > > > > + assigned-clock-parents: true > > > > + > > > > + power-domains: > > > > + maxItems: 1 > > > > + > > > > + iommus: > > > > + minItems: 1 > > > > + maxItems: 32 > > > > + description: | > > > > + List of the hardware port in respective IOMMU block for current Socs. > > > > + Refer to bindings/iommu/mediatek,iommu.yaml. > > > > + > > > > + dma-ranges: > > > > + maxItems: 1 > > > > + description: | > > > > + Describes the physical address space of IOMMU maps to memory. > > > > + > > > > + mediatek,scp: > > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > > + maxItems: 1 > > > > + description: > > > > + Describes point to scp. > > > > + > > > > +required: > > > > + - compatible > > > > + - reg > > > > + - iommus > > > > + - dma-ranges > > > > + > > > > +allOf: > > > > + - if: #master node > > > > + properties: > > > > + compatible: > > > > + contains: > > > > + enum: > > > > + - mediatek,mt8192-vcodec-dec # for lat hardware > > > > + > > > > + then: > > > > + required: > > > > + - mediatek,scp > > > > + > > > > + - if: #component node > > > > + properties: > > > > + compatible: > > > > + contains: > > > > + enum: > > > > + - mediatek,mtk-vcodec-lat # for core hardware > > > > + - mediatek,mtk-vcodec-core > > > > + > > > > + then: > > > > + required: > > > > + - interrupts > > > > + - clocks > > > > + - clock-names > > > > + - assigned-clocks > > > > + - assigned-clock-parents > > > > + - power-domains > > > > + > > > > + > > > > +additionalProperties: false > > > > + > > > > +examples: > > > > + - | > > > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > + #include <dt-bindings/memory/mt8192-larb-port.h> > > > > + #include <dt-bindings/interrupt-controller/irq.h> > > > > + #include <dt-bindings/clock/mt8192-clk.h> > > > > + #include <dt-bindings/power/mt8192-power.h> > > > > + > > > > + vcodec_dec: vcodec_dec@16000000 { > > > > + compatible = "mediatek,mt8192-vcodec-dec"; > > > > + reg = <0 0x16000000 0 0x1000>; /* VDEC_SYS */ > > > > + mediatek,scp = <&scp>; > > > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; > > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > > + }; > > > > + > > > > + vcodec_lat: vcodec_lat@0x16010000 { > > > > + compatible = "mediatek,mtk-vcodec-lat"; > > > > + reg = <0 0x16010000 0 0x800>; /* VDEC_MISC */ > > > > + interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; > > > > + iommus = <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD_EXT>, > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD2_EXT>, > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_AVC_MV_EXT>, > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_PRED_RD_EXT>, > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_TILE_EXT>, > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_WDMA_EXT>, > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_RG_CTRL_DMA_EXT>, > > > > + <&iommu0 M4U_PORT_L5_VDEC_UFO_ENC_EXT>; > > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > > > + <&vdecsys_soc CLK_VDEC_SOC_VDEC>, > > > > + <&vdecsys_soc CLK_VDEC_SOC_LAT>, > > > > + <&vdecsys_soc CLK_VDEC_SOC_LARB1>, > > > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-lat", > > > > + "vdec-vdec", "vdec-top"; > > > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > > > + assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>; > > > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC>; > > > > + }; > > > > + > > > > + vcodec_core: vcodec_core@0x16025000 { > > > > + compatible = "mediatek,mtk-vcodec-core"; > > > > + reg = <0 0x16025000 0 0x1000>; /* VDEC_CORE_MISC */ > > > > + interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; > > > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_UFO_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_PP_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_RD_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_WR_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_PPWRAP_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_TILE_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_VLD_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_VLD2_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_AVC_MV_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_RG_CTRL_DMA_EXT>; > > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > > > + <&vdecsys CLK_VDEC_VDEC>, > > > > + <&vdecsys CLK_VDEC_LAT>, > > > > + <&vdecsys CLK_VDEC_LARB1>, > > > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-lat", > > > > + "vdec-vdec", "vdec-top"; > > > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > > > + assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>; > > > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC2>; > > > > + }; > > > > > > I'm a bit late in the game, reviewing v5 only, but I'm wondering if > > > those IP cores need to be modelled in separate nodes. It would be much > > > easier, from a software point of view, to have a single node, with > > > multiple register ranges. > > > > > > Are some of those IP cores used in different SoCs, combined in different > > > ways, that make a modular design better ? > > 1. Each a node should respect a HW node. Defining a complex node that > contain multiple register is not better, for they belong to different > hardware. Different platforms has different hardware count, mt8195 has > five hardwares. > 2. Another reason is from the IOMMU point, the vcodec HW include core > and lat hardwares, each of them connect to the independent IOMMU > hardware for mt8195, can't write all iommu ports together, or hardware > can't access dram data, so we must separate them. Maybe it could help understanding the situation if the DT bindings could include more information about the hardware architecture, such as a block diagram for instance, and how the pieces interact with each other ? > > Yeah, I agree with Laurent here. This way of modelling the different parts > > of the CODEC as different pieces is the reason why you need a framework > > to pull them together, such as the component API (I guess v4l2-async > > could have been used as well). > > We must separate each hardware node, only master need to register video > and media device, component just used for store clk/power/irq/register > information for current hardware. > > > I normally don't bother with driver internals, as its up to each driver > > author to decide what is best. However, I believe this design is too > > convoluted, and it may lead other developers to follow the same pattern, > > so please avoid it. > > According to your information, it looks that component and v4l2 async > are ok for this architecture, only component is too convoluted(iommu > driver also use component). > > For we are not very familiar with v4l2 async, out architecture is > designed for component include next forty patches. It is a very big > change for out driver. I will try to spend a lot of time to change it. > > > There are several ways of modelling this and initializing or probing > > the sub-devices, > > without using any async framework. > > You may misunderstand, I'm not objecting your suggestion every time, > for our driver is designed for component api, need to spend a lot of > time to change it in many platforms. I will change it if v4l2 async has > many advantages then component as you said. I will try to use v4l2 > async as compared.
Hi Ezequiel, Thanks for your feedback. On Sun, 2021-08-29 at 17:54 -0300, Ezequiel Garcia wrote: > On Tue, 17 Aug 2021 at 00:52, yunfei.dong@mediatek.com > <yunfei.dong@mediatek.com> wrote: > > > > Hi Laurent, > > > > Thanks for your detail suggestion. > > > > On Wed, 2021-08-11 at 20:59 +0300, Laurent Pinchart wrote: > > > Hi Yunfei, > > > > > > Thank you for the patch. > > > > > > On Wed, Aug 11, 2021 at 10:57:59AM +0800, Yunfei Dong wrote: > > > > Adds decoder dt-bindings for mt8192. > > > > > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > > > --- > > > > v5: no changes > > > > > > > > This patch depends on "Mediatek MT8192 clock support"[1]. > > > > > > > > The definition of decoder clocks are in mt8192-clk.h, need to > > > > include them in case of build fail [1]. > > > > > > > > [1] > > > > https://patchwork.kernel.org/project/linux-mediatek/list/?series=511175 > > > > --- > > > > .../media/mediatek,vcodec-comp-decoder.yaml | 172 > > > > ++++++++++++++++++ > > > > 1 file changed, 172 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > > > decoder.yaml > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > > > decoder.yaml > > > > b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > > > decoder.yaml > > > > new file mode 100644 > > > > index 000000000000..083c89933917 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec- > > > > comp- > > > > decoder.yaml > > > > @@ -0,0 +1,172 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: > > > > http://devicetree.org/schemas/iommu/mediatek,vcodec-comp-decoder.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Mediatek Video Decode Accelerator With Component > > > > + > > > > +maintainers: > > > > + - Yunfei Dong <yunfei.dong@mediatek.com> > > > > + > > > > +description: |+ > > > > + Mediatek Video Decode is the video decode hardware present > > > > in > > > > Mediatek > > > > + SoCs which supports high resolution decoding > > > > functionalities. > > > > Required > > > > + master and component node. > > > > > > This should explain how the three IP cores relate to each other. > > > > > > > I will explain it in next patch. > > > > + > > > > +properties: > > > > + compatible: > > > > + oneOf: > > > > + - enum: > > > > + - mediatek,mt8192-vcodec-dec # for lat hardware > > > > + - mediatek,mtk-vcodec-lat # for core hardware > > > > + - mediatek,mtk-vcodec-core > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + interrupts: > > > > + maxItems: 1 > > > > + > > > > + clocks: > > > > + maxItems: 5 > > > > + > > > > + clock-names: > > > > + items: > > > > + - const: vdec-sel > > > > + - const: vdec-soc-vdec > > > > + - const: vdec-soc-lat > > > > + - const: vdec-vdec > > > > + - const: vdec-top > > > > + > > > > + assigned-clocks: true > > > > + > > > > + assigned-clock-parents: true > > > > + > > > > + power-domains: > > > > + maxItems: 1 > > > > + > > > > + iommus: > > > > + minItems: 1 > > > > + maxItems: 32 > > > > + description: | > > > > + List of the hardware port in respective IOMMU block for > > > > current Socs. > > > > + Refer to bindings/iommu/mediatek,iommu.yaml. > > > > + > > > > + dma-ranges: > > > > + maxItems: 1 > > > > + description: | > > > > + Describes the physical address space of IOMMU maps to > > > > memory. > > > > + > > > > + mediatek,scp: > > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > > + maxItems: 1 > > > > + description: > > > > + Describes point to scp. > > > > + > > > > +required: > > > > + - compatible > > > > + - reg > > > > + - iommus > > > > + - dma-ranges > > > > + > > > > +allOf: > > > > + - if: #master node > > > > + properties: > > > > + compatible: > > > > + contains: > > > > + enum: > > > > + - mediatek,mt8192-vcodec-dec # for lat hardware > > > > + > > > > + then: > > > > + required: > > > > + - mediatek,scp > > > > + > > > > + - if: #component node > > > > + properties: > > > > + compatible: > > > > + contains: > > > > + enum: > > > > + - mediatek,mtk-vcodec-lat # for core > > > > hardware > > > > + - mediatek,mtk-vcodec-core > > > > + > > > > + then: > > > > + required: > > > > + - interrupts > > > > + - clocks > > > > + - clock-names > > > > + - assigned-clocks > > > > + - assigned-clock-parents > > > > + - power-domains > > > > + > > > > + > > > > +additionalProperties: false > > > > + > > > > +examples: > > > > + - | > > > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > + #include <dt-bindings/memory/mt8192-larb-port.h> > > > > + #include <dt-bindings/interrupt-controller/irq.h> > > > > + #include <dt-bindings/clock/mt8192-clk.h> > > > > + #include <dt-bindings/power/mt8192-power.h> > > > > + > > > > + vcodec_dec: vcodec_dec@16000000 { > > > > + compatible = "mediatek,mt8192-vcodec-dec"; > > > > + reg = <0 0x16000000 0 0x1000>; /* VDEC_SYS > > > > */ > > > > + mediatek,scp = <&scp>; > > > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; > > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > > + }; > > > > + > > > > + vcodec_lat: vcodec_lat@0x16010000 { > > > > + compatible = "mediatek,mtk-vcodec-lat"; > > > > + reg = <0 0x16010000 0 0x800>; /* > > > > VDEC_MISC */ > > > > + interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; > > > > + iommus = <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD_EXT>, > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD2_EXT>, > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_AVC_MV_EXT>, > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_PRED_RD_EXT>, > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_TILE_EXT>, > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_WDMA_EXT>, > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_RG_CTRL_DMA_EXT>, > > > > + <&iommu0 M4U_PORT_L5_VDEC_UFO_ENC_EXT>; > > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > > > + <&vdecsys_soc CLK_VDEC_SOC_VDEC>, > > > > + <&vdecsys_soc CLK_VDEC_SOC_LAT>, > > > > + <&vdecsys_soc CLK_VDEC_SOC_LARB1>, > > > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc- > > > > lat", > > > > + "vdec-vdec", "vdec-top"; > > > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > > > + assigned-clock-parents = <&topckgen > > > > CLK_TOP_MAINPLL_D4>; > > > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC>; > > > > + }; > > > > + > > > > + vcodec_core: vcodec_core@0x16025000 { > > > > + compatible = "mediatek,mtk-vcodec-core"; > > > > + reg = <0 0x16025000 0 0x1000>; /* > > > > VDEC_CORE_MISC */ > > > > + interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; > > > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_UFO_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_PP_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_RD_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_WR_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_PPWRAP_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_TILE_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_VLD_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_VLD2_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_AVC_MV_EXT>, > > > > + <&iommu0 M4U_PORT_L4_VDEC_RG_CTRL_DMA_EXT>; > > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > > > + <&vdecsys CLK_VDEC_VDEC>, > > > > + <&vdecsys CLK_VDEC_LAT>, > > > > + <&vdecsys CLK_VDEC_LARB1>, > > > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc- > > > > lat", > > > > + "vdec-vdec", "vdec-top"; > > > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > > > + assigned-clock-parents = <&topckgen > > > > CLK_TOP_MAINPLL_D4>; > > > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC2>; > > > > + }; > > > > > > I'm a bit late in the game, reviewing v5 only, but I'm wondering > > > if > > > those IP cores need to be modelled in separate nodes. It would be > > > much > > > easier, from a software point of view, to have a single node, > > > with > > > multiple register ranges. > > > > > > Are some of those IP cores used in different SoCs, combined in > > > different > > > ways, that make a modular design better ? > > > > > > > Different platform has different hardware, for mt8192 only has > > three > > nodes. but mt8195 will has five nodes. and the clk/power/irq/iommu > > are > > different. It is not easy to manage all hardware at the same time > > in > > one node, need to enable different hardware at the same time, the > > logic > > will be very complex. > > It is much easier to handle different hardware using component, > > enable > > different hardware when we need it. > > > > > > You can still have one device-tree node for each device, which means > you can still manage your resources (clk/power/irq/iommu) easily, but > doing > this so that it avoids an async framework to pull the parts together. > It mean that v4l2 async also can't be used? > I gave you this feedback several times, and you have been objecting > it > every time without even trying to consider a different approach. > I'm not objecting your suggestion every time, for our driver is designed for component api, need to spend a lot of time to change it in many platforms. We need to get a final solution for this architecture, or it's not easy to change all patches in the feature. > Thanks, > Ezequiel 1. Each a node should respect a HW node. Defining a complex node that contain multiple register is not better, for they belong to different hardware. Different platforms has different hardware count, mt8195 has five hardwares. 2. Another reason is from the IOMMU point, the vcodec HW include core and lat hardwares, each of them connect to the independent IOMMU hardware for mt8195, can't write all iommu ports together, or hardware can't access dram data, so we must separate them. For these limitation, can't combine all hardware together. I will write the hardware diagram in next patch. Thanks, Yunfei Dong
On Wed, 1 Sept 2021 at 00:49, yunfei.dong@mediatek.com <yunfei.dong@mediatek.com> wrote: > > Hi Ezequiel, > > Thanks for your feedback. > On Sun, 2021-08-29 at 17:54 -0300, Ezequiel Garcia wrote: > > On Tue, 17 Aug 2021 at 00:52, yunfei.dong@mediatek.com > > <yunfei.dong@mediatek.com> wrote: > > > > > > Hi Laurent, > > > > > > Thanks for your detail suggestion. > > > > > > On Wed, 2021-08-11 at 20:59 +0300, Laurent Pinchart wrote: > > > > Hi Yunfei, > > > > > > > > Thank you for the patch. > > > > > > > > On Wed, Aug 11, 2021 at 10:57:59AM +0800, Yunfei Dong wrote: > > > > > Adds decoder dt-bindings for mt8192. > > > > > > > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > > > > --- > > > > > v5: no changes > > > > > > > > > > This patch depends on "Mediatek MT8192 clock support"[1]. > > > > > > > > > > The definition of decoder clocks are in mt8192-clk.h, need to > > > > > include them in case of build fail [1]. > > > > > > > > > > [1] > > > > > > https://patchwork.kernel.org/project/linux-mediatek/list/?series=511175 > > > > > --- > > > > > .../media/mediatek,vcodec-comp-decoder.yaml | 172 > > > > > ++++++++++++++++++ > > > > > 1 file changed, 172 insertions(+) > > > > > create mode 100644 > > > > > Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > > > > decoder.yaml > > > > > > > > > > diff --git > > > > > a/Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > > > > decoder.yaml > > > > > b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp- > > > > > decoder.yaml > > > > > new file mode 100644 > > > > > index 000000000000..083c89933917 > > > > > --- /dev/null > > > > > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec- > > > > > comp- > > > > > decoder.yaml > > > > > @@ -0,0 +1,172 @@ > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > > +%YAML 1.2 > > > > > +--- > > > > > +$id: > > > > > > http://devicetree.org/schemas/iommu/mediatek,vcodec-comp-decoder.yaml# > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > + > > > > > +title: Mediatek Video Decode Accelerator With Component > > > > > + > > > > > +maintainers: > > > > > + - Yunfei Dong <yunfei.dong@mediatek.com> > > > > > + > > > > > +description: |+ > > > > > + Mediatek Video Decode is the video decode hardware present > > > > > in > > > > > Mediatek > > > > > + SoCs which supports high resolution decoding > > > > > functionalities. > > > > > Required > > > > > + master and component node. > > > > > > > > This should explain how the three IP cores relate to each other. > > > > > > > > > > I will explain it in next patch. > > > > > + > > > > > +properties: > > > > > + compatible: > > > > > + oneOf: > > > > > + - enum: > > > > > + - mediatek,mt8192-vcodec-dec # for lat hardware > > > > > + - mediatek,mtk-vcodec-lat # for core hardware > > > > > + - mediatek,mtk-vcodec-core > > > > > + > > > > > + reg: > > > > > + maxItems: 1 > > > > > + > > > > > + interrupts: > > > > > + maxItems: 1 > > > > > + > > > > > + clocks: > > > > > + maxItems: 5 > > > > > + > > > > > + clock-names: > > > > > + items: > > > > > + - const: vdec-sel > > > > > + - const: vdec-soc-vdec > > > > > + - const: vdec-soc-lat > > > > > + - const: vdec-vdec > > > > > + - const: vdec-top > > > > > + > > > > > + assigned-clocks: true > > > > > + > > > > > + assigned-clock-parents: true > > > > > + > > > > > + power-domains: > > > > > + maxItems: 1 > > > > > + > > > > > + iommus: > > > > > + minItems: 1 > > > > > + maxItems: 32 > > > > > + description: | > > > > > + List of the hardware port in respective IOMMU block for > > > > > current Socs. > > > > > + Refer to bindings/iommu/mediatek,iommu.yaml. > > > > > + > > > > > + dma-ranges: > > > > > + maxItems: 1 > > > > > + description: | > > > > > + Describes the physical address space of IOMMU maps to > > > > > memory. > > > > > + > > > > > + mediatek,scp: > > > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > > > + maxItems: 1 > > > > > + description: > > > > > + Describes point to scp. > > > > > + > > > > > +required: > > > > > + - compatible > > > > > + - reg > > > > > + - iommus > > > > > + - dma-ranges > > > > > + > > > > > +allOf: > > > > > + - if: #master node > > > > > + properties: > > > > > + compatible: > > > > > + contains: > > > > > + enum: > > > > > + - mediatek,mt8192-vcodec-dec # for lat hardware > > > > > + > > > > > + then: > > > > > + required: > > > > > + - mediatek,scp > > > > > + > > > > > + - if: #component node > > > > > + properties: > > > > > + compatible: > > > > > + contains: > > > > > + enum: > > > > > + - mediatek,mtk-vcodec-lat # for core > > > > > hardware > > > > > + - mediatek,mtk-vcodec-core > > > > > + > > > > > + then: > > > > > + required: > > > > > + - interrupts > > > > > + - clocks > > > > > + - clock-names > > > > > + - assigned-clocks > > > > > + - assigned-clock-parents > > > > > + - power-domains > > > > > + > > > > > + > > > > > +additionalProperties: false > > > > > + > > > > > +examples: > > > > > + - | > > > > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > > + #include <dt-bindings/memory/mt8192-larb-port.h> > > > > > + #include <dt-bindings/interrupt-controller/irq.h> > > > > > + #include <dt-bindings/clock/mt8192-clk.h> > > > > > + #include <dt-bindings/power/mt8192-power.h> > > > > > + > > > > > + vcodec_dec: vcodec_dec@16000000 { > > > > > + compatible = "mediatek,mt8192-vcodec-dec"; > > > > > + reg = <0 0x16000000 0 0x1000>; /* VDEC_SYS > > > > > */ > > > > > + mediatek,scp = <&scp>; > > > > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; > > > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > > > + }; > > > > > + > > > > > + vcodec_lat: vcodec_lat@0x16010000 { > > > > > + compatible = "mediatek,mtk-vcodec-lat"; > > > > > + reg = <0 0x16010000 0 0x800>; /* > > > > > VDEC_MISC */ > > > > > + interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; > > > > > + iommus = <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD_EXT>, > > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD2_EXT>, > > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_AVC_MV_EXT>, > > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_PRED_RD_EXT>, > > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_TILE_EXT>, > > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_WDMA_EXT>, > > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_RG_CTRL_DMA_EXT>, > > > > > + <&iommu0 M4U_PORT_L5_VDEC_UFO_ENC_EXT>; > > > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > > > > + <&vdecsys_soc CLK_VDEC_SOC_VDEC>, > > > > > + <&vdecsys_soc CLK_VDEC_SOC_LAT>, > > > > > + <&vdecsys_soc CLK_VDEC_SOC_LARB1>, > > > > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > > > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc- > > > > > lat", > > > > > + "vdec-vdec", "vdec-top"; > > > > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > > > > + assigned-clock-parents = <&topckgen > > > > > CLK_TOP_MAINPLL_D4>; > > > > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC>; > > > > > + }; > > > > > + > > > > > + vcodec_core: vcodec_core@0x16025000 { > > > > > + compatible = "mediatek,mtk-vcodec-core"; > > > > > + reg = <0 0x16025000 0 0x1000>; /* > > > > > VDEC_CORE_MISC */ > > > > > + interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; > > > > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>, > > > > > + <&iommu0 M4U_PORT_L4_VDEC_UFO_EXT>, > > > > > + <&iommu0 M4U_PORT_L4_VDEC_PP_EXT>, > > > > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_RD_EXT>, > > > > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_WR_EXT>, > > > > > + <&iommu0 M4U_PORT_L4_VDEC_PPWRAP_EXT>, > > > > > + <&iommu0 M4U_PORT_L4_VDEC_TILE_EXT>, > > > > > + <&iommu0 M4U_PORT_L4_VDEC_VLD_EXT>, > > > > > + <&iommu0 M4U_PORT_L4_VDEC_VLD2_EXT>, > > > > > + <&iommu0 M4U_PORT_L4_VDEC_AVC_MV_EXT>, > > > > > + <&iommu0 M4U_PORT_L4_VDEC_RG_CTRL_DMA_EXT>; > > > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > > > > + <&vdecsys CLK_VDEC_VDEC>, > > > > > + <&vdecsys CLK_VDEC_LAT>, > > > > > + <&vdecsys CLK_VDEC_LARB1>, > > > > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > > > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc- > > > > > lat", > > > > > + "vdec-vdec", "vdec-top"; > > > > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > > > > + assigned-clock-parents = <&topckgen > > > > > CLK_TOP_MAINPLL_D4>; > > > > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC2>; > > > > > + }; > > > > > > > > I'm a bit late in the game, reviewing v5 only, but I'm wondering > > > > if > > > > those IP cores need to be modelled in separate nodes. It would be > > > > much > > > > easier, from a software point of view, to have a single node, > > > > with > > > > multiple register ranges. > > > > > > > > Are some of those IP cores used in different SoCs, combined in > > > > different > > > > ways, that make a modular design better ? > > > > > > > > > > Different platform has different hardware, for mt8192 only has > > > three > > > nodes. but mt8195 will has five nodes. and the clk/power/irq/iommu > > > are > > > different. It is not easy to manage all hardware at the same time > > > in > > > one node, need to enable different hardware at the same time, the > > > logic > > > will be very complex. > > > It is much easier to handle different hardware using component, > > > enable > > > different hardware when we need it. > > > > > > > > > > You can still have one device-tree node for each device, which means > > you can still manage your resources (clk/power/irq/iommu) easily, but > > doing > > this so that it avoids an async framework to pull the parts together. > > > It mean that v4l2 async also can't be used? No, I don't think it's needed. I will take a look at the device tree and see if I can show you an alternative approach that doesn't require an async framework. > > I gave you this feedback several times, and you have been objecting > > it > > every time without even trying to consider a different approach. > > > I'm not objecting your suggestion every time, for our driver is > designed for component api, need to spend a lot of time to change it in > many platforms. We need to get a final solution for this architecture, > or it's not easy to change all patches in the feature. > > Thanks, > > Ezequiel > 1. Each a node should respect a HW node. Defining a complex node that > contain multiple register is not better, for they belong to different > hardware. Different platforms has different hardware count, mt8195 has > five hardwares. > 2. Another reason is from the IOMMU point, the vcodec HW include core > and lat hardwares, each of them connect to the independent IOMMU > hardware for mt8195, can't write all iommu ports together, or hardware > can't access dram data, so we must separate them. > > For these limitation, can't combine all hardware together. I will write > the hardware diagram in next patch. > No need for another patch, you can write that hardware diagram now, so we can help with some design ideas. Thanks! Ezequiel
Hi Ezequiel, Thanks for your suggestion. On Wed, 2021-09-01 at 08:50 -0300, Ezequiel Garcia wrote: > On Wed, 1 Sept 2021 at 00:49, yunfei.dong@mediatek.com > <yunfei.dong@mediatek.com> wrote: > > > > Hi Ezequiel, > > > > Thanks for your feedback. > > On Sun, 2021-08-29 at 17:54 -0300, Ezequiel Garcia wrote: > > > On Tue, 17 Aug 2021 at 00:52, yunfei.dong@mediatek.com > > > <yunfei.dong@mediatek.com> wrote: > > > > > > > > Hi Laurent, > > > > > > > > Thanks for your detail suggestion. > > > > > > > > On Wed, 2021-08-11 at 20:59 +0300, Laurent Pinchart wrote: > > > > > Hi Yunfei, > > > > > > > > > > Thank you for the patch. > > > > > > > > > > On Wed, Aug 11, 2021 at 10:57:59AM +0800, Yunfei Dong wrote: > > > > > > Adds decoder dt-bindings for mt8192. > > > > > > > > > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > > > > > --- > > > > > > v5: no changes > > > > > > > > > > > > This patch depends on "Mediatek MT8192 clock support"[1]. > > > > > > > > > > > > The definition of decoder clocks are in mt8192-clk.h, need > > > > > > to > > > > > > include them in case of build fail [1]. > > > > > > > > > > > > [1] > > > > > > > > > > https://patchwork.kernel.org/project/linux-mediatek/list/?series=511175 > > > > > > --- > > > > > > .../media/mediatek,vcodec-comp-decoder.yaml | 172 > > > > > > ++++++++++++++++++ > > > > > > 1 file changed, 172 insertions(+) > > > > > > create mode 100644 > > > > > > Documentation/devicetree/bindings/media/mediatek,vcodec- > > > > > > comp- > > > > > > decoder.yaml > > > > > > > > > > > > diff --git > > > > > > a/Documentation/devicetree/bindings/media/mediatek,vcodec- > > > > > > comp- > > > > > > decoder.yaml > > > > > > b/Documentation/devicetree/bindings/media/mediatek,vcodec- > > > > > > comp- > > > > > > decoder.yaml > > > > > > new file mode 100644 > > > > > > index 000000000000..083c89933917 > > > > > > --- /dev/null > > > > > > +++ > > > > > > b/Documentation/devicetree/bindings/media/mediatek,vcodec- > > > > > > comp- > > > > > > decoder.yaml > > > > > > @@ -0,0 +1,172 @@ > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > > > +%YAML 1.2 > > > > > > +--- > > > > > > +$id: > > > > > > > > > > http://devicetree.org/schemas/iommu/mediatek,vcodec-comp-decoder.yaml# > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > > + > > > > > > +title: Mediatek Video Decode Accelerator With Component > > > > > > + > > > > > > +maintainers: > > > > > > + - Yunfei Dong <yunfei.dong@mediatek.com> > > > > > > + > > > > > > +description: |+ > > > > > > + Mediatek Video Decode is the video decode hardware > > > > > > present > > > > > > in > > > > > > Mediatek > > > > > > + SoCs which supports high resolution decoding > > > > > > functionalities. > > > > > > Required > > > > > > + master and component node. > > > > > > > > > > This should explain how the three IP cores relate to each > > > > > other. > > > > > > > > > > > > > I will explain it in next patch. > > > > > > + > > > > > > +properties: > > > > > > + compatible: > > > > > > + oneOf: > > > > > > + - enum: > > > > > > + - mediatek,mt8192-vcodec-dec # for lat hardware > > > > > > + - mediatek,mtk-vcodec-lat # for core > > > > > > hardware > > > > > > + - mediatek,mtk-vcodec-core > > > > > > + > > > > > > + reg: > > > > > > + maxItems: 1 > > > > > > + > > > > > > + interrupts: > > > > > > + maxItems: 1 > > > > > > + > > > > > > + clocks: > > > > > > + maxItems: 5 > > > > > > + > > > > > > + clock-names: > > > > > > + items: > > > > > > + - const: vdec-sel > > > > > > + - const: vdec-soc-vdec > > > > > > + - const: vdec-soc-lat > > > > > > + - const: vdec-vdec > > > > > > + - const: vdec-top > > > > > > + > > > > > > + assigned-clocks: true > > > > > > + > > > > > > + assigned-clock-parents: true > > > > > > + > > > > > > + power-domains: > > > > > > + maxItems: 1 > > > > > > + > > > > > > + iommus: > > > > > > + minItems: 1 > > > > > > + maxItems: 32 > > > > > > + description: | > > > > > > + List of the hardware port in respective IOMMU block > > > > > > for > > > > > > current Socs. > > > > > > + Refer to bindings/iommu/mediatek,iommu.yaml. > > > > > > + > > > > > > + dma-ranges: > > > > > > + maxItems: 1 > > > > > > + description: | > > > > > > + Describes the physical address space of IOMMU maps > > > > > > to > > > > > > memory. > > > > > > + > > > > > > + mediatek,scp: > > > > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > > > > + maxItems: 1 > > > > > > + description: > > > > > > + Describes point to scp. > > > > > > + > > > > > > +required: > > > > > > + - compatible > > > > > > + - reg > > > > > > + - iommus > > > > > > + - dma-ranges > > > > > > + > > > > > > +allOf: > > > > > > + - if: #master node > > > > > > + properties: > > > > > > + compatible: > > > > > > + contains: > > > > > > + enum: > > > > > > + - mediatek,mt8192-vcodec-dec # for lat > > > > > > hardware > > > > > > + > > > > > > + then: > > > > > > + required: > > > > > > + - mediatek,scp > > > > > > + > > > > > > + - if: #component node > > > > > > + properties: > > > > > > + compatible: > > > > > > + contains: > > > > > > + enum: > > > > > > + - mediatek,mtk-vcodec-lat # for core > > > > > > hardware > > > > > > + - mediatek,mtk-vcodec-core > > > > > > + > > > > > > + then: > > > > > > + required: > > > > > > + - interrupts > > > > > > + - clocks > > > > > > + - clock-names > > > > > > + - assigned-clocks > > > > > > + - assigned-clock-parents > > > > > > + - power-domains > > > > > > + > > > > > > + > > > > > > +additionalProperties: false > > > > > > + > > > > > > +examples: > > > > > > + - | > > > > > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > > > + #include <dt-bindings/memory/mt8192-larb-port.h> > > > > > > + #include <dt-bindings/interrupt-controller/irq.h> > > > > > > + #include <dt-bindings/clock/mt8192-clk.h> > > > > > > + #include <dt-bindings/power/mt8192-power.h> > > > > > > + > > > > > > + vcodec_dec: vcodec_dec@16000000 { > > > > > > + compatible = "mediatek,mt8192-vcodec-dec"; > > > > > > + reg = <0 0x16000000 0 0x1000>; /* > > > > > > VDEC_SYS > > > > > > */ > > > > > > + mediatek,scp = <&scp>; > > > > > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; > > > > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 > > > > > > 0xfff00000>; > > > > > > + }; > > > > > > + > > > > > > + vcodec_lat: vcodec_lat@0x16010000 { > > > > > > + compatible = "mediatek,mtk-vcodec-lat"; > > > > > > + reg = <0 0x16010000 0 0x800>; /* > > > > > > VDEC_MISC */ > > > > > > + interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; > > > > > > + iommus = <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD_EXT>, > > > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD2_EXT>, > > > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_AVC_MV_EXT>, > > > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_PRED_RD_EXT>, > > > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_TILE_EXT>, > > > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_WDMA_EXT>, > > > > > > + <&iommu0 > > > > > > M4U_PORT_L5_VDEC_LAT0_RG_CTRL_DMA_EXT>, > > > > > > + <&iommu0 M4U_PORT_L5_VDEC_UFO_ENC_EXT>; > > > > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 > > > > > > 0xfff00000>; > > > > > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > > > > > + <&vdecsys_soc CLK_VDEC_SOC_VDEC>, > > > > > > + <&vdecsys_soc CLK_VDEC_SOC_LAT>, > > > > > > + <&vdecsys_soc CLK_VDEC_SOC_LARB1>, > > > > > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > > > > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec- > > > > > > soc- > > > > > > lat", > > > > > > + "vdec-vdec", "vdec-top"; > > > > > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > > > > > + assigned-clock-parents = <&topckgen > > > > > > CLK_TOP_MAINPLL_D4>; > > > > > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC>; > > > > > > + }; > > > > > > + > > > > > > + vcodec_core: vcodec_core@0x16025000 { > > > > > > + compatible = "mediatek,mtk-vcodec-core"; > > > > > > + reg = <0 0x16025000 0 0x1000>; /* > > > > > > VDEC_CORE_MISC */ > > > > > > + interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; > > > > > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>, > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_UFO_EXT>, > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_PP_EXT>, > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_RD_EXT>, > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_WR_EXT>, > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_PPWRAP_EXT>, > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_TILE_EXT>, > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_VLD_EXT>, > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_VLD2_EXT>, > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_AVC_MV_EXT>, > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_RG_CTRL_DMA_EXT>; > > > > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 > > > > > > 0xfff00000>; > > > > > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > > > > > + <&vdecsys CLK_VDEC_VDEC>, > > > > > > + <&vdecsys CLK_VDEC_LAT>, > > > > > > + <&vdecsys CLK_VDEC_LARB1>, > > > > > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > > > > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec- > > > > > > soc- > > > > > > lat", > > > > > > + "vdec-vdec", "vdec-top"; > > > > > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > > > > > + assigned-clock-parents = <&topckgen > > > > > > CLK_TOP_MAINPLL_D4>; > > > > > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC2>; > > > > > > + }; > > > > > > > > > > I'm a bit late in the game, reviewing v5 only, but I'm > > > > > wondering > > > > > if > > > > > those IP cores need to be modelled in separate nodes. It > > > > > would be > > > > > much > > > > > easier, from a software point of view, to have a single node, > > > > > with > > > > > multiple register ranges. > > > > > > > > > > Are some of those IP cores used in different SoCs, combined > > > > > in > > > > > different > > > > > ways, that make a modular design better ? > > > > > > > > > > > > > Different platform has different hardware, for mt8192 only has > > > > three > > > > nodes. but mt8195 will has five nodes. and the > > > > clk/power/irq/iommu > > > > are > > > > different. It is not easy to manage all hardware at the same > > > > time > > > > in > > > > one node, need to enable different hardware at the same time, > > > > the > > > > logic > > > > will be very complex. > > > > It is much easier to handle different hardware using component, > > > > enable > > > > different hardware when we need it. > > > > > > > > > > > > > > You can still have one device-tree node for each device, which > > > means > > > you can still manage your resources (clk/power/irq/iommu) easily, > > > but > > > doing > > > this so that it avoids an async framework to pull the parts > > > together. > > > > > > > It mean that v4l2 async also can't be used? > > No, I don't think it's needed. I will take a look at the device tree > and see if I can show you an alternative approach that doesn't > require an async framework. > Thanks. > > > I gave you this feedback several times, and you have been > > > objecting > > > it > > > every time without even trying to consider a different approach. > > > > > > > I'm not objecting your suggestion every time, for our driver is > > designed for component api, need to spend a lot of time to change > > it in > > many platforms. We need to get a final solution for this > > architecture, > > or it's not easy to change all patches in the feature. > > > Thanks, > > > Ezequiel > > > > 1. Each a node should respect a HW node. Defining a complex node > > that > > contain multiple register is not better, for they belong to > > different > > hardware. Different platforms has different hardware count, mt8195 > > has > > five hardwares. > > 2. Another reason is from the IOMMU point, the vcodec HW include > > core > > and lat hardwares, each of them connect to the independent IOMMU > > hardware for mt8195, can't write all iommu ports together, or > > hardware > > can't access dram data, so we must separate them. > > > > For these limitation, can't combine all hardware together. I will > > write > > the hardware diagram in next patch. > > > > No need for another patch, you can write that hardware diagram > now, so we can help with some design ideas. > You can see the block diagram in patch: https://patchwork.linuxtv.org/project/linux-media/patch/20210901083215.25984-14-yunfei.dong@mediatek.com/ > Thanks! > Ezequiel
On Thu, Sep 2, 2021 at 2:09 PM yunfei.dong@mediatek.com <yunfei.dong@mediatek.com> wrote: > > Hi Ezequiel, > > Thanks for your suggestion. > On Wed, 2021-09-01 at 08:50 -0300, Ezequiel Garcia wrote: > > On Wed, 1 Sept 2021 at 00:49, yunfei.dong@mediatek.com > > <yunfei.dong@mediatek.com> wrote: > > > > > > Hi Ezequiel, > > > > > > Thanks for your feedback. > > > On Sun, 2021-08-29 at 17:54 -0300, Ezequiel Garcia wrote: > > > > On Tue, 17 Aug 2021 at 00:52, yunfei.dong@mediatek.com > > > > <yunfei.dong@mediatek.com> wrote: > > > > > > > > > > Hi Laurent, > > > > > > > > > > Thanks for your detail suggestion. > > > > > > > > > > On Wed, 2021-08-11 at 20:59 +0300, Laurent Pinchart wrote: > > > > > > Hi Yunfei, > > > > > > > > > > > > Thank you for the patch. > > > > > > > > > > > > On Wed, Aug 11, 2021 at 10:57:59AM +0800, Yunfei Dong wrote: > > > > > > > Adds decoder dt-bindings for mt8192. > > > > > > > > > > > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > > > > > > --- > > > > > > > v5: no changes > > > > > > > > > > > > > > This patch depends on "Mediatek MT8192 clock support"[1]. > > > > > > > > > > > > > > The definition of decoder clocks are in mt8192-clk.h, need > > > > > > > to > > > > > > > include them in case of build fail [1]. > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > https://patchwork.kernel.org/project/linux-mediatek/list/?series=511175 > > > > > > > --- > > > > > > > .../media/mediatek,vcodec-comp-decoder.yaml | 172 > > > > > > > ++++++++++++++++++ > > > > > > > 1 file changed, 172 insertions(+) > > > > > > > create mode 100644 > > > > > > > Documentation/devicetree/bindings/media/mediatek,vcodec- > > > > > > > comp- > > > > > > > decoder.yaml > > > > > > > > > > > > > > diff --git > > > > > > > a/Documentation/devicetree/bindings/media/mediatek,vcodec- > > > > > > > comp- > > > > > > > decoder.yaml > > > > > > > b/Documentation/devicetree/bindings/media/mediatek,vcodec- > > > > > > > comp- > > > > > > > decoder.yaml > > > > > > > new file mode 100644 > > > > > > > index 000000000000..083c89933917 > > > > > > > --- /dev/null > > > > > > > +++ > > > > > > > b/Documentation/devicetree/bindings/media/mediatek,vcodec- > > > > > > > comp- > > > > > > > decoder.yaml > > > > > > > @@ -0,0 +1,172 @@ > > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > > > > +%YAML 1.2 > > > > > > > +--- > > > > > > > +$id: > > > > > > > > > > > > > > http://devicetree.org/schemas/iommu/mediatek,vcodec-comp-decoder.yaml# > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > > > + > > > > > > > +title: Mediatek Video Decode Accelerator With Component > > > > > > > + > > > > > > > +maintainers: > > > > > > > + - Yunfei Dong <yunfei.dong@mediatek.com> > > > > > > > + > > > > > > > +description: |+ > > > > > > > + Mediatek Video Decode is the video decode hardware > > > > > > > present > > > > > > > in > > > > > > > Mediatek > > > > > > > + SoCs which supports high resolution decoding > > > > > > > functionalities. > > > > > > > Required > > > > > > > + master and component node. > > > > > > > > > > > > This should explain how the three IP cores relate to each > > > > > > other. > > > > > > > > > > > > > > > > I will explain it in next patch. > > > > > > > + > > > > > > > +properties: > > > > > > > + compatible: > > > > > > > + oneOf: > > > > > > > + - enum: > > > > > > > + - mediatek,mt8192-vcodec-dec # for lat hardware > > > > > > > + - mediatek,mtk-vcodec-lat # for core > > > > > > > hardware > > > > > > > + - mediatek,mtk-vcodec-core > > > > > > > + > > > > > > > + reg: > > > > > > > + maxItems: 1 > > > > > > > + > > > > > > > + interrupts: > > > > > > > + maxItems: 1 > > > > > > > + > > > > > > > + clocks: > > > > > > > + maxItems: 5 > > > > > > > + > > > > > > > + clock-names: > > > > > > > + items: > > > > > > > + - const: vdec-sel > > > > > > > + - const: vdec-soc-vdec > > > > > > > + - const: vdec-soc-lat > > > > > > > + - const: vdec-vdec > > > > > > > + - const: vdec-top > > > > > > > + > > > > > > > + assigned-clocks: true > > > > > > > + > > > > > > > + assigned-clock-parents: true > > > > > > > + > > > > > > > + power-domains: > > > > > > > + maxItems: 1 > > > > > > > + > > > > > > > + iommus: > > > > > > > + minItems: 1 > > > > > > > + maxItems: 32 > > > > > > > + description: | > > > > > > > + List of the hardware port in respective IOMMU block > > > > > > > for > > > > > > > current Socs. > > > > > > > + Refer to bindings/iommu/mediatek,iommu.yaml. > > > > > > > + > > > > > > > + dma-ranges: > > > > > > > + maxItems: 1 > > > > > > > + description: | > > > > > > > + Describes the physical address space of IOMMU maps > > > > > > > to > > > > > > > memory. > > > > > > > + > > > > > > > + mediatek,scp: > > > > > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > > > > > + maxItems: 1 > > > > > > > + description: > > > > > > > + Describes point to scp. > > > > > > > + > > > > > > > +required: > > > > > > > + - compatible > > > > > > > + - reg > > > > > > > + - iommus > > > > > > > + - dma-ranges > > > > > > > + > > > > > > > +allOf: > > > > > > > + - if: #master node > > > > > > > + properties: > > > > > > > + compatible: > > > > > > > + contains: > > > > > > > + enum: > > > > > > > + - mediatek,mt8192-vcodec-dec # for lat > > > > > > > hardware > > > > > > > + > > > > > > > + then: > > > > > > > + required: > > > > > > > + - mediatek,scp > > > > > > > + > > > > > > > + - if: #component node > > > > > > > + properties: > > > > > > > + compatible: > > > > > > > + contains: > > > > > > > + enum: > > > > > > > + - mediatek,mtk-vcodec-lat # for core > > > > > > > hardware > > > > > > > + - mediatek,mtk-vcodec-core > > > > > > > + > > > > > > > + then: > > > > > > > + required: > > > > > > > + - interrupts > > > > > > > + - clocks > > > > > > > + - clock-names > > > > > > > + - assigned-clocks > > > > > > > + - assigned-clock-parents > > > > > > > + - power-domains > > > > > > > + > > > > > > > + > > > > > > > +additionalProperties: false > > > > > > > + > > > > > > > +examples: > > > > > > > + - | > > > > > > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > > > > + #include <dt-bindings/memory/mt8192-larb-port.h> > > > > > > > + #include <dt-bindings/interrupt-controller/irq.h> > > > > > > > + #include <dt-bindings/clock/mt8192-clk.h> > > > > > > > + #include <dt-bindings/power/mt8192-power.h> > > > > > > > + > > > > > > > + vcodec_dec: vcodec_dec@16000000 { > > > > > > > + compatible = "mediatek,mt8192-vcodec-dec"; > > > > > > > + reg = <0 0x16000000 0 0x1000>; /* > > > > > > > VDEC_SYS > > > > > > > */ > > > > > > > + mediatek,scp = <&scp>; > > > > > > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; > > > > > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 > > > > > > > 0xfff00000>; > > > > > > > + }; > > > > > > > + > > > > > > > + vcodec_lat: vcodec_lat@0x16010000 { > > > > > > > + compatible = "mediatek,mtk-vcodec-lat"; > > > > > > > + reg = <0 0x16010000 0 0x800>; /* > > > > > > > VDEC_MISC */ > > > > > > > + interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; > > > > > > > + iommus = <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD_EXT>, > > > > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD2_EXT>, > > > > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_AVC_MV_EXT>, > > > > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_PRED_RD_EXT>, > > > > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_TILE_EXT>, > > > > > > > + <&iommu0 M4U_PORT_L5_VDEC_LAT0_WDMA_EXT>, > > > > > > > + <&iommu0 > > > > > > > M4U_PORT_L5_VDEC_LAT0_RG_CTRL_DMA_EXT>, > > > > > > > + <&iommu0 M4U_PORT_L5_VDEC_UFO_ENC_EXT>; > > > > > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 > > > > > > > 0xfff00000>; > > > > > > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > > > > > > + <&vdecsys_soc CLK_VDEC_SOC_VDEC>, > > > > > > > + <&vdecsys_soc CLK_VDEC_SOC_LAT>, > > > > > > > + <&vdecsys_soc CLK_VDEC_SOC_LARB1>, > > > > > > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > > > > > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec- > > > > > > > soc- > > > > > > > lat", > > > > > > > + "vdec-vdec", "vdec-top"; > > > > > > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > > > > > > + assigned-clock-parents = <&topckgen > > > > > > > CLK_TOP_MAINPLL_D4>; > > > > > > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC>; > > > > > > > + }; > > > > > > > + > > > > > > > + vcodec_core: vcodec_core@0x16025000 { > > > > > > > + compatible = "mediatek,mtk-vcodec-core"; > > > > > > > + reg = <0 0x16025000 0 0x1000>; /* > > > > > > > VDEC_CORE_MISC */ > > > > > > > + interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; > > > > > > > + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>, > > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_UFO_EXT>, > > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_PP_EXT>, > > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_RD_EXT>, > > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_PRED_WR_EXT>, > > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_PPWRAP_EXT>, > > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_TILE_EXT>, > > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_VLD_EXT>, > > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_VLD2_EXT>, > > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_AVC_MV_EXT>, > > > > > > > + <&iommu0 M4U_PORT_L4_VDEC_RG_CTRL_DMA_EXT>; > > > > > > > + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 > > > > > > > 0xfff00000>; > > > > > > > + clocks = <&topckgen CLK_TOP_VDEC_SEL>, > > > > > > > + <&vdecsys CLK_VDEC_VDEC>, > > > > > > > + <&vdecsys CLK_VDEC_LAT>, > > > > > > > + <&vdecsys CLK_VDEC_LARB1>, > > > > > > > + <&topckgen CLK_TOP_MAINPLL_D4>; > > > > > > > + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec- > > > > > > > soc- > > > > > > > lat", > > > > > > > + "vdec-vdec", "vdec-top"; > > > > > > > + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; > > > > > > > + assigned-clock-parents = <&topckgen > > > > > > > CLK_TOP_MAINPLL_D4>; > > > > > > > + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC2>; > > > > > > > + }; > > > > > > > > > > > > I'm a bit late in the game, reviewing v5 only, but I'm > > > > > > wondering > > > > > > if > > > > > > those IP cores need to be modelled in separate nodes. It > > > > > > would be > > > > > > much > > > > > > easier, from a software point of view, to have a single node, > > > > > > with > > > > > > multiple register ranges. > > > > > > > > > > > > Are some of those IP cores used in different SoCs, combined > > > > > > in > > > > > > different > > > > > > ways, that make a modular design better ? > > > > > > > > > > > > > > > > Different platform has different hardware, for mt8192 only has > > > > > three > > > > > nodes. but mt8195 will has five nodes. and the > > > > > clk/power/irq/iommu > > > > > are > > > > > different. It is not easy to manage all hardware at the same > > > > > time > > > > > in > > > > > one node, need to enable different hardware at the same time, > > > > > the > > > > > logic > > > > > will be very complex. > > > > > It is much easier to handle different hardware using component, > > > > > enable > > > > > different hardware when we need it. > > > > > > > > > > > > > > > > > > You can still have one device-tree node for each device, which > > > > means > > > > you can still manage your resources (clk/power/irq/iommu) easily, > > > > but > > > > doing > > > > this so that it avoids an async framework to pull the parts > > > > together. > > > > > > > > > > It mean that v4l2 async also can't be used? > > > > No, I don't think it's needed. I will take a look at the device tree > > and see if I can show you an alternative approach that doesn't > > require an async framework. > > > Thanks. > > > > I gave you this feedback several times, and you have been > > > > objecting > > > > it > > > > every time without even trying to consider a different approach. > > > > > > > > > > I'm not objecting your suggestion every time, for our driver is > > > designed for component api, need to spend a lot of time to change > > > it in > > > many platforms. We need to get a final solution for this > > > architecture, > > > or it's not easy to change all patches in the feature. > > > > Thanks, > > > > Ezequiel > > > > > > 1. Each a node should respect a HW node. Defining a complex node > > > that > > > contain multiple register is not better, for they belong to > > > different > > > hardware. Different platforms has different hardware count, mt8195 > > > has > > > five hardwares. > > > 2. Another reason is from the IOMMU point, the vcodec HW include > > > core > > > and lat hardwares, each of them connect to the independent IOMMU > > > hardware for mt8195, can't write all iommu ports together, or > > > hardware > > > can't access dram data, so we must separate them. > > > > > > For these limitation, can't combine all hardware together. I will > > > write > > > the hardware diagram in next patch. > > > > > > > No need for another patch, you can write that hardware diagram > > now, so we can help with some design ideas. > > > You can see the block diagram in patch: > > https://patchwork.linuxtv.org/project/linux-media/patch/20210901083215.25984-14-yunfei.dong@mediatek.com/ This isn't what was really requested though. This is a diagram of how you designed the software to operate the hardware. Ezequiel is asking for a diagram of how the hardware is actually designed, how the different blocks fit together, and how the hardware itself operates. Something like the block diagrams commonly seen in the datasheets. ChenYu
diff --git a/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml new file mode 100644 index 000000000000..083c89933917 --- /dev/null +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml @@ -0,0 +1,172 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iommu/mediatek,vcodec-comp-decoder.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Mediatek Video Decode Accelerator With Component + +maintainers: + - Yunfei Dong <yunfei.dong@mediatek.com> + +description: |+ + Mediatek Video Decode is the video decode hardware present in Mediatek + SoCs which supports high resolution decoding functionalities. Required + master and component node. + +properties: + compatible: + oneOf: + - enum: + - mediatek,mt8192-vcodec-dec # for lat hardware + - mediatek,mtk-vcodec-lat # for core hardware + - mediatek,mtk-vcodec-core + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + maxItems: 5 + + clock-names: + items: + - const: vdec-sel + - const: vdec-soc-vdec + - const: vdec-soc-lat + - const: vdec-vdec + - const: vdec-top + + assigned-clocks: true + + assigned-clock-parents: true + + power-domains: + maxItems: 1 + + iommus: + minItems: 1 + maxItems: 32 + description: | + List of the hardware port in respective IOMMU block for current Socs. + Refer to bindings/iommu/mediatek,iommu.yaml. + + dma-ranges: + maxItems: 1 + description: | + Describes the physical address space of IOMMU maps to memory. + + mediatek,scp: + $ref: /schemas/types.yaml#/definitions/phandle + maxItems: 1 + description: + Describes point to scp. + +required: + - compatible + - reg + - iommus + - dma-ranges + +allOf: + - if: #master node + properties: + compatible: + contains: + enum: + - mediatek,mt8192-vcodec-dec # for lat hardware + + then: + required: + - mediatek,scp + + - if: #component node + properties: + compatible: + contains: + enum: + - mediatek,mtk-vcodec-lat # for core hardware + - mediatek,mtk-vcodec-core + + then: + required: + - interrupts + - clocks + - clock-names + - assigned-clocks + - assigned-clock-parents + - power-domains + + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/memory/mt8192-larb-port.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/clock/mt8192-clk.h> + #include <dt-bindings/power/mt8192-power.h> + + vcodec_dec: vcodec_dec@16000000 { + compatible = "mediatek,mt8192-vcodec-dec"; + reg = <0 0x16000000 0 0x1000>; /* VDEC_SYS */ + mediatek,scp = <&scp>; + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; + }; + + vcodec_lat: vcodec_lat@0x16010000 { + compatible = "mediatek,mtk-vcodec-lat"; + reg = <0 0x16010000 0 0x800>; /* VDEC_MISC */ + interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; + iommus = <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD_EXT>, + <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD2_EXT>, + <&iommu0 M4U_PORT_L5_VDEC_LAT0_AVC_MV_EXT>, + <&iommu0 M4U_PORT_L5_VDEC_LAT0_PRED_RD_EXT>, + <&iommu0 M4U_PORT_L5_VDEC_LAT0_TILE_EXT>, + <&iommu0 M4U_PORT_L5_VDEC_LAT0_WDMA_EXT>, + <&iommu0 M4U_PORT_L5_VDEC_LAT0_RG_CTRL_DMA_EXT>, + <&iommu0 M4U_PORT_L5_VDEC_UFO_ENC_EXT>; + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; + clocks = <&topckgen CLK_TOP_VDEC_SEL>, + <&vdecsys_soc CLK_VDEC_SOC_VDEC>, + <&vdecsys_soc CLK_VDEC_SOC_LAT>, + <&vdecsys_soc CLK_VDEC_SOC_LARB1>, + <&topckgen CLK_TOP_MAINPLL_D4>; + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-lat", + "vdec-vdec", "vdec-top"; + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; + assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>; + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC>; + }; + + vcodec_core: vcodec_core@0x16025000 { + compatible = "mediatek,mtk-vcodec-core"; + reg = <0 0x16025000 0 0x1000>; /* VDEC_CORE_MISC */ + interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; + iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>, + <&iommu0 M4U_PORT_L4_VDEC_UFO_EXT>, + <&iommu0 M4U_PORT_L4_VDEC_PP_EXT>, + <&iommu0 M4U_PORT_L4_VDEC_PRED_RD_EXT>, + <&iommu0 M4U_PORT_L4_VDEC_PRED_WR_EXT>, + <&iommu0 M4U_PORT_L4_VDEC_PPWRAP_EXT>, + <&iommu0 M4U_PORT_L4_VDEC_TILE_EXT>, + <&iommu0 M4U_PORT_L4_VDEC_VLD_EXT>, + <&iommu0 M4U_PORT_L4_VDEC_VLD2_EXT>, + <&iommu0 M4U_PORT_L4_VDEC_AVC_MV_EXT>, + <&iommu0 M4U_PORT_L4_VDEC_RG_CTRL_DMA_EXT>; + dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; + clocks = <&topckgen CLK_TOP_VDEC_SEL>, + <&vdecsys CLK_VDEC_VDEC>, + <&vdecsys CLK_VDEC_LAT>, + <&vdecsys CLK_VDEC_LARB1>, + <&topckgen CLK_TOP_MAINPLL_D4>; + clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-lat", + "vdec-vdec", "vdec-top"; + assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>; + assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>; + power-domains = <&spm MT8192_POWER_DOMAIN_VDEC2>; + };
Adds decoder dt-bindings for mt8192. Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> --- v5: no changes This patch depends on "Mediatek MT8192 clock support"[1]. The definition of decoder clocks are in mt8192-clk.h, need to include them in case of build fail [1]. [1]https://patchwork.kernel.org/project/linux-mediatek/list/?series=511175 --- .../media/mediatek,vcodec-comp-decoder.yaml | 172 ++++++++++++++++++ 1 file changed, 172 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml