[PATCH v2 0/5] Add ARM Mali Midgard device tree bindings and gpu node for rk3288

[PATCH v2 0/5] Add ARM Mali Midgard device tree bindings and gpu node for rk3288

[Guillaume Tucker]

The ARM Mali Midgard GPU kernel driver is only available
out-of-tree and is not going to be merged in its current form.
However, it would be useful to have its device tree bindings
merged.  In particular, this would enable distributions to create
working driver packages (dkms...) without having to patch the
kernel.

The bindings for the earlier Mali Utgard GPU family have already
been merged, so this is essentially the same scenario but for
newer GPUs (Mali-T604 ~ Mali-T880).

This series of patches first imports the bindings from the latest
driver release with some clean-up then adds a gpu node for the
rk3288 SoC.  This was successfully tested on Radxa Rock2 Square,
Firefly, Veyron Minnie and Jerry boards board using Mali kernel
driver r16p0 and r12p0 user-space binary.

Changes since v1:
- enabled gpu on rk3288-veyron boards

Enric Balletbo i Serra (1):
  ARM: dts: rockchip: enable ARM Mali GPU on rk3288-veyron

Guillaume Tucker (4):
  dt-bindings: gpu: add bindings for the ARM Mali Midgard GPU
  ARM: dts: rockchip: add ARM Mali GPU node for rk3288
  ARM: dts: rockchip: enable ARM Mali GPU on rk3288-rock2-som
  ARM: dts: rockchip: enable ARM Mali GPU on rk3288-firefly

 .../devicetree/bindings/gpu/arm,mali-midgard.txt   | 53 ++++++++++++++++++++++
 arch/arm/boot/dts/rk3288-firefly.dtsi              |  5 ++
 arch/arm/boot/dts/rk3288-rock2-som.dtsi            |  5 ++
 arch/arm/boot/dts/rk3288-veyron.dtsi               |  5 ++
 arch/arm/boot/dts/rk3288.dtsi                      | 23 ++++++++++
 5 files changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt

--
2.11.0

[PATCH v2 1/5] dt-bindings: gpu: add bindings for the ARM Mali Midgard GPU

[Guillaume Tucker]

The ARM Mali Midgard GPU family is present in a number of SoCs
from many different vendors such as Samsung Exynos and Rockchip.

Import the device tree bindings documentation from the r16p0
release of the Mali Midgard GPU kernel driver:

  https://developer.arm.com/-/media/Files/downloads/mali-drivers/kernel/mali-midgard-gpu/TX011-SW-99002-r16p0-00rel0.tgz

The following optional bindings have been omitted in this initial
version as they are only used in very specific cases:

  * snoop_enable_smc
  * snoop_disable_smc
  * jm_config
  * power_model
  * system-coherency
  * ipa-model

The example has been simplified accordingly.

The compatible string definition has been limited to
"arm,mali-midgard" to avoid checkpatch.pl warnings and to match
what the driver actually expects (as of r16p0 out-of-tree).

CC: John Reitan <john.reitan@arm.com>
Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
---
 .../devicetree/bindings/gpu/arm,mali-midgard.txt   | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt

diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
new file mode 100644
index 000000000000..da8fc6d21bbf
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
@@ -0,0 +1,53 @@
+#
+# (C) COPYRIGHT 2013-2016 ARM Limited.
+# Copyright (C) 2017 Collabora Ltd
+#
+# This program is free software and is provided to you under the terms of the
+# GNU General Public License version 2 as published by the Free Software
+# Foundation, and any use by you of this program is subject to the terms
+# of such GNU licence.
+#
+
+
+ARM Mali Midgard GPU
+====================
+
+Required properties:
+
+- compatible : Should be "arm,mali-midgard".
+- reg : Physical base address of the device and length of the register area.
+- interrupts : Contains the three IRQ lines required by Mali Midgard devices.
+- interrupt-names : Contains the names of IRQ resources in the order they were
+  provided in the interrupts property. Must contain: "JOB, "MMU", "GPU".
+
+Optional:
+
+- clocks : Phandle to clock for the Mali Midgard device.
+- clock-names : Shall be "clk_mali".
+- mali-supply : Phandle to regulator for the Mali device. Refer to
+  Documentation/devicetree/bindings/regulator/regulator.txt for details.
+- operating-points : Refer to Documentation/devicetree/bindings/power/opp.txt
+  for details.
+
+Example for a Mali-T602:
+
+gpu at 0xfc010000 {
+	compatible = "arm,mali-midgard";
+	reg = <0xfc010000 0x4000>;
+	interrupts = <0 36 4>, <0 37 4>, <0 38 4>;
+	interrupt-names = "JOB", "MMU", "GPU";
+
+	clocks = <&pclk_mali>;
+	clock-names = "clk_mali";
+	mali-supply = <&vdd_mali>;
+	operating-points = <
+		/* KHz   uV */
+		533000 1250000,
+		450000 1150000,
+		400000 1125000,
+		350000 1075000,
+		266000 1025000,
+		160000  925000,
+		100000  912500,
+	>;
+};
-- 
2.11.0

[PATCH v2 2/5] ARM: dts: rockchip: add ARM Mali GPU node for rk3288

[Guillaume Tucker]

Add Mali GPU device tree node for the rk3288 SoC, with devfreq
opp table.

Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
---
 arch/arm/boot/dts/rk3288.dtsi | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index df8a0dbe9d91..213224ea309e 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -43,6 +43,7 @@
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/pinctrl/rockchip.h>
 #include <dt-bindings/clock/rk3288-cru.h>
+#include <dt-bindings/power/rk3288-power.h>
 #include <dt-bindings/thermal/thermal.h>
 #include <dt-bindings/power/rk3288-power.h>
 #include <dt-bindings/soc/rockchip,boot-mode.h>
@@ -227,6 +228,28 @@
 		ports = <&vopl_out>, <&vopb_out>;
 	};
 
