| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <52549A4C-F49D-4FE9-9CD2-B331FB486BA9@goldelico.com>
Date: Thu, 7 Nov 2019 17:55:31 +0100
From: "H. Nikolaus Schaller" <hns@...delico.com>
To: Rob Herring <robh+dt@...nel.org>
Cc: David Airlie <airlied@...ux.ie>, Daniel Vetter <daniel@...ll.ch>,
Mark Rutland <mark.rutland@....com>,
Benoît Cousson <bcousson@...libre.com>,
Tony Lindgren <tony@...mide.com>,
Paul Cercueil <paul@...pouillou.net>,
Ralf Baechle <ralf@...ux-mips.org>,
Paul Burton <paulburton@...nel.org>,
James Hogan <jhogan@...nel.org>,
dri-devel <dri-devel@...ts.freedesktop.org>,
devicetree <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
linux-omap <linux-omap@...r.kernel.org>,
OpenPVRSGX Linux Driver Group <openpvrsgx-devgroup@...ux.org>,
Discussions about the Letux Kernel
<letux-kernel@...nphoenux.org>, kernel@...a-handheld.com,
"open list:MIPS" <linux-mips@...r.kernel.org>
Subject: Re: [PATCH v2 1/8] RFC: dt-bindings: add img,pvrsgx.yaml for Imagination GPUs
> Am 07.11.2019 um 15:35 schrieb Rob Herring <robh+dt@...nel.org>:
>
> On Thu, Nov 7, 2019 at 5:06 AM H. Nikolaus Schaller <hns@...delico.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@...delico.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@...0 {
>> + 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@...00000 {
>> + 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@...0 {
>> + 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
Powered by blists - more mailing lists