diff mbox series

[v2,1/8] RFC: dt-bindings: add img,pvrsgx.yaml for Imagination GPUs

Message ID 4292cec1fd82cbd7d42742d749557adb01705574.1573124770.git.hns@goldelico.com (mailing list archive)
State New, archived
Headers show
Series ARM/MIPS: DTS: add child nodes describing the PVRSGX present in some OMAP SoC and JZ4780 | expand

Commit Message

H. Nikolaus Schaller Nov. 7, 2019, 11:06 a.m. UTC
The Imagination PVR/SGX GPU is part of several SoC from
multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo
and others.

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

Clock, Reset and power management should be handled
by a parent node or elsewhere.

---

I have used the doc2yaml script to get a first veryion
but I am still stuggling with the yaml thing. My impression
is that while it is human readable, it is not very human
writable... Unfortunately I haven't found a good tutorial
for Dummies (like me) for bindings in YAML.

The big problem is not the YAML syntax but what the schema
should contain and how to correctly formulate ideas in this
new language.

Specific questions for this RFC:

* formatting: is space/tab indentation correct?
* are strings with "" correct or without?
* how do I specify that there is a list of compatible strings required in a specific order?
* but there are multiple such lists, and only one of them is to be chosen?
* how can be described in the binding that there should be certain values in
  the parent node (ranges) to make it work?

I was not able to run

	make dt_binding_check dtbs_check

due to some missing dependencies (which I did not want to
invest time to research them) on my build host, so I could
not get automated help from those.
---
 .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 128 ++++++++++++++++++
 1 file changed, 128 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml

Comments

Rob Herring Nov. 7, 2019, 2:35 p.m. UTC | #1
On Thu, Nov 7, 2019 at 5:06 AM H. Nikolaus Schaller <hns@goldelico.com> wrote:
>
> The Imagination PVR/SGX GPU is part of several SoC from
> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo
> and others.
>
> With this binding, we describe how the SGX processor is
> interfaced to the SoC (registers, interrupt etc.).
>
> Clock, Reset and power management should be handled
> by a parent node or elsewhere.

That's probably TI specific...

> ---
>
> I have used the doc2yaml script to get a first veryion
> but I am still stuggling with the yaml thing. My impression
> is that while it is human readable, it is not very human
> writable... Unfortunately I haven't found a good tutorial
> for Dummies (like me) for bindings in YAML.

Did you read .../bindings/example-schema.yaml? It explains the common
cases and what schema are doing. I recently added to it, so look at
the version in linux-next.

> The big problem is not the YAML syntax but what the schema
> should contain and how to correctly formulate ideas in this
> new language.
>
> Specific questions for this RFC:
>
> * formatting: is space/tab indentation correct?

YAML requires spaces.

> * are strings with "" correct or without?

Generally only keys or values starting with '#' need quotes. There's
other cases, but we simply don't hit them with DT. We tend to quote
$ref values, but that's not strictly needed.

> * how do I specify that there is a list of compatible strings required in a specific order?

An 'items' list defines the order.

> * but there are multiple such lists, and only one of them is to be chosen?

                                                ^^^^^^
'oneOf' is the schema keyword you are looking for.

> * how can be described in the binding that there should be certain values in
>   the parent node (ranges) to make it work?