+	gpu: mali at ffa30000 {
+		compatible = "arm,mali-midgard";
+		reg = <0xffa30000 0x10000>;
+		interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "JOB", "MMU", "GPU";
+		clocks = <&cru ACLK_GPU>;
+		clock-names = "clk_mali";
+		operating-points = <
+			/* KHz uV */
+			100000 950000
+			200000 950000
+			300000 1000000
+			400000 1100000
+			500000 1200000
+			600000 1250000
+		>;
+		power-domains = <&power RK3288_PD_GPU>;
+		status = "disabled";
+	};
+
 	sdmmc: dwmmc at ff0c0000 {
 		compatible = "rockchip,rk3288-dw-mshc";
 		max-frequency = <150000000>;
-- 
2.11.0

[PATCH v2 3/5] ARM: dts: rockchip: enable ARM Mali GPU on rk3288-rock2-som

[Guillaume Tucker]

Add reference to the Mali GPU device tree node on the
rk3288-rock2-som platform.  Tested on a Radxa Rock2 Square board.

Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
---
 arch/arm/boot/dts/rk3288-rock2-som.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-rock2-som.dtsi b/arch/arm/boot/dts/rk3288-rock2-som.dtsi
index 1c0bbc9b928b..f694867fa46a 100644
--- a/arch/arm/boot/dts/rk3288-rock2-som.dtsi
+++ b/arch/arm/boot/dts/rk3288-rock2-som.dtsi
@@ -301,3 +301,8 @@
 &wdt {
 	status = "okay";
 };
+
+&gpu {
+	mali-supply = <&vdd_gpu>;
+	status = "okay";
+};
-- 
2.11.0

[PATCH v2 4/5] ARM: dts: rockchip: enable ARM Mali GPU on rk3288-firefly

[Guillaume Tucker]

Add reference to the Mali GPU device tree node on rk3288-firefly.
Tested on Firefly board.

Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
---
 arch/arm/boot/dts/rk3288-firefly.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-firefly.dtsi b/arch/arm/boot/dts/rk3288-firefly.dtsi
index 10793ac18599..f520589493b4 100644
--- a/arch/arm/boot/dts/rk3288-firefly.dtsi
+++ b/arch/arm/boot/dts/rk3288-firefly.dtsi
@@ -594,3 +594,8 @@
 &wdt {
 	status = "okay";
 };
+
+&gpu {
+	mali-supply = <&vdd_gpu>;
+	status = "okay";
+};
-- 
2.11.0

[PATCH v2 5/5] ARM: dts: rockchip: enable ARM Mali GPU on rk3288-veyron

[Guillaume Tucker]

From: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Add reference to the Mali GPU device tree node on rk3288-veyron.
Tested on Minnie and Jerry boards.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
---
 arch/arm/boot/dts/rk3288-veyron.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-veyron.dtsi b/arch/arm/boot/dts/rk3288-veyron.dtsi
index 5d1eb0a25827..9847d5c6db3b 100644
--- a/arch/arm/boot/dts/rk3288-veyron.dtsi
+++ b/arch/arm/boot/dts/rk3288-veyron.dtsi
@@ -447,6 +447,11 @@
 	status = "okay";
 };
 
+&gpu {
+	mali-supply = <&vdd_gpu>;
+	status = "okay";
+};
+
 &wdt {
 	status = "okay";
 };
-- 
2.11.0

[PATCH v2 2/5] ARM: dts: rockchip: add ARM Mali GPU node for rk3288

[Enric Balletbo i Serra]

On 02/04/17 09:59, Guillaume Tucker wrote:
> Add Mali GPU device tree node for the rk3288 SoC, with devfreq
> opp table.
> 
> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
> ---
>  arch/arm/boot/dts/rk3288.dtsi | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index df8a0dbe9d91..213224ea309e 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -43,6 +43,7 @@
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/pinctrl/rockchip.h>
>  #include <dt-bindings/clock/rk3288-cru.h>
> +#include <dt-bindings/power/rk3288-power.h>
>  #include <dt-bindings/thermal/thermal.h>
>  #include <dt-bindings/power/rk3288-power.h>
>  #include <dt-bindings/soc/rockchip,boot-mode.h>
> @@ -227,6 +228,28 @@
>  		ports = <&vopl_out>, <&vopb_out>;
>  	};
>  
> +	gpu: mali at ffa30000 {
> +		compatible = "arm,mali-midgard";
> +		reg = <0xffa30000 0x10000>;
> +		interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-names = "JOB", "MMU", "GPU";
> +		clocks = <&cru ACLK_GPU>;
> +		clock-names = "clk_mali";
> +		operating-points = <
> +			/* KHz uV */
> +			100000 950000
> +			200000 950000
> +			300000 1000000
> +			400000 1100000
> +			500000 1200000
> +			600000 1250000
> +		>;
> +		power-domains = <&power RK3288_PD_GPU>;
> +		status = "disabled";
> +	};
> +
>  	sdmmc: dwmmc at ff0c0000 {
>  		compatible = "rockchip,rk3288-dw-mshc";
>  		max-frequency = <150000000>;
> 

Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

[PATCH v2 1/5] dt-bindings: gpu: add bindings for the ARM Mali Midgard GPU

[Neil Armstrong]

On 04/02/2017 09:59 AM, Guillaume Tucker wrote:
> The ARM Mali Midgard GPU family is present in a number of SoCs
> from many different vendors such as Samsung Exynos and Rockchip.
> 
> Import the device tree bindings documentation from the r16p0
> release of the Mali Midgard GPU kernel driver:
> 
>   https://developer.arm.com/-/media/Files/downloads/mali-drivers/kernel/mali-midgard-gpu/TX011-SW-99002-r16p0-00rel0.tgz
> 
> The following optional bindings have been omitted in this initial
> version as they are only used in very specific cases:
> 
>   * snoop_enable_smc
>   * snoop_disable_smc
>   * jm_config
>   * power_model
>   * system-coherency
>   * ipa-model
> 
> The example has been simplified accordingly.
> 
> The compatible string definition has been limited to
> "arm,mali-midgard" to avoid checkpatch.pl warnings and to match
> what the driver actually expects (as of r16p0 out-of-tree).
> 
> CC: John Reitan <john.reitan@arm.com>
> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
> ---
>  .../devicetree/bindings/gpu/arm,mali-midgard.txt   | 53 ++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
> new file mode 100644
> index 000000000000..da8fc6d21bbf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
> @@ -0,0 +1,53 @@
> +#
> +# (C) COPYRIGHT 2013-2016 ARM Limited.
> +# Copyright (C) 2017 Collabora Ltd
> +#
> +# This program is free software and is provided to you under the terms of the
> +# GNU General Public License version 2 as published by the Free Software
> +# Foundation, and any use by you of this program is subject to the terms
> +# of such GNU licence.
> +#
Hi Guillaume,

This is unnecessary, please remove.

> +
> +
> +ARM Mali Midgard GPU
> +====================
> +
> +Required properties:
> +
> +- compatible : Should be "arm,mali-midgard".
> +- reg : Physical base address of the device and length of the register area.
> +- interrupts : Contains the three IRQ lines required by Mali Midgard devices.
> +- interrupt-names : Contains the names of IRQ resources in the order they were
> +  provided in the interrupts property. Must contain: "JOB, "MMU", "GPU".


Please follow the bindings introduced for the utgard family :
https://patchwork.kernel.org/patch/9553745/

- an entry for each mali-midgard revision, i.e. "arm,mali-t820"
- an entry for each vendor specific wrapping if necessary, i.e. "amlogic,meson-gxm-mali"
- low-case for interrupt names

> +
> +Optional:
> +
> +- clocks : Phandle to clock for the Mali Midgard device.
> +- clock-names : Shall be "clk_mali".
> +- mali-supply : Phandle to regulator for the Mali device. Refer to
> +  Documentation/devicetree/bindings/regulator/regulator.txt for details.
> +- operating-points : Refer to Documentation/devicetree/bindings/power/opp.txt
> +  for details.

Please add :
   * Must be one of the following:
      "arm,mali-t820"
   * And, optionally, one of the vendor specific compatible:
      "amlogic,meson-gxm-mali"

with my Ack for the amlogic platform.

> +
> +Example for a Mali-T602:
> +
> +gpu at 0xfc010000 {
> +	compatible = "arm,mali-midgard";
> +	reg = <0xfc010000 0x4000>;
> +	interrupts = <0 36 4>, <0 37 4>, <0 38 4>;
> +	interrupt-names = "JOB", "MMU", "GPU";
> +
> +	clocks = <&pclk_mali>;
> +	clock-names = "clk_mali";
> +	mali-supply = <&vdd_mali>;
> +	operating-points = <
> +		/* KHz   uV */
> +		533000 1250000,
> +		450000 1150000,
> +		400000 1125000,
> +		350000 1075000,
> +		266000 1025000,
> +		160000  925000,
> +		100000  912500,
> +	>;
> +};
> 

Thanks,
Neil

[PATCH v2 0/5] Add ARM Mali Midgard device tree bindings and gpu node for rk3288

[Heiko Stübner]

Hi Guillaume,

please keep in mind to also add me (as Rockchip maintainer) as recipient in 
further submissions - as also listed by scripts/get_maintainer.pl.


Thanks
Heiko

Am Sonntag, 2. April 2017, 08:59:43 CEST schrieb Guillaume Tucker:
> The ARM Mali Midgard GPU kernel driver is only available
> out-of-tree and is not going to be merged in its current form.
> However, it would be useful to have its device tree bindings
> merged.  In particular, this would enable distributions to create
> working driver packages (dkms...) without having to patch the
> kernel.
> 
> The bindings for the earlier Mali Utgard GPU family have already
> been merged, so this is essentially the same scenario but for
> newer GPUs (Mali-T604 ~ Mali-T880).
> 
> This series of patches first imports the bindings from the latest
> driver release with some clean-up then adds a gpu node for the
> rk3288 SoC.  This was successfully tested on Radxa Rock2 Square,
> Firefly, Veyron Minnie and Jerry boards board using Mali kernel
> driver r16p0 and r12p0 user-space binary.
> 
> Changes since v1:
> - enabled gpu on rk3288-veyron boards
> 
> Enric Balletbo i Serra (1):
>   ARM: dts: rockchip: enable ARM Mali GPU on rk3288-veyron
> 
> Guillaume Tucker (4):
>   dt-bindings: gpu: add bindings for the ARM Mali Midgard GPU
>   ARM: dts: rockchip: add ARM Mali GPU node for rk3288
>   ARM: dts: rockchip: enable ARM Mali GPU on rk3288-rock2-som
>   ARM: dts: rockchip: enable ARM Mali GPU on rk3288-firefly
> 
>  .../devicetree/bindings/gpu/arm,mali-midgard.txt   | 53
> ++++++++++++++++++++++ arch/arm/boot/dts/rk3288-firefly.dtsi              |
>  5 ++
>  arch/arm/boot/dts/rk3288-rock2-som.dtsi            |  5 ++
>  arch/arm/boot/dts/rk3288-veyron.dtsi               |  5 ++
>  arch/arm/boot/dts/rk3288.dtsi                      | 23 ++++++++++
>  5 files changed, 91 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
> 
> --
> 2.11.0
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 1/5] dt-bindings: gpu: add bindings for the ARM Mali Midgard GPU

