| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 5 Jul 2022 17:14:45 +0530
From: "Smitha T Murthy" <smitha.t@...sung.com>
To: "'Krzysztof Kozlowski'" <krzysztof.kozlowski@...aro.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-media@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<devicetree@...r.kernel.org>
Cc: <m.szyprowski@...sung.com>, <andrzej.hajda@...el.com>,
<mchehab@...nel.org>, <hverkuil-cisco@...all.nl>,
<ezequiel@...guardiasur.com.ar>, <jernej.skrabec@...il.com>,
<benjamin.gaignard@...labora.com>, <stanimir.varbanov@...aro.org>,
<dillon.minfei@...il.com>, <david.plowman@...pberrypi.com>,
<mark.rutland@....com>, <robh+dt@...nel.org>, <krzk+dt@...nel.org>,
<andi@...zian.org>, <alim.akhtar@...sung.com>,
<aswani.reddy@...sung.com>, <pankaj.dubey@...sung.com>,
<linux-fsd@...la.com>
Subject: RE: [PATCH 02/20] dt-bindings: media: s5p-mfc: Convert s5p-mfc.txt
to new DT schema
> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@...aro.org]
> Sent: Tuesday, May 17, 2022 7:26 PM
> To: Smitha T Murthy <smitha.t@...sung.com>; linux-arm-
> kernel@...ts.infradead.org; linux-media@...r.kernel.org; linux-
> kernel@...r.kernel.org; devicetree@...r.kernel.org
> Cc: m.szyprowski@...sung.com; andrzej.hajda@...el.com;
> mchehab@...nel.org; hverkuil-cisco@...all.nl;
> ezequiel@...guardiasur.com.ar; jernej.skrabec@...il.com;
> benjamin.gaignard@...labora.com; stanimir.varbanov@...aro.org;
> dillon.minfei@...il.com; david.plowman@...pberrypi.com;
> mark.rutland@....com; robh+dt@...nel.org; krzk+dt@...nel.org;
> andi@...zian.org; alim.akhtar@...sung.com; aswani.reddy@...sung.com;
> pankaj.dubey@...sung.com; linux-fsd@...la.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@...la.com
> > Signed-off-by: Smitha T Murthy <smitha.t@...sung.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@...00000 {
> > - 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@...00000 {
> > - compatible = "shared-dma-pool";
> > - no-map;
> > - reg = <0x51000000 0x800000>;
> > - };
> > -
> > - mfc_right: region@...00000 {
> > - compatible = "shared-dma-pool";
> > - no-map;
> > - reg = <0x43000000 0x800000>;
> > - };
> > -};
> > -
> > -Board specific DT entry:
> > -
> > -codec@...00000 {
> > - 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@...nel.org>
> > + - Rob Herring <robh+dt@...nel.org>
> > + - Mark Rutland <mark.rutland@....com>
> > + - Smitha T Murthy <smitha.t@...sung.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@...00000 {
> > + compatible = "shared-dma-pool";
> > + no-map;
> > + reg = <0x84000000 0x800000>;
> > + };
> > +
> > + mfc_right: region@...00000 {
>
> lower case hex addresses, everywhere.
>
Ok
> > + compatible = "shared-dma-pool";
> > + no-map;
> > + reg = <0xA9000000 0x800000>;
> > + };
> > + };
> > +
> > + mfc_0: mfc0@...80000 {
>
> 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
Powered by blists - more mailing lists