You can't. Schemas match on a node and work down from there. So you
can do it, but it's more complicated. You'd need a custom 'select'
select that matches on the parent node having the child node you are
looking for (assuming the parent is something generic like
'simple-bus' which you can't match on). However, based on the example,
I'd say checking 'ranges' is outside the scope of schema checks.
'ranges' doesn't have to be a certain value any more than every case
of 'reg' (except maybe i2c devices with fixed addresses). It's up to
the .dts author how exactly to do address translation.

I would like to have more ranges/reg checks such as bounds checks and
overlapping addresses, but I think we'd do those with code, not
schema.

> I was not able to run
>
>         make dt_binding_check dtbs_check
>
> due to some missing dependencies (which I did not want to
> invest time to research them) on my build host, so I could
> not get automated help from those.

Dependencies are documented in Documentation/devicetree/writing-schema.rst.

> ---
>  .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 128 ++++++++++++++++++
>  1 file changed, 128 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..b1b021601c47
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
> @@ -0,0 +1,128 @@
> +# SPDX-License-Identifier: None

Obviously not valid.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bindings/gpu/img,pvrsgx.yaml#

This should have been correct with the script, but you need to drop 'bindings'.

> +$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.
> +
> +  Only the Imagination SGX530, SGX540 and SGX544 GPUs are currently covered by
> +  this binding.
> +
> +  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:
> +      - item:

'item/items'

> +        # BeagleBoard ABC, OpenPandora 600MHz
> +        - const: "ti,omap3-sgx530-121", "img,sgx530-121", "img,sgx530", "img,sgx5"

Not valid YAML nor json-schema. Each value needs to be list item with 'const:'

Plenty of examples in bindings/arm/ with board/soc bindings.

> +        # BeagleBoard XM, GTA04, OpenPandora 1GHz
> +        - const: "ti,omap3-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5"

This needs to be a new 'items' list under 'oneOf'.

> +        # BeagleBone Black
> +        - const: "ti,am335x-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5"
> +        # Pandaboard (ES)
> +        - const: "ti,omap4-sgx540-120", "img,sgx540-120", "img,sgx540", "img,sgx5"
> +        - const "ti,omap4-sgx544-112", "img,sgx544-112", "img,sgx544", "img,sgx5"
> +        # OMAP5 UEVM, Pyra Handheld
> +        "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5"
> +        "ti,dra7-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5"

Just gave up on trying to write a schema here?

> +        # CI20
> +        "ingenic,jz4780-sgx540-120", "img,sgx540-120", "img,sgx540", "img,sgx5";
> +
> +  reg:
> +    items:
> +      - description: physical base address and length of the register area

For single entries, just 'maxItems: 1' is enough. Unless you have
something special about this device, you don't need a description
here.

> +
> +  interrupts:
> +     items:
> +      - description: interrupt from SGX subsystem to core processor
> +
> +  clocks:
> +     items:
> +      - description: optional clocks
> +
> +  required:
> +    - compatible
> +    - reg
> +    - interrupts
> +
> +examples: |
> +  gpu@fe00 {
> +       compatible = "ti,omap-omap5-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5";
> +       reg = <0xfe00 0x200>;
> +       interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> +  };
> +
> +
> +historical: |

This should be dropped. It's just for reference as you write the schema.

> +  Imagination PVR/SGX GPU
> +
> +  Only the Imagination SGX530, SGX540 and SGX544 GPUs are currently covered by this binding.
> +
> +  Required properties:
> +  - compatible:        Should be one of
> +               "ti,omap3-sgx530-121", "img,sgx530-121", "img,sgx530", "img,sgx5"; - BeagleBoard ABC, OpenPandora 600MHz
> +               "ti,omap3-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5"; - BeagleBoard XM, GTA04, OpenPandora 1GHz
> +               "ti,am3517-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5";
> +               "ti,am335x-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5"; - BeagleBone Black
> +               "ti,omap4-sgx540-120", "img,sgx540-120", "img,sgx540", "img,sgx5"; - Pandaboard (ES)
> +               "ti,omap4-sgx544-112", "img,sgx544-112", "img,sgx544", "img,sgx5";
> +               "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5"; - OMAP5 UEVM, Pyra Handheld
> +               "ti,dra7-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5";
> +               "ti,am3517-sgx530-?", "img,sgx530-?", "img,sgx530", "img,sgx5";
> +               "ti,am43xx-sgx530-?", "img,sgx530-?", "img,sgx530", "img,sgx5";
> +               "ti,ti81xx-sgx530-?", "img,sgx530-?", "img,sgx530", "img,sgx5";
> +               "img,jz4780-sgx540-?", "img,sgx540-?", "img,sgx540", "img,sgx5"; - CI20
> +               "allwinner,sun8i-a83t-sgx544-?", "img,sgx544-116", "img,sgx544", "img,sgx5"; - Banana-Pi-M3 (Allwinner A83T)
> +               "intel,poulsbo-gma500-sgx535", "img,sgx535-116", "img,sgx535", "img,sgx5"; - Atom Z5xx
> +               "intel,medfield-gma-sgx540", "img,sgx540-116", "img,sgx540", "img,sgx5"; - Atom Z24xx
> +               "intel,cedarview-gma3600-sgx545", "img,sgx545-116", "img,sgx545", "img,sgx5"; - Atom N2600, D2500
> +
> +               The "ti,omap..." entries are needed temporarily to handle SoC
> +               specific builds of the kernel module.
> +
> +               In the long run, only the "img,sgx..." entry should suffice
> +               to match a generic driver for all architectures and driver
> +               code can dynamically find out on which SoC it is running.
> +
> +
> +  - reg:               Physical base address and length of the register area.
> +  - interrupts:        The interrupt numbers.
> +
> +  / {
> +       ocp {
> +               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@fe00 {
> +                               compatible = "ti,omap-omap5-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5";
> +                               reg = <0xfe00 0x200>;
> +                               interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> +                       };
> +               };
> +       };
> +  };
> --
> 2.23.0
>
Tony Lindgren Nov. 7, 2019, 3:54 p.m. UTC | #2
* H. Nikolaus Schaller <hns@goldelico.com> [191107 11:07]:
> +        - const: "ti,am335x-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5"