[Rob Herring]

On Sun, Apr 02, 2017 at 08:59:44AM +0100, Guillaume Tucker wrote:
> The ARM Mali Midgard GPU family is present in a number of SoCs
> from many different vendors such as Samsung Exynos and Rockchip.
> 
> Import the device tree bindings documentation from the r16p0
> release of the Mali Midgard GPU kernel driver:
> 
>   https://developer.arm.com/-/media/Files/downloads/mali-drivers/kernel/mali-midgard-gpu/TX011-SW-99002-r16p0-00rel0.tgz
> 
> The following optional bindings have been omitted in this initial
> version as they are only used in very specific cases:
> 
>   * snoop_enable_smc
>   * snoop_disable_smc
>   * jm_config
>   * power_model
>   * system-coherency
>   * ipa-model
> 
> The example has been simplified accordingly.
> 
> The compatible string definition has been limited to
> "arm,mali-midgard" to avoid checkpatch.pl warnings and to match
> what the driver actually expects (as of r16p0 out-of-tree).
> 
> CC: John Reitan <john.reitan@arm.com>
> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
> ---
>  .../devicetree/bindings/gpu/arm,mali-midgard.txt   | 53 ++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
> new file mode 100644
> index 000000000000..da8fc6d21bbf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
> @@ -0,0 +1,53 @@
> +#
> +# (C) COPYRIGHT 2013-2016 ARM Limited.
> +# Copyright (C) 2017 Collabora Ltd
> +#
> +# This program is free software and is provided to you under the terms of the
> +# GNU General Public License version 2 as published by the Free Software
> +# Foundation, and any use by you of this program is subject to the terms
> +# of such GNU licence.
> +#
> +
> +
> +ARM Mali Midgard GPU
> +====================
> +
> +Required properties:
> +
> +- compatible : Should be "arm,mali-midgard".

As Neil said...

> +- reg : Physical base address of the device and length of the register area.
> +- interrupts : Contains the three IRQ lines required by Mali Midgard devices.
> +- interrupt-names : Contains the names of IRQ resources in the order they were
> +  provided in the interrupts property. Must contain: "JOB, "MMU", "GPU".
> +
> +Optional:
> +
> +- clocks : Phandle to clock for the Mali Midgard device.
> +- clock-names : Shall be "clk_mali".

"clk_" is redundant. Actually, if there is only 1 clock, then just drop 
names. But there's not at least a core and bus clock?

> +- mali-supply : Phandle to regulator for the Mali device. Refer to
> +  Documentation/devicetree/bindings/regulator/regulator.txt for details.
> +- operating-points : Refer to Documentation/devicetree/bindings/power/opp.txt
> +  for details.

Is this going to be sufficient vs. operating-points-v2? Or should it be 
a power domain with OPPs? 

> +
> +Example for a Mali-T602:
> +
> +gpu at 0xfc010000 {

Drop the '0x'.

> +	compatible = "arm,mali-midgard";
> +	reg = <0xfc010000 0x4000>;
> +	interrupts = <0 36 4>, <0 37 4>, <0 38 4>;
> +	interrupt-names = "JOB", "MMU", "GPU";
> +
> +	clocks = <&pclk_mali>;
> +	clock-names = "clk_mali";
> +	mali-supply = <&vdd_mali>;
> +	operating-points = <
> +		/* KHz   uV */
> +		533000 1250000,
> +		450000 1150000,
> +		400000 1125000,
> +		350000 1075000,
> +		266000 1025000,
> +		160000  925000,
> +		100000  912500,
> +	>;
> +};
> -- 
> 2.11.0
> 

[PATCH v2 1/5] dt-bindings: gpu: add bindings for the ARM Mali Midgard GPU

[Guillaume Tucker]

