[PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[H. Nikolaus Schaller]

* rebased to v5.7-rc1
* added DTS for for a31, a31s, a83t - by Philipp Rossak <embed3d@gmail.com>
* added DTS for "samsung,s5pv210-sgx540-120" - by Jonathan Bakker <xc-racer2@live.ca>
* bindings.yaml fixes:
  - added a31, a31
  - fixes for omap4470
  - jz4780 contains an sgx540-130 and not -120
  - a83t contains an sgx544-115 and not -116
  - removed "additionalProperties: false" because some SoC may need additional properties

PATCH V5 2020-03-29 19:38:32:
* reworked YAML bindings to pass dt_binding_check and be better grouped
* rename all nodes to "gpu: gpu@<address>"
* removed "img,sgx5" from example - suggested by Rob Herring <robh+dt@kernel.org>

PATCH V4 2019-12-17 19:02:11:
* MIPS: DTS: jz4780: removed "img,sgx5" from bindings
* YAML bindings: updated according to suggestions by Rob Herring
* MIPS: DTS: jz4780: insert-sorted gpu node by register address - suggested by Paul Cercueil

PATCH V3 2019-11-24 12:40:33:
* reworked YAML format with help by Rob Herring
* removed .txt binding document
* change compatible "ti,am335x-sgx" to "ti,am3352-sgx" - suggested by Tony Lindgren

PATCH V2 2019-11-07 12:06:17:
* tried to convert bindings to YAML format - suggested by Rob Herring
* added JZ4780 DTS node (proven to load the driver)
* removed timer and img,cores properties until we know we really need them - suggested by Rob Herring

PATCH V1 2019-10-18 20:46:35:

This patch series defines child nodes for the SGX5xx interface inside
different SoC so that a driver can be found and probed by the
compatible strings and can retrieve information about the SGX revision
that is included in a specific SoC. It also defines the interrupt number
to be used by the SGX driver.

There is currently no mainline driver for these GPUs, but a project [1]
is ongoing with the goal to get the open-source part as provided by TI/IMG
and others into drivers/gpu/drm/pvrsgx.

The kernel modules built from this project have successfully demonstrated
to work with the DTS definitions from this patch set on AM335x BeagleBone
Black, DM3730 and OMAP5 Pyra and Droid 4. They partially work on OMAP3530 and
PandaBoard ES but that is likely a problem in the kernel driver or the
(non-free) user-space libraries and binaries.

Wotk for JZ4780 (CI20 board) is in progress and there is potential to extend
this work to e.g. BananaPi-M3 (A83) and  some Intel Poulsbo and CedarView
devices.

[1]: https://github.com/openpvrsgx-devgroup


H. Nikolaus Schaller (8):
  dt-bindings: add img,pvrsgx.yaml for Imagination GPUs
  ARM: DTS: am33xx: add sgx gpu child node
  ARM: DTS: am3517: add sgx gpu child node
  ARM: DTS: omap34xx: add sgx gpu child node
  ARM: DTS: omap36xx: add sgx gpu child node
  ARM: DTS: omap4: add sgx gpu child node
  ARM: DTS: omap5: add sgx gpu child node
  MIPS: DTS: jz4780: add sgx gpu node

Jonathan Bakker (1):
  arm: dts: s5pv210: Add G3D node

Philipp Rossak (3):
  ARM: dts: sun6i: a31: add sgx gpu child node
  ARM: dts: sun6i: a31s: add sgx gpu child node
  ARM: dts: sun8i: a83t: add sgx gpu child node

 .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 122 ++++++++++++++++++
 arch/arm/boot/dts/am33xx.dtsi                 |  11 +-
 arch/arm/boot/dts/am3517.dtsi                 |   9 +-
 arch/arm/boot/dts/omap34xx.dtsi               |  11 +-
 arch/arm/boot/dts/omap36xx.dtsi               |   9 +-
 arch/arm/boot/dts/omap4.dtsi                  |  11 +-
 arch/arm/boot/dts/omap4470.dts                |  15 +++
 arch/arm/boot/dts/omap5.dtsi                  |  11 +-
 arch/arm/boot/dts/s5pv210.dtsi                |  15 +++
 arch/arm/boot/dts/sun6i-a31.dtsi              |  11 ++
 arch/arm/boot/dts/sun6i-a31s.dtsi             |  10 ++
 arch/arm/boot/dts/sun8i-a83t.dtsi             |  11 ++
 arch/mips/boot/dts/ingenic/jz4780.dtsi        |  11 ++
 13 files changed, 229 insertions(+), 28 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
 create mode 100644 arch/arm/boot/dts/omap4470.dts

-- 
2.25.1

[PATCH v6 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

The Imagination PVR/SGX GPU is part of several SoC from
multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo,
Allwinner A83 and others.

With this binding, we describe how the SGX processor is
interfaced to the SoC (registers, interrupt etc.).

In most cases, Clock, Reset and power management is handled
by a parent node or elsewhere (e.g. code in the driver).

Tested by make dt_binding_check dtbs_check

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 122 ++++++++++++++++++
 1 file changed, 122 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml

diff --git a/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
new file mode 100644
index 000000000000..e3a4208dfab1
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
@@ -0,0 +1,122 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpu/img,pvrsgx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Imagination PVR/SGX GPU
+
+maintainers:
+  - H. Nikolaus Schaller <hns@goldelico.com>
+
+description: |+
+  This binding describes the Imagination SGX5 series of 3D accelerators which
+  are found in several different SoC like TI OMAP, Sitara, Ingenic JZ4780,
+  Allwinner A83, and Intel Poulsbo and CedarView and more.
+
+  For an extensive list see: https://en.wikipedia.org/wiki/PowerVR#Implementations
+
+  The SGX node is usually a child node of some DT node belonging to the SoC
+  which handles clocks, reset and general address space mapping of the SGX
+  register area.
+
+properties:
+  compatible:
+    oneOf:
+      - description: SGX530-121 based SoC
+        items:
+          - enum:
+            - ti,omap3-sgx530-121 # BeagleBoard A/B/C, OpenPandora 600MHz and similar
+          - const: img,sgx530-121
+          - const: img,sgx530
+
+      - description: SGX530-125 based SoC
+        items:
+          - enum:
+            - ti,am3352-sgx530-125 # BeagleBone Black
+            - ti,am3517-sgx530-125
+            - ti,am4-sgx530-125
+            - ti,omap3-sgx530-125 # BeagleBoard XM, GTA04, OpenPandora 1GHz and similar
+            - ti,ti81xx-sgx530-125
+          - const: ti,omap3-sgx530-125
+          - const: img,sgx530-125
+          - const: img,sgx530
+
+      - description: SGX535-116 based SoC
+        items:
+          - const: intel,poulsbo-gma500-sgx535 # Atom Z5xx
+          - const: img,sgx535-116
+          - const: img,sgx535
+
+      - description: SGX540-116 based SoC
+        items:
+          - const: intel,medfield-gma-sgx540 # Atom Z24xx
+          - const: img,sgx540-116
+          - const: img,sgx540
+
+      - description: SGX540-120 based SoC
+        items:
+          - enum:
+            - samsung,s5pv210-sgx540-120
+            - ti,omap4-sgx540-120 # Pandaboard, Pandaboard ES and similar
+          - const: img,sgx540-120
+          - const: img,sgx540
+
+      - description: SGX540-130 based SoC
+        items:
+          - enum:
+            - ingenic,jz4780-sgx540-130 # CI20
+          - const: img,sgx540-130
+          - const: img,sgx540
+
+      - description: SGX544-112 based SoC
+        items:
+          - const: "ti,omap4470-sgx544-112
+          - const: img,sgx544-112
+          - const: img,sgx544
+
+      - description: SGX544-115 based SoC
+        items:
+          - enum:
+            - allwinner,sun8i-a31-sgx544-115
+            - allwinner,sun8i-a31s-sgx544-115
+            - allwinner,sun8i-a83t-sgx544-115 # Banana-Pi-M3 (Allwinner A83T) and similar
+          - const: img,sgx544-115
+          - const: img,sgx544
+
+      - description: SGX544-116 based SoC
+        items:
+          - enum:
+            - ti,dra7-sgx544-116 # DRA7
+            - ti,omap5-sgx544-116 # OMAP5 UEVM, Pyra Handheld and similar
+          - const: img,sgx544-116
+          - const: img,sgx544
+
+      - description: SGX545 based SoC
+        items:
+          - const: intel,cedarview-gma3600-sgx545 # Atom N2600, D2500
+          - const: img,sgx545-116
+          - const: img,sgx545
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+examples:
+  - |+
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    gpu: gpu@fe00 {
+      compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
+      reg = <0xfe00 0x200>;
+      interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+    };
+
+...
-- 
2.25.1

Re: [PATCH v6 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

Hi,

On Wed, Apr 15, 2020 at 10:35:08AM +0200, H. Nikolaus Schaller wrote:
> The Imagination PVR/SGX GPU is part of several SoC from
> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo,
> Allwinner A83 and others.
>
> With this binding, we describe how the SGX processor is
> interfaced to the SoC (registers, interrupt etc.).
>
> In most cases, Clock, Reset and power management is handled
> by a parent node or elsewhere (e.g. code in the driver).

Wouldn't the "code in the driver" still require the clock / reset /
power domain to be set in the DT?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

> Am 15.04.2020 um 12:12 schrieb Maxime Ripard <maxime@cerno.tech>:
> 
> Hi,
> 
> On Wed, Apr 15, 2020 at 10:35:08AM +0200, H. Nikolaus Schaller wrote:
>> The Imagination PVR/SGX GPU is part of several SoC from
>> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo,
>> Allwinner A83 and others.
>> 
>> With this binding, we describe how the SGX processor is
>> interfaced to the SoC (registers, interrupt etc.).
>> 
>> In most cases, Clock, Reset and power management is handled
>> by a parent node or elsewhere (e.g. code in the driver).
> 
> Wouldn't the "code in the driver" still require the clock / reset /
> power domain to be set in the DT?

Well, some SoC seem to use existing clocks and have no reset.
Or, although not recommended, they may have the io-address range
hard-coded.

BR,
Nikolaus

Re: [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml for Imagination GPUs

[Neil Armstrong]

Hi,

On 15/04/2020 14:43, H. Nikolaus Schaller wrote:
> 
>> Am 15.04.2020 um 12:12 schrieb Maxime Ripard <maxime@cerno.tech>:
>>
>> Hi,
>>
>> On Wed, Apr 15, 2020 at 10:35:08AM +0200, H. Nikolaus Schaller wrote:
>>> The Imagination PVR/SGX GPU is part of several SoC from
>>> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo,
>>> Allwinner A83 and others.
>>>
>>> With this binding, we describe how the SGX processor is
>>> interfaced to the SoC (registers, interrupt etc.).
>>>
>>> In most cases, Clock, Reset and power management is handled
>>> by a parent node or elsewhere (e.g. code in the driver).
>>
>> Wouldn't the "code in the driver" still require the clock / reset /
>> power domain to be set in the DT?
> 
> Well, some SoC seem to use existing clocks and have no reset.
> Or, although not recommended, they may have the io-address range
> hard-coded.

The possible clocks and resets should be added, even if optional.

Please look at the arm utgard, midgard and bifrost bindings.

Neil

> 
> BR,
> Nikolaus
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

Hi Neil,

> Am 15.04.2020 um 14:54 schrieb Neil Armstrong <narmstrong@baylibre.com>:
> 
> Hi,
> 
> On 15/04/2020 14:43, H. Nikolaus Schaller wrote:
>> 
>>> Am 15.04.2020 um 12:12 schrieb Maxime Ripard <maxime@cerno.tech>:
>>> 
>>> Hi,
>>> 
>>> On Wed, Apr 15, 2020 at 10:35:08AM +0200, H. Nikolaus Schaller wrote:
>>>> The Imagination PVR/SGX GPU is part of several SoC from
>>>> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo,
>>>> Allwinner A83 and others.
>>>> 
>>>> With this binding, we describe how the SGX processor is
>>>> interfaced to the SoC (registers, interrupt etc.).
>>>> 
>>>> In most cases, Clock, Reset and power management is handled
>>>> by a parent node or elsewhere (e.g. code in the driver).
>>> 
>>> Wouldn't the "code in the driver" still require the clock / reset /
>>> power domain to be set in the DT?
>> 
>> Well, some SoC seem to use existing clocks and have no reset.
>> Or, although not recommended, they may have the io-address range
>> hard-coded.
> 
> The possible clocks and resets should be added, even if optional.
> 
> Please look at the arm utgard, midgard and bifrost bindings.

Interesting to compare to. Maybe we should also add the
$nodename: pattern: '^gpu@[a-f0-9]+$'

But the sgx binding is difficult to grasp here. Some SoC like the
omap series have their own ti,sysc based target modules and the
gpu nodes is a child of it lacking any clock and reset references
for purpose.

The jz4780 and some other need a clocks definition, but no reset.
Having a reset seems to be an option for the SoC designer and
not mandated by img. So is it part of the pvrsgx bindings or the
SoC?

Well we could add clocks and resets as optional but that would
allow to wrongly define omap.

Or delegate them to a parent "simple-pm-bus" node.

I have to study that material more to understand what you seem
to expect.

BR and thanks,
Nikolaus Schaller

Re: [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml for Imagination GPUs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2705 bytes --]

On Wed, Apr 15, 2020 at 03:17:25PM +0200, H. Nikolaus Schaller wrote:
> Hi Neil,
>
> > Am 15.04.2020 um 14:54 schrieb Neil Armstrong <narmstrong@baylibre.com>:
> >
> > Hi,
> >
> > On 15/04/2020 14:43, H. Nikolaus Schaller wrote:
> >>
> >>> Am 15.04.2020 um 12:12 schrieb Maxime Ripard <maxime@cerno.tech>:
> >>>
> >>> Hi,
> >>>
> >>> On Wed, Apr 15, 2020 at 10:35:08AM +0200, H. Nikolaus Schaller wrote:
> >>>> The Imagination PVR/SGX GPU is part of several SoC from
> >>>> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo,
> >>>> Allwinner A83 and others.
> >>>>
> >>>> With this binding, we describe how the SGX processor is
> >>>> interfaced to the SoC (registers, interrupt etc.).
> >>>>
> >>>> In most cases, Clock, Reset and power management is handled
> >>>> by a parent node or elsewhere (e.g. code in the driver).
> >>>
> >>> Wouldn't the "code in the driver" still require the clock / reset /
> >>> power domain to be set in the DT?
> >>
> >> Well, some SoC seem to use existing clocks and have no reset.
> >> Or, although not recommended, they may have the io-address range
> >> hard-coded.
> >
> > The possible clocks and resets should be added, even if optional.
> >
> > Please look at the arm utgard, midgard and bifrost bindings.
>
> Interesting to compare to. Maybe we should also add the
> $nodename: pattern: '^gpu@[a-f0-9]+$'
>
> But the sgx binding is difficult to grasp here. Some SoC like the
> omap series have their own ti,sysc based target modules and the
> gpu nodes is a child of it lacking any clock and reset references
> for purpose.
>
> The jz4780 and some other need a clocks definition, but no reset.
> Having a reset seems to be an option for the SoC designer and
> not mandated by img. So is it part of the pvrsgx bindings or the
> SoC?
>
> Well we could add clocks and resets as optional but that would
> allow to wrongly define omap.
>
> Or delegate them to a parent "simple-pm-bus" node.
>
> I have to study that material more to understand what you seem
> to expect.

The thing is, once that binding is in, it has to be backward
compatible. So every thing that you leave out is something that you'll
need to support in the driver eventually.

If you don't want it to be a complete nightmare, you'll want to figure
out as much as possible on how the GPU is integrated and make a
binding out of that. If OMAP is too much of a pain, you can also make
a separate binding for it, and a generic one for the rest of us.

I'd say that it's pretty unlikely that the clocks, interrupts (and
even regulators) are optional. It might be fixed on some SoCs, but
that's up to the DT to express that using fixed clocks / regulators,
not the GPU binding itself.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

Hi Maxime,

> Am 15.04.2020 um 16:21 schrieb Maxime Ripard <maxime@cerno.tech>:
> 
>> 
>> Well we could add clocks and resets as optional but that would
>> allow to wrongly define omap.
>> 
>> Or delegate them to a parent "simple-pm-bus" node.
>> 
>> I have to study that material more to understand what you seem
>> to expect.
> 
> The thing is, once that binding is in, it has to be backward
> compatible. So every thing that you leave out is something that you'll
> need to support in the driver eventually.

> 
> If you don't want it to be a complete nightmare, you'll want to figure
> out as much as possible on how the GPU is integrated and make a
> binding out of that.

Hm. Yes. We know that there likely are clocks and maybe reset
but for some SoC this seems to be undocumented and the reset
line the VHDL of the sgx gpu provides may be permanently tied
to "inactive".

So if clocks are optional and not provided, a driver simply can assume
they are enabled somewhere else and does not have to care about. If
they are specified, the driver can enable/disable them.

> If OMAP is too much of a pain, you can also make
> a separate binding for it, and a generic one for the rest of us.

No, omap isn't any pain at all.

The pain is that some other SoC are most easily defined by clocks in
the gpu node which the omap doesn't need to explicitly specify.

I would expect a much bigger nightmare if we split this into two
bindings variants.

> I'd say that it's pretty unlikely that the clocks, interrupts (and
> even regulators) are optional. It might be fixed on some SoCs, but
> that's up to the DT to express that using fixed clocks / regulators,
> not the GPU binding itself.

omap already has these defined them not to be part of the GPU binding.
The reason seems to be that this needs special clock gating control
especially for idle states which is beyond simple clock-enable.

This sysc target-module@56000000 node is already merged and therefore
we are only adding the gpu child node. Without defining clocks.

For example:

		sgx_module: target-module@56000000 {
			compatible = "ti,sysc-omap4", "ti,sysc";
			reg = <0x5600fe00 0x4>,
			      <0x5600fe10 0x4>;
			reg-names = "rev", "sysc";
			ti,sysc-midle = <SYSC_IDLE_FORCE>,
					<SYSC_IDLE_NO>,
					<SYSC_IDLE_SMART>;
			ti,sysc-sidle = <SYSC_IDLE_FORCE>,
					<SYSC_IDLE_NO>,
					<SYSC_IDLE_SMART>;
			clocks = <&gpu_clkctrl OMAP5_GPU_CLKCTRL 0>;
			clock-names = "fck";
			#address-cells = <1>;
			#size-cells = <1>;
			ranges = <0 0x56000000 0x2000000>;

			gpu: gpu@0 {
				compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
				reg = <0x0 0x10000>;
				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
			};
		};

The jz4780 example will like this:

	gpu: gpu@13040000 {
		compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
		reg = <0x13040000 0x4000>;

		clocks = <&cgu JZ4780_CLK_GPU>;
		clock-names = "gpu";

		interrupt-parent = <&intc>;
		interrupts = <63>;
	};

So the question is which one is "generic for the rest of us"?

And how can we make a single binding for the sgx. Not one for each
special SoC variant that may exist.

IMHO the best answer is to make clocks an optional property.
Or if we do not want to define them explicitly, we use
additionalProperties: true.

An alternative could be to use a simple-pm-bus like:

	sgx_module: sgx_module@13040000 {
		compatible = "simple-pm-bus";

		clocks = <&cgu JZ4780_CLK_GPU>;
		clock-names = "gpu";
		
		#address-cells = <1>;
		#size-cells = <1>;
		ranges = <0 0x13040000 0x10000>;

		gpu: gpu@0 {
			compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
			reg = <0x0 0x4000>;

			interrupt-parent = <&intc>;
			interrupts = <63>;
		};
	};

This gets rid of any clock, reset and pm definitions for the sgx bindings.
But how this is done is outside this sgx bindings.

With such a scheme, the binding I propose here would be complete and fully
generic. We can even add additionalProperties: false.

BR,
Nikolaus

Re: [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml for Imagination GPUs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4807 bytes --]

On Wed, Apr 15, 2020 at 05:09:45PM +0200, H. Nikolaus Schaller wrote:
> Hi Maxime,
>
> > Am 15.04.2020 um 16:21 schrieb Maxime Ripard <maxime@cerno.tech>:
> >
> >>
> >> Well we could add clocks and resets as optional but that would
> >> allow to wrongly define omap.
> >>
> >> Or delegate them to a parent "simple-pm-bus" node.
> >>
> >> I have to study that material more to understand what you seem
> >> to expect.
> >
> > The thing is, once that binding is in, it has to be backward
> > compatible. So every thing that you leave out is something that you'll
> > need to support in the driver eventually.
>
> >
> > If you don't want it to be a complete nightmare, you'll want to figure
> > out as much as possible on how the GPU is integrated and make a
> > binding out of that.
>
> Hm. Yes. We know that there likely are clocks and maybe reset
> but for some SoC this seems to be undocumented and the reset
> line the VHDL of the sgx gpu provides may be permanently tied
> to "inactive".
>
> So if clocks are optional and not provided, a driver simply can assume
> they are enabled somewhere else and does not have to care about. If
> they are specified, the driver can enable/disable them.

Except that at the hardware level, the clock is always going to be
there. You can't control it, but it's there.

> > If OMAP is too much of a pain, you can also make
> > a separate binding for it, and a generic one for the rest of us.
>
> No, omap isn't any pain at all.
>
> The pain is that some other SoC are most easily defined by clocks in
> the gpu node which the omap doesn't need to explicitly specify.
>
> I would expect a much bigger nightmare if we split this into two
> bindings variants.
>
> > I'd say that it's pretty unlikely that the clocks, interrupts (and
> > even regulators) are optional. It might be fixed on some SoCs, but
> > that's up to the DT to express that using fixed clocks / regulators,
> > not the GPU binding itself.
>
> omap already has these defined them not to be part of the GPU binding.
> The reason seems to be that this needs special clock gating control
> especially for idle states which is beyond simple clock-enable.
>
> This sysc target-module@56000000 node is already merged and therefore
> we are only adding the gpu child node. Without defining clocks.
>
> For example:
>
> 		sgx_module: target-module@56000000 {
> 			compatible = "ti,sysc-omap4", "ti,sysc";
> 			reg = <0x5600fe00 0x4>,
> 			      <0x5600fe10 0x4>;
> 			reg-names = "rev", "sysc";
> 			ti,sysc-midle = <SYSC_IDLE_FORCE>,
> 					<SYSC_IDLE_NO>,
> 					<SYSC_IDLE_SMART>;
> 			ti,sysc-sidle = <SYSC_IDLE_FORCE>,
> 					<SYSC_IDLE_NO>,
> 					<SYSC_IDLE_SMART>;
> 			clocks = <&gpu_clkctrl OMAP5_GPU_CLKCTRL 0>;
> 			clock-names = "fck";
> 			#address-cells = <1>;
> 			#size-cells = <1>;
> 			ranges = <0 0x56000000 0x2000000>;
>
> 			gpu: gpu@0 {
> 				compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
> 				reg = <0x0 0x10000>;
> 				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> 			};
> 		};
>
> The jz4780 example will like this:
>
> 	gpu: gpu@13040000 {
> 		compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
> 		reg = <0x13040000 0x4000>;
>
> 		clocks = <&cgu JZ4780_CLK_GPU>;
> 		clock-names = "gpu";
>
> 		interrupt-parent = <&intc>;
> 		interrupts = <63>;
> 	};
>
> So the question is which one is "generic for the rest of us"?

I'd say the latter.

> And how can we make a single binding for the sgx. Not one for each
> special SoC variant that may exist.
>
> IMHO the best answer is to make clocks an optional property.
> Or if we do not want to define them explicitly, we use
> additionalProperties: true.

If your clock is optional, then you define it but don't mandate
it. Not documenting it will only result in a mess where everyone will
put some clock into it, possibly with different semantics each and
every time.

> An alternative could be to use a simple-pm-bus like:
>
> 	sgx_module: sgx_module@13040000 {
> 		compatible = "simple-pm-bus";
>
> 		clocks = <&cgu JZ4780_CLK_GPU>;
> 		clock-names = "gpu";
>
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		ranges = <0 0x13040000 0x10000>;
>
> 		gpu: gpu@0 {
> 			compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
> 			reg = <0x0 0x4000>;
>
> 			interrupt-parent = <&intc>;
> 			interrupts = <63>;
> 		};
> 	};
>
> This gets rid of any clock, reset and pm definitions for the sgx bindings.
> But how this is done is outside this sgx bindings.
>
> With such a scheme, the binding I propose here would be complete and fully
> generic. We can even add additionalProperties: false.

This has nothing to do with the binding being complete. And if you use
a binding like this one, you'll be severely limited when you'll want
to implement things like DVFS.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

Hi Maxime,

> Am 15.04.2020 um 18:21 schrieb Maxime Ripard <maxime@cerno.tech>:
> 
> On Wed, Apr 15, 2020 at 05:09:45PM +0200, H. Nikolaus Schaller wrote:
>> Hi Maxime,
>> 
>> Hm. Yes. We know that there likely are clocks and maybe reset
>> but for some SoC this seems to be undocumented and the reset
>> line the VHDL of the sgx gpu provides may be permanently tied
>> to "inactive".
>> 
>> So if clocks are optional and not provided, a driver simply can assume
>> they are enabled somewhere else and does not have to care about. If
>> they are specified, the driver can enable/disable them.
> 
> Except that at the hardware level, the clock is always going to be
> there. You can't control it, but it's there.

Sure, we can deduce that from general hardware design knowledge.
But not every detail must be described in DT. Only the important
ones.

> 
>>> If OMAP is too much of a pain, you can also make
>>> a separate binding for it, and a generic one for the rest of us.
>> 
>> No, omap isn't any pain at all.
>> 
>> The pain is that some other SoC are most easily defined by clocks in
>> the gpu node which the omap doesn't need to explicitly specify.
>> 
>> I would expect a much bigger nightmare if we split this into two
>> bindings variants.
>> 
>>> I'd say that it's pretty unlikely that the clocks, interrupts (and
>>> even regulators) are optional. It might be fixed on some SoCs, but
>>> that's up to the DT to express that using fixed clocks / regulators,
>>> not the GPU binding itself.
>> 
>> omap already has these defined them not to be part of the GPU binding.
>> The reason seems to be that this needs special clock gating control
>> especially for idle states which is beyond simple clock-enable.
>> 
>> This sysc target-module@56000000 node is already merged and therefore
>> we are only adding the gpu child node. Without defining clocks.
>> 
>> For example:
>> 
>> 		sgx_module: target-module@56000000 {
>> 			compatible = "ti,sysc-omap4", "ti,sysc";
>> 			reg = <0x5600fe00 0x4>,
>> 			      <0x5600fe10 0x4>;
>> 			reg-names = "rev", "sysc";
>> 			ti,sysc-midle = <SYSC_IDLE_FORCE>,
>> 					<SYSC_IDLE_NO>,
>> 					<SYSC_IDLE_SMART>;
>> 			ti,sysc-sidle = <SYSC_IDLE_FORCE>,
>> 					<SYSC_IDLE_NO>,
>> 					<SYSC_IDLE_SMART>;
>> 			clocks = <&gpu_clkctrl OMAP5_GPU_CLKCTRL 0>;
>> 			clock-names = "fck";
>> 			#address-cells = <1>;
>> 			#size-cells = <1>;
>> 			ranges = <0 0x56000000 0x2000000>;
>> 
>> 			gpu: gpu@0 {
>> 				compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
>> 				reg = <0x0 0x10000>;
>> 				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
>> 			};
>> 		};
>> 
>> The jz4780 example will like this:
>> 
>> 	gpu: gpu@13040000 {
>> 		compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
>> 		reg = <0x13040000 0x4000>;
>> 
>> 		clocks = <&cgu JZ4780_CLK_GPU>;
>> 		clock-names = "gpu";
>> 
>> 		interrupt-parent = <&intc>;
>> 		interrupts = <63>;
>> 	};
>> 
>> So the question is which one is "generic for the rest of us"?
> 
> I'd say the latter.

Why?

TI SoC seem to be the broadest number of available users
of sgx5xx in the past and nowadays. Others are more the exception.

> If your clock is optional, then you define it but don't mandate
> it. Not documenting it will only result in a mess where everyone will
> put some clock into it, possibly with different semantics each and
> every time.

So you mean that we should require a dummy clock for the omap gpu node
or did I misunderstand that?

Well, yes there is of course a clock connection between the
omap target-module and the sgx but it is IMHO pointless to
describe it because it can't and does not need to be controlled
separately.

As said the target-module is already accepted and upstream and my
proposal is to get the gpu node described there. There is simply
no need for a clocks node for the omap.

What I also assume is that developers of DTS know what they do.
So the risk that there is different semantics is IMHO very low.

If you agree I can add the clocks/clock-names property as an
optional property. This should solve omap and all others.

> 
> This has nothing to do with the binding being complete. And if you use
> a binding like this one, you'll be severely limited when you'll want
> to implement things like DVFS.

Now you have unhooked me... Nobody seems to know if and how DVFS can be
applied to SGX. IMHO we should bake small bread first and get initial
support into mainline.

BR,
Nikolaus

Re: [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml for Imagination GPUs

[Tony Lindgren]

* H. Nikolaus Schaller <hns@goldelico.com> [200415 16:43]:
> If you agree I can add the clocks/clock-names property as an
> optional property. This should solve omap and all others.

Yes the clock can be optional property no problem. If we have
a clock, we just enable/disable it from the pvr_runtime_suspend()
and pvr_runtime_resume() we alaready have in pvr-drv.c.

Regards,

Tony

Re: [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml for Imagination GPUs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6575 bytes --]

Hi,

On Wed, Apr 15, 2020 at 06:42:18PM +0200, H. Nikolaus Schaller wrote:
> > Am 15.04.2020 um 18:21 schrieb Maxime Ripard <maxime@cerno.tech>:
> > 
> > On Wed, Apr 15, 2020 at 05:09:45PM +0200, H. Nikolaus Schaller wrote:
> >> Hi Maxime,
> >> 
> >> Hm. Yes. We know that there likely are clocks and maybe reset
> >> but for some SoC this seems to be undocumented and the reset
> >> line the VHDL of the sgx gpu provides may be permanently tied
> >> to "inactive".
> >> 
> >> So if clocks are optional and not provided, a driver simply can assume
> >> they are enabled somewhere else and does not have to care about. If
> >> they are specified, the driver can enable/disable them.
> > 
> > Except that at the hardware level, the clock is always going to be
> > there. You can't control it, but it's there.
> 
> Sure, we can deduce that from general hardware design knowledge.
> But not every detail must be described in DT. Only the important
> ones.
> 
> >>> If OMAP is too much of a pain, you can also make
> >>> a separate binding for it, and a generic one for the rest of us.
> >> 
> >> No, omap isn't any pain at all.
> >> 
> >> The pain is that some other SoC are most easily defined by clocks in
> >> the gpu node which the omap doesn't need to explicitly specify.
> >> 
> >> I would expect a much bigger nightmare if we split this into two
> >> bindings variants.
> >> 
> >>> I'd say that it's pretty unlikely that the clocks, interrupts (and
> >>> even regulators) are optional. It might be fixed on some SoCs, but
> >>> that's up to the DT to express that using fixed clocks / regulators,
> >>> not the GPU binding itself.
> >> 
> >> omap already has these defined them not to be part of the GPU binding.
> >> The reason seems to be that this needs special clock gating control
> >> especially for idle states which is beyond simple clock-enable.
> >> 
> >> This sysc target-module@56000000 node is already merged and therefore
> >> we are only adding the gpu child node. Without defining clocks.
> >> 
> >> For example:
> >> 
> >> 		sgx_module: target-module@56000000 {
> >> 			compatible = "ti,sysc-omap4", "ti,sysc";
> >> 			reg = <0x5600fe00 0x4>,
> >> 			      <0x5600fe10 0x4>;
> >> 			reg-names = "rev", "sysc";
> >> 			ti,sysc-midle = <SYSC_IDLE_FORCE>,
> >> 					<SYSC_IDLE_NO>,
> >> 					<SYSC_IDLE_SMART>;
> >> 			ti,sysc-sidle = <SYSC_IDLE_FORCE>,
> >> 					<SYSC_IDLE_NO>,
> >> 					<SYSC_IDLE_SMART>;
> >> 			clocks = <&gpu_clkctrl OMAP5_GPU_CLKCTRL 0>;
> >> 			clock-names = "fck";
> >> 			#address-cells = <1>;
> >> 			#size-cells = <1>;
> >> 			ranges = <0 0x56000000 0x2000000>;
> >> 
> >> 			gpu: gpu@0 {
> >> 				compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
> >> 				reg = <0x0 0x10000>;
> >> 				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> >> 			};
> >> 		};
> >> 
> >> The jz4780 example will like this:
> >> 
> >> 	gpu: gpu@13040000 {
> >> 		compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
> >> 		reg = <0x13040000 0x4000>;
> >> 
> >> 		clocks = <&cgu JZ4780_CLK_GPU>;
> >> 		clock-names = "gpu";
> >> 
> >> 		interrupt-parent = <&intc>;
> >> 		interrupts = <63>;
> >> 	};
> >> 
> >> So the question is which one is "generic for the rest of us"?
> > 
> > I'd say the latter.
> 
> Why?
> 
> TI SoC seem to be the broadest number of available users
> of sgx5xx in the past and nowadays. Others are more the exception.

And maybe TI has some complicated stuff around the GPU that others don't have?
If I look quickly at the Allwinner stuff, I see nothing looking alike in the
SoC, so making the binding like that for everyone just because TI did something
doesn't really make much sense.

> > If your clock is optional, then you define it but don't mandate
> > it. Not documenting it will only result in a mess where everyone will
> > put some clock into it, possibly with different semantics each and
> > every time.
> 
> So you mean that we should require a dummy clock for the omap gpu node
> or did I misunderstand that?
>
> Well, yes there is of course a clock connection between the
> omap target-module and the sgx but it is IMHO pointless to
> describe it because it can't and does not need to be controlled
> separately.
> 
> As said the target-module is already accepted and upstream and my
> proposal is to get the gpu node described there. There is simply
> no need for a clocks node for the omap.

There is no need for a clocks property *currently* *on the OMAP*.

> What I also assume is that developers of DTS know what they do.
> So the risk that there is different semantics is IMHO very low.

Well, they know what they do if you document the binding. Let's say I have two
clocks now on my SoC, and you just document that you want a clocks property,
with a generic name in clock-names like "gpu".

> If you agree I can add the clocks/clock-names property as an
> optional property. This should solve omap and all others.

With the above example, what clock should I put in there? In which order? This
isn't some random example pulled out of nowhere. The Allwinner A31 has (at
least) 4 clocks for the GPU, 1 reset line and 1 regulator, so I can only assume
that the GPU actually needs at least that amount to be properly integrated into
an SoC.

This has nothing to do with being dumb or smart.

> > This has nothing to do with the binding being complete. And if you use
> > a binding like this one, you'll be severely limited when you'll want
> > to implement things like DVFS.
> 
> Now you have unhooked me... Nobody seems to know if and how DVFS can be
> applied to SGX. IMHO we should bake small bread first and get initial
> support into mainline.

On the software side, yes, of course. But the discussion here doesn't have much
to do with software support, this is about the hardware. No matter if you enable
DVFS or not, you'll have those resources connected to the GPU.

And if you want to enable the strict minimum in DT for now and expand it later
as the software gains support for more stuff, then you'll have to deal with the
minimal stuff in software later-on to keep the backward compatibility.

But given that the current state on the Allwinner SoCs (at least) is that you
can't even read a register, it might be a good idea to delay the introduction of
that binding until you have something that works to avoid drowning under the
number of special cases to deal with backward compatibility.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml for Imagination GPUs

[H. Nikolaus Schaller]

Hi Maxime,

> Am 17.04.2020 um 12:25 schrieb Maxime Ripard <maxime@cerno.tech>:
> 
> Hi,
> 
> On Wed, Apr 15, 2020 at 06:42:18PM +0200, H. Nikolaus Schaller wrote:
>>> Am 15.04.2020 um 18:21 schrieb Maxime Ripard <maxime@cerno.tech>:
>>> 
>>> On Wed, Apr 15, 2020 at 05:09:45PM +0200, H. Nikolaus Schaller wrote:
>>>> Hi Maxime,
>>>> 
>>>> Hm. Yes. We know that there likely are clocks and maybe reset
>>>> but for some SoC this seems to be undocumented and the reset
>>>> line the VHDL of the sgx gpu provides may be permanently tied
>>>> to "inactive".
>>>> 
>>>> So if clocks are optional and not provided, a driver simply can assume
>>>> they are enabled somewhere else and does not have to care about. If
>>>> they are specified, the driver can enable/disable them.
>>> 
>>> Except that at the hardware level, the clock is always going to be
>>> there. You can't control it, but it's there.
>> 
>> Sure, we can deduce that from general hardware design knowledge.
>> But not every detail must be described in DT. Only the important
>> ones.
>> 
>>>>> If OMAP is too much of a pain, you can also make
>>>>> a separate binding for it, and a generic one for the rest of us.
>>>> 
>>>> No, omap isn't any pain at all.
>>>> 
>>>> The pain is that some other SoC are most easily defined by clocks in
>>>> the gpu node which the omap doesn't need to explicitly specify.
>>>> 
>>>> I would expect a much bigger nightmare if we split this into two
>>>> bindings variants.
>>>> 
>>>>> I'd say that it's pretty unlikely that the clocks, interrupts (and
>>>>> even regulators) are optional. It might be fixed on some SoCs, but
>>>>> that's up to the DT to express that using fixed clocks / regulators,
>>>>> not the GPU binding itself.
>>>> 
>>>> omap already has these defined them not to be part of the GPU binding.
>>>> The reason seems to be that this needs special clock gating control
>>>> especially for idle states which is beyond simple clock-enable.
>>>> 
>>>> This sysc target-module@56000000 node is already merged and therefore
>>>> we are only adding the gpu child node. Without defining clocks.
>>>> 
>>>> For example:
>>>> 
>>>> 		sgx_module: target-module@56000000 {
>>>> 			compatible = "ti,sysc-omap4", "ti,sysc";
>>>> 			reg = <0x5600fe00 0x4>,
>>>> 			      <0x5600fe10 0x4>;
>>>> 			reg-names = "rev", "sysc";
>>>> 			ti,sysc-midle = <SYSC_IDLE_FORCE>,
>>>> 					<SYSC_IDLE_NO>,
>>>> 					<SYSC_IDLE_SMART>;
>>>> 			ti,sysc-sidle = <SYSC_IDLE_FORCE>,
>>>> 					<SYSC_IDLE_NO>,
>>>> 					<SYSC_IDLE_SMART>;
>>>> 			clocks = <&gpu_clkctrl OMAP5_GPU_CLKCTRL 0>;
>>>> 			clock-names = "fck";
>>>> 			#address-cells = <1>;
>>>> 			#size-cells = <1>;
>>>> 			ranges = <0 0x56000000 0x2000000>;
>>>> 
>>>> 			gpu: gpu@0 {
>>>> 				compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
>>>> 				reg = <0x0 0x10000>;
>>>> 				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
>>>> 			};
>>>> 		};
>>>> 
>>>> The jz4780 example will like this:
>>>> 
>>>> 	gpu: gpu@13040000 {
>>>> 		compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
>>>> 		reg = <0x13040000 0x4000>;
>>>> 
>>>> 		clocks = <&cgu JZ4780_CLK_GPU>;
>>>> 		clock-names = "gpu";
>>>> 
>>>> 		interrupt-parent = <&intc>;
>>>> 		interrupts = <63>;
>>>> 	};
>>>> 
>>>> So the question is which one is "generic for the rest of us"?
>>> 
>>> I'd say the latter.
>> 
>> Why?
>> 
>> TI SoC seem to be the broadest number of available users
>> of sgx5xx in the past and nowadays. Others are more the exception.
> 
> And maybe TI has some complicated stuff around the GPU that others don't have?

Looks so.

> If I look quickly at the Allwinner stuff, I see nothing looking alike in the
> SoC, so making the binding like that for everyone just because TI did something
> doesn't really make much sense.

That is why I propose to make the clocks optional. This solves both
cases in a simple and neat way.

> 
>>> If your clock is optional, then you define it but don't mandate
>>> it. Not documenting it will only result in a mess where everyone will
>>> put some clock into it, possibly with different semantics each and
>>> every time.
>> 
>> So you mean that we should require a dummy clock for the omap gpu node
>> or did I misunderstand that?
>> 
>> Well, yes there is of course a clock connection between the
>> omap target-module and the sgx but it is IMHO pointless to
>> describe it because it can't and does not need to be controlled
>> separately.
>> 
>> As said the target-module is already accepted and upstream and my
>> proposal is to get the gpu node described there. There is simply
>> no need for a clocks node for the omap.
> 
> There is no need for a clocks property *currently* *on the OMAP*.

Yes. But why "currently"? Do you think the OMAPs we already have
defined and tested will change?

> 
>> What I also assume is that developers of DTS know what they do.
>> So the risk that there is different semantics is IMHO very low.
> 
> Well, they know what they do if you document the binding. Let's say I have two
> clocks now on my SoC, and you just document that you want a clocks property,
> with a generic name in clock-names like "gpu".

Yes, that is what I want to propose for v7:

  clocks:
    maxItems: 1

  clock-names:
    maxItems: 1
    items:
      - const: gpu

> 
>> If you agree I can add the clocks/clock-names property as an
>> optional property. This should solve omap and all others.
> 
> With the above example, what clock should I put in there? In which order? This
> isn't some random example pulled out of nowhere. The Allwinner A31 has (at
> least) 4 clocks for the GPU, 1 reset line and 1 regulator, so I can only assume
> that the GPU actually needs at least that amount to be properly integrated into
> an SoC.

Ah, now I understand your motivation: you have access and experience with
the A31 and you know that our proposal doesn't fit to it.

From what I know from your description is that the A31 is quite special with
4 GPU clocks... Are they all really for the GPU or 3 of them for the interface
logic (like on OMAP which separates between "functional clocks" and "interface
clocks")? Or are there 4 groups of GPU cores with a separate clock for each one?

So what would be your proposal for the A31 DT?

Then I get a chance to compare DT snippets and try to make a mixture for
the bindings.

> 

>>> This has nothing to do with the binding being complete. And if you use
>>> a binding like this one, you'll be severely limited when you'll want
>>> to implement things like DVFS.
>> 
>> Now you have unhooked me... Nobody seems to know if and how DVFS can be
>> applied to SGX. IMHO we should bake small bread first and get initial
>> support into mainline.
> 
> On the software side, yes, of course. But the discussion here doesn't have much
> to do with software support, this is about the hardware. No matter if you enable
> DVFS or not, you'll have those resources connected to the GPU.
> 
> And if you want to enable the strict minimum in DT for now and expand it later
> as the software gains support for more stuff, then you'll have to deal with the
> minimal stuff in software later-on to keep the backward compatibility.

That is IMHO common practise everywhere. Sometimes you even have to adapt
years old DT to new limitations of the drivers (this happened recently for
combination of SPI and GPIO). 

And you can still write two different drivers for a single bindings document
or use the .data field of the compatibility table. And I think clocks and regulators
can also be referenced by name if they are not defined in DT. This is not a
"single variety" style, but a potential solution.

What I want to say: there are many roads to Rome.

> But given that the current state on the Allwinner SoCs (at least) is that you
> can't even read a register, it might be a good idea to delay the introduction of
> that binding until you have something that works to avoid drowning under the
> number of special cases to deal with backward compatibility.

Philipp has something minimal working on the A83 which works with the proposed
binding and enabled him to read out the sgx revision register.

So if you are a specialist for the A31 SGX, please make a proposal for a binding
that covers all the A31 needs and all other SoC we know. Or define a separate
bindings for the A31.

Thank you very much,
Nikolaus Schaller

Re: [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml for Imagination GPUs

[Philipp Rossak]

Hi Nikolaus,
Hi Maxime,

>>> TI SoC seem to be the broadest number of available users
>>> of sgx5xx in the past and nowadays. Others are more the exception.
>>
>> And maybe TI has some complicated stuff around the GPU that others don't have?
> 
> Looks so.

I can only agree on this.

> 
>>
>>> What I also assume is that developers of DTS know what they do.
>>> So the risk that there is different semantics is IMHO very low.
>>
>> Well, they know what they do if you document the binding. Let's say I have two
>> clocks now on my SoC, and you just document that you want a clocks property,
>> with a generic name in clock-names like "gpu".
> 
> Yes, that is what I want to propose for v7:
> 
>    clocks:
>      maxItems: 1
> 
>    clock-names:
>      maxItems: 1
>      items:
>        - const: gpu
> 
>>
>>> If you agree I can add the clocks/clock-names property as an
>>> optional property. This should solve omap and all others.
>>
>> With the above example, what clock should I put in there? In which order? This
>> isn't some random example pulled out of nowhere. The Allwinner A31 has (at
>> least) 4 clocks for the GPU, 1 reset line and 1 regulator, so I can only assume
>> that the GPU actually needs at least that amount to be properly integrated into
>> an SoC.
> 
> Ah, now I understand your motivation: you have access and experience with
> the A31 and you know that our proposal doesn't fit to it.
> 
>  From what I know from your description is that the A31 is quite special with
> 4 GPU clocks... Are they all really for the GPU or 3 of them for the interface
> logic (like on OMAP which separates between "functional clocks" and "interface
> clocks")? Or are there 4 groups of GPU cores with a separate clock for each one?
> 
> So what would be your proposal for the A31 DT?
> 
> Then I get a chance to compare DT snippets and try to make a mixture for
> the bindings.
> 

This is my current binding for the A83T, the A31 looks similar but there 
is still something missing, since some parts are not mainlined yet:

sun8i-a83t.dtsi:
gpu: gpu@1c400000 {
	compatible = "allwinner,sun8i-a83t-sgx544-115",
		     "img,sgx544-115", "img,sgx544";
	reg = <0x01c40000 0x10000>;
	interrupts = <GIC_SPI 129 IRQ_TYPE_LEVEL_HIGH>;
	clocks = <&ccu CLK_BUS_GPU>, <&ccu CLK_GPU_CORE>,
		 <&ccu CLK_GPU_MEMORY>, <&ccu CLK_GPU_HYD>;
	clock-names = "bus", "core", "memory", "hyd";

	resets = <&ccu RST_BUS_GPU>;
};

sun8i-a83t-bananapi-m3.dts:
&gpu {
	gpu-supply = <&reg_dcdc4>;
};


> 
>> But given that the current state on the Allwinner SoCs (at least) is that you
>> can't even read a register, it might be a good idea to delay the introduction of
>> that binding until you have something that works to avoid drowning under the
>> number of special cases to deal with backward compatibility.
> 

Maxime is right. Even if you enable the regulator, write 0x0 to the GPU 
Power Off Gating Register, deassert the reset and enable the clocks you 
are not able to read the register.
You must first run: pvrsrvctl --no-module --start (user space binaries) 
to access registers otherwise the system will stuck with the following 
message when accessing them:

./devmem2 0x01C40024
/dev/mem opened.

> Philipp has something minimal working on the A83 which works with the proposed
> binding and enabled him to read out the sgx revision register.
> 
This is not correct. In the other mail I talked about my reference 
system. This is an old 3.4.39 kernel, modified by allwinner to run on 
their SOC's which don't use the common kernel techniques. So it's very 
hacky, but they got the gpu running. I'm using this system for 
comparison, to read out registers and for reverse engineering.

My current kernel module behaves similar like the reference design, but 
right now I'm not able to run "pvrsrvctl --no-module --start" without 
errors. So the initialization never get's finalized and I see the issue 
described above.

> So if you are a specialist for the A31 SGX, please make a proposal for a binding
> that covers all the A31 needs and all other SoC we know. Or define a separate
> bindings for the A31.

The A31 and the A31s have some additional clocks mentioned in their 
datasheet (@ System Control Register/SRAM Controler). Those are not 
available in the A83T datasheet, but might be there since the memory map 
looks similar and allwinner might use the same userspace binaries for 
their devices.

 From the knowledge I gained the last 3 days, we should delay the 
patches for the A83T, A31 and A31s.
The idea of the placeholder patches was to show that from this binding 
also other devices could benefit.

Cheers,
Philipp

Re: [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml for Imagination GPUs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 9368 bytes --]

On Fri, Apr 17, 2020 at 02:15:44PM +0200, H. Nikolaus Schaller wrote:
> > Am 17.04.2020 um 12:25 schrieb Maxime Ripard <maxime@cerno.tech>:
> > On Wed, Apr 15, 2020 at 06:42:18PM +0200, H. Nikolaus Schaller wrote:
> >>> Am 15.04.2020 um 18:21 schrieb Maxime Ripard <maxime@cerno.tech>:
> >>> 
> >>> On Wed, Apr 15, 2020 at 05:09:45PM +0200, H. Nikolaus Schaller wrote:
> >>>> Hi Maxime,
> >>>> 
> >>>> Hm. Yes. We know that there likely are clocks and maybe reset
> >>>> but for some SoC this seems to be undocumented and the reset
> >>>> line the VHDL of the sgx gpu provides may be permanently tied
> >>>> to "inactive".
> >>>> 
> >>>> So if clocks are optional and not provided, a driver simply can assume
> >>>> they are enabled somewhere else and does not have to care about. If
> >>>> they are specified, the driver can enable/disable them.
> >>> 
> >>> Except that at the hardware level, the clock is always going to be
> >>> there. You can't control it, but it's there.
> >> 
> >> Sure, we can deduce that from general hardware design knowledge.
> >> But not every detail must be described in DT. Only the important
> >> ones.
> >> 
> >>>>> If OMAP is too much of a pain, you can also make
> >>>>> a separate binding for it, and a generic one for the rest of us.
> >>>> 
> >>>> No, omap isn't any pain at all.
> >>>> 
> >>>> The pain is that some other SoC are most easily defined by clocks in
> >>>> the gpu node which the omap doesn't need to explicitly specify.
> >>>> 
> >>>> I would expect a much bigger nightmare if we split this into two
> >>>> bindings variants.
> >>>> 
> >>>>> I'd say that it's pretty unlikely that the clocks, interrupts (and
> >>>>> even regulators) are optional. It might be fixed on some SoCs, but
> >>>>> that's up to the DT to express that using fixed clocks / regulators,
> >>>>> not the GPU binding itself.
> >>>> 
> >>>> omap already has these defined them not to be part of the GPU binding.
> >>>> The reason seems to be that this needs special clock gating control
> >>>> especially for idle states which is beyond simple clock-enable.
> >>>> 
> >>>> This sysc target-module@56000000 node is already merged and therefore
> >>>> we are only adding the gpu child node. Without defining clocks.
> >>>> 
> >>>> For example:
> >>>> 
> >>>> 		sgx_module: target-module@56000000 {
> >>>> 			compatible = "ti,sysc-omap4", "ti,sysc";
> >>>> 			reg = <0x5600fe00 0x4>,
> >>>> 			      <0x5600fe10 0x4>;
> >>>> 			reg-names = "rev", "sysc";
> >>>> 			ti,sysc-midle = <SYSC_IDLE_FORCE>,
> >>>> 					<SYSC_IDLE_NO>,
> >>>> 					<SYSC_IDLE_SMART>;
> >>>> 			ti,sysc-sidle = <SYSC_IDLE_FORCE>,
> >>>> 					<SYSC_IDLE_NO>,
> >>>> 					<SYSC_IDLE_SMART>;
> >>>> 			clocks = <&gpu_clkctrl OMAP5_GPU_CLKCTRL 0>;
> >>>> 			clock-names = "fck";
> >>>> 			#address-cells = <1>;
> >>>> 			#size-cells = <1>;
> >>>> 			ranges = <0 0x56000000 0x2000000>;
> >>>> 
> >>>> 			gpu: gpu@0 {
> >>>> 				compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
> >>>> 				reg = <0x0 0x10000>;
> >>>> 				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> >>>> 			};
> >>>> 		};
> >>>> 
> >>>> The jz4780 example will like this:
> >>>> 
> >>>> 	gpu: gpu@13040000 {
> >>>> 		compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
> >>>> 		reg = <0x13040000 0x4000>;
> >>>> 
> >>>> 		clocks = <&cgu JZ4780_CLK_GPU>;
> >>>> 		clock-names = "gpu";
> >>>> 
> >>>> 		interrupt-parent = <&intc>;
> >>>> 		interrupts = <63>;
> >>>> 	};
> >>>> 
> >>>> So the question is which one is "generic for the rest of us"?
> >>> 
> >>> I'd say the latter.
> >> 
> >> Why?
> >> 
> >> TI SoC seem to be the broadest number of available users
> >> of sgx5xx in the past and nowadays. Others are more the exception.
> > 
> > And maybe TI has some complicated stuff around the GPU that others don't have?
> 
> Looks so.
> 
> > If I look quickly at the Allwinner stuff, I see nothing looking alike in the
> > SoC, so making the binding like that for everyone just because TI did something
> > doesn't really make much sense.
> 
> That is why I propose to make the clocks optional. This solves both
> cases in a simple and neat way.
> 
> > 
> >>> If your clock is optional, then you define it but don't mandate
> >>> it. Not documenting it will only result in a mess where everyone will
> >>> put some clock into it, possibly with different semantics each and
> >>> every time.
> >> 
> >> So you mean that we should require a dummy clock for the omap gpu node
> >> or did I misunderstand that?
> >> 
> >> Well, yes there is of course a clock connection between the
> >> omap target-module and the sgx but it is IMHO pointless to
> >> describe it because it can't and does not need to be controlled
> >> separately.
> >> 
> >> As said the target-module is already accepted and upstream and my
> >> proposal is to get the gpu node described there. There is simply
> >> no need for a clocks node for the omap.
> > 
> > There is no need for a clocks property *currently* *on the OMAP*.
> 
> Yes. But why "currently"? Do you think the OMAPs we already have
> defined and tested will change?

Like I said, DVFS is likely to be one in the future.

> >> What I also assume is that developers of DTS know what they do.
> >> So the risk that there is different semantics is IMHO very low.
> > 
> > Well, they know what they do if you document the binding. Let's say I have two
> > clocks now on my SoC, and you just document that you want a clocks property,
> > with a generic name in clock-names like "gpu".
> 
> Yes, that is what I want to propose for v7:
> 
>   clocks:
>     maxItems: 1
> 
>   clock-names:
>     maxItems: 1
>     items:
>       - const: gpu

If you document what the "gpu" clock is supposed to be.

Is it the clock for the bus (clocking the register part of the GPU), the clock
for the GPU cores? Something else?

> >> If you agree I can add the clocks/clock-names property as an
> >> optional property. This should solve omap and all others.
> > 
> > With the above example, what clock should I put in there? In which order? This
> > isn't some random example pulled out of nowhere. The Allwinner A31 has (at
> > least) 4 clocks for the GPU, 1 reset line and 1 regulator, so I can only assume
> > that the GPU actually needs at least that amount to be properly integrated into
> > an SoC.
> 
> Ah, now I understand your motivation: you have access and experience with
> the A31 and you know that our proposal doesn't fit to it.

Not only the A31. If you don't document what your expectations are for a generic
component like that, every SoC will assume that your GPU clock is something
different and you won't be able to make any sense of it in your driver.

> From what I know from your description is that the A31 is quite special with
> 4 GPU clocks... Are they all really for the GPU or 3 of them for the interface
> logic (like on OMAP which separates between "functional clocks" and "interface
> clocks")? Or are there 4 groups of GPU cores with a separate clock for each one?

1 is the equivalent of the interface clock, the others seem to be for the
functional clocks.

> So what would be your proposal for the A31 DT?
> 
> Then I get a chance to compare DT snippets and try to make a mixture for
> the bindings.

You'd have to know the GPU to do that, and I don't.

> >>> This has nothing to do with the binding being complete. And if you use
> >>> a binding like this one, you'll be severely limited when you'll want
> >>> to implement things like DVFS.
> >> 
> >> Now you have unhooked me... Nobody seems to know if and how DVFS can be
> >> applied to SGX. IMHO we should bake small bread first and get initial
> >> support into mainline.
> > 
> > On the software side, yes, of course. But the discussion here doesn't have much
> > to do with software support, this is about the hardware. No matter if you enable
> > DVFS or not, you'll have those resources connected to the GPU.
> > 
> > And if you want to enable the strict minimum in DT for now and expand it later
> > as the software gains support for more stuff, then you'll have to deal with the
> > minimal stuff in software later-on to keep the backward compatibility.
> 
> That is IMHO common practise everywhere. Sometimes you even have to adapt
> years old DT to new limitations of the drivers (this happened recently for
> combination of SPI and GPIO).

To some extent, yes. But those old bindings that turn out to be wrong at least
contain most infos about the hardware, even though it's incomplete or flawed.
Your proposal doesn't.

> And you can still write two different drivers for a single bindings document
> or use the .data field of the compatibility table. And I think clocks and regulators
> can also be referenced by name if they are not defined in DT. This is not a
> "single variety" style, but a potential solution.
> 
> What I want to say: there are many roads to Rome.

What I want to say is: all the roads you listed above are going to be painful.
Take your time, have a generic driver running from your generic binding you want
to introduce on all the SoCs you want to support, and *then* start this
discussion again.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 01/12] dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

[Rob Herring]

On Wed, 15 Apr 2020 10:35:08 +0200, "H. Nikolaus Schaller" wrote:
> The Imagination PVR/SGX GPU is part of several SoC from
> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo,
> Allwinner A83 and others.
> 
> With this binding, we describe how the SGX processor is
> interfaced to the SoC (registers, interrupt etc.).
> 
> In most cases, Clock, Reset and power management is handled
> by a parent node or elsewhere (e.g. code in the driver).
> 
> Tested by make dt_binding_check dtbs_check
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 122 ++++++++++++++++++
>  1 file changed, 122 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml:  while parsing a block mapping
  in "<unicode string>", line 74, column 13
did not find expected key
  in "<unicode string>", line 117, column 21
Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/gpu/img,pvrsgx.example.dts' failed
make[1]: *** [Documentation/devicetree/bindings/gpu/img,pvrsgx.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
Makefile:1264: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1270997

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.

Re: [PATCH v6 01/12] dt-bindings: add img, pvrsgx.yaml for Imagination GPUs

[Rob Herring]

On Fri, Apr 17, 2020 at 7:16 AM H. Nikolaus Schaller <hns@goldelico.com> wrote:
>
> Hi Rob,
>
> > Am 16.04.2020 um 22:41 schrieb Rob Herring <robh@kernel.org>:
> >
> > On Wed, 15 Apr 2020 10:35:08 +0200, "H. Nikolaus Schaller" wrote:
> >> The Imagination PVR/SGX GPU is part of several SoC from
> >> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo,
> >> Allwinner A83 and others.
> >>
> >> With this binding, we describe how the SGX processor is
> >> interfaced to the SoC (registers, interrupt etc.).
> >>
> >> In most cases, Clock, Reset and power management is handled
> >> by a parent node or elsewhere (e.g. code in the driver).
> >>
> >> Tested by make dt_binding_check dtbs_check
> >>
> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> >> ---
> >> .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 122 ++++++++++++++++++
> >> 1 file changed, 122 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
> >>
> >
> > My bot found errors running 'make dt_binding_check' on your patch:
> >
> > Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml:  while parsing a block mapping
> >  in "<unicode string>", line 74, column 13
>
> It turned out that there was a stray " in line 74 from the last editing phase.
> Will be fixed in v7.
>
> Would it be possible to make dt_binding_check not only report line/column but the
> contents of the line instead of "<unicode string>"?

This comes from ruamel.yaml module. I believe "<unicode string>" is
supposed to be the type of the data, not what it is. However, it is
possible to get something a bit more helpful, but it depends on which
parser is used. By default we use the C based parser (aka 'safe'). If
we use the round trip parser, we get this:

ruamel.yaml.scanner.ScannerError: while scanning a simple key
  in "<unicode string>", line 84, column 5:
        maxItems
        ^ (line: 84)

This can be enabled by passing '-n' (line numbers) to dt-doc-validate.
Currently, you have have to edit the Makefile to do this. The C parser
is a big difference in speed, so I don't want to change the default.

I can probably work around this and dump the erroring line, but I'm
not sure that's always useful. Many errors are indentation related and
those need some context. Plus just dumping the line can be done easily
with sed:

sed -n ${LINE}p <file>

Rob

[PATCH v6 02/12] ARM: DTS: am33xx: add sgx gpu child node

[H. Nikolaus Schaller]

and add interrupt.

Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # BeagleBone Black
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/am33xx.dtsi | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index a35f5052d76f..155424d87156 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -564,7 +564,7 @@ aes: aes@0 {
 			};
 		};
 
-		target-module@56000000 {
+		sgx_module: target-module@56000000 {
 			compatible = "ti,sysc-omap4", "ti,sysc";
 			reg = <0x5600fe00 0x4>,
 			      <0x5600fe10 0x4>;
@@ -583,10 +583,11 @@ target-module@56000000 {
 			#size-cells = <1>;
 			ranges = <0 0x56000000 0x1000000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			gpu: gpu@0 {
+				compatible = "ti,am3352-sgx530-125", "img,sgx530-125", "img,sgx530";
+				reg = <0x00 0x1000000>;	/* 16 MB */
+				interrupts = <37>;
+			};
 		};
 	};
 };
-- 
2.25.1

[PATCH v6 03/12] ARM: DTS: am3517: add sgx gpu child node

[H. Nikolaus Schaller]

and add interrupt.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/am3517.dtsi | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/am3517.dtsi b/arch/arm/boot/dts/am3517.dtsi
index e0b5a00e2078..3fce56a646d1 100644
--- a/arch/arm/boot/dts/am3517.dtsi
+++ b/arch/arm/boot/dts/am3517.dtsi
@@ -138,10 +138,11 @@ sgx_module: target-module@50000000 {
 			#size-cells = <1>;
 			ranges = <0 0x50000000 0x4000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			gpu: gpu@0 {
+				compatible = "ti,am3517-sgx530-125", "img,sgx530-125", "img,sgx530";
+				reg = <0x0 0x4000>;
+				interrupts = <21>;
+			};
 		};
 	};
 };
-- 
2.25.1

[PATCH v6 04/12] ARM: DTS: omap34xx: add sgx gpu child node

[H. Nikolaus Schaller]

and add interrupt.

According to omap3530 TRM the SGX register block is 64kB.
See: 13.4  SGX Register Mapping, Table 13-2

Reported-by: Andrew F. Davis <afd@ti.com>	# register size
Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # OpenPandora 600 MHz.
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/omap34xx.dtsi | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/omap34xx.dtsi b/arch/arm/boot/dts/omap34xx.dtsi
index c4dd9801840d..51c60ee2b68d 100644
--- a/arch/arm/boot/dts/omap34xx.dtsi
+++ b/arch/arm/boot/dts/omap34xx.dtsi
@@ -167,12 +167,13 @@ sgx_module: target-module@50000000 {
 			clock-names = "fck", "ick";
 			#address-cells = <1>;
 			#size-cells = <1>;
-			ranges = <0 0x50000000 0x4000>;
+			ranges = <0 0x50000000 0x10000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			gpu: gpu@0 {
+				compatible = "ti,omap3-sgx530-121", "img,sgx530-121", "img,sgx530";
+				reg = <0x0 0x10000>;	/* 64kB */
+				interrupts = <21>;
+			};
 		};
 	};
 
-- 
2.25.1

[PATCH v6 05/12] ARM: DTS: omap36xx: add sgx gpu child node

[H. Nikolaus Schaller]

and add interrupt.

Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # GTA04, BeagleBoard XM
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/omap36xx.dtsi | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/omap36xx.dtsi b/arch/arm/boot/dts/omap36xx.dtsi
index 71f3c8f1f924..b308dbb3b1bb 100644
--- a/arch/arm/boot/dts/omap36xx.dtsi
+++ b/arch/arm/boot/dts/omap36xx.dtsi
@@ -211,10 +211,11 @@ sgx_module: target-module@50000000 {
 			#size-cells = <1>;
 			ranges = <0 0x50000000 0x2000000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			gpu: gpu@0 {
+				compatible = "ti,omap3-sgx530-125", "img,sgx530-125", "img,sgx530";
+				reg = <0x0 0x10000>;	/* 64kB */
+				interrupts = <21>;
+			};
 		};
 	};
 
