Am 24.04.2020 um 22:34 schrieb H. Nikolaus Schaller hns@goldelico.com:

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 and interrupt).

The interface also consists of clocks, reset, power but information from data sheets is vague and some SoC integrators (TI) deciced to use a PRCM wrapper (ti,sysc) which does

s/deciced/decided/

all clock, reset and power-management through registers outside of the sgx register block.

Therefore all these properties are optional.

Tested by make dt_binding_check

Signed-off-by: H. Nikolaus Schaller hns@goldelico.com

.../devicetree/bindings/gpu/img,pvrsgx.yaml | 150 ++++++++++++++++++ 1 file changed, 150 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..33a9c4c6e784 --- /dev/null +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml @@ -0,0 +1,150 @@ +# 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. If not, an optional clock can be specified here.

^^^ this is no longer that way. now clocks, reset etc. are part of this node but can be omitted if done by the parent node.

=> either remove this sentence or rewrite.

+properties:

• $nodename:
• pattern: '^gpu@[a-f0-9]+$'
• 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
• interrupt-names:
• maxItems: 1
• items:

•  - const: sgx
  

• clocks:
• maxItems: 4
• clock-names:
• maxItems: 4
• items:

•  - const: core
  

•  - const: sys
  

•  - const: mem
  

•  - const: hyd
  

• sgx-supply: true
• power-domains:
• maxItems: 1
• resets:
• maxItems: 1

+required:

• • compatible
• • reg
• • interrupts

+additionalProperties: false

+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

Hi Paul,

Am 26.04.2020 um 15:11 schrieb Paul Cercueil paul@crapouillou.net:

Hi Nikolaus,

Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller hns@goldelico.com a écrit :

...

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 and interrupt). The interface also consists of clocks, reset, power but information from data sheets is vague and some SoC integrators (TI) deciced to use a PRCM wrapper (ti,sysc) which does all clock, reset and power-management through registers outside of the sgx register block. Therefore all these properties are optional. Tested by make dt_binding_check Signed-off-by: H. Nikolaus Schaller hns@goldelico.com

---

.../devicetree/bindings/gpu/img,pvrsgx.yaml | 150 ++++++++++++++++++ 1 file changed, 150 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..33a9c4c6e784 --- /dev/null +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml @@ -0,0 +1,150 @@ +# 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. If not, an optional clock can be specified here.

+properties:

• $nodename:
• pattern: '^gpu@[a-f0-9]+$'
• 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
• interrupt-names:
• maxItems: 1
• items:

•  - const: sgx
  

• clocks:
• maxItems: 4
• clock-names:
• maxItems: 4
• items:

•  - const: core
  

•  - const: sys
  

•  - const: mem
  

•  - const: hyd
  

• sgx-supply: true
• power-domains:
• maxItems: 1
• resets:
• maxItems: 1

+required:

• • compatible
• • reg
• • interrupts

By not making 'clocks' required you make it possible to create broken bindings; according to your schema, a GPU node without a 'clocks' for the JZ4780 would be perfectly valid.

Yes. But it will never pass a test with real hardware. So it can't be omitted anyways.

On a more general thought, this argument holds for any optional property. So it is not specific to clocks. Since the reg address values are also never specified you can still create broken bindings. Or by connecting the wrong clock. So the ways to create broken bindings are numerous.

I also assume that SGX integrators are not beginners and do you think they need to find out through a make dt_binding_check dtbs_check that they should define a clock? based on *assumptions* we do without having access to all systems?

IMHO the bindings documentation is a documentation. So it needs to be helpful but not perfect. Formalizing all corner cases in a bindings document (just because we can since .yaml was introduced) is IMHO overkill.

In times before the introduction of more formal .yaml I think we would not even have considered this for a comment in the bindings.txt.

It's possible to forbid the presence of the 'clocks' property on some implementations, and require it on others.

To be precise we have to specify the exact number of clocks (between 0 and 4) for every architecture.