On 03/04/17 09:12, Neil Armstrong wrote:
> On 04/02/2017 09:59 AM, Guillaume Tucker wrote:
>> The ARM Mali Midgard GPU family is present in a number of SoCs
>> from many different vendors such as Samsung Exynos and Rockchip.
>>
>> Import the device tree bindings documentation from the r16p0
>> release of the Mali Midgard GPU kernel driver:
>>
>>   https://developer.arm.com/-/media/Files/downloads/mali-drivers/kernel/mali-midgard-gpu/TX011-SW-99002-r16p0-00rel0.tgz
>>
>> The following optional bindings have been omitted in this initial
>> version as they are only used in very specific cases:
>>
>>   * snoop_enable_smc
>>   * snoop_disable_smc
>>   * jm_config
>>   * power_model
>>   * system-coherency
>>   * ipa-model
>>
>> The example has been simplified accordingly.
>>
>> The compatible string definition has been limited to
>> "arm,mali-midgard" to avoid checkpatch.pl warnings and to match
>> what the driver actually expects (as of r16p0 out-of-tree).
>>
>> CC: John Reitan <john.reitan@arm.com>
>> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
>> ---
>>  .../devicetree/bindings/gpu/arm,mali-midgard.txt   | 53 ++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
>> new file mode 100644
>> index 000000000000..da8fc6d21bbf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
>> @@ -0,0 +1,53 @@
>> +#
>> +# (C) COPYRIGHT 2013-2016 ARM Limited.
>> +# Copyright (C) 2017 Collabora Ltd
>> +#
>> +# This program is free software and is provided to you under the terms of the
>> +# GNU General Public License version 2 as published by the Free Software
>> +# Foundation, and any use by you of this program is subject to the terms
>> +# of such GNU licence.
>> +#
> Hi Guillaume,
> This is unnecessary, please remove.

Hi Neil,

I see most other documentation files don't have such a header,
including the arm,mali-utgard.txt one.  I left it in my patch
after copying the file from the driver tarball as removing it
didn't seem right from a GPL and copyright point of view.  If
it's safe in practice to remove it then fine.

>> +
>> +
>> +ARM Mali Midgard GPU
>> +====================
>> +
>> +Required properties:
>> +
>> +- compatible : Should be "arm,mali-midgard".
>> +- reg : Physical base address of the device and length of the register area.
>> +- interrupts : Contains the three IRQ lines required by Mali Midgard devices.
>> +- interrupt-names : Contains the names of IRQ resources in the order they were
>> +  provided in the interrupts property. Must contain: "JOB, "MMU", "GPU".
>
>
> Please follow the bindings introduced for the utgard family :
> https://patchwork.kernel.org/patch/9553745/
>
> - an entry for each mali-midgard revision, i.e. "arm,mali-t820"

Sure.  It's a bit more complicated with Midgard (more variants,
some have the number of cores in the last digit...) but it should
be possible to put together a suitable list in v3.

> - an entry for each vendor specific wrapping if necessary, i.e. "amlogic,meson-gxm-mali"

Well, fine although I'm a bit confused about this - please see
below.

> - low-case for interrupt names

OK, can change that in v3.  It means however that the out-of-tree
driver will need to be patched as it's looking for these names in
capital letters.  This shouldn't be a big issue but adds a bit of
work to anyone maintaining a kernel driver package.

>> +
>> +Optional:
>> +
>> +- clocks : Phandle to clock for the Mali Midgard device.
>> +- clock-names : Shall be "clk_mali".
>> +- mali-supply : Phandle to regulator for the Mali device. Refer to
>> +  Documentation/devicetree/bindings/regulator/regulator.txt for details.
>> +- operating-points : Refer to Documentation/devicetree/bindings/power/opp.txt
>> +  for details.
>
> Please add :
>    * Must be one of the following:
>       "arm,mali-t820"
>    * And, optionally, one of the vendor specific compatible:
>       "amlogic,meson-gxm-mali"
>
> with my Ack for the amlogic platform.

It seems to me that as long as the GPU architecture hasn't been
modified (I don't think I've ever encountered such a case) then
it has to be a standard ARM Mali type regardless of the SoC
vendor.  So unless a Mali-T820 in the Amlogic S912 SoC is not the
same as a T820 in a different SoC, please forgive me but I don't
understand why a vendor compatible string is needed.  My main
concern is that it's going to be very hard to keep that list
up-to-date with all existing Midgard SoC variants.  If do we need
to add vendor compatible strings to correctly describe the
hardware then I'm happy to add the amlogic one in my patch v3; I
would just like to understand why that's necessary.


Thanks,
Guillaume

[PATCH v2 1/5] dt-bindings: gpu: add bindings for the ARM Mali Midgard GPU

[Heiko Stübner]

Hi Guillaume,

Am Dienstag, 11. April 2017, 18:40:37 CEST schrieb Guillaume Tucker:
> On 03/04/17 09:12, Neil Armstrong wrote:
> > On 04/02/2017 09:59 AM, Guillaume Tucker wrote:
> >> +Optional:
> >> +
> >> +- clocks : Phandle to clock for the Mali Midgard device.
> >> +- clock-names : Shall be "clk_mali".
> >> +- mali-supply : Phandle to regulator for the Mali device. Refer to
> >> +  Documentation/devicetree/bindings/regulator/regulator.txt for details.
> >> +- operating-points : Refer to
> >> Documentation/devicetree/bindings/power/opp.txt +  for details.
> > 
> > Please add :
> >    * Must be one of the following:
> >       "arm,mali-t820"
> >    
> >    * And, optionally, one of the vendor specific compatible:
> >       "amlogic,meson-gxm-mali"
> > 
> > with my Ack for the amlogic platform.
> 
> It seems to me that as long as the GPU architecture hasn't been
> modified (I don't think I've ever encountered such a case) then
> it has to be a standard ARM Mali type regardless of the SoC
> vendor.  So unless a Mali-T820 in the Amlogic S912 SoC is not the
> same as a T820 in a different SoC, please forgive me but I don't
> understand why a vendor compatible string is needed.  My main
> concern is that it's going to be very hard to keep that list
> up-to-date with all existing Midgard SoC variants.  If do we need
> to add vendor compatible strings to correctly describe the
> hardware then I'm happy to add the amlogic one in my patch v3; I
> would just like to understand why that's necessary.

SoC vendors in most cases hook ip blocks into their socs in different
and often strange ways. After all it's not some discrete ic you solder
onto a board, but instead a part of the soc itself.

So in most cases you will have some hooks outside the actual gpu iospace
that can be used to tune different things about how the gpu interacts with
the system. Which is probably also the reason the midgard kernel driver
has this ugly "platform" subdirectory for compile-time platform selection.

On my rk3288 for example we have [0] in the chromeos tree, that handles
the oddities of the midgard on the rk3288 used in a lot of Chromebooks.
There are soc-specific oddities of frequencies, frequency-scaling and
whatnot. And there are also more gpu-specific setting in syscon areas
of the soc (pmu and grf) that can also influence the gpus performance
and might need tweaking at some point.

That doesn't even take into account that there may even be differences
on how things are synthesized that we don't know about. See all the
variants of the dw_hdmi ip block (imx, rockchip, meson [more?]) .

So we really want to have the special compatibles in place, to be prepared
for the future per-soc oddities that always appear :-) .


Heiko

[0] https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/drivers/gpu/arm/midgard/platform/rk/

[PATCH v2 1/5] dt-bindings: gpu: add bindings for the ARM Mali Midgard GPU

[Guillaume Tucker]

Hi Heiko,