-- 
2.25.1

[PATCH v6 06/12] ARM: DTS: omap4: add sgx gpu child node

[H. Nikolaus Schaller]

and add interrupt.

Since omap4420/30/60 and omap4470 come with different SGX variants
we need to introduce a new omap4470.dtsi. If an omap4470 board
does not want to use SGX it is no problem to still include
omap4460.dtsi.

Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # PandaBoard ES
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/omap4.dtsi   | 11 ++++++-----
 arch/arm/boot/dts/omap4470.dts | 15 +++++++++++++++
 2 files changed, 21 insertions(+), 5 deletions(-)
 create mode 100644 arch/arm/boot/dts/omap4470.dts

diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
index 763bdea8c829..15ff3d7146af 100644
--- a/arch/arm/boot/dts/omap4.dtsi
+++ b/arch/arm/boot/dts/omap4.dtsi
@@ -389,7 +389,7 @@ abb_iva: regulator-abb-iva {
 			status = "disabled";
 		};
 
-		target-module@56000000 {
+		sgx_module: target-module@56000000 {
 			compatible = "ti,sysc-omap4", "ti,sysc";
 			reg = <0x5600fe00 0x4>,
 			      <0x5600fe10 0x4>;
@@ -408,10 +408,11 @@ target-module@56000000 {
 			#size-cells = <1>;
 			ranges = <0 0x56000000 0x2000000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			gpu: gpu@0 {
+				compatible = "ti,omap4-sgx540-120", "img,sgx540-120", "img,sgx540";
+				reg = <0x0 0x2000000>;	/* 32MB */
+				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+			};
 		};
 
 		/*
diff --git a/arch/arm/boot/dts/omap4470.dts b/arch/arm/boot/dts/omap4470.dts
new file mode 100644
index 000000000000..f29c581300bf
--- /dev/null
+++ b/arch/arm/boot/dts/omap4470.dts
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Device Tree Source for OMAP4470 SoC
+ *
+ * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2.  This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ */
+#include "omap4460.dtsi"
+
+&sgx {
+	compatible = "ti,omap4470-sgx544-112", "img,sgx544-112", "img,sgx544";
+};
-- 
2.25.1

[PATCH v6 07/12] ARM: DTS: omap5: add sgx gpu child node

[H. Nikolaus Schaller]

and add interrupt.

Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # Pyra-Handheld.
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/omap5.dtsi | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/omap5.dtsi b/arch/arm/boot/dts/omap5.dtsi
index 2ac7f021c284..1cf41664fd00 100644
--- a/arch/arm/boot/dts/omap5.dtsi
+++ b/arch/arm/boot/dts/omap5.dtsi
@@ -270,7 +270,7 @@ sata: sata@4a141100 {
 			ports-implemented = <0x1>;
 		};
 
-		target-module@56000000 {
+		sgx_module: target-module@56000000 {
 			compatible = "ti,sysc-omap4", "ti,sysc";
 			reg = <0x5600fe00 0x4>,
 			      <0x5600fe10 0x4>;
@@ -287,10 +287,11 @@ target-module@56000000 {
 			#size-cells = <1>;
 			ranges = <0 0x56000000 0x2000000>;
 
-			/*
-			 * Closed source PowerVR driver, no child device
-			 * binding or driver in mainline
-			 */
+			gpu: gpu@0 {
+				compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
+				reg = <0x0 0x10000>;
+				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+			};
 		};
 
 		target-module@58000000 {
-- 
2.25.1

Re: [PATCH v6 07/12] ARM: DTS: omap5: add sgx gpu child node

[Krzysztof Kozlowski]

On Wed, 15 Apr 2020 at 10:36, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>
> and add interrupt.
>
> Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # Pyra-Handheld.

Don't add your own Tested-by tags. These are implied by authorship,
otherwise all patches people make should have such tag.

Best regards,
Krzysztof

Re: [PATCH v6 07/12] ARM: DTS: omap5: add sgx gpu child node

[H. Nikolaus Schaller]

Hi Krzysztof,

> Am 15.04.2020 um 13:38 schrieb Krzysztof Kozlowski <krzk@kernel.org>:
> 
> On Wed, 15 Apr 2020 at 10:36, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> 
>> and add interrupt.
>> 
>> Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # Pyra-Handheld.
> 
> Don't add your own Tested-by tags. These are implied by authorship,
> otherwise all patches people make should have such tag.

Ok I see. AFAIR it originates in several phases of editing to report on which device it was tested.

Is there a canonical way of writing "tested-on: ${HARDWARE}"?

E.g. would this be ok?

Signed-off: H. Nikolaus Schaller <hns@goldelico.com> # tested on Pyra-Handheld

BR and thanks,
Nikolaus Schaller

Re: [PATCH v6 07/12] ARM: DTS: omap5: add sgx gpu child node

[Krzysztof Kozlowski]

On Wed, Apr 15, 2020 at 01:46:06PM +0200, H. Nikolaus Schaller wrote:
> Hi Krzysztof,
> 
> > Am 15.04.2020 um 13:38 schrieb Krzysztof Kozlowski <krzk@kernel.org>:
> > 
> > On Wed, 15 Apr 2020 at 10:36, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> >> 
> >> and add interrupt.
> >> 
> >> Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # Pyra-Handheld.
> > 
> > Don't add your own Tested-by tags. These are implied by authorship,
> > otherwise all patches people make should have such tag.
> 
> Ok I see. AFAIR it originates in several phases of editing to report on which device it was tested.
> 
> Is there a canonical way of writing "tested-on: ${HARDWARE}"?
> 
> E.g. would this be ok?
> 
> Signed-off: H. Nikolaus Schaller <hns@goldelico.com> # tested on Pyra-Handheld

If you think tested platform is worth mentioning in the commit msg
(it will stay there forever, ever, ever) then just add a line like:

"Add SGX GPU node. Tested on Pyra-Handheld."

From time to time we add such information to note that only one platform
was actually tested.  I am not sure what benefit it brings to most
cases... but your commit msg is so short that adding one more sentence
seems reasonable. :)

Best regards,
Krzysztof

Re: [PATCH v6 07/12] ARM: DTS: omap5: add sgx gpu child node

[H. Nikolaus Schaller]

> Am 15.04.2020 um 15:47 schrieb Krzysztof Kozlowski <krzk@kernel.org>:
> 
> On Wed, Apr 15, 2020 at 01:46:06PM +0200, H. Nikolaus Schaller wrote:
>> Hi Krzysztof,
>> 
>>> Am 15.04.2020 um 13:38 schrieb Krzysztof Kozlowski <krzk@kernel.org>:
>>> 
>>> On Wed, 15 Apr 2020 at 10:36, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>>> 
>>>> and add interrupt.
>>>> 
>>>> Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # Pyra-Handheld.
>>> 
>>> Don't add your own Tested-by tags. These are implied by authorship,
>>> otherwise all patches people make should have such tag.
>> 
>> Ok I see. AFAIR it originates in several phases of editing to report on which device it was tested.
>> 
>> Is there a canonical way of writing "tested-on: ${HARDWARE}"?
>> 
>> E.g. would this be ok?
>> 
>> Signed-off: H. Nikolaus Schaller <hns@goldelico.com> # tested on Pyra-Handheld
> 
> If you think tested platform is worth mentioning in the commit msg
> (it will stay there forever, ever, ever) then just add a line like:
> 
> "Add SGX GPU node. Tested on Pyra-Handheld."
> 
> From time to time we add such information to note that only one platform
> was actually tested.

Yes that is what it should express.

>  I am not sure what benefit it brings to most
> cases... but your commit msg is so short that adding one more sentence
> seems reasonable. :)

Ok, will queue for v7.

> 
> Best regards,
> Krzysztof

BR and thanks,
Nikolaus

[PATCH v6 08/12] arm: dts: s5pv210: Add G3D node

[H. Nikolaus Schaller]

From: Jonathan Bakker <xc-racer2@live.ca>

to add support for SGX540 GPU.

Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/s5pv210.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi
index 2ad642f51fd9..e7fc709c0cca 100644
--- a/arch/arm/boot/dts/s5pv210.dtsi
+++ b/arch/arm/boot/dts/s5pv210.dtsi
@@ -512,6 +512,21 @@ vic3: interrupt-controller@f2300000 {
 			#interrupt-cells = <1>;
 		};
 
+		g3d: g3d@f3000000 {
+			compatible = "samsung,s5pv210-sgx540-120";
+			reg = <0xf3000000 0x10000>;
+			interrupt-parent = <&vic2>;
+			interrupts = <10>;
+			clock-names = "sclk";
+			clocks = <&clocks CLK_G3D>;
+
+			power-domains = <&pd S5PV210_PD_G3D>;
+
+			assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
+			assigned-clock-rates = <0>, <66700000>;
+			assigned-clock-parents = <&clocks MOUT_MPLL>;
+		};
+
 		fimd: fimd@f8000000 {
 			compatible = "samsung,s5pv210-fimd";
 			interrupt-parent = <&vic2>;
-- 
2.25.1

Re: [PATCH v6 08/12] arm: dts: s5pv210: Add G3D node

[Sergei Shtylyov]

Hello!

On 15.04.2020 11:35, H. Nikolaus Schaller wrote:

> From: Jonathan Bakker <xc-racer2@live.ca>
> 
> to add support for SGX540 GPU.
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>   arch/arm/boot/dts/s5pv210.dtsi | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi
> index 2ad642f51fd9..e7fc709c0cca 100644
> --- a/arch/arm/boot/dts/s5pv210.dtsi
> +++ b/arch/arm/boot/dts/s5pv210.dtsi
> @@ -512,6 +512,21 @@ vic3: interrupt-controller@f2300000 {
>   			#interrupt-cells = <1>;
>   		};
>   
> +		g3d: g3d@f3000000 {

    Should be named generically, "gpu@f3000000", according to the DT spec 0.2, 
section 2.2.2. It's either "gpu" or "display" TTBOMK...

[...]

MBR, Sergei

Re: [PATCH v6 08/12] arm: dts: s5pv210: Add G3D node

[H. Nikolaus Schaller]

Hi Sergei and Jonathan,

> Am 15.04.2020 um 11:15 schrieb Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>:
> 
> Hello!
> 
> On 15.04.2020 11:35, H. Nikolaus Schaller wrote:
> 
>> From: Jonathan Bakker <xc-racer2@live.ca>
>> to add support for SGX540 GPU.
>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>>  arch/arm/boot/dts/s5pv210.dtsi | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi
>> index 2ad642f51fd9..e7fc709c0cca 100644
>> --- a/arch/arm/boot/dts/s5pv210.dtsi
>> +++ b/arch/arm/boot/dts/s5pv210.dtsi
>> @@ -512,6 +512,21 @@ vic3: interrupt-controller@f2300000 {
>>  			#interrupt-cells = <1>;
>>  		};
>>  +		g3d: g3d@f3000000 {
> 
>   Should be named generically, "gpu@f3000000", according to the DT spec 0.2, section 2.2.2. It's either "gpu" or "display" TTBOMK...

Yes, you are right and we have named it such for all other
devices in this series. I just missed that.

Jonathan, if you are ok, I'll fix that.

> 
> [...]
> 
> MBR, Sergei

BR and thanks,
Nikolaus

Re: [PATCH v6 08/12] arm: dts: s5pv210: Add G3D node

[Krzysztof Kozlowski]

On Wed, 15 Apr 2020 at 10:36, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>
> From: Jonathan Bakker <xc-racer2@live.ca>
>
> to add support for SGX540 GPU.

Do not continue the subject in commit msg like it is one sentence.
These are two separate sentences, so commit msg starts with capital
letter and it is sentence by itself.

> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  arch/arm/boot/dts/s5pv210.dtsi | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi
> index 2ad642f51fd9..e7fc709c0cca 100644
> --- a/arch/arm/boot/dts/s5pv210.dtsi
> +++ b/arch/arm/boot/dts/s5pv210.dtsi
> @@ -512,6 +512,21 @@ vic3: interrupt-controller@f2300000 {
>                         #interrupt-cells = <1>;
>                 };
>
> +               g3d: g3d@f3000000 {
> +                       compatible = "samsung,s5pv210-sgx540-120";
> +                       reg = <0xf3000000 0x10000>;
> +                       interrupt-parent = <&vic2>;
> +                       interrupts = <10>;
> +                       clock-names = "sclk";
> +                       clocks = <&clocks CLK_G3D>;

Not part of bindings, please remove or add to the bindings.

> +
> +                       power-domains = <&pd S5PV210_PD_G3D>;

Ditto

> +
> +                       assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
> +                       assigned-clock-rates = <0>, <66700000>;
> +                       assigned-clock-parents = <&clocks MOUT_MPLL>;

Probably this should have status disabled because you do not set
regulator supply.

Best regards,
Krzysztof

Re: [PATCH v6 08/12] arm: dts: s5pv210: Add G3D node

[H. Nikolaus Schaller]

> Am 15.04.2020 um 13:49 schrieb Krzysztof Kozlowski <krzk@kernel.org>:
> 
> On Wed, 15 Apr 2020 at 10:36, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> 
>> From: Jonathan Bakker <xc-racer2@live.ca>
>> 
>> to add support for SGX540 GPU.
> 
> Do not continue the subject in commit msg like it is one sentence.
> These are two separate sentences, so commit msg starts with capital
> letter and it is sentence by itself.
> 
>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> arch/arm/boot/dts/s5pv210.dtsi | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>> 
>> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi
>> index 2ad642f51fd9..e7fc709c0cca 100644
>> --- a/arch/arm/boot/dts/s5pv210.dtsi
>> +++ b/arch/arm/boot/dts/s5pv210.dtsi
>> @@ -512,6 +512,21 @@ vic3: interrupt-controller@f2300000 {
>>                        #interrupt-cells = <1>;
>>                };
>> 
>> +               g3d: g3d@f3000000 {
>> +                       compatible = "samsung,s5pv210-sgx540-120";
>> +                       reg = <0xf3000000 0x10000>;
>> +                       interrupt-parent = <&vic2>;
>> +                       interrupts = <10>;
>> +                       clock-names = "sclk";
>> +                       clocks = <&clocks CLK_G3D>;
> 
> Not part of bindings, please remove or add to the bindings.

Well, the bindings should describe what is common for all SoC
and they are quite different in what they need in addition.

Thererfore we have no "additionalProperties: false" in the
bindings [PATCH v6 01/12].

> 
>> +
>> +                       power-domains = <&pd S5PV210_PD_G3D>;
> 
> Ditto

In this case it might be possible to add the clock/power-domains
etc. to a wrapper node compatible to "simple-pm-bus" or similar
and make the gpu a child of it.

@Jontahan: can you please give it a try?


> 
>> +
>> +                       assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
>> +                       assigned-clock-rates = <0>, <66700000>;
>> +                       assigned-clock-parents = <&clocks MOUT_MPLL>;
> 
> Probably this should have status disabled because you do not set
> regulator supply.
> 
> Best regards,
> Krzysztof

BR and thanks,
Nikolaus

Re: [PATCH v6 08/12] arm: dts: s5pv210: Add G3D node

[Krzysztof Kozlowski]

On Wed, Apr 15, 2020 at 02:50:31PM +0200, H. Nikolaus Schaller wrote:
> 
> > Am 15.04.2020 um 13:49 schrieb Krzysztof Kozlowski <krzk@kernel.org>:
> > 
> > On Wed, 15 Apr 2020 at 10:36, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> >> 
> >> From: Jonathan Bakker <xc-racer2@live.ca>
> >> 
> >> to add support for SGX540 GPU.
> > 
> > Do not continue the subject in commit msg like it is one sentence.
> > These are two separate sentences, so commit msg starts with capital
> > letter and it is sentence by itself.
> > 
> >> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> >> ---
> >> arch/arm/boot/dts/s5pv210.dtsi | 15 +++++++++++++++
> >> 1 file changed, 15 insertions(+)
> >> 
> >> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi
> >> index 2ad642f51fd9..e7fc709c0cca 100644
> >> --- a/arch/arm/boot/dts/s5pv210.dtsi
> >> +++ b/arch/arm/boot/dts/s5pv210.dtsi
> >> @@ -512,6 +512,21 @@ vic3: interrupt-controller@f2300000 {
> >>                        #interrupt-cells = <1>;
> >>                };
> >> 
> >> +               g3d: g3d@f3000000 {
> >> +                       compatible = "samsung,s5pv210-sgx540-120";
> >> +                       reg = <0xf3000000 0x10000>;
> >> +                       interrupt-parent = <&vic2>;
> >> +                       interrupts = <10>;
> >> +                       clock-names = "sclk";
> >> +                       clocks = <&clocks CLK_G3D>;
> > 
> > Not part of bindings, please remove or add to the bindings.
> 
> Well, the bindings should describe what is common for all SoC
> and they are quite different in what they need in addition.
> 
> Thererfore we have no "additionalProperties: false" in the
> bindings [PATCH v6 01/12].

If these properties are needed for Exynos-specific implementation, they
should be in the bindings. If they are not needed, they should not be
here.

Take a look at midgard bindings for example. This is a nice starting
point for these here.

Best regards,
Krzysztof

Re: [PATCH v6 08/12] arm: dts: s5pv210: Add G3D node

[Jonathan Bakker]

Hi Nikolaus,

On 2020-04-15 5:50 a.m., H. Nikolaus Schaller wrote:
> 
>> Am 15.04.2020 um 13:49 schrieb Krzysztof Kozlowski <krzk@kernel.org>:
>>
>> On Wed, 15 Apr 2020 at 10:36, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>>
>>> From: Jonathan Bakker <xc-racer2@live.ca>
>>>
>>> to add support for SGX540 GPU.
>>
>> Do not continue the subject in commit msg like it is one sentence.
>> These are two separate sentences, so commit msg starts with capital
>> letter and it is sentence by itself.
>>

Sorry, that's my fault, I should know better.

Nikolaus took this from my testing tree and I apparently didn't have it in
as good as state as I should have.

>>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> ---
>>> arch/arm/boot/dts/s5pv210.dtsi | 15 +++++++++++++++
>>> 1 file changed, 15 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi
>>> index 2ad642f51fd9..e7fc709c0cca 100644
>>> --- a/arch/arm/boot/dts/s5pv210.dtsi
>>> +++ b/arch/arm/boot/dts/s5pv210.dtsi
>>> @@ -512,6 +512,21 @@ vic3: interrupt-controller@f2300000 {
>>>                        #interrupt-cells = <1>;
>>>                };
>>>
>>> +               g3d: g3d@f3000000 {
>>> +                       compatible = "samsung,s5pv210-sgx540-120";
>>> +                       reg = <0xf3000000 0x10000>;
>>> +                       interrupt-parent = <&vic2>;
>>> +                       interrupts = <10>;
>>> +                       clock-names = "sclk";
>>> +                       clocks = <&clocks CLK_G3D>;
>>
>> Not part of bindings, please remove or add to the bindings.
> 
> Well, the bindings should describe what is common for all SoC
> and they are quite different in what they need in addition.
> 
> Thererfore we have no "additionalProperties: false" in the
> bindings [PATCH v6 01/12].
> 
>>
>>> +
>>> +                       power-domains = <&pd S5PV210_PD_G3D>;
>>
>> Ditto
> 
> In this case it might be possible to add the clock/power-domains
> etc. to a wrapper node compatible to "simple-pm-bus" or similar
> and make the gpu a child of it.
> 
> @Jontahan: can you please give it a try?
> 
> 

The power-domains comes from a (so far) non-upstreamed power domain driver
for s5pv210 that I've been playing around with.  It's not necessary for proper
operation as it's on by default.

Looking at simple-pm-bus, I don't really understand its purpose.  Is it a way of separating
out a power domain from a main device's node?  Or is it designed for when you have multiple
devices under the same power domain?

Nikolaus, I can regenerate a proper patch for you if you want that's not based on my testing tree.

>>
>>> +
>>> +                       assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
>>> +                       assigned-clock-rates = <0>, <66700000>;
>>> +                       assigned-clock-parents = <&clocks MOUT_MPLL>;
>>
>> Probably this should have status disabled because you do not set
>> regulator supply.

I don't believe there is a regulator on s5pv210, if there is, then it is a
fixed regulator with no control on both s5pv210 devices that I have.

The vendor driver did use the regulator framework for its power domain
implementation, but that definitely shouldn't be upstreamed.

> BR and thanks,
> Nikolaus
> 

Thanks,
Jonathan

Re: [PATCH v6 08/12] arm: dts: s5pv210: Add G3D node

[Krzysztof Kozlowski]

On Wed, Apr 15, 2020 at 11:17:16AM -0700, Jonathan Bakker wrote:
 
> >>
> >>> +
> >>> +                       assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
> >>> +                       assigned-clock-rates = <0>, <66700000>;
> >>> +                       assigned-clock-parents = <&clocks MOUT_MPLL>;
> >>
> >> Probably this should have status disabled because you do not set
> >> regulator supply.
> 
> I don't believe there is a regulator on s5pv210, if there is, then it is a
> fixed regulator with no control on both s5pv210 devices that I have.
> 
> The vendor driver did use the regulator framework for its power domain
> implementation, but that definitely shouldn't be upstreamed.

Starting with Exynos4210 usually they have separate regulator from PMIC
but maybe S5Pv210 indeed is different.  Leave it then without it.

Best regards,
Krzysztof

Re: [PATCH v6 08/12] arm: dts: s5pv210: Add G3D node

[H. Nikolaus Schaller]

Hi Jonathan,

> Am 15.04.2020 um 20:17 schrieb Jonathan Bakker <xc-racer2@live.ca>:
> 
> Hi Nikolaus,
> 
> On 2020-04-15 5:50 a.m., H. Nikolaus Schaller wrote:
>> 
>>> Am 15.04.2020 um 13:49 schrieb Krzysztof Kozlowski <krzk@kernel.org>:
>>> 
>>> On Wed, 15 Apr 2020 at 10:36, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>>> 
>>>> From: Jonathan Bakker <xc-racer2@live.ca>
>>>> 
>>>> to add support for SGX540 GPU.
>>> 
>>> Do not continue the subject in commit msg like it is one sentence.
>>> These are two separate sentences, so commit msg starts with capital
>>> letter and it is sentence by itself.
>>> 
> 
> Sorry, that's my fault, I should know better.

Mine as well...

> 
> Nikolaus took this from my testing tree and I apparently didn't have it in
> as good as state as I should have.
> 
>>>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>> ---
>>>> arch/arm/boot/dts/s5pv210.dtsi | 15 +++++++++++++++
>>>> 1 file changed, 15 insertions(+)
>>>> 
>>>> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi
>>>> index 2ad642f51fd9..e7fc709c0cca 100644
>>>> --- a/arch/arm/boot/dts/s5pv210.dtsi
>>>> +++ b/arch/arm/boot/dts/s5pv210.dtsi
>>>> @@ -512,6 +512,21 @@ vic3: interrupt-controller@f2300000 {
>>>>                       #interrupt-cells = <1>;
>>>>               };
>>>> 
>>>> +               g3d: g3d@f3000000 {
>>>> +                       compatible = "samsung,s5pv210-sgx540-120";
>>>> +                       reg = <0xf3000000 0x10000>;
>>>> +                       interrupt-parent = <&vic2>;
>>>> +                       interrupts = <10>;
>>>> +                       clock-names = "sclk";
>>>> +                       clocks = <&clocks CLK_G3D>;
>>> 
>>> Not part of bindings, please remove or add to the bindings.
>> 
>> Well, the bindings should describe what is common for all SoC
>> and they are quite different in what they need in addition.
>> 
>> Thererfore we have no "additionalProperties: false" in the
>> bindings [PATCH v6 01/12].
>> 
>>> 
>>>> +
>>>> +                       power-domains = <&pd S5PV210_PD_G3D>;
>>> 
>>> Ditto
>> 
>> In this case it might be possible to add the clock/power-domains
>> etc. to a wrapper node compatible to "simple-pm-bus" or similar
>> and make the gpu a child of it.
>> 
>> @Jontahan: can you please give it a try?
>> 
>> 
> 
> The power-domains comes from a (so far) non-upstreamed power domain driver
> for s5pv210 that I've been playing around with.  It's not necessary for proper
> operation as it's on by default.
> 
> Looking at simple-pm-bus, I don't really understand its purpose.  Is it a way of separating
> out a power domain from a main device's node?  Or is it designed for when you have multiple
> devices under the same power domain?
> 
> Nikolaus, I can regenerate a proper patch for you if you want that's not based on my testing tree.

Yes, please.

> 
>>> 
>>>> +
>>>> +                       assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
>>>> +                       assigned-clock-rates = <0>, <66700000>;
>>>> +                       assigned-clock-parents = <&clocks MOUT_MPLL>;
>>> 
>>> Probably this should have status disabled because you do not set
>>> regulator supply.
> 
> I don't believe there is a regulator on s5pv210, if there is, then it is a
> fixed regulator with no control on both s5pv210 devices that I have.
> 
> The vendor driver did use the regulator framework for its power domain
> implementation, but that definitely shouldn't be upstreamed.

Ok, this means there is no need for regulators in the bindings.

BR,
Nikolaus

Re: [PATCH v6 08/12] arm: dts: s5pv210: Add G3D node

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1436 bytes --]

Hi Nikolaus,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on sunxi/sunxi/for-next v5.7-rc2 next-20200421]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/H-Nikolaus-Schaller/ARM-MIPS-DTS-add-child-nodes-describing-the-PVRSGX-GPU-present-in-some-OMAP-SoC-and-JZ4780-and-many-more/20200416-022658
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> Error: arch/arm/boot/dts/s5pv210.dtsi:523.25-26 syntax error
   FATAL ERROR: Unable to parse input tree

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 74458 bytes --]

[PATCH v6 09/12] ARM: dts: sun6i: a31: add sgx gpu child node

[H. Nikolaus Schaller]

From: Philipp Rossak <embed3d@gmail.com>

We are adding the devicetree binding for the PVR-SGX-544-115 gpu.

This driver is currently under development in the openpvrsgx-devgroup.
Right now the full binding is not figured out, so we provide here a
placeholder. It will be completed as soon as there is a demo available.

The currently used binding that is used during development is more
complete and was already verifyed by loading the kernelmodule successful.

Signed-off-by: Philipp Rossak <embed3d@gmail.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index f3425a66fc0a..933a825bf460 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -1417,5 +1417,16 @@ p2wi: i2c@1f03400 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 		};
+
+		gpu: gpu@1c400000 {
+			compatible = "allwinner,sun8i-a31-sgx544-115",
+				     "img,sgx544-115", "img,sgx544";
+			reg = <0x01c40000 0x10000>;
+			/*
+			 * This node is currently a placeholder for the gpu.
+			 * This will be completed when a full demonstration
+			 * of the openpvrsgx driver is available for this board.
+			 */
+		};
 	};
 };
-- 
2.25.1

[PATCH v6 10/12] ARM: dts: sun6i: a31s: add sgx gpu child node

[H. Nikolaus Schaller]

From: Philipp Rossak <embed3d@gmail.com>

We are adding the devicetree binding for the PVR-SGX-544-115 gpu.

This driver is currently under development in the openpvrsgx-devgroup.
Right now the full binding is not figured out, so we provide here a
placeholder. It will be completed as soon as there is a demo available.

The currently used binding that is used during development is more
complete and was already verifyed by loading the kernelmodule successful.

Signed-off-by: Philipp Rossak <embed3d@gmail.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/sun6i-a31s.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31s.dtsi b/arch/arm/boot/dts/sun6i-a31s.dtsi
index 97e2c51d0aea..669770d2934a 100644
--- a/arch/arm/boot/dts/sun6i-a31s.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31s.dtsi
@@ -59,3 +59,13 @@ &pio {
 &tcon0 {
 	compatible = "allwinner,sun6i-a31s-tcon";
 };
+
+&gpu {
+	compatible = "allwinner,sun8i-a31s-sgx544-115",
+		     "img,sgx544-115", "img,sgx544";
+	/*
+	 * This node is currently a placeholder for the gpu.
+	 * This will be completed when a full demonstration
+	 * of the openpvrsgx driver is available for this board.
+	 */
+};
-- 
2.25.1

[PATCH v6 11/12] ARM: dts: sun8i: a83t: add sgx gpu child node

[H. Nikolaus Schaller]

From: Philipp Rossak <embed3d@gmail.com>

We are adding the devicetree binding for the PVR-SGX-544-115 gpu.

This driver is currently under development in the openpvrsgx-devgroup.
Right now the full binding is not figured out, so we provide here a
placeholder. It will be completed as soon as there is a demo available.

The currently used binding that is used during development is more
complete and was already verifyed by loading the kernelmodule successful.

Signed-off-by: Philipp Rossak <embed3d@gmail.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/arm/boot/dts/sun8i-a83t.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi b/arch/arm/boot/dts/sun8i-a83t.dtsi
index 655404d6d3a3..bfb900622bf6 100644
--- a/arch/arm/boot/dts/sun8i-a83t.dtsi
+++ b/arch/arm/boot/dts/sun8i-a83t.dtsi
@@ -1192,6 +1192,17 @@ ths: thermal-sensor@1f04000 {
 			nvmem-cell-names = "calibration";
 			#thermal-sensor-cells = <1>;
 		};
+
+		gpu: gpu@1c400000 {
+			compatible = "allwinner,sun8i-a83t-sgx544-115",
+				     "img,sgx544-115", "img,sgx544";
+			reg = <0x01c40000 0x10000>;
+			/*
+			 * This node is currently a placeholder for the gpu.
+			 * This will be completed when a full demonstration
+			 * of the openpvrsgx driver is available for this board.
+			 */
+		};
 	};
 
 	thermal-zones {
-- 
2.25.1

[PATCH v6 12/12] MIPS: DTS: jz4780: add sgx gpu node

[H. Nikolaus Schaller]

and add interrupt and clocks.

Tested to build for CI20 board and load a driver.
Setup can not yet be fully tested since there is no working
HDMI driver for jz4780.

Suggested-by: Paul Boddie <paul@boddie.org.uk>
Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # CI20.
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index bb89653d16a3..883fe2c4c9e1 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -357,6 +357,17 @@ i2c4: i2c@10054000 {
 		status = "disabled";
 	};
 
+	gpu: gpu@13040000 {
+		compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
+		reg = <0x13040000 0x4000>;
+
+		clocks = <&cgu JZ4780_CLK_GPU>;
+		clock-names = "gpu";
+
+		interrupt-parent = <&intc>;
+		interrupts = <63>;
+	};
+
 	nemc: nemc@13410000 {
 		compatible = "ingenic,jz4780-nemc";
 		reg = <0x13410000 0x10000>;
-- 
2.25.1

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2889 bytes --]

Hi,

On Wed, Apr 15, 2020 at 10:35:07AM +0200, H. Nikolaus Schaller wrote:
> * rebased to v5.7-rc1
> * added DTS for for a31, a31s, a83t - by Philipp Rossak <embed3d@gmail.com>
> * added DTS for "samsung,s5pv210-sgx540-120" - by Jonathan Bakker <xc-racer2@live.ca>
> * bindings.yaml fixes:
>   - added a31, a31
>   - fixes for omap4470
>   - jz4780 contains an sgx540-130 and not -120
>   - a83t contains an sgx544-115 and not -116
>   - removed "additionalProperties: false" because some SoC may need additional properties
>
> PATCH V5 2020-03-29 19:38:32:
> * reworked YAML bindings to pass dt_binding_check and be better grouped
> * rename all nodes to "gpu: gpu@<address>"
> * removed "img,sgx5" from example - suggested by Rob Herring <robh+dt@kernel.org>
>
> PATCH V4 2019-12-17 19:02:11:
> * MIPS: DTS: jz4780: removed "img,sgx5" from bindings
> * YAML bindings: updated according to suggestions by Rob Herring
> * MIPS: DTS: jz4780: insert-sorted gpu node by register address - suggested by Paul Cercueil
>
> PATCH V3 2019-11-24 12:40:33:
> * reworked YAML format with help by Rob Herring
> * removed .txt binding document
> * change compatible "ti,am335x-sgx" to "ti,am3352-sgx" - suggested by Tony Lindgren
>
> PATCH V2 2019-11-07 12:06:17:
> * tried to convert bindings to YAML format - suggested by Rob Herring
> * added JZ4780 DTS node (proven to load the driver)
> * removed timer and img,cores properties until we know we really need them - suggested by Rob Herring
>
> PATCH V1 2019-10-18 20:46:35:
>
> This patch series defines child nodes for the SGX5xx interface inside
> different SoC so that a driver can be found and probed by the
> compatible strings and can retrieve information about the SGX revision
> that is included in a specific SoC. It also defines the interrupt number
> to be used by the SGX driver.
>
> There is currently no mainline driver for these GPUs, but a project
> [1] is ongoing with the goal to get the open-source part as provided
> by TI/IMG and others into drivers/gpu/drm/pvrsgx.

Just a heads up, DRM requires an open-source user-space, so if your
plan is to move the open-source kernel driver while using the
closed-source library (as that page seem to suggest), that might
change a few things.

> The kernel modules built from this project have successfully
> demonstrated to work with the DTS definitions from this patch set on
> AM335x BeagleBone Black, DM3730 and OMAP5 Pyra and Droid 4. They
> partially work on OMAP3530 and PandaBoard ES but that is likely a
> problem in the kernel driver or the (non-free) user-space libraries
> and binaries.
>
> Wotk for JZ4780 (CI20 board) is in progress and there is potential
> to extend this work to e.g. BananaPi-M3 (A83) and some Intel Poulsbo
> and CedarView devices.

If it's not been tested on any Allwinner board yet, I'll leave it
aside until it's been properly shown to work.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[H. Nikolaus Schaller]

Hi Maxime,

> Am 15.04.2020 um 12:10 schrieb Maxime Ripard <maxime@cerno.tech>:
> 
> Hi,
> 
> On Wed, Apr 15, 2020 at 10:35:07AM +0200, H. Nikolaus Schaller wrote:
>> * rebased to v5.7-rc1
>> * added DTS for for a31, a31s, a83t - by Philipp Rossak <embed3d@gmail.com>
>> * added DTS for "samsung,s5pv210-sgx540-120" - by Jonathan Bakker <xc-racer2@live.ca>
>> * bindings.yaml fixes:
>>  - added a31, a31
>>  - fixes for omap4470
>>  - jz4780 contains an sgx540-130 and not -120
>>  - a83t contains an sgx544-115 and not -116
>>  - removed "additionalProperties: false" because some SoC may need additional properties
>> 
>> PATCH V5 2020-03-29 19:38:32:
>> * reworked YAML bindings to pass dt_binding_check and be better grouped
>> * rename all nodes to "gpu: gpu@<address>"
>> * removed "img,sgx5" from example - suggested by Rob Herring <robh+dt@kernel.org>
>> 
>> PATCH V4 2019-12-17 19:02:11:
>> * MIPS: DTS: jz4780: removed "img,sgx5" from bindings
>> * YAML bindings: updated according to suggestions by Rob Herring
>> * MIPS: DTS: jz4780: insert-sorted gpu node by register address - suggested by Paul Cercueil
>> 
>> PATCH V3 2019-11-24 12:40:33:
>> * reworked YAML format with help by Rob Herring
>> * removed .txt binding document
>> * change compatible "ti,am335x-sgx" to "ti,am3352-sgx" - suggested by Tony Lindgren
>> 
>> PATCH V2 2019-11-07 12:06:17:
>> * tried to convert bindings to YAML format - suggested by Rob Herring
>> * added JZ4780 DTS node (proven to load the driver)
>> * removed timer and img,cores properties until we know we really need them - suggested by Rob Herring
>> 
>> PATCH V1 2019-10-18 20:46:35:
>> 
>> This patch series defines child nodes for the SGX5xx interface inside
>> different SoC so that a driver can be found and probed by the
>> compatible strings and can retrieve information about the SGX revision
>> that is included in a specific SoC. It also defines the interrupt number
>> to be used by the SGX driver.
>> 
>> There is currently no mainline driver for these GPUs, but a project
>> [1] is ongoing with the goal to get the open-source part as provided
>> by TI/IMG and others into drivers/gpu/drm/pvrsgx.
> 
> Just a heads up, DRM requires an open-source user-space, so if your
> plan is to move the open-source kernel driver while using the
> closed-source library (as that page seem to suggest), that might
> change a few things.

The far future goal is to arrive at a completely open implementation,
but nobody knows how to get there. Therefore we bake smaller bread :)

step 1: get SoC integration right and stable (this is what this series is for)
step 2: make the open source kernel driver work with closed-source libs
step 3: write open-source replacements for user-space

> 
>> The kernel modules built from this project have successfully
>> demonstrated to work with the DTS definitions from this patch set on
>> AM335x BeagleBone Black, DM3730 and OMAP5 Pyra and Droid 4. They
>> partially work on OMAP3530 and PandaBoard ES but that is likely a
>> problem in the kernel driver or the (non-free) user-space libraries
>> and binaries.
>> 
>> Wotk for JZ4780 (CI20 board) is in progress and there is potential
>> to extend this work to e.g. BananaPi-M3 (A83) and some Intel Poulsbo
>> and CedarView devices.
> 
> If it's not been tested on any Allwinner board yet, I'll leave it
> aside until it's been properly shown to work.

Phillip has testes something on a83.

BR and thanks,
Nikolaus

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Daniel Vetter]

On Wed, Apr 15, 2020 at 02:41:52PM +0200, H. Nikolaus Schaller wrote:
> Hi Maxime,
> 
> > Am 15.04.2020 um 12:10 schrieb Maxime Ripard <maxime@cerno.tech>:
> > 
> > Hi,
> > 
> > On Wed, Apr 15, 2020 at 10:35:07AM +0200, H. Nikolaus Schaller wrote:
> >> * rebased to v5.7-rc1
> >> * added DTS for for a31, a31s, a83t - by Philipp Rossak <embed3d@gmail.com>
> >> * added DTS for "samsung,s5pv210-sgx540-120" - by Jonathan Bakker <xc-racer2@live.ca>
> >> * bindings.yaml fixes:
> >>  - added a31, a31
> >>  - fixes for omap4470
> >>  - jz4780 contains an sgx540-130 and not -120
> >>  - a83t contains an sgx544-115 and not -116
> >>  - removed "additionalProperties: false" because some SoC may need additional properties
> >> 
> >> PATCH V5 2020-03-29 19:38:32:
> >> * reworked YAML bindings to pass dt_binding_check and be better grouped
> >> * rename all nodes to "gpu: gpu@<address>"
> >> * removed "img,sgx5" from example - suggested by Rob Herring <robh+dt@kernel.org>
> >> 
> >> PATCH V4 2019-12-17 19:02:11:
> >> * MIPS: DTS: jz4780: removed "img,sgx5" from bindings
> >> * YAML bindings: updated according to suggestions by Rob Herring
> >> * MIPS: DTS: jz4780: insert-sorted gpu node by register address - suggested by Paul Cercueil
> >> 
> >> PATCH V3 2019-11-24 12:40:33:
> >> * reworked YAML format with help by Rob Herring
> >> * removed .txt binding document
> >> * change compatible "ti,am335x-sgx" to "ti,am3352-sgx" - suggested by Tony Lindgren
> >> 
> >> PATCH V2 2019-11-07 12:06:17:
> >> * tried to convert bindings to YAML format - suggested by Rob Herring
> >> * added JZ4780 DTS node (proven to load the driver)
> >> * removed timer and img,cores properties until we know we really need them - suggested by Rob Herring
> >> 
> >> PATCH V1 2019-10-18 20:46:35:
> >> 
> >> This patch series defines child nodes for the SGX5xx interface inside
> >> different SoC so that a driver can be found and probed by the
> >> compatible strings and can retrieve information about the SGX revision
> >> that is included in a specific SoC. It also defines the interrupt number
> >> to be used by the SGX driver.
> >> 
> >> There is currently no mainline driver for these GPUs, but a project
> >> [1] is ongoing with the goal to get the open-source part as provided
> >> by TI/IMG and others into drivers/gpu/drm/pvrsgx.
> > 
> > Just a heads up, DRM requires an open-source user-space, so if your
> > plan is to move the open-source kernel driver while using the
> > closed-source library (as that page seem to suggest), that might
> > change a few things.
> 
> The far future goal is to arrive at a completely open implementation,
> but nobody knows how to get there. Therefore we bake smaller bread :)
> 
> step 1: get SoC integration right and stable (this is what this series is for)
> step 2: make the open source kernel driver work with closed-source libs
> step 3: write open-source replacements for user-space

step4: clean up the kernel driver
step5: get the mesa driver and kernel driver reviewed
step6: get it all merged

It's a very long road, but awesome to hear that someone is taking care of
pvrsgx. And I'm totally fine with landing stuff like you propose in step
1. Just not the driver/uapi itself.

Goog luck and have fun!

Cheers, Daniel

> 
> > 
> >> The kernel modules built from this project have successfully
> >> demonstrated to work with the DTS definitions from this patch set on
> >> AM335x BeagleBone Black, DM3730 and OMAP5 Pyra and Droid 4. They
> >> partially work on OMAP3530 and PandaBoard ES but that is likely a
> >> problem in the kernel driver or the (non-free) user-space libraries
> >> and binaries.
> >> 
> >> Wotk for JZ4780 (CI20 board) is in progress and there is potential
> >> to extend this work to e.g. BananaPi-M3 (A83) and some Intel Poulsbo
> >> and CedarView devices.
> > 
> > If it's not been tested on any Allwinner board yet, I'll leave it
> > aside until it's been properly shown to work.
> 
> Phillip has testes something on a83.
> 
> BR and thanks,
> Nikolaus

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

On Wed, Apr 15, 2020 at 02:41:52PM +0200, H. Nikolaus Schaller wrote:
> >> The kernel modules built from this project have successfully
> >> demonstrated to work with the DTS definitions from this patch set on
> >> AM335x BeagleBone Black, DM3730 and OMAP5 Pyra and Droid 4. They
> >> partially work on OMAP3530 and PandaBoard ES but that is likely a
> >> problem in the kernel driver or the (non-free) user-space libraries
> >> and binaries.
> >>
> >> Wotk for JZ4780 (CI20 board) is in progress and there is potential
> >> to extend this work to e.g. BananaPi-M3 (A83) and some Intel Poulsbo
> >> and CedarView devices.
> >
> > If it's not been tested on any Allwinner board yet, I'll leave it
> > aside until it's been properly shown to work.
>
> Phillip has testes something on a83.

I'm a bit skeptical on that one since it doesn't even list the
interrupts connected to the GPU that the binding mandates.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[H. Nikolaus Schaller]

> Am 15.04.2020 um 15:02 schrieb Maxime Ripard <maxime@cerno.tech>:
> 
> On Wed, Apr 15, 2020 at 02:41:52PM +0200, H. Nikolaus Schaller wrote:
>>>> The kernel modules built from this project have successfully
>>>> demonstrated to work with the DTS definitions from this patch set on
>>>> AM335x BeagleBone Black, DM3730 and OMAP5 Pyra and Droid 4. They
>>>> partially work on OMAP3530 and PandaBoard ES but that is likely a
>>>> problem in the kernel driver or the (non-free) user-space libraries
>>>> and binaries.
>>>> 
>>>> Wotk for JZ4780 (CI20 board) is in progress and there is potential
>>>> to extend this work to e.g. BananaPi-M3 (A83) and some Intel Poulsbo
>>>> and CedarView devices.
>>> 
>>> If it's not been tested on any Allwinner board yet, I'll leave it
>>> aside until it's been properly shown to work.
>> 
>> Phillip has tested something on a83.
> 
> I'm a bit skeptical on that one since it doesn't even list the
> interrupts connected to the GPU that the binding mandates.

I think he left it out for a future update.
But best he comments himself.

BR and thanks,
Nikolaus

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Philipp Rossak]

Hi all,

On 15.04.20 15:04, H. Nikolaus Schaller wrote:
> 
>> Am 15.04.2020 um 15:02 schrieb Maxime Ripard <maxime@cerno.tech>:
>>
>> On Wed, Apr 15, 2020 at 02:41:52PM +0200, H. Nikolaus Schaller wrote:
>>>>> The kernel modules built from this project have successfully
>>>>> demonstrated to work with the DTS definitions from this patch set on
>>>>> AM335x BeagleBone Black, DM3730 and OMAP5 Pyra and Droid 4. They
>>>>> partially work on OMAP3530 and PandaBoard ES but that is likely a
>>>>> problem in the kernel driver or the (non-free) user-space libraries
>>>>> and binaries.
>>>>>
>>>>> Wotk for JZ4780 (CI20 board) is in progress and there is potential
>>>>> to extend this work to e.g. BananaPi-M3 (A83) and some Intel Poulsbo
>>>>> and CedarView devices.
>>>>
>>>> If it's not been tested on any Allwinner board yet, I'll leave it
>>>> aside until it's been properly shown to work.
>>>
>>> Phillip has tested something on a83.
>>
Yes I'm currently working on the a83t demo. The kernel module is loading 
correctly and the clocks, interrupts and resets seems to be working 
correctly.

I'm currently working on getting the users space driver working with the 
kernel driver. This is hopefully done soon.

>> I'm a bit skeptical on that one since it doesn't even list the
>> interrupts connected to the GPU that the binding mandates.
> 
> I think he left it out for a future update.
> But best he comments himself.

I'm currently working on those bindings. They are now 90% done, but they 
are not finished till now. Currently there is some mainline support 
missing to add the full binding. The A83T and also the A31/A31s have a 
GPU Power Off Gating Register in the R_PRCM module, that is not 
supported right now in Mainline. The Register need to be written when 
the GPU is powered on and off.

@Maxime: I totally agree on your point that a demo needs to be provided 
before the related DTS patches should be provided. That's the reason why 
I added the gpu placeholder patches.
Do you have an idea how a driver for the R_PRCM stuff can look like? I'm 
not that experienced with the clock driver framework.

The big question is right now how to proceed with the A83T and A31s 
patches. I see there three options, which one do you prefer?:

1. Provide now placeholder patches and send new patches, if everything 
is clear and other things are mainlined
2. Provide now patches as complete as possible and provide later patches 
to complete them when the R_PRCM things are mainlined
3. Leave them out, till the related work is mainlined and the bindings 
are final.


Since this GPU IP core is very flexible and the SOC manufactures can 
configure it on their needs, I think the binding will extend in the 
future. For example the SGX544 GPU is available in different 
configurations: there is a SGX544 core and SGX544MPx core. The x stands 
for the count of the USSE (Universal Scalable Shader Engine) cores. For 
example the GPU in the A83T is a MP1 and the A31/A31s a MP2.
In addition to that some of the GPU's have also a 2D engine.
There might be even more differences in the GPU's that we don't know 
right now and should be described in the Devicetree, but that's a 
different topic that we should keep in mind.

Cheers
Philipp

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2798 bytes --]

Hi,

On Fri, Apr 17, 2020 at 02:09:06PM +0200, Philipp Rossak wrote:
> > > I'm a bit skeptical on that one since it doesn't even list the
> > > interrupts connected to the GPU that the binding mandates.
> > 
> > I think he left it out for a future update.
> > But best he comments himself.
> 
> I'm currently working on those bindings. They are now 90% done, but they are
> not finished till now. Currently there is some mainline support missing to
> add the full binding. The A83T and also the A31/A31s have a GPU Power Off
> Gating Register in the R_PRCM module, that is not supported right now in
> Mainline. The Register need to be written when the GPU is powered on and
> off.
> 
> @Maxime: I totally agree on your point that a demo needs to be provided
> before the related DTS patches should be provided. That's the reason why I
> added the gpu placeholder patches.
> Do you have an idea how a driver for the R_PRCM stuff can look like? I'm not
> that experienced with the clock driver framework.

It looks like a power-domain to me, so you'd rather plug that into the genpd
framework.

> The big question is right now how to proceed with the A83T and A31s patches.
> I see there three options, which one do you prefer?:
> 
> 1. Provide now placeholder patches and send new patches, if everything is
> clear and other things are mainlined
> 2. Provide now patches as complete as possible and provide later patches to
> complete them when the R_PRCM things are mainlined
> 3. Leave them out, till the related work is mainlined and the bindings are
> final.

Like I said, the DT *has* to be backward-compatible, so for any DT patch that
you are asking to be merged, you should be prepared to support it indefinitely
and be able to run from it, and you won't be able to change the bindings later
on.

> Since this GPU IP core is very flexible and the SOC manufactures can
> configure it on their needs, I think the binding will extend in the future.
> For example the SGX544 GPU is available in different configurations: there
> is a SGX544 core and SGX544MPx core. The x stands for the count of the USSE
> (Universal Scalable Shader Engine) cores. For example the GPU in the A83T is
> a MP1 and the A31/A31s a MP2.

Mali is in the same situation and it didn't cause much trouble.

> In addition to that some of the GPU's have also a 2D engine.

In the same memory region, running from the same interrupts, or is it a
completely separate IP that happens to be sold by the same vendor?

> There might be even more differences in the GPU's that we don't know right
> now and should be described in the Devicetree, but that's a different topic
> that we should keep in mind.

Like I said, it's not a completely different topic.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Philipp Rossak]

Hi,

On 20.04.20 09:38, Maxime Ripard wrote:
> Hi,
> 
> On Fri, Apr 17, 2020 at 02:09:06PM +0200, Philipp Rossak wrote:
>>>> I'm a bit skeptical on that one since it doesn't even list the
>>>> interrupts connected to the GPU that the binding mandates.
>>>
>>> I think he left it out for a future update.
>>> But best he comments himself.
>>
>> I'm currently working on those bindings. They are now 90% done, but they are
>> not finished till now. Currently there is some mainline support missing to
>> add the full binding. The A83T and also the A31/A31s have a GPU Power Off
>> Gating Register in the R_PRCM module, that is not supported right now in
>> Mainline. The Register need to be written when the GPU is powered on and
>> off.
>>
>> @Maxime: I totally agree on your point that a demo needs to be provided
>> before the related DTS patches should be provided. That's the reason why I
>> added the gpu placeholder patches.
>> Do you have an idea how a driver for the R_PRCM stuff can look like? I'm not
>> that experienced with the clock driver framework.
> 
> It looks like a power-domain to me, so you'd rather plug that into the genpd
> framework.

I had a look on genpd and I'm not really sure if that fits.

It is basically some bit that verify that the clocks should be enabled 
or disabled. I think this is better placed somewhere in the clocking 
framework.
I see there more similarities to the gating stuff.
Do you think it is suitable to implement it like the clock gating?


>> The big question is right now how to proceed with the A83T and A31s patches.
>> I see there three options, which one do you prefer?:
>>
>> 1. Provide now placeholder patches and send new patches, if everything is
>> clear and other things are mainlined
>> 2. Provide now patches as complete as possible and provide later patches to
>> complete them when the R_PRCM things are mainlined
>> 3. Leave them out, till the related work is mainlined and the bindings are
>> final.
> 
> Like I said, the DT *has* to be backward-compatible, so for any DT patch that
> you are asking to be merged, you should be prepared to support it indefinitely
> and be able to run from it, and you won't be able to change the bindings later
> on.

I agree on your points. But is this also suitable to drivers that are 
currently off tree and might be merged in one or two years?

>> Since this GPU IP core is very flexible and the SOC manufactures can
>> configure it on their needs, I think the binding will extend in the future.
>> For example the SGX544 GPU is available in different configurations: there
>> is a SGX544 core and SGX544MPx core. The x stands for the count of the USSE
>> (Universal Scalable Shader Engine) cores. For example the GPU in the A83T is
>> a MP1 and the A31/A31s a MP2.
> 
> Mali is in the same situation and it didn't cause much trouble.
> 
Good to know.

>> In addition to that some of the GPU's have also a 2D engine.
> 
> In the same memory region, running from the same interrupts, or is it a
> completely separate IP that happens to be sold by the same vendor?
> 
What I know till now this is part of the PowerVR IP and not separated. 
So it should use the same memory region, clocks and interrupts.

Cheers
Philipp

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3165 bytes --]

