| Message ID | 20230517145237.295461-8-abailon@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a DRM driver to support AI Processing Unit (APU) | expand |
On Wed, 17 May 2023 16:52:37 +0200, Alexandre Bailon wrote: > This adds the device tree bindings for the APU DRM driver. > > Signed-off-by: Alexandre Bailon <abailon@baylibre.com> > Reviewed-by: Julien Stephan <jstephan@baylibre.com> > --- > .../devicetree/bindings/gpu/mtk,apu-drm.yaml | 38 +++++++++++++++++++ > 1 file changed, 38 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpu/mtk,apu-drm.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: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml: 'maintainers' is a required property hint: Metaschema for devicetree binding documentation from schema $id: http://devicetree.org/meta-schemas/base.yaml# ./Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml: $id: relative path/filename doesn't match actual path or filename expected: http://devicetree.org/schemas/gpu/mtk,apu-drm.yaml# Documentation/devicetree/bindings/gpu/mtk,apu-drm.example.dts:18.15-22.11: Warning (unit_address_vs_reg): /example-0/apu@0: node has a unit name, but no reg or ranges property /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/gpu/mtk,apu-drm.example.dtb: apu@0: remoteproc: [[4294967295, 4294967295]] is too short From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1782720 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, May 17, 2023 at 05:04:00PM +0200, AngeloGioacchino Del Regno wrote: > Il 17/05/23 16:52, Alexandre Bailon ha scritto: > > This adds the device tree bindings for the APU DRM driver. > > > > Signed-off-by: Alexandre Bailon <abailon@baylibre.com> > > Reviewed-by: Julien Stephan <jstephan@baylibre.com> > > --- > > .../devicetree/bindings/gpu/mtk,apu-drm.yaml | 38 +++++++++++++++++++ > > mediatek,mt(model)-apu.yaml > > > 1 file changed, 38 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml > > > > diff --git a/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml b/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml > > new file mode 100644 > > index 000000000000..6f432d3ea478 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml > > @@ -0,0 +1,38 @@ > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/gpu/mediatek,apu-drm.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: AI Processor Unit DRM > > + > > +properties: > > + compatible: > > + const: mediatek,apu-drm > > const: mediatek,mt8195-apu (or whatever else). Aye, and drop the references to DRM in the title field too (and add the vendor name?). > > ...besides, I don't think that this patch even belongs to this series? :-) > Spoiler alert! :-) Well, I do not know what this means - but if it is being respun as part of some other work, a description field should be added to the binding. Cheers, Conor.
On 17/05/2023 16:52, Alexandre Bailon wrote: > This adds the device tree bindings for the APU DRM driver. > > Signed-off-by: Alexandre Bailon <abailon@baylibre.com> > Reviewed-by: Julien Stephan <jstephan@baylibre.com> There are so many errors in this patch... that for sure it was not tested. Reduced review, except what was already said: > --- > .../devicetree/bindings/gpu/mtk,apu-drm.yaml | 38 +++++++++++++++++++ > 1 file changed, 38 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml > > diff --git a/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml b/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml > new file mode 100644 > index 000000000000..6f432d3ea478 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml > @@ -0,0 +1,38 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/gpu/mediatek,apu-drm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: AI Processor Unit DRM > + > +properties: > + compatible: > + const: mediatek,apu-drm drm is not hardware. Drop everywhere or explain the acronym. If you explain it like Linux explains, then: drm is not hardware. > + > + remoteproc: > + maxItems: 2 > + description: > + Handle to remoteproc devices controlling the APU Missing type/ref. Does not look like generic property, so missing vendor prefix. > + > + iova: > + maxItems: 1 > + description: > + Address and size of virtual memory that could used by the APU So it is a reg? > + > +required: > + - compatible > + - remoteproc > + - iova > + > +additionalProperties: false > + > +examples: > + - | > + apu@0 { Where is reg? @0 says you have it... > + compatible = "mediatek,apu-drm"; > + remoteproc = <&vpu0>, <&vpu1>; > + iova = <0 0x60000000 0 0x10000000>; Why would you store virtual address, not real, in DT? Let's say you have some randomization like KASLR. How is it going to work? Drop, it is not hardware property. Best regards, Krzysztof
On 17/05/2023 21:38, Krzysztof Kozlowski wrote: > On 17/05/2023 16:52, Alexandre Bailon wrote: >> This adds the device tree bindings for the APU DRM driver. >> >> Signed-off-by: Alexandre Bailon <abailon@baylibre.com> >> Reviewed-by: Julien Stephan <jstephan@baylibre.com> > > There are so many errors in this patch... that for sure it was not > tested. Reduced review, except what was already said: > >> --- >> .../devicetree/bindings/gpu/mtk,apu-drm.yaml | 38 +++++++++++++++++++ >> 1 file changed, 38 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml >> >> diff --git a/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml b/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml >> new file mode 100644 >> index 000000000000..6f432d3ea478 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml >> @@ -0,0 +1,38 @@ >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/gpu/mediatek,apu-drm.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: AI Processor Unit DRM >> + >> +properties: >> + compatible: >> + const: mediatek,apu-drm > > drm is not hardware. Drop everywhere or explain the acronym. If you > explain it like Linux explains, then: drm is not hardware. > >> + >> + remoteproc: >> + maxItems: 2 >> + description: >> + Handle to remoteproc devices controlling the APU > > Missing type/ref. Does not look like generic property, so missing vendor > prefix. > >> + >> + iova: >> + maxItems: 1 >> + description: >> + Address and size of virtual memory that could used by the APU > > So it is a reg? > >> + >> +required: >> + - compatible >> + - remoteproc >> + - iova >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + apu@0 { > > Where is reg? @0 says you have it... > >> + compatible = "mediatek,apu-drm"; >> + remoteproc = <&vpu0>, <&vpu1>; >> + iova = <0 0x60000000 0 0x10000000>; > > Why would you store virtual address, not real, in DT? Let's say you have > some randomization like KASLR. How is it going to work? Drop, it is not > hardware property. Actually RANDOMIZE_BASE. KASLR randomizes the physical. Best regards, Krzysztof
On 5/17/23 17:04, AngeloGioacchino Del Regno wrote: > Il 17/05/23 16:52, Alexandre Bailon ha scritto: >> This adds the device tree bindings for the APU DRM driver. >> >> Signed-off-by: Alexandre Bailon <abailon@baylibre.com> >> Reviewed-by: Julien Stephan <jstephan@baylibre.com> >> --- >> .../devicetree/bindings/gpu/mtk,apu-drm.yaml | 38 +++++++++++++++++++ > > mediatek,mt(model)-apu.yaml > >> 1 file changed, 38 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml >> >> diff --git a/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml >> b/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml >> new file mode 100644 >> index 000000000000..6f432d3ea478 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml >> @@ -0,0 +1,38 @@ >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/gpu/mediatek,apu-drm.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: AI Processor Unit DRM >> + >> +properties: >> + compatible: >> + const: mediatek,apu-drm > > const: mediatek,mt8195-apu (or whatever else). > > ...besides, I don't think that this patch even belongs to this series? :-) > Spoiler alert! :-) Actually, it does! I forgot to send the patch that adds the platform driver ^^' Thanks, Alexandre > > Cheers, > Angelo > >
diff --git a/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml b/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml new file mode 100644 index 000000000000..6f432d3ea478 --- /dev/null +++ b/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml @@ -0,0 +1,38 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/gpu/mediatek,apu-drm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: AI Processor Unit DRM + +properties: + compatible: + const: mediatek,apu-drm + + remoteproc: + maxItems: 2 + description: + Handle to remoteproc devices controlling the APU + + iova: + maxItems: 1 + description: + Address and size of virtual memory that could used by the APU + +required: + - compatible + - remoteproc + - iova + +additionalProperties: false + +examples: + - | + apu@0 { + compatible = "mediatek,apu-drm"; + remoteproc = <&vpu0>, <&vpu1>; + iova = <0 0x60000000 0 0x10000000>; + }; + +...