On 11/04/17 21:52, Heiko St?bner wrote:
> Hi Guillaume,
>
> Am Dienstag, 11. April 2017, 18:40:37 CEST schrieb Guillaume Tucker:
>> On 03/04/17 09:12, Neil Armstrong wrote:
>>> On 04/02/2017 09:59 AM, Guillaume Tucker wrote:
>>>> +Optional:
>>>> +
>>>> +- clocks : Phandle to clock for the Mali Midgard device.
>>>> +- clock-names : Shall be "clk_mali".
>>>> +- mali-supply : Phandle to regulator for the Mali device. Refer to
>>>> +  Documentation/devicetree/bindings/regulator/regulator.txt for details.
>>>> +- operating-points : Refer to
>>>> Documentation/devicetree/bindings/power/opp.txt +  for details.
>>>
>>> Please add :
>>>    * Must be one of the following:
>>>       "arm,mali-t820"
>>>
>>>    * And, optionally, one of the vendor specific compatible:
>>>       "amlogic,meson-gxm-mali"
>>>
>>> with my Ack for the amlogic platform.
>>
>> It seems to me that as long as the GPU architecture hasn't been
>> modified (I don't think I've ever encountered such a case) then
>> it has to be a standard ARM Mali type regardless of the SoC
>> vendor.  So unless a Mali-T820 in the Amlogic S912 SoC is not the
>> same as a T820 in a different SoC, please forgive me but I don't
>> understand why a vendor compatible string is needed.  My main
>> concern is that it's going to be very hard to keep that list
>> up-to-date with all existing Midgard SoC variants.  If do we need
>> to add vendor compatible strings to correctly describe the
>> hardware then I'm happy to add the amlogic one in my patch v3; I
>> would just like to understand why that's necessary.
>
> SoC vendors in most cases hook ip blocks into their socs in different
> and often strange ways. After all it's not some discrete ic you solder
> onto a board, but instead a part of the soc itself.

Thanks for your explanation.  I see, it's really about special
things that are not supported by the standard Midgard kernel
driver.

> So in most cases you will have some hooks outside the actual gpu iospace
> that can be used to tune different things about how the gpu interacts with
> the system. Which is probably also the reason the midgard kernel driver
> has this ugly "platform" subdirectory for compile-time platform selection.

I see the "platform" directory approach as an old and deprecated
way of supporting platforms, upstreaming the dt bindings goes in
the direction of using solely the device tree to describe the GPU
hardware (i.e. CONFIG_MALI_PLATFORM_DEVICETREE).  If something
quirky is needed in the platform, it should be possible to
support it outside the GPU driver (platform devfreq etc...).

Back to the original intent of enabling distros to make Mali GPU
driver packages, when using the device tree you can have a single
kernel driver package for all Midgard platforms.  When using the
third-party platform sources approach, you need to make an extra
package for each one of them.

So if there is value in supporting platforms that absolutely
require something special due to their hardware GPU integration,
then yes I see why vendor dt bindings might be useful.  However
it seems to me that this should really be an exception and
avoided whenever possible.

> On my rk3288 for example we have [0] in the chromeos tree, that handles
> the oddities of the midgard on the rk3288 used in a lot of Chromebooks.
> There are soc-specific oddities of frequencies, frequency-scaling and
> whatnot. And there are also more gpu-specific setting in syscon areas
> of the soc (pmu and grf) that can also influence the gpus performance
> and might need tweaking at some point.

For the rk3288, this is purely a software implementation issue on
the chromeos-3.14 branch.  With mainline kernel, you can use
devfreq and no platform files at all (that's how I tested these
dt bindings).  So as far as I know, there's no need for a vendor
compatible string on rk3288.

> That doesn't even take into account that there may even be differences
> on how things are synthesized that we don't know about. See all the
> variants of the dw_hdmi ip block (imx, rockchip, meson [more?]) .

I'm not too familiar with that driver, just had a quick look and
it seems to be a different issue as there's a kernel config for
each platform to build separate driver modules.  And it looks
like they are actually needed to cope with variants of the
hardware inside the display processor block, unlike with the Mali
GPU which in principle should always be the same.  I've run this
Midgard driver without any platform files and using devfreq at
least on rk3288 Firefly, Exynos 5422 ODROID-XU3 and Juno and they
all have a vanilla Mali GPU hw block.  It's just wired
differently in each SoC.

> So we really want to have the special compatibles in place, to be prepared
> for the future per-soc oddities that always appear :-) .

How about aiming for the ideal case where vendor-specific things
are not needed and add them if and when they really become
inevitable and worth the cost?

Thanks,
Guillaume

> [0] https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/drivers/gpu/arm/midgard/platform/rk/

[PATCH v2 1/5] dt-bindings: gpu: add bindings for the ARM Mali Midgard GPU

[Neil Armstrong]

Hi Guillaume,

On 04/12/2017 10:25 AM, Guillaume Tucker wrote:
> Hi Heiko,
> 
> On 11/04/17 21:52, Heiko St?bner wrote:
>> Hi Guillaume,
>>
>> Am Dienstag, 11. April 2017, 18:40:37 CEST schrieb Guillaume Tucker:
>>> On 03/04/17 09:12, Neil Armstrong wrote:
>>>> On 04/02/2017 09:59 AM, Guillaume Tucker wrote:
>>>>> +Optional:
>>>>> +
>>>>> +- clocks : Phandle to clock for the Mali Midgard device.
>>>>> +- clock-names : Shall be "clk_mali".
>>>>> +- mali-supply : Phandle to regulator for the Mali device. Refer to
>>>>> +  Documentation/devicetree/bindings/regulator/regulator.txt for details.
>>>>> +- operating-points : Refer to
>>>>> Documentation/devicetree/bindings/power/opp.txt +  for details.
>>>>
>>>> Please add :
>>>>    * Must be one of the following:
>>>>       "arm,mali-t820"
>>>>
>>>>    * And, optionally, one of the vendor specific compatible:
>>>>       "amlogic,meson-gxm-mali"
>>>>
>>>> with my Ack for the amlogic platform.
>>>
>>> It seems to me that as long as the GPU architecture hasn't been
>>> modified (I don't think I've ever encountered such a case) then
>>> it has to be a standard ARM Mali type regardless of the SoC
>>> vendor.  So unless a Mali-T820 in the Amlogic S912 SoC is not the
>>> same as a T820 in a different SoC, please forgive me but I don't
>>> understand why a vendor compatible string is needed.  My main
>>> concern is that it's going to be very hard to keep that list
>>> up-to-date with all existing Midgard SoC variants.  If do we need
>>> to add vendor compatible strings to correctly describe the
>>> hardware then I'm happy to add the amlogic one in my patch v3; I
>>> would just like to understand why that's necessary.
>>
>> SoC vendors in most cases hook ip blocks into their socs in different
>> and often strange ways. After all it's not some discrete ic you solder
>> onto a board, but instead a part of the soc itself.
> 
> Thanks for your explanation.  I see, it's really about special
> things that are not supported by the standard Midgard kernel
> driver.
> 
>> So in most cases you will have some hooks outside the actual gpu iospace
>> that can be used to tune different things about how the gpu interacts with
>> the system. Which is probably also the reason the midgard kernel driver
>> has this ugly "platform" subdirectory for compile-time platform selection.
> 
> I see the "platform" directory approach as an old and deprecated
> way of supporting platforms, upstreaming the dt bindings goes in
> the direction of using solely the device tree to describe the GPU
> hardware (i.e. CONFIG_MALI_PLATFORM_DEVICETREE).  If something
> quirky is needed in the platform, it should be possible to
> support it outside the GPU driver (platform devfreq etc...).

If this was so simple...

This is why the "vendor" compatible is optional.