Hi,

On Tue, Apr 21, 2020 at 11:57:33AM +0200, Philipp Rossak wrote:
> On 20.04.20 09:38, Maxime Ripard wrote:
> > Hi,
> > 
> > On Fri, Apr 17, 2020 at 02:09:06PM +0200, Philipp Rossak wrote:
> > > > > I'm a bit skeptical on that one since it doesn't even list the
> > > > > interrupts connected to the GPU that the binding mandates.
> > > > 
> > > > I think he left it out for a future update.
> > > > But best he comments himself.
> > > 
> > > I'm currently working on those bindings. They are now 90% done, but they are
> > > not finished till now. Currently there is some mainline support missing to
> > > add the full binding. The A83T and also the A31/A31s have a GPU Power Off
> > > Gating Register in the R_PRCM module, that is not supported right now in
> > > Mainline. The Register need to be written when the GPU is powered on and
> > > off.
> > > 
> > > @Maxime: I totally agree on your point that a demo needs to be provided
> > > before the related DTS patches should be provided. That's the reason why I
> > > added the gpu placeholder patches.
> > > Do you have an idea how a driver for the R_PRCM stuff can look like? I'm not
> > > that experienced with the clock driver framework.
> > 
> > It looks like a power-domain to me, so you'd rather plug that into the genpd
> > framework.
> 
> I had a look on genpd and I'm not really sure if that fits.
> 
> It is basically some bit that verify that the clocks should be enabled or
> disabled.

