Re: [PATCH v3, 03/10] dt-bindings: media: mtk-vcodec: Adds encoder cores dt-bindings for mt8195

[Irui Wang <irui.wang@mediatek.com>]

Dear Rob,

Many thanks for your attention.

On Mon, 2022-03-28 at 08:48 -0500, Rob Herring wrote:
> On Sat, Mar 26, 2022 at 10:00:55AM +0800, Irui Wang wrote:
> > Dear Rob,
> > 
> > Thanks for your review and comments.
> > 
> > On Fri, 2022-03-25 at 15:57 -0500, Rob Herring wrote:
> > > On Thu, Mar 17, 2022 at 04:22:23PM +0800, Irui Wang wrote:
> > > > Adds encoder cores dt-bindings for mt8195.
> > > 
> > > Why?
> > 
> > mt8195 has two h264 encoder cores and we want to enable both of
> > them.
> > the original encoder bindings file is for single core which may can
> > not
> > handle it, so we new a core dt-bindings. We will think about it
> > again
> > about this dt-bindings.
> > > 
> > > > 
> > > > Signed-off-by: Irui Wang <irui.wang@mediatek.com>
> > > > ---
> > > >  .../media/mediatek,vcodec-encoder-core.yaml   | 181
> > > > ++++++++++++++++++
> > > >  .../media/mediatek,vcodec-encoder.yaml        |   1 -
> > > >  2 files changed, 181 insertions(+), 1 deletion(-)
> > > >  create mode 100644
> > > > Documentation/devicetree/bindings/media/mediatek,vcodec-
> > > > encoder-
> > > > core.yaml
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/media/mediatek,vcodec-
> > > > encoder-
> > > > core.yaml
> > > > b/Documentation/devicetree/bindings/media/mediatek,vcodec-
> > > > encoder-
> > > > core.yaml
> > > > new file mode 100644
> > > > index 000000000000..fcfb48900c76
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-
> > > > encoder-core.yaml
> > > > @@ -0,0 +1,181 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: "
> > > > 
http://devicetree.org/schemas/media/mediatek,vcodec-encoder-core.yaml#
> > > > "
> > > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > > +
> > > > +title: Mediatek Video Encoder Accelerator With Multi Core
> > > > +
> > > > +maintainers:
> > > > +  - Irui Wang <irui.wang@mediatek.com>
> > > > +
> > > > +description: |
> > > > +  Mediatek Video Encode is the video encode hardware present
> > > > in
> > > > Mediatek
> > > > +  SoCs which supports high resolution encoding
> > > > functionalities.
> > > > Required
> > > > +  parent and child device node.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    items:
> > > > +      - enum:
> > > > +          - mediatek,mt8195-vcodec-enc
> > > > +
> > > > +  mediatek,scp:
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > +    description: |
> > > > +      The node of system control processor (SCP), using
> > > > +      the remoteproc & rpmsg framework.
> > > > +
> > > > +  mediatek,venc-multi-core:
> > > > +    type: boolean
> > > > +    description: |
> > > > +      Indicates whether the encoder has multiple cores or not.
> > > 
> > > Isn't this implied by how many child nodes you have?
> > 
> > It implies that we have child nodes, distinguish with single
> > core,  
> 
> Just check if you have a child node or not.
Yes.
> 
> 
> > > > +  iommus:
> > > > +    minItems: 1
> > > > +    maxItems: 32
> > > 
> > > You really have up to 32 IOMMUs?
> > 
> > Currently, totally 27 IOMMUs and used 9 of them.
> > > 
> > > > +    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.
> > > > +
> > > > +  "#address-cells":
> > > > +    const: 2
> > > > +
> > > > +  "#size-cells":
> > > > +    const: 2
> > > > +
> > > > +  ranges: true
> > > > +
> > > > +# Required child node:
> > > > +patternProperties:
> > > > +  "^venc-core@[0-9a-f]+$":
> > > > +    type: object
> > > > +    description: |
> > > > +      The video encoder core device node which should be added
> > > > as
> > > > subnodes to
> > > > +      the main venc node.
> > > > +
> > > > +    properties:
> > > > +      compatible:
> > > > +        items:
> > > > +         - const: mediatek,mtk-venc-core
> > > > +
> > > > +      reg:
> > > > +        maxItems: 1
> > > > +
> > > > +      mediatek,core-id:
> > > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > > +        description: |
> > > > +          Current encoder core id.
> > > 
> > > What is this for and what does its value correspond to in the
> > > h/w.
> > > We 
> > > generally don't do made up indices in DT.
> > 
> > It's for encoder core id, core@1a020000 must be core-0, 
> > core@1b020000
> > must be core-1, we add this property in each child node, so we can 
> > get core-id in drivers. If it can't ref "uint32" types yaml, would 
> > you mind giving some more suggestions ?
> 
> I still don't understand why it is needed. What is 'core-0'?
> 
> Is there some functional difference between the cores? If so,
> describe 
> that difference.
> 
> Rob

They are two different pieces of hardware, it's our encoder hardware
design. There are two encoder hardware cores inside MT8195, named core0
and core1(we can rename it, but core id should be declared),
for core0, its module base address is 0x1A02_0000, uses IOMMU
"vdo0_iommu" and power domain "POWER_DOMAIN_VENC",
for core1, its module base address is 0x1B02_0000, uses IOMMU
"vpp_iommu" and power domain "POWER_DOMAIN_VENC_CORE1".
So the two encoder cores have their own base, IRQ, clock, power, etc.
Each core can encode independently, moreover, they can work together
for higher performance. 
We will describe more details in YAML about it if it's OK for you.

Thanks
Best Regards.