And on another side, the binding were written by ARM, are may not be
compatible with how the mainline Linux handles these uses-cases.

ARM added some tweaks to handle some weird integration using DT properties,
but this should definitely go in platform specific code instead.

> 
> Back to the original intent of enabling distros to make Mali GPU
> driver packages, when using the device tree you can have a single
> kernel driver package for all Midgard platforms.  When using the
> third-party platform sources approach, you need to make an extra
> package for each one of them.

Well, if the midgard driver was maintained in community-driven way, all vendor
could actually push their platform code, but it's not the case and somehow
never will.

> 
> So if there is value in supporting platforms that absolutely
> require something special due to their hardware GPU integration,
> then yes I see why vendor dt bindings might be useful.  However
> it seems to me that this should really be an exception and
> avoided whenever possible.

I understand you would avoid changing the (horrible) way the midgard
driver handles the DT...

But ultimately, if we consider the eventual integration of the midgard
driver in mainline kernel (because why not ?), the driver would need
a complete re-write to conform the actual driver coding structure,
and so would also need to conform to the DT mainline rules.

> 
>> On my rk3288 for example we have [0] in the chromeos tree, that handles
>> the oddities of the midgard on the rk3288 used in a lot of Chromebooks.
>> There are soc-specific oddities of frequencies, frequency-scaling and
>> whatnot. And there are also more gpu-specific setting in syscon areas
>> of the soc (pmu and grf) that can also influence the gpus performance
>> and might need tweaking at some point.
> 
> For the rk3288, this is purely a software implementation issue on
> the chromeos-3.14 branch.  With mainline kernel, you can use
> devfreq and no platform files at all (that's how I tested these
> dt bindings).  So as far as I know, there's no need for a vendor
> compatible string on rk3288.
> 
>> That doesn't even take into account that there may even be differences
>> on how things are synthesized that we don't know about. See all the
>> variants of the dw_hdmi ip block (imx, rockchip, meson [more?]) .
> 
> I'm not too familiar with that driver, just had a quick look and
> it seems to be a different issue as there's a kernel config for
> each platform to build separate driver modules.  And it looks
> like they are actually needed to cope with variants of the
> hardware inside the display processor block, unlike with the Mali
> GPU which in principle should always be the same.  I've run this
> Midgard driver without any platform files and using devfreq at
> least on rk3288 Firefly, Exynos 5422 ODROID-XU3 and Juno and they
> all have a vanilla Mali GPU hw block.  It's just wired
> differently in each SoC.

Please, do not consider the "driver" in dt-bindings discussion, the
way it's implemented is irrelevant here, but the way the IP is
integrated is crucial.
And trust me, HW designers and integrators are very very creative
guys, and we *need* some platform code to handle integration corner
cases not handled by the "IP" driver.

In the case of "dw-hdmi", the "driver" acts like a "library" that
can be used differently among the various platforms.

I can talk about the "meson" Amlogic integration, in this particular
case Amlogic used their owm PHY and wrapped all the stuff in a way
we need to handle the PHY+HDMI+Wrapper in a single block and then
connect it to the Video pipeline.

For the midgard integration made by Amlogic, the vendor platform code
uses tweaks to start the IP and initialize the clocks.
And this code is part of the "amlogic,meson-gxm-mali" HW, not the
"arm,midgard".

> 
>> So we really want to have the special compatibles in place, to be prepared
>> for the future per-soc oddities that always appear :-) .
> 
> How about aiming for the ideal case where vendor-specific things
> are not needed and add them if and when they really become
> inevitable and worth the cost?

Trust me, an amlogic compatible is needed ;-)

Neil

> 
> Thanks,
> Guillaume
> 
>> [0] https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/drivers/gpu/arm/midgard/platform/rk/
> 

[PATCH v2 1/5] dt-bindings: gpu: add bindings for the ARM Mali Midgard GPU

[Guillaume Tucker]

Hi Neil,

On 12/04/17 09:48, Neil Armstrong wrote:
> Hi Guillaume,
>
> On 04/12/2017 10:25 AM, Guillaume Tucker wrote:
>> Hi Heiko,
>>
>> On 11/04/17 21:52, Heiko St?bner wrote:
>>> Hi Guillaume,
>>>
>>> Am Dienstag, 11. April 2017, 18:40:37 CEST schrieb Guillaume Tucker:
>>>> On 03/04/17 09:12, Neil Armstrong wrote:
>>>>> On 04/02/2017 09:59 AM, Guillaume Tucker wrote:
>>>>>> +Optional:
>>>>>> +
>>>>>> +- clocks : Phandle to clock for the Mali Midgard device.
>>>>>> +- clock-names : Shall be "clk_mali".
>>>>>> +- mali-supply : Phandle to regulator for the Mali device. Refer to
>>>>>> +  Documentation/devicetree/bindings/regulator/regulator.txt for details.
>>>>>> +- operating-points : Refer to
>>>>>> Documentation/devicetree/bindings/power/opp.txt +  for details.
>>>>>
>>>>> Please add :
>>>>>    * Must be one of the following:
>>>>>       "arm,mali-t820"
>>>>>
>>>>>    * And, optionally, one of the vendor specific compatible:
>>>>>       "amlogic,meson-gxm-mali"
>>>>>
>>>>> with my Ack for the amlogic platform.
>>>>
>>>> It seems to me that as long as the GPU architecture hasn't been
>>>> modified (I don't think I've ever encountered such a case) then
>>>> it has to be a standard ARM Mali type regardless of the SoC
>>>> vendor.  So unless a Mali-T820 in the Amlogic S912 SoC is not the
>>>> same as a T820 in a different SoC, please forgive me but I don't
>>>> understand why a vendor compatible string is needed.  My main
>>>> concern is that it's going to be very hard to keep that list
>>>> up-to-date with all existing Midgard SoC variants.  If do we need
>>>> to add vendor compatible strings to correctly describe the
>>>> hardware then I'm happy to add the amlogic one in my patch v3; I
>>>> would just like to understand why that's necessary.
>>>
>>> SoC vendors in most cases hook ip blocks into their socs in different
>>> and often strange ways. After all it's not some discrete ic you solder
>>> onto a board, but instead a part of the soc itself.
>>
>> Thanks for your explanation.  I see, it's really about special
>> things that are not supported by the standard Midgard kernel
>> driver.
>>
>>> So in most cases you will have some hooks outside the actual gpu iospace
>>> that can be used to tune different things about how the gpu interacts with
>>> the system. Which is probably also the reason the midgard kernel driver
>>> has this ugly "platform" subdirectory for compile-time platform selection.
>>
>> I see the "platform" directory approach as an old and deprecated
>> way of supporting platforms, upstreaming the dt bindings goes in
>> the direction of using solely the device tree to describe the GPU
>> hardware (i.e. CONFIG_MALI_PLATFORM_DEVICETREE).  If something
>> quirky is needed in the platform, it should be possible to
>> support it outside the GPU driver (platform devfreq etc...).
>
> If this was so simple...
>
> This is why the "vendor" compatible is optional.
>
> And on another side, the binding were written by ARM, are may not be
> compatible with how the mainline Linux handles these uses-cases.
>
> ARM added some tweaks to handle some weird integration using DT properties,
> but this should definitely go in platform specific code instead.

