RE: [Patch v4 01/11] dt-bindings: media: s5p-mfc: Add mfcv12 variant

[Alim Akhtar]

Hi Aakarsh

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Wednesday, November 1, 2023 1:54 PM
> To: Aakarsh Jain <aakarsh.jain@xxxxxxxxxxx>; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Cc: m.szyprowski@xxxxxxxxxxx; andrzej.hajda@xxxxxxxxx;
> mchehab@xxxxxxxxxx; hverkuil-cisco@xxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; dillon.minfei@xxxxxxxxx;
> david.plowman@xxxxxxxxxxxxxxx; mark.rutland@xxxxxxx;
> robh+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; linux-samsung-
> soc@xxxxxxxxxxxxxxx; andi@xxxxxxxxxxx; gost.dev@xxxxxxxxxxx;
> alim.akhtar@xxxxxxxxxxx; aswani.reddy@xxxxxxxxxxx;
> pankaj.dubey@xxxxxxxxxxx; ajaykumar.rs@xxxxxxxxxxx; linux-
> fsd@xxxxxxxxx
> Subject: Re: [Patch v4 01/11] dt-bindings: media: s5p-mfc: Add mfcv12
> variant
> 
> On 26/10/2023 15:31, Aakarsh Jain wrote:
> > Hello Krzysztof
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> >> Sent: 25 October 2023 18:30
> >> To: Aakarsh Jain <aakarsh.jain@xxxxxxxxxxx>; linux-arm-
> >> kernel@xxxxxxxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx; linux-
> >> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> >> Cc: m.szyprowski@xxxxxxxxxxx; andrzej.hajda@xxxxxxxxx;
> >> mchehab@xxxxxxxxxx; hverkuil-cisco@xxxxxxxxx;
> >> krzysztof.kozlowski+dt@xxxxxxxxxx; dillon.minfei@xxxxxxxxx;
> >> david.plowman@xxxxxxxxxxxxxxx; mark.rutland@xxxxxxx;
> >> robh+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; linux-samsung-
> >> soc@xxxxxxxxxxxxxxx; andi@xxxxxxxxxxx; gost.dev@xxxxxxxxxxx;
> >> alim.akhtar@xxxxxxxxxxx; aswani.reddy@xxxxxxxxxxx;
> >> pankaj.dubey@xxxxxxxxxxx; ajaykumar.rs@xxxxxxxxxxx; linux-
> >> fsd@xxxxxxxxx
> >> Subject: Re: [Patch v4 01/11] dt-bindings: media: s5p-mfc: Add mfcv12
> >> variant
> >>
> >> On 25/10/2023 12:22, Aakarsh Jain wrote:
> >>> Add Tesla FSD MFC(MFC v12) compatible.
> >>>
> >>> Cc: linux-fsd@xxxxxxxxx
> >>> Signed-off-by: Aakarsh Jain <aakarsh.jain@xxxxxxxxxxx>
> >>> ---
> >>
> >> No changelog and your cover letter does not explain what happened
> here.
> >> Specifically, why did you decide to ignore received tag.
> >>
> > Last patch series we had two different patches for schema which was one
> for adding MFCv12 compatible string and other for adding its HW properties.
> > In one of the patches you gave reviewed-by tag. Since mfc dt_schema got
> merged already, and this is relatively  new patch so thought of getting
> reviewed again.
> >
> > Link to those patches:
> > https://patchwork.kernel.org/project/linux-
> media/patch/20221011122516.32135-2-aakarsh.jain@xxxxxxxxxxx/
> > https://patchwork.kernel.org/project/linux-
> media/patch/20221011122516.32135-3-aakarsh.jain@xxxxxxxxxxx/
> >
> > if you are ok, I will add your reviewed-by in next patch series.
> 
> It is okay to drop Reviewed-by tag, but this should be explicitly mentioned in
> the changelog with a reason.
> 
> >
> >>>  .../bindings/media/samsung,s5p-mfc.yaml          | 16
> ++++++++++++++++
> >>>  1 file changed, 16 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/media/samsung,s5p-
> >> mfc.yaml b/Documentation/devicetree/bindings/media/samsung,s5p-
> >> mfc.yaml
> >>> index 084b44582a43..c30eb309f670 100644
> >>> --- a/Documentation/devicetree/bindings/media/samsung,s5p-
> mfc.yaml
> >>> +++ b/Documentation/devicetree/bindings/media/samsung,s5p-
> mfc.yaml
> >>> @@ -24,6 +24,7 @@ properties:
> >>>            - samsung,mfc-v7                # Exynos5420
> >>>            - samsung,mfc-v8                # Exynos5800
> >>>            - samsung,mfc-v10               # Exynos7880
> >>> +          - tesla,fsd-mfc                 # Tesla FSD
> >>>        - items:
> >>>            - enum:
> >>>                - samsung,exynos3250-mfc    # Exynos3250
> >>> @@ -165,6 +166,21 @@ allOf:
> >>>            minItems: 1
> >>>            maxItems: 2
> >>>
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            enum:
> >>> +              - tesla,fsd-mfc
> >>> +    then:
> >>> +      properties:
> >>> +        clocks:
> >>> +          maxItems: 1
> >>> +        clock-names:
> >>> +          items:
> >>> +            - const: mfc
> >>> +        iommus: false
> >>
> >> That's odd. How so? MFC v12 does not support IOMMU?
> >>
> > MFC v12 do support IOMMU. But currently it is not enabled in SW (has
> dependencies on some of the floating dma-mapping patches) and not tested
> on upstream kernel.
> 
> Bindings describe hardware, not software.
> 
> > Current patch sets intend to add support for MFCv12 using reserve
> memory and later patches related to enable iommu will be posted (after
> resolving the dependencies). So I marked iommu property as false.
> > Now what is your suggestion here? Should I keep iommu as false or add
> memory-region as below?
> 
> I expect complete picture of the hardware, not something limited to current
> driver, so for sure iommus must be there.
> 
As Krzysztof mentioned, DT binding should explain all the hardware features supported by SoC / IPs. 
Incase a feature is not enabled for some reason, that need to be handled in the dts file.

> Please wrap your emails according to mailing lists rules.
> 
> Best regards,
> Krzysztof