RE: [PATCH 02/20] dt-bindings: media: s5p-mfc: Convert s5p-mfc.txt to new DT schema

["Smitha T Murthy" <smitha.t@samsung.com>]

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: Tuesday, May 17, 2022 7:26 PM
> To: Smitha T Murthy <smitha.t@samsung.com>; linux-arm-
> kernel@lists.infradead.org; linux-media@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Cc: m.szyprowski@samsung.com; andrzej.hajda@intel.com;
> mchehab@kernel.org; hverkuil-cisco@xs4all.nl;
> ezequiel@vanguardiasur.com.ar; jernej.skrabec@gmail.com;
> benjamin.gaignard@collabora.com; stanimir.varbanov@linaro.org;
> dillon.minfei@gmail.com; david.plowman@raspberrypi.com;
> mark.rutland@arm.com; robh+dt@kernel.org; krzk+dt@kernel.org;
> andi@etezian.org; alim.akhtar@samsung.com; aswani.reddy@samsung.com;
> pankaj.dubey@samsung.com; linux-fsd@tesla.com
> Subject: Re: [PATCH 02/20] dt-bindings: media: s5p-mfc: Convert s5p-mfc.txt
> to new DT schema
> 
> On 17/05/2022 14:55, Smitha T Murthy wrote:
> > Adds DT schema for s5p-mfc in yaml format.
> >
> 
> Thank you for your patch. There is something to discuss/improve.
> 

Thank you for the review. 

> > Cc: linux-fsd@tesla.com
> > Signed-off-by: Smitha T Murthy <smitha.t@samsung.com>
> > ---
> >  .../devicetree/bindings/media/s5p-mfc.txt     | 77 +--------------
> >  .../devicetree/bindings/media/s5p-mfc.yaml    | 98
> +++++++++++++++++++
> >  2 files changed, 99 insertions(+), 76 deletions(-)  create mode
> > 100644 Documentation/devicetree/bindings/media/s5p-mfc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt
> > b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> > index aa54c8159d9f..f00241ed407f 100644
> > --- a/Documentation/devicetree/bindings/media/s5p-mfc.txt
> > +++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> > @@ -1,76 +1 @@
> > -* Samsung Multi Format Codec (MFC)
> > -
> > -Multi Format Codec (MFC) is the IP present in Samsung SoCs which
> > -supports high resolution decoding and encoding functionalities.
> > -The MFC device driver is a v4l2 driver which can encode/decode -video
> > raw/elementary streams and has support for all popular -video codecs.
> > -
> > -Required properties:
> > -  - compatible : value should be either one among the following
> > -	(a) "samsung,mfc-v5" for MFC v5 present in Exynos4 SoCs
> > -	(b) "samsung,mfc-v6" for MFC v6 present in Exynos5 SoCs
> > -	(c) "samsung,mfc-v7" for MFC v7 present in Exynos5420 SoC
> > -	(d) "samsung,mfc-v8" for MFC v8 present in Exynos5800 SoC
> > -	(e) "samsung,exynos5433-mfc" for MFC v8 present in Exynos5433
> SoC
> > -	(f) "samsung,mfc-v10" for MFC v10 present in Exynos7880 SoC
> > -
> > -  - reg : Physical base address of the IP registers and length of memory
> > -	  mapped region.
> > -
> > -  - interrupts : MFC interrupt number to the CPU.
> > -  - clocks : from common clock binding: handle to mfc clock.
> > -  - clock-names : from common clock binding: must contain "mfc",
> > -		  corresponding to entry in the clocks property.
> > -
> > -Optional properties:
> > -  - power-domains : power-domain property defined with a phandle
> > -			   to respective power domain.
> > -  - memory-region : from reserved memory binding: phandles to two
> reserved
> > -	memory regions, first is for "left" mfc memory bus interfaces,
> > -	second if for the "right" mfc memory bus, used when no SYSMMU
> > -	support is available; used only by MFC v5 present in Exynos4 SoCs
> > -
> > -Obsolete properties:
> > -  - samsung,mfc-r, samsung,mfc-l : support removed, please use memory-
> region
> > -	property instead
> > -
> > -
> > -Example:
> > -SoC specific DT entry:
> > -
> > -mfc: codec@13400000 {
> > -	compatible = "samsung,mfc-v5";
> > -	reg = <0x13400000 0x10000>;
> > -	interrupts = <0 94 0>;
> > -	power-domains = <&pd_mfc>;
> > -	clocks = <&clock 273>;
> > -	clock-names = "mfc";
> > -};
> > -
> > -Reserved memory specific DT entry for given board (see reserved
> > memory binding -for more information):
> > -
> > -reserved-memory {
> > -	#address-cells = <1>;
> > -	#size-cells = <1>;
> > -	ranges;
> > -
> > -	mfc_left: region@51000000 {
> > -		compatible = "shared-dma-pool";
> > -		no-map;
> > -		reg = <0x51000000 0x800000>;
> > -	};
> > -
> > -	mfc_right: region@43000000 {
> > -		compatible = "shared-dma-pool";
> > -		no-map;
> > -		reg = <0x43000000 0x800000>;
> > -	};
> > -};
> > -
> > -Board specific DT entry:
> > -
> > -codec@13400000 {
> > -	memory-region = <&mfc_left>, <&mfc_right>;
> > -};
> > +This file has moved to s5p-mfc.yaml
> 
> Instead entirely remove the file.
> 

Ok, I will remove this file.

> > diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.yaml
> > b/Documentation/devicetree/bindings/media/s5p-mfc.yaml
> > new file mode 100644
> > index 000000000000..fff7c7e0d575
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/s5p-mfc.yaml
> > @@ -0,0 +1,98 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id:
> > +https://protect2.fireeye.com/v1/url?k=8b32845f-ea492ed7-8b330f10-
> 74fe
> > +4860018a-302429095d2fce6e&q=1&e=73087855-3649-4be8-a878-
> d487e0ae58f4&
> > +u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fmedia%2Fs5p-
> mfc.yaml%23
> 
> Let's convert the name as well, so "samsung,s5p-mfc.yaml"
> 

Ok, I will change the name in next series.

> > +$schema:
> > +https://protect2.fireeye.com/v1/url?k=a01c09cc-c167a344-a01d8283-74fe
> > +4860018a-b8e3ca9e8a791d49&q=1&e=73087855-3649-4be8-a878-
> d487e0ae58f4&
> > +u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
> > +
> > +title: Samsung Exynos Multi Format Codec (MFC)
> > +
> > +maintainers:
> > +  - Mauro Carvalho Chehab <mchehab@kernel.org>
> > +  - Rob Herring <robh+dt@kernel.org>
> > +  - Mark Rutland <mark.rutland@arm.com>
> > +  - Smitha T Murthy <smitha.t@samsung.com>
> 
> Only people with access to HW, so you can put here Marek and yourself.
> 

Ok, I will change the authors list.

> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - samsung,mfc-v5                  # Exynos4
> > +      - samsung,mfc-v6                  # Exynos5
> > +      - samsung,mfc-v7                  # Exynos5420
> > +      - samsung,mfc-v8                  # Exynos5800
> > +      - samsung,exynos5433-mfc          # Exynos5433
> > +      - samsung,mfc-v10                 # Exynos7880
> 
> Ugh, how MFCv10 appeared here? Since 5433 we moved from versions to
> Soc compatibles as recommended... eh, please follow this convention, don't
> reverse it to other way.
> 
> I propose to deprecated this in next patch and instead use SoC-based
> compatible.
> 

MFCv10 was already mainlined as mfc-v10, maybe for v10 I will add it post this series.
For MFCv12 I will add SoC-based in the next series.

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    description:
> > +      Phandle to MFC IP clock.
> 
> Here and other places: s/Phandle//
> Instead describe what is it, e.g. "MFC IP clock"
> 
> 

Ok noted.

> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    description:
> > +      Must contain clock name (mfc) matching phandle in clocks
> > +      property.
> 
> Skip description, its obvious. Instead list the items.
> 

Ok

> > +    maxItems: 1
> 
> No need, list the items.
> 
> > +
> > +  interrupts:
> > +    description:
> > +      MFC interrupt number to the CPU.
> 
> Skip description, it's obvious.
> 

Ok

> > +    maxItems: 1
> > +
> > +  memory-region:
> > +    description:
> > +      From reserved memory binding phandles to two reserved
> > +      memory regions, first is for "left" mfc memory bus interfaces,
> > +      second if for the "right" mfc memory bus, used when no SYSMMU
> > +      support is available; used only by MFC v5 present in Exynos4 SoCs.
> > +    minItems: 1
> > +    maxItems: 2
> 
> This needs allOf:if:then restricting two items to specific compatible.
> 

Ok, I will make this change.

> > +
> > +  iommus:
> > +    description:
> > +      Include the IOMMU domain MFC belong to.
> 
> Skip description, it's obvious.
> 

Ok

> > +    maxItems: 2
> > +
> 
> What happened to power domains? You also removed them from the
> example... Does this pass dtbs_check?
> 

This file passed the dtbs_check. For MFCv10 and v12 power domains are not required.
I will add in the example.

> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - iommus
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +        /* Reserved memory specific DT entry for given board */
> > +        reserved-memory {
> 
> Wrong indentation. Four spaces. See example schema.
> 

Ok

> > +                #address-cells = <1>;
> > +                #size-cells = <1>;
> > +                ranges;
> > +
> > +                mfc_left: region@84000000 {
> > +                        compatible = "shared-dma-pool";
> > +                        no-map;
> > +                        reg = <0x84000000 0x800000>;
> > +                };
> > +
> > +                mfc_right: region@A9000000 {
> 
> lower case hex addresses, everywhere.
> 

Ok

> > +                        compatible = "shared-dma-pool";
> > +                        no-map;
> > +                        reg = <0xA9000000 0x800000>;
> > +                };
> > +        };
> > +
> > +        mfc_0: mfc0@12880000 {
> 
> Generic node names, so mfc.
> 

Ok

> > +                compatible = "samsung,mfc-v12";
> 
> Does not look like you tested the bindings. Please run `make
> dt_binding_check` (see Documentation/devicetree/bindings/writing-
> schema.rst for instructions).
> Be sure to test your bindings before sending them.
> 

I did do make dtbs and dt_binding_check using v2022.3, I will recheck post these changes.

> > +                reg = <0x12880000 0x10000>;
> > +                clock-names = "mfc";
> > +                interrupts = <0 137 4>;
> 
> Use interrupt defines.
> 

When I use interrupt defines I get errors as "1.	Syntax error: This was due to interrupts field has some macro reference and needed to give absolute value.", hence I gave absolute values.

Regards,
Smitha

> > +                clocks = <&clock_mfc 1>;
> > +                memory-region = <&mfc_left>, <&mfc_right>;
> > +                /* If IOMMU is present use below instead of memory-region
> property */
> > +                iommus = <&smmu_isp 0x1000 0x0>, <&smmu_isp 0x1400 0x0>;
> > +        };
> 
> 
> Best regards,
> Krzysztof