This also contradicts my dream to get rid of the architecture specific components in the long run. My dream (because I can't tell how it can be done) is that we can one day develop something which just needs compatible = img,530 or imp,540 or img,544. Then we can't make the number clocks depend on the implementation any more.

See how it's done for instance on Documentation/devicetree/bindings/serial/samsung_uart.yaml.

Yes I know the design pattern, but I wonder if such a move makes the whole thing even less maintainable.

Assume we have finished DTS for some SoC. Then these DTS have been tested on real hardware and are working. Clocks are there where needed and missing where not. We may now forbid or not forbid them for some implementations in the bindings.yaml but the result of dtbs_check won't change! Because they are tested and working and the bindings.yaml has been adapted to the result. So we have just duplicated something for no practical benefit.

Next, assume there is coming support for more and more new SoC. Then, developers not only have to figure out which clocks they need in the DTS but they also have to add a patch to the implementation specific part of the bindings.yaml to clearly define exactly the same what they already have written into their .dts (the clocks are either there for the of_node or they are not). So again the rules are for no benefit, since a new SoC is introduced exactly once. And tested if it works. And if it is there, it will stay as it is. It is just work for maintainers to review that patch as well.

It boils down to the question if we need to formalize the rule how a working DTS was derived. Or just have a working DTS and not formalize everything.

So IMHO carrying along such a detail (forbid clocks on some architectures) is nice to have (and fun to learn the .yaml thing) but not of benefit for anyone. Not for the DTS developer nor for the maintainers nor for the users of a Linux kernel. "Keep it simple" is always a good rule for maintainability.

In summary I don't see a good reason to follow this in v8. But you could add it by a separate patch later if the DTS have been reviewed and agreed.

BR and thanks, Nikolaus

Hi Nikolaus,

On 24.04.20 22:34, 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 and interrupt).

The interface also consists of clocks, reset, power but information from data sheets is vague and some SoC integrators (TI) deciced to use a PRCM wrapper (ti,sysc) which does all clock, reset and power-management through registers outside of the sgx register block.

Therefore all these properties are optional.

Tested by make dt_binding_check

Signed-off-by: H. Nikolaus Schaller hns@goldelico.com

.../devicetree/bindings/gpu/img,pvrsgx.yaml | 150 ++++++++++++++++++ 1 file changed, 150 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..33a9c4c6e784 --- /dev/null +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml @@ -0,0 +1,150 @@ +# 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. If not, an optional clock can be specified here.

+properties:

• $nodename:
• pattern: '^gpu@[a-f0-9]+$'
• 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
  

those two bindings are wrong. It should be allwinner,sun6i-a31-sgx544-115 and allwinner,sun6i-a31s-sgx544-115. I did a copy paste error in the patches that I provided for this series.

Cheers, Philipp

On Fri, Apr 24, 2020 at 10:34:04PM +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 and interrupt).

The interface also consists of clocks, reset, power but information from data sheets is vague and some SoC integrators (TI) deciced to use a PRCM wrapper (ti,sysc) which does all clock, reset and power-management through registers outside of the sgx register block.

Therefore all these properties are optional.

Tested by make dt_binding_check

Signed-off-by: H. Nikolaus Schaller hns@goldelico.com

.../devicetree/bindings/gpu/img,pvrsgx.yaml | 150 ++++++++++++++++++ 1 file changed, 150 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..33a9c4c6e784 --- /dev/null +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml @@ -0,0 +1,150 @@ +# 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. If not, an optional clock can be specified here.

+properties:

• $nodename:
• pattern: '^gpu@[a-f0-9]+$'
• compatible:
• oneOf:

•  - description: SGX530-121 based SoC
  

•    items:
  

•      - enum:
  

•        - ti,omap3-sgx530-121 # BeagleBoard A/B/C, OpenPandora 600MHz and similar
  

Should be indented 2 more here and elsewhere where you have a list under a list.

•      - 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
• interrupt-names:
• maxItems: 1
• items:

•  - const: sgx
  

• clocks:
• maxItems: 4
• clock-names:
• maxItems: 4
• items:

•  - const: core
  

•  - const: sys
  

•  - const: mem
  

•  - const: hyd
  

• sgx-supply: true
• power-domains:
• maxItems: 1
• resets:
• maxItems: 1

+required:

• • compatible
• • reg
• • interrupts

+additionalProperties: false

+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

Hi Paul,

Am 26.04.2020 um 14:50 schrieb Paul Cercueil paul@crapouillou.net:

Hi Nikolaus,

Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller hns@goldelico.com a écrit :

...