This should be without the x, maybe use the earliest one here
for "ti,am3352-sgx530-125" like we have for "ti,am3352-uart".

We could use "ti,am3-sgx530-125" but that can get confused
then with am3517 then.

Regards,

Tony
H. Nikolaus Schaller Nov. 7, 2019, 4:37 p.m. UTC | #3
> Am 07.11.2019 um 16:54 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [191107 11:07]:
>> +        - const: "ti,am335x-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5"
> 
> This should be without the x, maybe use the earliest one here
> for "ti,am3352-sgx530-125" like we have for "ti,am3352-uart".

Ok, fine. Will update accordingly.

> We could use "ti,am3-sgx530-125" but that can get confused
> then with am3517 then.

Indeed.

BR and thanks,
Nikolaus
H. Nikolaus Schaller Nov. 7, 2019, 4:55 p.m. UTC | #4
> Am 07.11.2019 um 15:35 schrieb Rob Herring <robh+dt@kernel.org>:
> 
> On Thu, Nov 7, 2019 at 5:06 AM H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> 
>> The Imagination PVR/SGX GPU is part of several SoC from
>> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo
>> and others.
>> 
>> With this binding, we describe how the SGX processor is
>> interfaced to the SoC (registers, interrupt etc.).
>> 
>> Clock, Reset and power management should be handled
>> by a parent node or elsewhere.
> 
> That's probably TI specific...

Yes and no.

For example the img4780 seems to need a clock reference in the
gpu node. But it could maybe connected in a parent node like recent
TI SoC do with the target-module approach.

And our goal is to end up with a common driver for all SoC and architectures
in far future. Then, probably clock, reset and power management should
be handled in the same way.

> 
>> ---
>> 
>> I have used the doc2yaml script to get a first veryion
>> but I am still stuggling with the yaml thing. My impression
>> is that while it is human readable, it is not very human
>> writable... Unfortunately I haven't found a good tutorial
>> for Dummies (like me) for bindings in YAML.
> 
> Did you read .../bindings/example-schema.yaml? It explains the common
> cases and what schema are doing.

Yes.

> I recently added to it, so look at
> the version in linux-next.

Ah, ok. I haven't read that one.

> 
>> The big problem is not the YAML syntax but what the schema
>> should contain and how to correctly formulate ideas in this
>> new language.
>> 
>> Specific questions for this RFC:
>> 
>> * formatting: is space/tab indentation correct?
> 
> YAML requires spaces.

Which is quite uncommon if you aren't a python programmer...

>> * are strings with "" correct or without?
> 
> Generally only keys or values starting with '#' need quotes. There's
> other cases, but we simply don't hit them with DT. We tend to quote
> $ref values, but that's not strictly needed.

Ok. Good.

> 
>> * how do I specify that there is a list of compatible strings required in a specific order?
> 
> An 'items' list defines the order.

I see.

> 
>> * but there are multiple such lists, and only one of them is to be chosen?
> 
>                                                ^^^^^^
> 'oneOf' is the schema keyword you are looking for.

Ok.

