| Message ID | 20220414025023.11516-1-Nick.Fan@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6,1/2] dt-bindings: Add DT schema for Arm Mali Valhall GPU | expand |
Il 14/04/22 04:50, Nick Fan ha scritto: > Add devicetree schema for Arm Mali Valhall GPU > > Define a compatible string for the Mali Valhall GPU > for MediaTek's SoC platform. > > Signed-off-by: Nick Fan <Nick.Fan@mediatek.com> Hello Nick, Unfortunately, this binding is completely wrong. First of all, there's no arm,mali-valhall driver upstream - this will be managed by panfrost later, yes, but right now there's no support. Then, you're also setting opp-microvolt in a way that will never (or, at least, not anytime soon) be supported by the upstream driver, as it manages only one supply for devfreq scaling. Besides, please don't push bindings that have no upstream driver, especially if these are for downstream drivers requiring proprietary components, while a completely open source implementation is in the works. Regards, Angelo
On 14/04/2022 12:51, AngeloGioacchino Del Regno wrote: > Il 14/04/22 04:50, Nick Fan ha scritto: >> Add devicetree schema for Arm Mali Valhall GPU >> >> Define a compatible string for the Mali Valhall GPU >> for MediaTek's SoC platform. >> >> Signed-off-by: Nick Fan <Nick.Fan@mediatek.com> > > Hello Nick, > Unfortunately, this binding is completely wrong. I think that's unfair, although there is room for improvement. > First of all, there's no arm,mali-valhall driver upstream - this will be > managed > by panfrost later, yes, but right now there's no support. We need a binding agreed upon before support can be added. > Then, you're also setting opp-microvolt in a way that will never (or, at > least, > not anytime soon) be supported by the upstream driver, as it manages > only one > supply for devfreq scaling. The mt8183 binding (already in tree) is very similar. The binding also should be describing the hardware not what the driver supports. There are indeed limitations in Panfrost for supporting multiple supplies, but that's something that needs improving in the driver not a reason to block a (presumably correct) description of the hardware. I can't comments on whether the specifics of the mt8192 are correct. > Besides, please don't push bindings that have no upstream driver, > especially if > these are for downstream drivers requiring proprietary components, while a > completely open source implementation is in the works. More constructively, Alyssa has already posted a patch (as part of the series adding driver support) which would extend the existing Bifrost bindings to (pre-CSF) Valhall: https://lore.kernel.org/dri-devel/20220211202728.6146-2-alyssa.rosenzweig@collabora.com/ I'm not sure I see the point of having a separate binding document for Valhall considering the (pre-CSF) hardware is the same from the kernel perspective. Thanks, Steve
On Thu, Apr 14, 2022 at 8:47 PM Steven Price <steven.price@arm.com> wrote: > > On 14/04/2022 12:51, AngeloGioacchino Del Regno wrote: > > Il 14/04/22 04:50, Nick Fan ha scritto: > >> Add devicetree schema for Arm Mali Valhall GPU > >> > >> Define a compatible string for the Mali Valhall GPU > >> for MediaTek's SoC platform. > >> > >> Signed-off-by: Nick Fan <Nick.Fan@mediatek.com> > > > > Hello Nick, > > Unfortunately, this binding is completely wrong. > > I think that's unfair, although there is room for improvement. > > > First of all, there's no arm,mali-valhall driver upstream - this will be > > managed > > by panfrost later, yes, but right now there's no support. > > We need a binding agreed upon before support can be added. +1. I asked them to send an updated binding for their hardware so that we could have a discussion about it and converge on something. > > Then, you're also setting opp-microvolt in a way that will never (or, at > > least, > > not anytime soon) be supported by the upstream driver, as it manages > > only one > > supply for devfreq scaling. > > The mt8183 binding (already in tree) is very similar. The binding also > should be describing the hardware not what the driver supports. There > are indeed limitations in Panfrost for supporting multiple supplies, but > that's something that needs improving in the driver not a reason to > block a (presumably correct) description of the hardware. I can't > comments on whether the specifics of the mt8192 are correct. Having an agreed upon binding also means that we can bring our downstream driver into alignment, instead of having to maintain a device tree fork. And +1 to being able to handle just one supply is a limitation of the driver. Panfrost in its current state would just not enable devfreq if more than supply is given [1]. Looking deeper, the OPP core currently doesn't support more than one regulator for a given device. > > Besides, please don't push bindings that have no upstream driver, > > especially if > > these are for downstream drivers requiring proprietary components, while a > > completely open source implementation is in the works. > > More constructively, Alyssa has already posted a patch (as part of the > series adding driver support) which would extend the existing Bifrost > bindings to (pre-CSF) Valhall: > > https://lore.kernel.org/dri-devel/20220211202728.6146-2-alyssa.rosenzweig@collabora.com/ > > I'm not sure I see the point of having a separate binding document for > Valhall considering the (pre-CSF) hardware is the same from the kernel > perspective. So I suppose the next step should be to move the required MediaTek specific changes into the existing binding instead of having a new one? Separately I think we would need a new binding to spell out the requirements of MediaTek's two supply OPP table? Or maybe this could be in the description of the Mali binding? Thanks ChenYu [1] https://patchwork.freedesktop.org/patch/429782/
diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-valhall.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-valhall.yaml new file mode 100644 index 000000000000..526384d1e3ff --- /dev/null +++ b/Documentation/devicetree/bindings/gpu/arm,mali-valhall.yaml @@ -0,0 +1,201 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (c) 2020 MediaTek Inc. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/gpu/arm,mali-valhall.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ARM Mali Valhall GPU + +maintainers: + - Rob Herring <robh@kernel.org> + +properties: + $nodename: + pattern: '^gpu@[a-f0-9]+$' + + compatible: + items: + - enum: + - mediatek,mt8192-mali + - const: arm,mali-valhall + + reg: + maxItems: 1 + + interrupts: + items: + - description: Job interrupt + - description: MMU interrupt + - description: GPU interrupt + + interrupt-names: + items: + - const: job + - const: mmu + - const: gpu + + clocks: + minItems: 1 + + power-domains: + minItems: 1 + maxItems: 5 + + mali-supply: true + sram-supply: true + + operating-points-v2: true + opp-table: true + + "#cooling-cells": + const: 2 + +required: + - compatible + - reg + - interrupts + - interrupt-names + - clocks + +additionalProperties: false + +allOf: + - if: + properties: + compatible: + contains: + const: mediatek,mt8192-mali + then: + properties: + power-domains: + minItems: 5 + maxItems: 5 + + power-domain-names: + items: + - const: core0 + - const: core1 + - const: core2 + - const: core3 + - const: core4 + + required: + - sram-supply + - power-domains + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + + gpu@13000000 { + compatible = "mediatek,mt8192-mali", "arm,mali-valhall"; + reg = <0x13000000 0x4000>; + interrupts = + <GIC_SPI 363 IRQ_TYPE_LEVEL_HIGH 0>, + <GIC_SPI 364 IRQ_TYPE_LEVEL_HIGH 0>, + <GIC_SPI 365 IRQ_TYPE_LEVEL_HIGH 0>; + interrupt-names = + "gpu", + "mmu", + "job"; + + clocks = <&mfgcfg 0>; + + power-domains = + <&spm 4>, + <&spm 5>, + <&spm 6>, + <&spm 7>, + <&spm 8>; + + operating-points-v2 = <&gpu_opp_table>; + mali-supply = <&mt6315_7_vbuck1>; + sram-supply = <&mt6359_vsram_others_ldo_reg>; + gpu_opp_table: opp_table { + compatible = "operating-points-v2"; + opp-shared; + + opp-358000000 { + opp-hz = /bits/ 64 <358000000>; + opp-microvolt = <606250>, <750000>; + }; + + opp-399000000 { + opp-hz = /bits/ 64 <399000000>; + opp-microvolt = <618750>, <750000>; + }; + + opp-440000000 { + opp-hz = /bits/ 64 <440000000>; + opp-microvolt = <631250>, <750000>; + }; + + opp-482000000 { + opp-hz = /bits/ 64 <482000000>; + opp-microvolt = <643750>, <750000>; + }; + + opp-523000000 { + opp-hz = /bits/ 64 <523000000>; + opp-microvolt = <656250>, <750000>; + }; + + opp-564000000 { + opp-hz = /bits/ 64 <564000000>; + opp-microvolt = <668750>, <750000>; + }; + + opp-605000000 { + opp-hz = /bits/ 64 <605000000>; + opp-microvolt = <681250>, <750000>; + }; + + opp-647000000 { + opp-hz = /bits/ 64 <647000000>; + opp-microvolt = <693750>, <750000>; + }; + + opp-688000000 { + opp-hz = /bits/ 64 <688000000>; + opp-microvolt = <706250>, <750000>; + }; + + opp-724000000 { + opp-hz = /bits/ 64 <724000000>; + opp-microvolt = <725000>, <750000>; + }; + + opp-748000000 { + opp-hz = /bits/ 64 <748000000>; + opp-microvolt = <737500>, <750000>; + }; + + opp-772000000 { + opp-hz = /bits/ 64 <772000000>; + opp-microvolt = <750000>, <750000>; + }; + + opp-795000000 { + opp-hz = /bits/ 64 <795000000>; + opp-microvolt = <762500>, <762500>; + }; + + opp-819000000 { + opp-hz = /bits/ 64 <819000000>; + opp-microvolt = <775000>, <775000>; + }; + + opp-843000000 { + opp-hz = /bits/ 64 <843000000>; + opp-microvolt = <787500>, <787500>; + }; + + opp-866000000 { + opp-hz = /bits/ 64 <866000000>; + opp-microvolt = <800000>, <800000>; + }; + }; + }; +...
Add devicetree schema for Arm Mali Valhall GPU Define a compatible string for the Mali Valhall GPU for MediaTek's SoC platform. Signed-off-by: Nick Fan <Nick.Fan@mediatek.com> --- .../bindings/gpu/arm,mali-valhall.yaml | 201 ++++++++++++++++++ 1 file changed, 201 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpu/arm,mali-valhall.yaml