OK, sorry I was approaching the issue from a completely different
and somewhat more idealistic angle.  My impression is that if the
driver was in mainline then it would be maintained in such a way
that vendor compatible strings would not be required, but this is
all hypothetical.

So in practice, I think I now better understand why vendor
compatible strings may still be needed.  And they're optional, so
harmless to other platforms, so it's all fine with me :)

>> Back to the original intent of enabling distros to make Mali GPU
>> driver packages, when using the device tree you can have a single
>> kernel driver package for all Midgard platforms.  When using the
>> third-party platform sources approach, you need to make an extra
>> package for each one of them.
>
> Well, if the midgard driver was maintained in community-driven way, all vendor
> could actually push their platform code, but it's not the case and somehow
> never will.
>
>>
>> So if there is value in supporting platforms that absolutely
>> require something special due to their hardware GPU integration,
>> then yes I see why vendor dt bindings might be useful.  However
>> it seems to me that this should really be an exception and
>> avoided whenever possible.
>
> I understand you would avoid changing the (horrible) way the midgard
> driver handles the DT...
>
> But ultimately, if we consider the eventual integration of the midgard
> driver in mainline kernel (because why not ?), the driver would need
> a complete re-write to conform the actual driver coding structure,
> and so would also need to conform to the DT mainline rules.

Yes, it's hard to have a discussion without a mainline driver to
back things up.  So I understand the pragmatic approach here.

>>> On my rk3288 for example we have [0] in the chromeos tree, that handles
>>> the oddities of the midgard on the rk3288 used in a lot of Chromebooks.
>>> There are soc-specific oddities of frequencies, frequency-scaling and
>>> whatnot. And there are also more gpu-specific setting in syscon areas
>>> of the soc (pmu and grf) that can also influence the gpus performance
>>> and might need tweaking at some point.
>>
>> For the rk3288, this is purely a software implementation issue on
>> the chromeos-3.14 branch.  With mainline kernel, you can use
>> devfreq and no platform files at all (that's how I tested these
>> dt bindings).  So as far as I know, there's no need for a vendor
>> compatible string on rk3288.
>>
>>> That doesn't even take into account that there may even be differences
>>> on how things are synthesized that we don't know about. See all the
>>> variants of the dw_hdmi ip block (imx, rockchip, meson [more?]) .
>>
>> I'm not too familiar with that driver, just had a quick look and
>> it seems to be a different issue as there's a kernel config for
>> each platform to build separate driver modules.  And it looks
>> like they are actually needed to cope with variants of the
>> hardware inside the display processor block, unlike with the Mali
>> GPU which in principle should always be the same.  I've run this
>> Midgard driver without any platform files and using devfreq at
>> least on rk3288 Firefly, Exynos 5422 ODROID-XU3 and Juno and they
>> all have a vanilla Mali GPU hw block.  It's just wired
>> differently in each SoC.
>
> Please, do not consider the "driver" in dt-bindings discussion, the
> way it's implemented is irrelevant here, but the way the IP is
> integrated is crucial.
> And trust me, HW designers and integrators are very very creative
> guys, and we *need* some platform code to handle integration corner
> cases not handled by the "IP" driver.

Sure, I know this happens...  I can't help but think that if
something quirky is done in the SoC around the GPU then it
shouldn't need to be fixed in the GPU driver.  But that's talking
about the "driver" again, so not relevant.

> In the case of "dw-hdmi", the "driver" acts like a "library" that
> can be used differently among the various platforms.
>
> I can talk about the "meson" Amlogic integration, in this particular
> case Amlogic used their owm PHY and wrapped all the stuff in a way
> we need to handle the PHY+HDMI+Wrapper in a single block and then
> connect it to the Video pipeline.
>
> For the midgard integration made by Amlogic, the vendor platform code
> uses tweaks to start the IP and initialize the clocks.
> And this code is part of the "amlogic,meson-gxm-mali" HW, not the
> "arm,midgard".

Can't this be done in the clock driver or some other early init
stage of the SoC?  But again, that code is not in mainline so
it's a non-issue.

>>> So we really want to have the special compatibles in place, to be prepared
>>> for the future per-soc oddities that always appear :-) .
>>
>> How about aiming for the ideal case where vendor-specific things
>> are not needed and add them if and when they really become
>> inevitable and worth the cost?
>
> Trust me, an amlogic compatible is needed ;-)

Fair enough, will add the optional vendor compatible string in my
patch v3 :)  Glad this was discussed though, thank you.

Guillaume

>>> [0] https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/drivers/gpu/arm/midgard/platform/rk/

[PATCH v2 1/5] dt-bindings: gpu: add bindings for the ARM Mali Midgard GPU

[Guillaume Tucker]

Hi Rob,

On 04/04/17 03:00, Rob Herring wrote:
> On Sun, Apr 02, 2017 at 08:59:44AM +0100, Guillaume Tucker wrote:
>> The ARM Mali Midgard GPU family is present in a number of SoCs
>> from many different vendors such as Samsung Exynos and Rockchip.
>>
>> Import the device tree bindings documentation from the r16p0
>> release of the Mali Midgard GPU kernel driver:
>>
>>   https://developer.arm.com/-/media/Files/downloads/mali-drivers/kernel/mali-midgard-gpu/TX011-SW-99002-r16p0-00rel0.tgz
>>
>> The following optional bindings have been omitted in this initial
>> version as they are only used in very specific cases:
>>
>>   * snoop_enable_smc
>>   * snoop_disable_smc
>>   * jm_config
>>   * power_model
>>   * system-coherency
>>   * ipa-model
>>
>> The example has been simplified accordingly.
>>
>> The compatible string definition has been limited to
>> "arm,mali-midgard" to avoid checkpatch.pl warnings and to match
>> what the driver actually expects (as of r16p0 out-of-tree).
>>
>> CC: John Reitan <john.reitan@arm.com>
>> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
>> ---
>>  .../devicetree/bindings/gpu/arm,mali-midgard.txt   | 53 ++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
>> new file mode 100644
>> index 000000000000..da8fc6d21bbf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
>> @@ -0,0 +1,53 @@
>> +#
>> +# (C) COPYRIGHT 2013-2016 ARM Limited.
>> +# Copyright (C) 2017 Collabora Ltd
>> +#
>> +# This program is free software and is provided to you under the terms of the
>> +# GNU General Public License version 2 as published by the Free Software
>> +# Foundation, and any use by you of this program is subject to the terms
>> +# of such GNU licence.
>> +#
>> +
>> +
>> +ARM Mali Midgard GPU
>> +====================
>> +
>> +Required properties:
>> +
>> +- compatible : Should be "arm,mali-midgard".
>
> As Neil said...

Sure; as discussed in my previous emails.

>> +- reg : Physical base address of the device and length of the register area.
>> +- interrupts : Contains the three IRQ lines required by Mali Midgard devices.
>> +- interrupt-names : Contains the names of IRQ resources in the order they were
>> +  provided in the interrupts property. Must contain: "JOB, "MMU", "GPU".
>> +
>> +Optional:
>> +
>> +- clocks : Phandle to clock for the Mali Midgard device.
>> +- clock-names : Shall be "clk_mali".
>
> "clk_" is redundant. Actually, if there is only 1 clock, then just drop
> names. But there's not at least a core and bus clock?