No, it can do much more than that. It's a framework to control the SoCs power
domains, so clocks might be a part of it, but most of the time it's going to be
about powering up a particular device.

> I think this is better placed somewhere in the clocking framework.
> I see there more similarities to the gating stuff.
> Do you think it is suitable to implement it like the clock gating?

I'm really not sure what makes you think that this should be modelled as a
clock?

> > > The big question is right now how to proceed with the A83T and A31s patches.
> > > I see there three options, which one do you prefer?:
> > > 
> > > 1. Provide now placeholder patches and send new patches, if everything is
> > > clear and other things are mainlined
> > > 2. Provide now patches as complete as possible and provide later patches to
> > > complete them when the R_PRCM things are mainlined
> > > 3. Leave them out, till the related work is mainlined and the bindings are
> > > final.
> > 
> > Like I said, the DT *has* to be backward-compatible, so for any DT patch that
> > you are asking to be merged, you should be prepared to support it indefinitely
> > and be able to run from it, and you won't be able to change the bindings later
> > on.
> 
> I agree on your points. But is this also suitable to drivers that are
> currently off tree and might be merged in one or two years?

This is what we done for the Mali. The devicetree binding was first done for the
out-of-tree driver, and then lima/panfrost reused it.

The key thing here is to have enough confidence about how the hardware works so
that you can accurately describe it.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Tony Lindgren]