Add SGX GPU node with interrupt. Tested on PandaBoard ES. 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. 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

^^^ there is also a missing "i" in the file name

...

@@ -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 {

Does this even compile?

Good question.

So far there is no well known eval board in mainline that #includes this .dtsi (because it is new) and therefore it passes any compile tests.

DTC arch/arm/boot/dts/omap4470-test.dtb - due to target missing Error: arch/arm/boot/dts/omap4470.dtsi:13.1-5 Label or path sgx not found

I have now added a dummy board (not to be mainlined) for my own compile test...

The node's handle is named sgx_module, not sgx.

Indeed. A fix is queued for v8.

BR and thanks, Nikolaus

Hi Paul,

On 2020-04-26 5:56 a.m., Paul Cercueil wrote:

Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller hns@goldelico.com a écrit :

...

From: Jonathan Bakker xc-racer2@live.ca

All s5pv210 devices have a PowerVR SGX 540 (revision 120) attached.

There is no external regulator for it so it can be enabled by default.

Signed-off-by: Jonathan Bakker xc-racer2@live.ca Signed-off-by: H. Nikolaus Schaller hns@goldelico.com

---

arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++  1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi index 2ad642f51fd9..abbdda205c1b 100644 --- a/arch/arm/boot/dts/s5pv210.dtsi +++ b/arch/arm/boot/dts/s5pv210.dtsi @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 {              #interrupt-cells = <1>;          };

+        gpu: gpu@f3000000 { +            compatible = "samsung,s5pv210-sgx540-120"; +            reg = <0xf3000000 0x10000>; +            interrupt-parent = <&vic2>; +            interrupts = <10>; +            clock-names = "core"; +            clocks = <&clocks CLK_G3D>;

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

What are these clocks for, and why are they reparented / reclocked?

Shouldn't they be passed to 'clocks' as well?

-Paul

The G3D clock system can have multiple parents, and for stable operation it's recommended to use the MPLL clock as the parent (which in turn is actually a mux as well). MOUT_G3D is simply the mux for CLK_G3D (SGX core clock), DOUT_G3D is the divider. DOUT_G3D could equally be CLK_G3D (and probably should be, for readability) as CLK_G3D is simply the gate and DOUT_G3D is the divider for it.

The SGX clock layout on S5PV210 looks something like this:

MOUT_MPLL -----------> MOUT_G3D ---> DOUT_G3D ---> CLK_G3D (selectable parent clock) (mux) ---> (divider) ---> (gate)

This is fairly common for older Samsung SoCs, eg having a look at arch/arm/boot/dts/exynos4210-universal_c210.dts you can see that the FIMC clocks are parented to MPLL and have a rate set.

...

+        };

fimd: fimd@f8000000 {              compatible = "samsung,s5pv210-fimd";              interrupt-parent = <&vic2>; -- 2.25.1

Thanks, Jonathan

Hi Krzysztof,

On 2020-04-27 8:46 a.m., Krzysztof Kozlowski wrote:

On Sun, Apr 26, 2020 at 07:57:12AM -0700, Jonathan Bakker wrote:

...

Hi Paul,

On 2020-04-26 5:56 a.m., Paul Cercueil wrote:

...

Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller hns@goldelico.com a écrit :

...

From: Jonathan Bakker xc-racer2@live.ca

All s5pv210 devices have a PowerVR SGX 540 (revision 120) attached.

There is no external regulator for it so it can be enabled by default.

Signed-off-by: Jonathan Bakker xc-racer2@live.ca Signed-off-by: H. Nikolaus Schaller hns@goldelico.com

---

arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++  1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi index 2ad642f51fd9..abbdda205c1b 100644 --- a/arch/arm/boot/dts/s5pv210.dtsi +++ b/arch/arm/boot/dts/s5pv210.dtsi @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 {              #interrupt-cells = <1>;          };

+        gpu: gpu@f3000000 { +            compatible = "samsung,s5pv210-sgx540-120";

This should not pass the bindings check because you missed last compatibles.

Thanks for pointing that out, I'll add it and make sure it passes the bindings check.

...

...

...

+            reg = <0xf3000000 0x10000>; +            interrupt-parent = <&vic2>; +            interrupts = <10>; +            clock-names = "core"; +            clocks = <&clocks CLK_G3D>;

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

What are these clocks for, and why are they reparented / reclocked?

Shouldn't they be passed to 'clocks' as well?

-Paul

The G3D clock system can have multiple parents, and for stable operation it's recommended to use the MPLL clock as the parent (which in turn is actually a mux as well). MOUT_G3D is simply the mux for CLK_G3D (SGX core clock), DOUT_G3D is the divider. DOUT_G3D could equally be CLK_G3D (and probably should be, for readability) as CLK_G3D is simply the gate and DOUT_G3D is the divider for it.

Good point, it should be CLK_G3D instead of DOUT. Can you fix this as well?

Yep, will do. Nikolaus, I'll send you an updated patch to include.

Best regards, Krzysztof

Thanks, Jonathan

Hi,

On Tue, Apr 28, 2020 at 03:58:03PM -0700, Jonathan Bakker wrote:

On 2020-04-28 2:39 p.m., Jonathan Bakker wrote:

...

On 2020-04-27 8:46 a.m., Krzysztof Kozlowski wrote:

...

On Sun, Apr 26, 2020 at 07:57:12AM -0700, Jonathan Bakker wrote:

...

Hi Paul,

On 2020-04-26 5:56 a.m., Paul Cercueil wrote:

...

Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller hns@goldelico.com a écrit :

...

From: Jonathan Bakker xc-racer2@live.ca

All s5pv210 devices have a PowerVR SGX 540 (revision 120) attached.

There is no external regulator for it so it can be enabled by default.

Signed-off-by: Jonathan Bakker xc-racer2@live.ca Signed-off-by: H. Nikolaus Schaller hns@goldelico.com

---

arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++  1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi index 2ad642f51fd9..abbdda205c1b 100644 --- a/arch/arm/boot/dts/s5pv210.dtsi +++ b/arch/arm/boot/dts/s5pv210.dtsi @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 {              #interrupt-cells = <1>;          };

+        gpu: gpu@f3000000 { +            compatible = "samsung,s5pv210-sgx540-120";

This should not pass the bindings check because you missed last compatibles.

Thanks for pointing that out, I'll add it and make sure it passes the bindings check.

...

...

...

...

+            reg = <0xf3000000 0x10000>; +            interrupt-parent = <&vic2>; +            interrupts = <10>; +            clock-names = "core"; +            clocks = <&clocks CLK_G3D>;

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

What are these clocks for, and why are they reparented / reclocked?

Shouldn't they be passed to 'clocks' as well?

-Paul

The G3D clock system can have multiple parents, and for stable operation it's recommended to use the MPLL clock as the parent (which in turn is actually a mux as well). MOUT_G3D is simply the mux for CLK_G3D (SGX core clock), DOUT_G3D is the divider. DOUT_G3D could equally be CLK_G3D (and probably should be, for readability) as CLK_G3D is simply the gate and DOUT_G3D is the divider for it.

Good point, it should be CLK_G3D instead of DOUT. Can you fix this as well?

Yep, will do. Nikolaus, I'll send you an updated patch to include.

How are assigned-clocks handled in the yaml DT schema? When running make dtbs_check, I end up with messages such as

arch/arm/boot/dts/s5pv210-aquila.dt.yaml: gpu@f3000000: 'assigned-clock-parents', 'assigned-clock-rates', 'assigned-clocks' do not match any of the regexes: 'pinctrl-[0-9]+'

Do they need to explicitly be listed as valid entries?

If you have additionalProperties set to false, yes. But you should really consider not using them in the first place, since they provide no guarantee on whether the setup you did in the DT will remain during the life of the system.

I'm not sure why it's needed on that SoC in particular, but this should be fixed in the driver itself (either the clock or the GPU driver).

Maxime

Hi Paul,

On 26.04.20 14:53, Paul Cercueil wrote:

Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller hns@goldelico.com a écrit :

...

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",

looks like a copy paste error from my side this should be allwinner,sun6i-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. +             */

This node doesn't have clocks, so I don't see how it'd work.

Better delay the introduction of the GPU node for this board until you know it works.

-Paul

This was already discuss in an earlier version that series that this should be delayed.

I will send a follow up patch series, as soon as I mainlined an other driver that I'm working on, which is required to properly describe the gpu.

Cheers Philipp