Only one clock needs to be provided with the Midgard GPU
architecture.  It shares the same bus and system memory as the
CPU.  So I'll remove clock-names in patch v3.

>> +- mali-supply : Phandle to regulator for the Mali device. Refer to
>> +  Documentation/devicetree/bindings/regulator/regulator.txt for details.
>> +- operating-points : Refer to Documentation/devicetree/bindings/power/opp.txt
>> +  for details.
>
> Is this going to be sufficient vs. operating-points-v2? Or should it be
> a power domain with OPPs?

In principle, switching to operating-points-v2 should be very
straightforward.  I have smoke-tested the driver with an
operating-points-v2 table and a phandle to it inside the gpu node
in place of operating-points and it seems to be working fine.  At
least it parsed the OPPs and got initialised correctly.

My understanding is that operating-points (v1) are not deprecated
so we could keep the bindings as-is, but please let me know
otherwise and I can try to address that in my next patch version.
In the documentation, it should only be the case of replacing
operating-points with operating-points-v2.

>> +
>> +Example for a Mali-T602:
>> +
>> +gpu at 0xfc010000 {
>
> Drop the '0x'.

OK, I'm also updating the example with lower-case IRQ names, no
clock-names etc...

Thanks,
Guillaume

[PATCH v2 1/5] dt-bindings: gpu: add bindings for the ARM Mali Midgard GPU

[Guillaume Tucker]

On 18/04/17 10:15, Guillaume Tucker wrote:
> Hi Rob,
>
> On 04/04/17 03:00, Rob Herring wrote:
>> On Sun, Apr 02, 2017 at 08:59:44AM +0100, Guillaume Tucker wrote:

>>> +- operating-points : Refer to Documentation/devicetree/bindings/power/opp.txt
>>> +  for details.
>>
>> Is this going to be sufficient vs. operating-points-v2? Or should it be
>> a power domain with OPPs?
>
> In principle, switching to operating-points-v2 should be very
> straightforward.  I have smoke-tested the driver with an
> operating-points-v2 table and a phandle to it inside the gpu node
> in place of operating-points and it seems to be working fine.  At
> least it parsed the OPPs and got initialised correctly.
>
> My understanding is that operating-points (v1) are not deprecated
> so we could keep the bindings as-is, but please let me know
> otherwise and I can try to address that in my next patch version.
> In the documentation, it should only be the case of replacing
> operating-points with operating-points-v2.

While the opp bindings documentation doesn't mention anything
about operating-points being deprecated, the code and comments in
of.c are pretty clear about this:

	/*
	 * OPPs have two version of bindings now. The older one is deprecated,
	 * try for the new binding first.
	 */

So I shall use operating-points-v2 in my patch v4.

Thanks,
Guillaume

[PATCH v2 1/5] dt-bindings: gpu: add bindings for the ARM Mali Midgard GPU

[Rob Herring]

On Wed, Apr 12, 2017 at 4:36 AM, Guillaume Tucker
<guillaume.tucker@collabora.com> wrote:
> Hi Neil,
>
>
> On 12/04/17 09:48, Neil Armstrong wrote:
>>
>> Hi Guillaume,
>>
>> On 04/12/2017 10:25 AM, Guillaume Tucker wrote:
>>>
>>> Hi Heiko,
>>>
>>> On 11/04/17 21:52, Heiko St?bner wrote:
>>>>
>>>> Hi Guillaume,
>>>>
>>>> Am Dienstag, 11. April 2017, 18:40:37 CEST schrieb Guillaume Tucker:
>>>>>
>>>>> On 03/04/17 09:12, Neil Armstrong wrote:
>>>>>>
>>>>>> On 04/02/2017 09:59 AM, Guillaume Tucker wrote:
>>>>>>>
>>>>>>> +Optional:
>>>>>>> +
>>>>>>> +- clocks : Phandle to clock for the Mali Midgard device.
>>>>>>> +- clock-names : Shall be "clk_mali".
>>>>>>> +- mali-supply : Phandle to regulator for the Mali device. Refer to
>>>>>>> +  Documentation/devicetree/bindings/regulator/regulator.txt for
>>>>>>> details.
>>>>>>> +- operating-points : Refer to
>>>>>>> Documentation/devicetree/bindings/power/opp.txt +  for details.
>>>>>>
>>>>>>
>>>>>> Please add :
>>>>>>    * Must be one of the following:
>>>>>>       "arm,mali-t820"
>>>>>>
>>>>>>    * And, optionally, one of the vendor specific compatible:
>>>>>>       "amlogic,meson-gxm-mali"
>>>>>>
>>>>>> with my Ack for the amlogic platform.
>>>>>
>>>>>
>>>>> It seems to me that as long as the GPU architecture hasn't been
>>>>> modified (I don't think I've ever encountered such a case) then
>>>>> it has to be a standard ARM Mali type regardless of the SoC
>>>>> vendor.  So unless a Mali-T820 in the Amlogic S912 SoC is not the
>>>>> same as a T820 in a different SoC, please forgive me but I don't
>>>>> understand why a vendor compatible string is needed.  My main
>>>>> concern is that it's going to be very hard to keep that list
>>>>> up-to-date with all existing Midgard SoC variants.  If do we need
>>>>> to add vendor compatible strings to correctly describe the
>>>>> hardware then I'm happy to add the amlogic one in my patch v3; I
>>>>> would just like to understand why that's necessary.
>>>>
>>>>
>>>> SoC vendors in most cases hook ip blocks into their socs in different
>>>> and often strange ways. After all it's not some discrete ic you solder
>>>> onto a board, but instead a part of the soc itself.
>>>
>>>
>>> Thanks for your explanation.  I see, it's really about special
>>> things that are not supported by the standard Midgard kernel
>>> driver.
>>>
>>>> So in most cases you will have some hooks outside the actual gpu iospace
>>>> that can be used to tune different things about how the gpu interacts
>>>> with
>>>> the system. Which is probably also the reason the midgard kernel driver
>>>> has this ugly "platform" subdirectory for compile-time platform
>>>> selection.
>>>
>>>
>>> I see the "platform" directory approach as an old and deprecated
>>> way of supporting platforms, upstreaming the dt bindings goes in
>>> the direction of using solely the device tree to describe the GPU
>>> hardware (i.e. CONFIG_MALI_PLATFORM_DEVICETREE).  If something
>>> quirky is needed in the platform, it should be possible to
>>> support it outside the GPU driver (platform devfreq etc...).
>>
>>
>> If this was so simple...
>>
>> This is why the "vendor" compatible is optional.
>>
>> And on another side, the binding were written by ARM, are may not be
>> compatible with how the mainline Linux handles these uses-cases.
>>
>> ARM added some tweaks to handle some weird integration using DT
>> properties,
>> but this should definitely go in platform specific code instead.
>
>
> OK, sorry I was approaching the issue from a completely different
> and somewhat more idealistic angle.  My impression is that if the
> driver was in mainline then it would be maintained in such a way
> that vendor compatible strings would not be required, but this is
> all hypothetical.
>
> So in practice, I think I now better understand why vendor
> compatible strings may still be needed.  And they're optional, so
> harmless to other platforms, so it's all fine with me :)

SoC specific compatibles are required. They are only optional for a
driver to use.

Rob