* Maxime Ripard <maxime@cerno.tech> [200421 11:22]:
> On Tue, Apr 21, 2020 at 11:57:33AM +0200, Philipp Rossak wrote:
> > I had a look on genpd and I'm not really sure if that fits.
> > 
> > It is basically some bit that verify that the clocks should be enabled or
> > disabled.
> 
> No, it can do much more than that. It's a framework to control the SoCs power
> domains, so clocks might be a part of it, but most of the time it's going to be
> about powering up a particular device.

Note that on omaps there are actually SoC module specific registers.
And there can be multiple devices within a single target module on
omaps. So the extra dts node and device is justified there.

For other SoCs, the SGX clocks are probably best handled directly
in pvr-drv.c PM runtime functions unless a custom hardware wrapper
with SoC specific registers exists.

Regards,

Tony

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[H. Nikolaus Schaller]

> Am 21.04.2020 um 16:15 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * Maxime Ripard <maxime@cerno.tech> [200421 11:22]:
>> On Tue, Apr 21, 2020 at 11:57:33AM +0200, Philipp Rossak wrote:
>>> I had a look on genpd and I'm not really sure if that fits.
>>> 
>>> It is basically some bit that verify that the clocks should be enabled or
>>> disabled.
>> 
>> No, it can do much more than that. It's a framework to control the SoCs power
>> domains, so clocks might be a part of it, but most of the time it's going to be
>> about powering up a particular device.
> 
> Note that on omaps there are actually SoC module specific registers.

