> Am 07.11.2019 um 15:35 schrieb Rob Herring <robh+dt@xxxxxxxxxx>: > > On Thu, Nov 7, 2019 at 5:06 AM H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> >> +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