> 
>> * how can be described in the binding that there should be certain values in
>>  the parent node (ranges) to make it work?
> 
> You can't. Schemas match on a node and work down from there. So you
> can do it, but it's more complicated. You'd need a custom 'select'
> select that matches on the parent node having the child node you are
> looking for (assuming the parent is something generic like
> 'simple-bus' which you can't match on). However, based on the example,
> I'd say checking 'ranges' is outside the scope of schema checks.
> 'ranges' doesn't have to be a certain value any more than every case
> of 'reg' (except maybe i2c devices with fixed addresses).

Ok.

> It's up to
> the .dts author how exactly to do address translation.

Well, my concern as a regular .dts author is that I usually treat
bindings as documentation and giving hints how to write a .dts and
what to take care of. If it is not complete, I get into big trouble.

> I would like to have more ranges/reg checks such as bounds checks and
> overlapping addresses, but I think we'd do those with code, not
> schema.
> 
>> I was not able to run
>> 
>>        make dt_binding_check dtbs_check
>> 
>> due to some missing dependencies (which I did not want to
>> invest time to research them) on my build host, so I could
>> not get automated help from those.
> 
> Dependencies are documented in Documentation/devicetree/writing-schema.rst.

One says it needs a libyaml but after installing one my HOSTCC didn't find
it. The other asks for another script which seems to be missing.


> 
>> ---
>> .../devicetree/bindings/gpu/img,pvrsgx.yaml   | 128 ++++++++++++++++++
>> 1 file changed, 128 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..b1b021601c47
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
>> @@ -0,0 +1,128 @@
>> +# SPDX-License-Identifier: None
> 
> Obviously not valid.

:)

> 
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/bindings/gpu/img,pvrsgx.yaml#
> 
> This should have been correct with the script, but you need to drop 'bindings'.

Ok.

> 
>> +$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.
>> +
>> +  Only the Imagination SGX530, SGX540 and SGX544 GPUs are currently covered by
>> +  this binding.
>> +
>> +  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:
>> +      - item:
> 
> 'item/items'


Ok, as you described above we need "items".

> 
>> +        # BeagleBoard ABC, OpenPandora 600MHz
>> +        - const: "ti,omap3-sgx530-121", "img,sgx530-121", "img,sgx530", "img,sgx5"
> 
> Not valid YAML nor json-schema. Each value needs to be list item with 'const:'

Have to look up how that syntax is.

> Plenty of examples in bindings/arm/ with board/soc bindings.

Ok.

> 
>> +        # BeagleBoard XM, GTA04, OpenPandora 1GHz
>> +        - const: "ti,omap3-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5"
> 
> This needs to be a new 'items' list under 'oneOf'.

Ok!

> 
>> +        # BeagleBone Black
>> +        - const: "ti,am335x-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5"
>> +        # Pandaboard (ES)
>> +        - const: "ti,omap4-sgx540-120", "img,sgx540-120", "img,sgx540", "img,sgx5"
>> +        - const "ti,omap4-sgx544-112", "img,sgx544-112", "img,sgx544", "img,sgx5"
>> +        # OMAP5 UEVM, Pyra Handheld
>> +        "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5"
>> +        "ti,dra7-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5"
> 
> Just gave up on trying to write a schema here?

Yes...

You see into what issues a first time YAML/schema writer with 35 years C but no
YAML, Python or JSON experience runs into...

Writing bindings as .txt was easy :)

> 
>> +        # CI20
>> +        "ingenic,jz4780-sgx540-120", "img,sgx540-120", "img,sgx540", "img,sgx5";
>> +
>> +  reg:
>> +    items:
>> +      - description: physical base address and length of the register area
> 
> For single entries, just 'maxItems: 1' is enough. Unless you have
> something special about this device, you don't need a description
> here.

I am not sure if I understand that yet.

> 
>> +
>> +  interrupts:
>> +     items:
>> +      - description: interrupt from SGX subsystem to core processor
>> +
>> +  clocks:
>> +     items:
>> +      - description: optional clocks
>> +
>> +  required:
>> +    - compatible
>> +    - reg
>> +    - interrupts
>> +
>> +examples: |
>> +  gpu@fe00 {
>> +       compatible = "ti,omap-omap5-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5";
>> +       reg = <0xfe00 0x200>;
>> +       interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
>> +  };
>> +
>> +
>> +historical: |
> 
> This should be dropped. It's just for reference as you write the schema.

Yes that is clear. I kept it for reference what I intended to translate from.