Ah, I see. This is of course a difference that the TI glue logic has
its own registers in the same address range as the sgx and this can't
be easily handled by a common sgx driver.

This indeed seems to be unique with omap.

> And there can be multiple devices within a single target module on
> omaps. So the extra dts node and device is justified there.
> 
> For other SoCs, the SGX clocks are probably best handled directly
> in pvr-drv.c PM runtime functions unless a custom hardware wrapper
> with SoC specific registers exists.

That is why we need to evaluate what the better strategy is.

So we have
a) omap which has a custom wrapper around the sgx
b) others without, i.e. an empty (or pass-through) wrapper

Which one do we make the "standard" and which one the "exception"?
What are good reasons for either one?


I am currently in strong favour of a) being standard because it
makes the pvr-drv.c simpler and really generic (independent of
wrapping into any SoC).

This will likely avoid problems if we find more SoC with yet another
scheme how the SGX clocks are wrapped.

It also allows to handle different number of clocks (A31 seems to
need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings
or making big lists of conditionals. This variance would be handled
outside the sgx core bindings and driver.

So instead of an img+omap.yaml and an img+a81.yaml and an img+a31.yaml
etc. we have a single img,pvrsgx.yaml and individual wrappers (the omap
one already exists as bindings/bus/ti-sysc.txt).

The only drawback is that we need this "pass-through" wrapper in DTS
and driver code to handle clocks, power etc.


The second best solution in my view is to make b) the standard
and allow the clock(s) to be optional to cover the omap case.
And conditionals are added to properly describe the variance of
how the sgx is wrapped/integrated.


IMHO this is a decision which can not be easily revised later.
It is an architectural decision. So we should base it on strategic
goals.

