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_checkdue 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 areaFor 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, D2500The "ti,omap..." entries are needed temporarily to handle SoCspecific builds of the kernel module.In the long run, only the "img,sgx..." entry should sufficeto match a generic driver for all architectures and drivercode 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