> 
>> +  Imagination PVR/SGX GPU
>> +
>> +  Only the Imagination SGX530, SGX540 and SGX544 GPUs are currently covered by this binding.
>> +
>> +  Required properties:
>> +  - compatible:        Should be one of
>> +               "ti,omap3-sgx530-121", "img,sgx530-121", "img,sgx530", "img,sgx5"; - BeagleBoard ABC, OpenPandora 600MHz
>> +               "ti,omap3-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5"; - BeagleBoard XM, GTA04, OpenPandora 1GHz
>> +               "ti,am3517-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5";
>> +               "ti,am335x-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5"; - BeagleBone Black
>> +               "ti,omap4-sgx540-120", "img,sgx540-120", "img,sgx540", "img,sgx5"; - Pandaboard (ES)
>> +               "ti,omap4-sgx544-112", "img,sgx544-112", "img,sgx544", "img,sgx5";
>> +               "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5"; - OMAP5 UEVM, Pyra Handheld
>> +               "ti,dra7-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5";
>> +               "ti,am3517-sgx530-?", "img,sgx530-?", "img,sgx530", "img,sgx5";
>> +               "ti,am43xx-sgx530-?", "img,sgx530-?", "img,sgx530", "img,sgx5";
>> +               "ti,ti81xx-sgx530-?", "img,sgx530-?", "img,sgx530", "img,sgx5";
>> +               "img,jz4780-sgx540-?", "img,sgx540-?", "img,sgx540", "img,sgx5"; - CI20
>> +               "allwinner,sun8i-a83t-sgx544-?", "img,sgx544-116", "img,sgx544", "img,sgx5"; - Banana-Pi-M3 (Allwinner A83T)
>> +               "intel,poulsbo-gma500-sgx535", "img,sgx535-116", "img,sgx535", "img,sgx5"; - Atom Z5xx
>> +               "intel,medfield-gma-sgx540", "img,sgx540-116", "img,sgx540", "img,sgx5"; - Atom Z24xx
>> +               "intel,cedarview-gma3600-sgx545", "img,sgx545-116", "img,sgx545", "img,sgx5"; - Atom N2600, D2500
>> +
>> +               The "ti,omap..." entries are needed temporarily to handle SoC
>> +               specific builds of the kernel module.
>> +
>> +               In the long run, only the "img,sgx..." entry should suffice
>> +               to match a generic driver for all architectures and driver
>> +               code can dynamically find out on which SoC it is running.
>> +
>> +
>> +  - reg:               Physical base address and length of the register area.
>> +  - interrupts:        The interrupt numbers.
>> +
>> +  / {
>> +       ocp {
>> +               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@fe00 {
>> +                               compatible = "ti,omap-omap5-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5";
>> +                               reg = <0xfe00 0x200>;
>> +                               interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
>> +                       };
>> +               };
>> +       };
>> +  };
>> --
>> 2.23.0
>> 

BR and thanks for the help towards a PATCH v3,
Nikolaus
Tony Lindgren Nov. 8, 2019, 4:45 p.m. UTC | #5
* H. Nikolaus Schaller <hns@goldelico.com> [191107 16:56]:
> > Am 07.11.2019 um 15:35 schrieb Rob Herring <robh+dt@kernel.org>:
> > On Thu, Nov 7, 2019 at 5:06 AM H. Nikolaus Schaller <hns@goldelico.com> wrote:
> >> Clock, Reset and power management should be handled
> >> by a parent node or elsewhere.
> > 
> > That's probably TI specific...
> 
> Yes and no.
> 
> For example the img4780 seems to need a clock reference in the
> gpu node. But it could maybe connected in a parent node like recent
> TI SoC do with the target-module approach.

The clocks are implemented at the SoC glue layer and/or the
interconnect layer, and then the device probably has it's
own clock gate controls.

> And our goal is to end up with a common driver for all SoC and architectures
> in far future. Then, probably clock, reset and power management should
> be handled in the same way.

Yeah so that's standard Linux features such as PM runtime
and genpd basically :)

So you can just leave out the clocks paragraph from the
binding. Then if clocks are really needed beyond PM runtime
and genpd, those can always be added later.

We just need a super minimal binding to start with that
only uses standard properties, then more can be added
later if needed.

Regards,

Tony
Tony Lindgren Nov. 8, 2019, 4:49 p.m. UTC | #6
* H. Nikolaus Schaller <hns@goldelico.com> [191107 11:07]:
> +        - const: "ti,am335x-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5"

I did a quick probe test for the module and am4 is the same
as am335x, so please also add this for the next version:

"ti,am4-sgx530-125"

Regards,