> 
> 
> Regards,
> 
> Tony
> 

BR and thanks for clarification,
Nikolaus

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Tony Lindgren]

* H. Nikolaus Schaller <hns@goldelico.com> [200421 17:31]:
> > Am 21.04.2020 um 16:15 schrieb Tony Lindgren <tony@atomide.com>:
> > Note that on omaps there are actually SoC module specific registers.
> 
> Ah, I see. This is of course a difference that the TI glue logic has
> its own registers in the same address range as the sgx and this can't
> be easily handled by a common sgx driver.
> 
> This indeed seems to be unique with omap.
> 
> > And there can be multiple devices within a single target module on
> > omaps. So the extra dts node and device is justified there.
> > 
> > For other SoCs, the SGX clocks are probably best handled directly
> > in pvr-drv.c PM runtime functions unless a custom hardware wrapper
> > with SoC specific registers exists.
> 
> That is why we need to evaluate what the better strategy is.
> 
> So we have
> a) omap which has a custom wrapper around the sgx
> b) others without, i.e. an empty (or pass-through) wrapper
> 
> Which one do we make the "standard" and which one the "exception"?
> What are good reasons for either one?

The wrapper is already handled by the ti-sysc binding, the sgx
binding should be standard with optional clocks.

See for example the standard 8250 uart for am335x with:

$ git grep -B20 -A10 uart0 arch/arm/boot/dts/am33xx-l4.dtsi

The 8250 device configuration is described in the standard 8250
dts binding, and the am335x module in the ti-sysc binding.
The are separate devices :)

So for the sgx binding, you can just leave out TI specific
module wrapper completely from the example.

> It also allows to handle different number of clocks (A31 seems to
> need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings
> or making big lists of conditionals. This variance would be handled
> outside the sgx core bindings and driver.

Well if other SoCs implement genpd domains etc, that's then
again part of a separate binding and not part of the sgx binding.

Regards,

Tony

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [200421 10:39]:
> See for example the standard 8250 uart for am335x with:
> 
> $ git grep -B20 -A10 uart0 arch/arm/boot/dts/am33xx-l4.dtsi
> 
> The 8250 device configuration is described in the standard 8250
> dts binding, and the am335x module in the ti-sysc binding.
> The are separate devices :)

Just to clarify why it's like that, see for example
arch/arm/boot/dts/am33xx.dtsi, and target-module@47400000
in that file for the musb controller.

There's a single ti-sysc interconnect target module, but it has
multiple devices. There are two musb controler instances, two phy
instances and a cppi41 dma instance within a single module.

With sgx, I belive there is only the sgx IP within the
ti-sysc interconnect target module. They are still seprate
devices with their own control registers.

Regards,

Tony

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2326 bytes --]

On Tue, Apr 21, 2020 at 07:29:32PM +0200, H. Nikolaus Schaller wrote:
> 
> > Am 21.04.2020 um 16:15 schrieb Tony Lindgren <tony@atomide.com>:
> > 
> > * Maxime Ripard <maxime@cerno.tech> [200421 11:22]:
> >> On Tue, Apr 21, 2020 at 11:57:33AM +0200, Philipp Rossak wrote:
> >>> I had a look on genpd and I'm not really sure if that fits.
> >>> 
> >>> It is basically some bit that verify that the clocks should be enabled or
> >>> disabled.
> >> 
> >> No, it can do much more than that. It's a framework to control the SoCs power
> >> domains, so clocks might be a part of it, but most of the time it's going to be
> >> about powering up a particular device.
> > 
> > Note that on omaps there are actually SoC module specific registers.
> 
> Ah, I see. This is of course a difference that the TI glue logic has
> its own registers in the same address range as the sgx and this can't
> be easily handled by a common sgx driver.
> 
> This indeed seems to be unique with omap.
> 
> > And there can be multiple devices within a single target module on
> > omaps. So the extra dts node and device is justified there.
> > 
> > For other SoCs, the SGX clocks are probably best handled directly
> > in pvr-drv.c PM runtime functions unless a custom hardware wrapper
> > with SoC specific registers exists.
> 
> That is why we need to evaluate what the better strategy is.
> 
> So we have
> a) omap which has a custom wrapper around the sgx
> b) others without, i.e. an empty (or pass-through) wrapper
> 
> Which one do we make the "standard" and which one the "exception"?
> What are good reasons for either one?
> 
> 
> I am currently in strong favour of a) being standard because it
> makes the pvr-drv.c simpler and really generic (independent of
> wrapping into any SoC).
> 
> This will likely avoid problems if we find more SoC with yet another
> scheme how the SGX clocks are wrapped.
> 
> It also allows to handle different number of clocks (A31 seems to
> need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings
> or making big lists of conditionals. This variance would be handled
> outside the sgx core bindings and driver.

I disagree. Every other GPU binding and driver is handling that just fine, and
the SGX is not special in any case here.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[H. Nikolaus Schaller]

Hi Maxime,

> Am 22.04.2020 um 08:58 schrieb Maxime Ripard <maxime@cerno.tech>:
> 
> On Tue, Apr 21, 2020 at 07:29:32PM +0200, H. Nikolaus Schaller wrote:
>> 
>>> Am 21.04.2020 um 16:15 schrieb Tony Lindgren <tony@atomide.com>:
>>> 
>>> * Maxime Ripard <maxime@cerno.tech> [200421 11:22]:
>>>> On Tue, Apr 21, 2020 at 11:57:33AM +0200, Philipp Rossak wrote:
>>>>> I had a look on genpd and I'm not really sure if that fits.
>>>>> 
>>>>> It is basically some bit that verify that the clocks should be enabled or
>>>>> disabled.
>>>> 
>>>> No, it can do much more than that. It's a framework to control the SoCs power
>>>> domains, so clocks might be a part of it, but most of the time it's going to be
>>>> about powering up a particular device.
>>> 
>>> Note that on omaps there are actually SoC module specific registers.
>> 
>> Ah, I see. This is of course a difference that the TI glue logic has
>> its own registers in the same address range as the sgx and this can't
>> be easily handled by a common sgx driver.
>> 
>> This indeed seems to be unique with omap.
>> 
>>> And there can be multiple devices within a single target module on
>>> omaps. So the extra dts node and device is justified there.
>>> 
>>> For other SoCs, the SGX clocks are probably best handled directly
>>> in pvr-drv.c PM runtime functions unless a custom hardware wrapper
>>> with SoC specific registers exists.
>> 
>> That is why we need to evaluate what the better strategy is.
>> 
>> So we have
>> a) omap which has a custom wrapper around the sgx
>> b) others without, i.e. an empty (or pass-through) wrapper
>> 
>> Which one do we make the "standard" and which one the "exception"?
>> What are good reasons for either one?
>> 
>> 
>> I am currently in strong favour of a) being standard because it
>> makes the pvr-drv.c simpler and really generic (independent of
>> wrapping into any SoC).
>> 
>> This will likely avoid problems if we find more SoC with yet another
>> scheme how the SGX clocks are wrapped.
>> 
>> It also allows to handle different number of clocks (A31 seems to
>> need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings
>> or making big lists of conditionals. This variance would be handled
>> outside the sgx core bindings and driver.
> 
> I disagree. Every other GPU binding and driver is handling that just fine, and
> the SGX is not special in any case here.

Can you please better explain this? With example or a description
or a proposal?

I simply do not have your experience with "every other GPU" as you have.
And I admit that I can't read from your statement what we should do
to bring this topic forward.

So please make a proposal how it should be in your view.

BR and thanks,
Nikolaus

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3182 bytes --]

On Wed, Apr 22, 2020 at 09:10:57AM +0200, H. Nikolaus Schaller wrote:
> > Am 22.04.2020 um 08:58 schrieb Maxime Ripard <maxime@cerno.tech>:
> > 
> > On Tue, Apr 21, 2020 at 07:29:32PM +0200, H. Nikolaus Schaller wrote:
> >> 
> >>> Am 21.04.2020 um 16:15 schrieb Tony Lindgren <tony@atomide.com>:
> >>> 
> >>> * Maxime Ripard <maxime@cerno.tech> [200421 11:22]:
> >>>> On Tue, Apr 21, 2020 at 11:57:33AM +0200, Philipp Rossak wrote:
> >>>>> I had a look on genpd and I'm not really sure if that fits.
> >>>>> 
> >>>>> It is basically some bit that verify that the clocks should be enabled or
> >>>>> disabled.
> >>>> 
> >>>> No, it can do much more than that. It's a framework to control the SoCs power
> >>>> domains, so clocks might be a part of it, but most of the time it's going to be
> >>>> about powering up a particular device.
> >>> 
> >>> Note that on omaps there are actually SoC module specific registers.
> >> 
> >> Ah, I see. This is of course a difference that the TI glue logic has
> >> its own registers in the same address range as the sgx and this can't
> >> be easily handled by a common sgx driver.
> >> 
> >> This indeed seems to be unique with omap.
> >> 
> >>> And there can be multiple devices within a single target module on
> >>> omaps. So the extra dts node and device is justified there.
> >>> 
> >>> For other SoCs, the SGX clocks are probably best handled directly
> >>> in pvr-drv.c PM runtime functions unless a custom hardware wrapper
> >>> with SoC specific registers exists.
> >> 
> >> That is why we need to evaluate what the better strategy is.
> >> 
> >> So we have
> >> a) omap which has a custom wrapper around the sgx
> >> b) others without, i.e. an empty (or pass-through) wrapper
> >> 
> >> Which one do we make the "standard" and which one the "exception"?
> >> What are good reasons for either one?
> >> 
> >> 
> >> I am currently in strong favour of a) being standard because it
> >> makes the pvr-drv.c simpler and really generic (independent of
> >> wrapping into any SoC).
> >> 
> >> This will likely avoid problems if we find more SoC with yet another
> >> scheme how the SGX clocks are wrapped.
> >> 
> >> It also allows to handle different number of clocks (A31 seems to
> >> need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings
> >> or making big lists of conditionals. This variance would be handled
> >> outside the sgx core bindings and driver.
> > 
> > I disagree. Every other GPU binding and driver is handling that just fine, and
> > the SGX is not special in any case here.
> 
> Can you please better explain this? With example or a description
> or a proposal?

I can't, I don't have any knowledge about this GPU.

> I simply do not have your experience with "every other GPU" as you have.
> And I admit that I can't read from your statement what we should do
> to bring this topic forward.
> 
> So please make a proposal how it should be in your view.

If you need some inspiration, I guess you could look at the mali and vivante
bindings once you have an idea of what the GPU needs across the SoCs it's
integrated in.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[H. Nikolaus Schaller]

Hi Maxime,

> Am 22.04.2020 um 17:13 schrieb Maxime Ripard <maxime@cerno.tech>:
> 
> On Wed, Apr 22, 2020 at 09:10:57AM +0200, H. Nikolaus Schaller wrote:
>>> Am 22.04.2020 um 08:58 schrieb Maxime Ripard <maxime@cerno.tech>:
>>>> 
>>>> It also allows to handle different number of clocks (A31 seems to
>>>> need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings
>>>> or making big lists of conditionals. This variance would be handled
>>>> outside the sgx core bindings and driver.
>>> 
>>> I disagree. Every other GPU binding and driver is handling that just fine, and
>>> the SGX is not special in any case here.
>> 
>> Can you please better explain this? With example or a description
>> or a proposal?
> 
> I can't, I don't have any knowledge about this GPU.

Hm. Now I am fully puzzled.
You have no knowledge about this GPU but disagree with our proposal?
Is it just gut feeling?

Anyways, we need to find a solution. Together.

> 
>> I simply do not have your experience with "every other GPU" as you have.
>> And I admit that I can't read from your statement what we should do
>> to bring this topic forward.
>> 
>> So please make a proposal how it should be in your view.
> 
> If you need some inspiration, I guess you could look at the mali and vivante
> bindings once you have an idea of what the GPU needs across the SoCs it's
> integrated in.

Well, I do not need inspiration, we need to come to an agreement about
img,pvrsgx.yaml and we need some maintainer to finally pick it up.

I wonder how we can come to this stage.

If I look at vivante,gc.yaml or arm,mali-utgard.yaml I don't
see big differences to what we propose and those I see seem to come
from technical differences between sgx, vivante, mali etc. So there
is no single scheme that fits all different gpu types.

One thing we can learn is that "core" seems to be a de facto standard 
for the core clock-name. An alternative "gpu" is used by nvidia,gk20a.txt.

BR and thanks,
Nikolaus

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[H. Nikolaus Schaller]

Hi Paul,

> Am 22.04.2020 um 18:55 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus,
> 
> 
> Le mer. 22 avril 2020 à 18:09, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Maxime,
>>> Am 22.04.2020 um 17:13 schrieb Maxime Ripard <maxime@cerno.tech>:
>>> On Wed, Apr 22, 2020 at 09:10:57AM +0200, H. Nikolaus Schaller wrote:
>>>>> Am 22.04.2020 um 08:58 schrieb Maxime Ripard <maxime@cerno.tech>:
>>>>>> It also allows to handle different number of clocks (A31 seems to
>>>>>> need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings
>>>>>> or making big lists of conditionals. This variance would be handled
>>>>>> outside the sgx core bindings and driver.
>>>>> I disagree. Every other GPU binding and driver is handling that just fine, and
>>>>> the SGX is not special in any case here.
>>>> Can you please better explain this? With example or a description
>>>> or a proposal?
>>> I can't, I don't have any knowledge about this GPU.
>> Hm. Now I am fully puzzled.
>> You have no knowledge about this GPU but disagree with our proposal?
>> Is it just gut feeling?
>> Anyways, we need to find a solution. Together.
>>>> I simply do not have your experience with "every other GPU" as you have.
>>>> And I admit that I can't read from your statement what we should do
>>>> to bring this topic forward.
>>>> So please make a proposal how it should be in your view.
>>> If you need some inspiration, I guess you could look at the mali and vivante
>>> bindings once you have an idea of what the GPU needs across the SoCs it's
>>> integrated in.
>> Well, I do not need inspiration, we need to come to an agreement about
>> img,pvrsgx.yaml and we need some maintainer to finally pick it up.
>> I wonder how we can come to this stage.
>> If I look at vivante,gc.yaml or arm,mali-utgard.yaml I don't
>> see big differences to what we propose and those I see seem to come
>> from technical differences between sgx, vivante, mali etc. So there
>> is no single scheme that fits all different gpu types.
>> One thing we can learn is that "core" seems to be a de facto standard
>> for the core clock-name. An alternative "gpu" is used by nvidia,gk20a.txt.
> 
> The Vivante GPU binding requires "bus", "core" and "shader" clocks. But if your SoC only has one clock for the GPU, there's nothing that prevents you from passing the very same clock as "bus", "core" and "shader". This is what we do on the Ingenic JZ4770 SoC.

Fine and good to know.

Well, for the SGX we so far only know a single "core" clock (with different
names). Only the A31 seems to be different.

Fortunately I finally found a little time to scan through the a31
user manual: http://dl.linux-sunxi.org/A31/A31%20User%20Manual%20V1.20.pdf

There are 3 clock dividers. And there is a single clock PLL8 dedicated to
the gpu. The clock dividers are called "gpu core", "gpu mem", "gpu hyd".

Then, there are dedicated clock gating registers. And idle/power status
registers.

Unfortunately, chapter "5.1. GPU" is almost empty and has no block diagram.
So I have no idea what "HYD" stands for. And if the memory and HYD clocks
are needed and how they should be initialized. If they are different ones
or can all be driven by PLL8 in parallel.

That scarce information makes it difficult to form a "proper" bindings
document out of it. Any can fit or be false.

At least there is something common with all other SGX implementations I
am aware of: there is a "core" clock.

So I'd suggest to get things moving forwards:
* we add a "core" clock-names to the bindings
* this can't be wrong for the A31 since it is defined in the data sheet
* we make it optional since the omap chips have a clock wrapper
* "core" is a name I think all architectures/drivers can live with
  and can add later "shader", "bus" etc. if needed
* any additions for the A31 will be additions

If that sounds ok and nobody objects to it, I can submit a new patch
version for further review.

BR and thanks,
Nikolaus

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Philipp Rossak]

Hi,

On 22.04.20 19:23, H. Nikolaus Schaller wrote:
> Hi Paul,
> 
>> Am 22.04.2020 um 18:55 schrieb Paul Cercueil <paul@crapouillou.net>:
>>
>> Hi Nikolaus,
>>
>>
>> Le mer. 22 avril 2020 à 18:09, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>>> Hi Maxime,
>>>> Am 22.04.2020 um 17:13 schrieb Maxime Ripard <maxime@cerno.tech>:
>>>> On Wed, Apr 22, 2020 at 09:10:57AM +0200, H. Nikolaus Schaller wrote:
>>>>>> Am 22.04.2020 um 08:58 schrieb Maxime Ripard <maxime@cerno.tech>:
>>>>>>> It also allows to handle different number of clocks (A31 seems to
>>>>>>> need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings
>>>>>>> or making big lists of conditionals. This variance would be handled
>>>>>>> outside the sgx core bindings and driver.
>>>>>> I disagree. Every other GPU binding and driver is handling that just fine, and
>>>>>> the SGX is not special in any case here.
>>>>> Can you please better explain this? With example or a description
>>>>> or a proposal?
>>>> I can't, I don't have any knowledge about this GPU.
>>> Hm. Now I am fully puzzled.
>>> You have no knowledge about this GPU but disagree with our proposal?
>>> Is it just gut feeling?
>>> Anyways, we need to find a solution. Together.
>>>>> I simply do not have your experience with "every other GPU" as you have.
>>>>> And I admit that I can't read from your statement what we should do
>>>>> to bring this topic forward.
>>>>> So please make a proposal how it should be in your view.
>>>> If you need some inspiration, I guess you could look at the mali and vivante
>>>> bindings once you have an idea of what the GPU needs across the SoCs it's
>>>> integrated in.
>>> Well, I do not need inspiration, we need to come to an agreement about
>>> img,pvrsgx.yaml and we need some maintainer to finally pick it up.
>>> I wonder how we can come to this stage.
>>> If I look at vivante,gc.yaml or arm,mali-utgard.yaml I don't
>>> see big differences to what we propose and those I see seem to come
>>> from technical differences between sgx, vivante, mali etc. So there
>>> is no single scheme that fits all different gpu types.
>>> One thing we can learn is that "core" seems to be a de facto standard
>>> for the core clock-name. An alternative "gpu" is used by nvidia,gk20a.txt.
>>
>> The Vivante GPU binding requires "bus", "core" and "shader" clocks. But if your SoC only has one clock for the GPU, there's nothing that prevents you from passing the very same clock as "bus", "core" and "shader". This is what we do on the Ingenic JZ4770 SoC.
> 
> Fine and good to know.
> 
> Well, for the SGX we so far only know a single "core" clock (with different
> names). Only the A31 seems to be different.
> 
> Fortunately I finally found a little time to scan through the a31
> user manual: http://dl.linux-sunxi.org/A31/A31%20User%20Manual%20V1.20.pdf
> 
> There are 3 clock dividers. And there is a single clock PLL8 dedicated to
> the gpu. The clock dividers are called "gpu core", "gpu mem", "gpu hyd".
> 
> Then, there are dedicated clock gating registers. And idle/power status
> registers.
> 
> Unfortunately, chapter "5.1. GPU" is almost empty and has no block diagram.
> So I have no idea what "HYD" stands for. And if the memory and HYD clocks
> are needed and how they should be initialized. If they are different ones
> or can all be driven by PLL8 in parallel.
> 
> That scarce information makes it difficult to form a "proper" bindings
> document out of it. Any can fit or be false.
> 
> At least there is something common with all other SGX implementations I
> am aware of: there is a "core" clock.
> 
> So I'd suggest to get things moving forwards:
> * we add a "core" clock-names to the bindings
> * this can't be wrong for the A31 since it is defined in the data sheet
> * we make it optional since the omap chips have a clock wrapper
> * "core" is a name I think all architectures/drivers can live with
>    and can add later "shader", "bus" etc. if needed
> * any additions for the A31 will be additions
> 
> If that sounds ok and nobody objects to it, I can submit a new patch
> version for further review.
> 
> BR and thanks,
> Nikolaus
> 
A few years back, I did a big research on the PowerVR GPUs. Back then I 
found an interesting TI datasheet. I forgot about this till I have seen 
the right buzz words. Sorry that I remembered it that late.

Back then I came to the conclusion that all PowerVR GPU's have in 
general 3 Clocks.

A system clock, a memory clock and a core clock. [1].

The hyd_clk at sunxi devices seems to be the system clock.

With those additional information it should be very easy to get a proper 
binding.

Cheers
Philipp

[1]: 
https://github.com/embed-3d/PVRSGX_hwdoc/blob/master/sources/pdfs/Spruh73c_chapter_SGX_Graphics_Accelerator.pdf

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Tony Lindgren]

* Philipp Rossak <embed3d@gmail.com> [200422 19:05]:
> A few years back, I did a big research on the PowerVR GPUs. Back then I
> found an interesting TI datasheet. I forgot about this till I have seen the
> right buzz words. Sorry that I remembered it that late.
> 
> Back then I came to the conclusion that all PowerVR GPU's have in general 3
> Clocks.
> 
> A system clock, a memory clock and a core clock. [1].

Hmm I'm not sure if those names are sgx or SoC specific.

Anyways, the sgx clocks for omap variants are already handled
by the ti-sysc module as "fck" and "ick" so nothing to do there.

> The hyd_clk at sunxi devices seems to be the system clock.
> 
> With those additional information it should be very easy to get a proper
> binding.

It would be best to find the clock(s) name used in the sgx docs
to avoid using SoC specific naming :)

But yeah "sysclk" "memclk" and "coreclk" seem just fine for
me for the optional clocks if that works for other SoCs.

Regards,

Tony

> [1]: https://github.com/embed-3d/PVRSGX_hwdoc/blob/master/sources/pdfs/Spruh73c_chapter_SGX_Graphics_Accelerator.pdf

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[H. Nikolaus Schaller]

Hi Tony and Philip,

> Am 22.04.2020 um 21:33 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * Philipp Rossak <embed3d@gmail.com> [200422 19:05]:
>> A few years back, I did a big research on the PowerVR GPUs. Back then I
>> found an interesting TI datasheet. I forgot about this till I have seen the
>> right buzz words. Sorry that I remembered it that late.
>> 
>> Back then I came to the conclusion that all PowerVR GPU's have in general 3
>> Clocks.
>> 
>> A system clock, a memory clock and a core clock. [1].

Great!  This is an excerpt of the am335x TRM.
I may have seen this information in the past but also forgot about it.

Indeed, it seems to change a lot of our thinking.

> 
> Hmm I'm not sure if those names are sgx or SoC specific.

It depends. Here is some quick research:

the am335x lists:
  THALIAIRQ, SYSCLK & MEMCLK (connected in parallel), CORECLK

The omap3530 TRM has different information. It names them
  SGX_FCLK, SGX_ICLK, SGX_RST and SGX_IRQ
  but this is likely a TI nomenclature defined by the PRCM wrapper.

DM3730 and OMAP4 and TRM tells the same.

The OMAP5 TRM is interestingly different. It has:
  GPU_ICLK, GPU_FCLK1, GPU_FCLK2, GPU_RST and GPU_IRQ.
  Really surprising is that the PRCM outputs are called
    GPU_L3_GICLK, GPU_CORE_GCLK and GPU_HYS_GCLK.

  I.e. the same "HYD" as we have seen in the A31. It seems to
  be a feature of the sgx544 to have two functional clocks and
  one being called "HYD".

Now I know why it didn't play a role so far. Because the omap5
wrapper hides this detail from the sgx implementation.

Next I checked the AM572x TRM:
 it has also a hyd_clk, a core_clk, sys_clk, some reset and a gpu_irq

The DRA7xx TRM does the same as AM57xx.

So the "hyd" clock seems to be a second functional clock
with unknown function in some SGX variants. It seems to be
something different from the "memclock" of the am335x but may
be the same.

> 
> Anyways, the sgx clocks for omap variants are already handled
> by the ti-sysc module as "fck" and "ick" so nothing to do there.

Which brings back the question if this complexity and not well
defined clocks of the SGX core should really be part of the bindings
any why we have to care about...

What is the benefit of modeling at this level of pretend accuracy?

> 
>> The hyd_clk at sunxi devices seems to be the system clock.
>> 
>> With those additional information it should be very easy to get a proper
>> binding.
> 
> It would be best to find the clock(s) name used in the sgx docs
> to avoid using SoC specific naming :)

If there were specific SGX docs describing the VHDL signal names :)

> 
> But yeah "sysclk" "memclk" and "coreclk" seem just fine for
> me for the optional clocks if that works for other SoCs.

Well, if the other SoC would follow the PRCM/sysc approach
the omap uses, all these clocks would be part of the wrapper
and can be named and numbered as it best fits to the SoC
data sheet and clock control registers.

> 
> Regards,
> 
> Tony
> 
>> [1]: https://github.com/embed-3d/PVRSGX_hwdoc/blob/master/sources/pdfs/Spruh73c_chapter_SGX_Graphics_Accelerator.pdf

So a compromise could be to

* define

  clock-names:
    items:
      - const: core
      - const: mem
      - const: sys
      - const: hyd

* make clocks optional (for omap or others wanting to use a wrapper driver)
* DTs can request the same clock providers for core and hyd or mem if that fits best
* the driver must enable all 4 clocks if they exists

BR and thanks,
Nikolaus

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Neil Armstrong]

On 22/04/2020 18:09, H. Nikolaus Schaller wrote:
> Hi Maxime,
> 
>> Am 22.04.2020 um 17:13 schrieb Maxime Ripard <maxime@cerno.tech>:
>>
>> On Wed, Apr 22, 2020 at 09:10:57AM +0200, H. Nikolaus Schaller wrote:
>>>> Am 22.04.2020 um 08:58 schrieb Maxime Ripard <maxime@cerno.tech>:
>>>>>
>>>>> It also allows to handle different number of clocks (A31 seems to
>>>>> need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings
>>>>> or making big lists of conditionals. This variance would be handled
>>>>> outside the sgx core bindings and driver.
>>>>
>>>> I disagree. Every other GPU binding and driver is handling that just fine, and
>>>> the SGX is not special in any case here.
>>>
>>> Can you please better explain this? With example or a description
>>> or a proposal?
>>
>> I can't, I don't have any knowledge about this GPU.
> 
> Hm. Now I am fully puzzled.
> You have no knowledge about this GPU but disagree with our proposal?
> Is it just gut feeling?
> 
> Anyways, we need to find a solution. Together.
> 
>>
>>> I simply do not have your experience with "every other GPU" as you have.
>>> And I admit that I can't read from your statement what we should do
>>> to bring this topic forward.
>>>
>>> So please make a proposal how it should be in your view.
>>
>> If you need some inspiration, I guess you could look at the mali and vivante
>> bindings once you have an idea of what the GPU needs across the SoCs it's
>> integrated in.
> 
> Well, I do not need inspiration, we need to come to an agreement about
> img,pvrsgx.yaml and we need some maintainer to finally pick it up.
> 
> I wonder how we can come to this stage.
> 
> If I look at vivante,gc.yaml or arm,mali-utgard.yaml I don't
> see big differences to what we propose and those I see seem to come
> from technical differences between sgx, vivante, mali etc. So there
> is no single scheme that fits all different gpu types.
> 
> One thing we can learn is that "core" seems to be a de facto standard 
> for the core clock-name. An alternative "gpu" is used by nvidia,gk20a.txt.

Usually IPs needs a few clocks:
- pclk or apb or reg: the clock clocking the "slave" bus to serve the registers
- axi or bus or ahb: the bus clocking the the "master" bus to get data from system memory
- core: the actual clock feeding the GPU logic

Sometimes you have a single clock for slave and master bus.

But you can also have separate clocks for shader cores, .. this depends on the IP and it's architecture.
The IP can also have memories with separate clocks, etc...

But all these clocks can be source by an unique clock on a SoC, but different on another
SoC, this is why it's important to list them all, even optional.

You'll certainly have at least a reset signal, and a power domain, these should exist and be optional.

Neil

> 
> BR and thanks,
> Nikolaus
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[H. Nikolaus Schaller]

Hi Neil,

> Am 23.04.2020 um 17:00 schrieb Neil Armstrong <narmstrong@baylibre.com>:
>> One thing we can learn is that "core" seems to be a de facto standard 
>> for the core clock-name. An alternative "gpu" is used by nvidia,gk20a.txt.
> 
> Usually IPs needs a few clocks:
> - pclk or apb or reg: the clock clocking the "slave" bus to serve the registers
> - axi or bus or ahb: the bus clocking the the "master" bus to get data from system memory
> - core: the actual clock feeding the GPU logic

And the sgx544 seems to have two such clocks.

> Sometimes you have a single clock for slave and master bus.
> 
> But you can also have separate clocks for shader cores, .. this depends on the IP and it's architecture.
> The IP can also have memories with separate clocks, etc...

Indeed.

> But all these clocks can be source by an unique clock on a SoC, but different on another
> SoC, this is why it's important to list them all, even optional.
> 
> You'll certainly have at least a reset signal, and a power domain, these should exist and be optional.

Well, they exist only as hints in block diagrams of some SoC data sheets
(so we do not know if they represent the imagination definitions) and the
current driver code doesn't make use of it. Still the gpu core works.

So I do not see any urgent need to add a complete list to the bindings now.

Unless some special SoC integration makes use of them. Then it is IMHO easier
to extend the bindings by a follow-up patch than now thinking about all
potential options and bloating the bindings with things we (the open source
community) doesn't and can't know.

My goal is to keep the bindings as minimalistic as possible. And reset lines
and power domains are (at least for those we have in the works) not needed
to make working systems.

Therefore, for clocks I also would start with a minimalistic approach for
a single optional GPU core clock and leave out reset and power completely.

BR and thanks,
Nikolaus

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2651 bytes --]

On Thu, Apr 23, 2020 at 05:45:55PM +0200, H. Nikolaus Schaller wrote:
> > Am 23.04.2020 um 17:00 schrieb Neil Armstrong <narmstrong@baylibre.com>:
> >> One thing we can learn is that "core" seems to be a de facto standard 
> >> for the core clock-name. An alternative "gpu" is used by nvidia,gk20a.txt.
> > 
> > Usually IPs needs a few clocks:
> > - pclk or apb or reg: the clock clocking the "slave" bus to serve the registers
> > - axi or bus or ahb: the bus clocking the the "master" bus to get data from system memory
> > - core: the actual clock feeding the GPU logic
> 
> And the sgx544 seems to have two such clocks.
> 
> > Sometimes you have a single clock for slave and master bus.
> > 
> > But you can also have separate clocks for shader cores, .. this depends on the IP and it's architecture.
> > The IP can also have memories with separate clocks, etc...
> 
> Indeed.
> 
> > But all these clocks can be source by an unique clock on a SoC, but different on another
> > SoC, this is why it's important to list them all, even optional.
> > 
> > You'll certainly have at least a reset signal, and a power domain, these should exist and be optional.
> 
> Well, they exist only as hints in block diagrams of some SoC data
> sheets (so we do not know if they represent the imagination
> definitions) and the current driver code doesn't make use of it. Still
> the gpu core works.
> 
> So I do not see any urgent need to add a complete list to the bindings
> now.
> 
> Unless some special SoC integration makes use of them. Then it is IMHO
> easier to extend the bindings by a follow-up patch than now thinking
> about all potential options and bloating the bindings with things we
> (the open source community) doesn't and can't know.
> 
> My goal is to keep the bindings as minimalistic as possible. And reset
> lines and power domains are (at least for those we have in the works)
> not needed to make working systems.
> 
> Therefore, for clocks I also would start with a minimalistic approach
> for a single optional GPU core clock and leave out reset and power
> completely.

Like I said above, the DT is considered an ABI and you'll have to
maintain backward compatibility (ie, newer kernel running with older
DT). Therefore, you won't be able to require a new clock, reset or
power-domain later on for example.

I guess the question I'm really asking is: since you don't really know
how the hardware is integrated at the moment, why should we have that
discussion *now*. It's really not suprising that you don't know yet, so
I'm not sure why we need to rush in the bindings.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[H. Nikolaus Schaller]

Hi,

> Am 23.04.2020 um 22:36 schrieb Maxime Ripard <maxime@cerno.tech>:
>> My goal is to keep the bindings as minimalistic as possible. And reset
>> lines and power domains are (at least for those we have in the works)
>> not needed to make working systems.
>> 
>> Therefore, for clocks I also would start with a minimalistic approach
>> for a single optional GPU core clock and leave out reset and power
>> completely.
> 
> Like I said above, the DT is considered an ABI and you'll have to
> maintain backward compatibility (ie, newer kernel running with older
> DT).

Generally I fully agree to this rule (although I have experienced
that exceptions happen more often than I like).

But here, we don't have any older DT which define something about SGX.

We introduce SGX for the first time with bindings and DT in parallel.
So they are in sync.

Therefore, newer kernels with SGX support and older DT simply will
skip SGX and not load any drivers. So we can't break older DT and
older DT can't break SGX.

What we introduce is a DT code that is well hung and tested (originating
in vendor kernels). It is cast in a bindings.yaml where not everyone
is happy with for reasons outside the originally proposed DT.

For new SoC not yet supported, I don't see a need to touch the
existing ones.

This is because I only propose to *add* properties to the bindings
for devices that have not been supported with SGX before and are
not sufficiently covered by what exists.

So backward compatibility is a non-problem.

> Therefore, you won't be able to require a new clock, reset or
> power-domain later on for example.
> 
> I guess the question I'm really asking is: since you don't really know
> how the hardware is integrated at the moment,

Like I explained, we do not need to know and model all details about
the hardware integration. The register set of an SoC does not always
provide bits to control all signals we may see in a block diagram or
think they must exist.

We have a set of SoC where it is demonstrated to work without need
for more detailed knowledge about specific hardware integration.

So we know everything of importance for this initial set of SoC to
make it work.

> why should we have that
> discussion *now*. It's really not suprising that you don't know yet, so
> I'm not sure why we need to rush in the bindings.

Because:
* there are people who want to have upstream SGX support for an initial
  set of SoC *now*
* the discussion already lasts ca. 6 months since I posted v1,
  that should be enough and is not a rush
* it is not required to know more details to make a working system
* we will not gain more information by waiting for another year or two
* problems are not solved by postponing them
* there are DTS for some initial SoC, tested to work
* it is no longer possible to submit DT without bindings.yaml (or is it?)
* we just need to define a bindings.yaml for them, not invent something
  completely new
* we can start with a minimal bindings.yaml for the analysed SoC and
  *extend* it in the future if really needed
* we can discuss changes & extensions for the bindings when they are
  really proposed
* having this patch series upstream is a prerequisite for introducing
  the sgx kernel driver to staging

In other words: your suggestion to postpone everything will keep finished
work sitting in front of the door and rotting and blocking unfinished work...

And to be honest, we have postponed SGX support already for too long
time and could be much farther with more and broader community cooperation.
So we should not block ourselves.

So if you can contribute new information or proposals to specifically
improve the proposed bindings.yaml, you are very welcome. But please do
it *now*.

BR and thanks,
Nikolaus

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Philipp Rossak]

Hi,

On 21.04.20 13:21, Maxime Ripard wrote:
> Hi,
> 
> On Tue, Apr 21, 2020 at 11:57:33AM +0200, Philipp Rossak wrote:
>> On 20.04.20 09:38, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Fri, Apr 17, 2020 at 02:09:06PM +0200, Philipp Rossak wrote:
>>>>>> I'm a bit skeptical on that one since it doesn't even list the
>>>>>> interrupts connected to the GPU that the binding mandates.
>>>>>
>>>>> I think he left it out for a future update.
>>>>> But best he comments himself.
>>>>
>>>> I'm currently working on those bindings. They are now 90% done, but they are
>>>> not finished till now. Currently there is some mainline support missing to
>>>> add the full binding. The A83T and also the A31/A31s have a GPU Power Off
>>>> Gating Register in the R_PRCM module, that is not supported right now in
>>>> Mainline. The Register need to be written when the GPU is powered on and
>>>> off.
>>>>
>>>> @Maxime: I totally agree on your point that a demo needs to be provided
>>>> before the related DTS patches should be provided. That's the reason why I
>>>> added the gpu placeholder patches.
>>>> Do you have an idea how a driver for the R_PRCM stuff can look like? I'm not
>>>> that experienced with the clock driver framework.
>>>
>>> It looks like a power-domain to me, so you'd rather plug that into the genpd
>>> framework.
>>
>> I had a look on genpd and I'm not really sure if that fits.
>>
>> It is basically some bit that verify that the clocks should be enabled or
>> disabled.
> 
> No, it can do much more than that. It's a framework to control the SoCs power
> domains, so clocks might be a part of it, but most of the time it's going to be
> about powering up a particular device.
> 
So I think I've found now the right piece of documentation and a driver 
that implements something similar [1].

So I will write a similar driver like linked above that only sets the 
right bits for A83T and A31/A31s.
Do you think this is the right approach?

>> I think this is better placed somewhere in the clocking framework.
>> I see there more similarities to the gating stuff.
>> Do you think it is suitable to implement it like the clock gating?
> 
> I'm really not sure what makes you think that this should be modelled as a
> clock?
> 

Looks like I looked in the wrong place and got some information that are 
not suitable for this.

>>>> The big question is right now how to proceed with the A83T and A31s patches.
>>>> I see there three options, which one do you prefer?:
>>>>
>>>> 1. Provide now placeholder patches and send new patches, if everything is
>>>> clear and other things are mainlined
>>>> 2. Provide now patches as complete as possible and provide later patches to
>>>> complete them when the R_PRCM things are mainlined
>>>> 3. Leave them out, till the related work is mainlined and the bindings are
>>>> final.
>>>
>>> Like I said, the DT *has* to be backward-compatible, so for any DT patch that
>>> you are asking to be merged, you should be prepared to support it indefinitely
>>> and be able to run from it, and you won't be able to change the bindings later
>>> on.
>>
>> I agree on your points. But is this also suitable to drivers that are
>> currently off tree and might be merged in one or two years?
> 
> This is what we done for the Mali. The devicetree binding was first done for the
> out-of-tree driver, and then lima/panfrost reused it.
> 
> The key thing here is to have enough confidence about how the hardware works so
> that you can accurately describe it.

Ok thanks! So I will resend my patches when the work got a more mature 
state and we know enough about the Hardware.

Cheers,
Philipp


[1]: 
https://elixir.bootlin.com/linux/latest/source/drivers/soc/amlogic/meson-gx-pwrc-vpu.c

Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2280 bytes --]

On Tue, Apr 21, 2020 at 06:42:17PM +0200, Philipp Rossak wrote:
> Hi,
> 
> On 21.04.20 13:21, Maxime Ripard wrote:
> > Hi,
> > 
> > On Tue, Apr 21, 2020 at 11:57:33AM +0200, Philipp Rossak wrote:
> > > On 20.04.20 09:38, Maxime Ripard wrote:
> > > > Hi,
> > > > 
> > > > On Fri, Apr 17, 2020 at 02:09:06PM +0200, Philipp Rossak wrote:
> > > > > > > I'm a bit skeptical on that one since it doesn't even list the
> > > > > > > interrupts connected to the GPU that the binding mandates.
> > > > > > 
> > > > > > I think he left it out for a future update.
> > > > > > But best he comments himself.
> > > > > 
> > > > > I'm currently working on those bindings. They are now 90% done, but they are
> > > > > not finished till now. Currently there is some mainline support missing to
> > > > > add the full binding. The A83T and also the A31/A31s have a GPU Power Off
> > > > > Gating Register in the R_PRCM module, that is not supported right now in
> > > > > Mainline. The Register need to be written when the GPU is powered on and
> > > > > off.
> > > > > 
> > > > > @Maxime: I totally agree on your point that a demo needs to be provided
> > > > > before the related DTS patches should be provided. That's the reason why I
> > > > > added the gpu placeholder patches.
> > > > > Do you have an idea how a driver for the R_PRCM stuff can look like? I'm not
> > > > > that experienced with the clock driver framework.
> > > > 
> > > > It looks like a power-domain to me, so you'd rather plug that into the genpd
> > > > framework.
> > > 
> > > I had a look on genpd and I'm not really sure if that fits.
> > > 
> > > It is basically some bit that verify that the clocks should be enabled or
> > > disabled.
> > 
> > No, it can do much more than that. It's a framework to control the SoCs power
> > domains, so clocks might be a part of it, but most of the time it's going to be
> > about powering up a particular device.
> > 
> So I think I've found now the right piece of documentation and a driver that
> implements something similar [1].
> 
> So I will write a similar driver like linked above that only sets the right
> bits for A83T and A31/A31s.
> Do you think this is the right approach?

That sounds about right yes

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]