Tony
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
new file mode 100644
index 000000000000..b1b021601c47
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
@@ -0,0 +1,128 @@ 
+# SPDX-License-Identifier: None
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/bindings/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.
+
+  Only the Imagination SGX530, SGX540 and SGX544 GPUs are currently covered by
+  this binding.
+
+  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:
+      - item:
+        # BeagleBoard ABC, OpenPandora 600MHz
+        - const: "ti,omap3-sgx530-121", "img,sgx530-121", "img,sgx530", "img,sgx5"
+        # BeagleBoard XM, GTA04, OpenPandora 1GHz
+        - const: "ti,omap3-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5"
+        # BeagleBone Black
+        - const: "ti,am335x-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5"
+        # Pandaboard (ES)
+        - const: "ti,omap4-sgx540-120", "img,sgx540-120", "img,sgx540", "img,sgx5"
+        - const "ti,omap4-sgx544-112", "img,sgx544-112", "img,sgx544", "img,sgx5"
+        # OMAP5 UEVM, Pyra Handheld
+        "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5"
+        "ti,dra7-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5"
+        # CI20
+        "ingenic,jz4780-sgx540-120", "img,sgx540-120", "img,sgx540", "img,sgx5";
+
+  reg:
+    items:
+      - description: physical base address and length of the register area
+
+  interrupts:
+     items:
+      - description: interrupt from SGX subsystem to core processor
+
+  clocks:
+     items:
+      - description: optional clocks
+
+  required:
+    - compatible
+    - reg
+    - interrupts
+
+examples: |
+  gpu@fe00 {
+  	compatible = "ti,omap-omap5-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5";
+   	reg = <0xfe00 0x200>;
+   	interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+  };
+
+
+historical: |
+  Imagination PVR/SGX GPU
+
+  Only the Imagination SGX530, SGX540 and SGX544 GPUs are currently covered by this binding.
+
+  Required properties:
+  - compatible:	Should be one of
+  		"ti,omap3-sgx530-121", "img,sgx530-121", "img,sgx530", "img,sgx5"; - BeagleBoard ABC, OpenPandora 600MHz
+  		"ti,omap3-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5"; - BeagleBoard XM, GTA04, OpenPandora 1GHz
+  		"ti,am3517-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5";
+  		"ti,am335x-sgx530-125", "img,sgx530-125", "img,sgx530", "img,sgx5"; - BeagleBone Black
+  		"ti,omap4-sgx540-120", "img,sgx540-120", "img,sgx540", "img,sgx5"; - Pandaboard (ES)
+  		"ti,omap4-sgx544-112", "img,sgx544-112", "img,sgx544", "img,sgx5";
+  		"ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5"; - OMAP5 UEVM, Pyra Handheld
+  		"ti,dra7-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5";
+  		"ti,am3517-sgx530-?", "img,sgx530-?", "img,sgx530", "img,sgx5";
+  		"ti,am43xx-sgx530-?", "img,sgx530-?", "img,sgx530", "img,sgx5";
+  		"ti,ti81xx-sgx530-?", "img,sgx530-?", "img,sgx530", "img,sgx5";
+  		"img,jz4780-sgx540-?", "img,sgx540-?", "img,sgx540", "img,sgx5"; - CI20
+  		"allwinner,sun8i-a83t-sgx544-?", "img,sgx544-116", "img,sgx544", "img,sgx5"; - Banana-Pi-M3 (Allwinner A83T)
+  		"intel,poulsbo-gma500-sgx535", "img,sgx535-116", "img,sgx535", "img,sgx5"; - Atom Z5xx
+  		"intel,medfield-gma-sgx540", "img,sgx540-116", "img,sgx540", "img,sgx5"; - Atom Z24xx
+  		"intel,cedarview-gma3600-sgx545", "img,sgx545-116", "img,sgx545", "img,sgx5"; - Atom N2600, D2500
+
+  		The "ti,omap..." entries are needed temporarily to handle SoC
+  		specific builds of the kernel module.
+
+  		In the long run, only the "img,sgx..." entry should suffice
+  		to match a generic driver for all architectures and driver
+  		code can dynamically find out on which SoC it is running.
+
+
+  - reg:		Physical base address and length of the register area.
+  - interrupts:	The interrupt numbers.
+
+  / {
+  	ocp {
+  		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@fe00 {
+  				compatible = "ti,omap-omap5-sgx544-116", "img,sgx544-116", "img,sgx544", "img,sgx5";
+  				reg = <0xfe00 0x200>;
+  				interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+  			};
+  		};
+  	};
+  };