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, interrupt etc.).
In most cases, Clock, Reset and power management is handled
by a parent node or elsewhere (e.g. code in the driver).
Tested by make dt_binding_check dtbs_check
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
.../devicetree/bindings/gpu/img,pvrsgx.yaml | 122 ++++++++++++++++++
1 file changed, 122 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..e3a4208dfab1
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
@@ -0,0 +1,122 @@
+# 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 <[email protected]>
+
+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.
+
+properties:
+ 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
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+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,
On Wed, Apr 15, 2020 at 10:35:08AM +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, interrupt etc.).
>
> In most cases, Clock, Reset and power management is handled
> by a parent node or elsewhere (e.g. code in the driver).
Wouldn't the "code in the driver" still require the clock / reset /
power domain to be set in the DT?
Maxime
> Am 15.04.2020 um 12:12 schrieb Maxime Ripard <[email protected]>:
>
> Hi,
>
> On Wed, Apr 15, 2020 at 10:35:08AM +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, interrupt etc.).
>>
>> In most cases, Clock, Reset and power management is handled
>> by a parent node or elsewhere (e.g. code in the driver).
>
> Wouldn't the "code in the driver" still require the clock / reset /
> power domain to be set in the DT?
Well, some SoC seem to use existing clocks and have no reset.
Or, although not recommended, they may have the io-address range
hard-coded.
BR,
Nikolaus
Hi,
On 15/04/2020 14:43, H. Nikolaus Schaller wrote:
>
>> Am 15.04.2020 um 12:12 schrieb Maxime Ripard <[email protected]>:
>>
>> Hi,
>>
>> On Wed, Apr 15, 2020 at 10:35:08AM +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, interrupt etc.).
>>>
>>> In most cases, Clock, Reset and power management is handled
>>> by a parent node or elsewhere (e.g. code in the driver).
>>
>> Wouldn't the "code in the driver" still require the clock / reset /
>> power domain to be set in the DT?
>
> Well, some SoC seem to use existing clocks and have no reset.
> Or, although not recommended, they may have the io-address range
> hard-coded.
The possible clocks and resets should be added, even if optional.
Please look at the arm utgard, midgard and bifrost bindings.
Neil
>
> BR,
> Nikolaus
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Hi Neil,
> Am 15.04.2020 um 14:54 schrieb Neil Armstrong <[email protected]>:
>
> Hi,
>
> On 15/04/2020 14:43, H. Nikolaus Schaller wrote:
>>
>>> Am 15.04.2020 um 12:12 schrieb Maxime Ripard <[email protected]>:
>>>
>>> Hi,
>>>
>>> On Wed, Apr 15, 2020 at 10:35:08AM +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, interrupt etc.).
>>>>
>>>> In most cases, Clock, Reset and power management is handled
>>>> by a parent node or elsewhere (e.g. code in the driver).
>>>
>>> Wouldn't the "code in the driver" still require the clock / reset /
>>> power domain to be set in the DT?
>>
>> Well, some SoC seem to use existing clocks and have no reset.
>> Or, although not recommended, they may have the io-address range
>> hard-coded.
>
> The possible clocks and resets should be added, even if optional.
>
> Please look at the arm utgard, midgard and bifrost bindings.
Interesting to compare to. Maybe we should also add the
$nodename: pattern: '^gpu@[a-f0-9]+$'
But the sgx binding is difficult to grasp here. Some SoC like the
omap series have their own ti,sysc based target modules and the
gpu nodes is a child of it lacking any clock and reset references
for purpose.
The jz4780 and some other need a clocks definition, but no reset.
Having a reset seems to be an option for the SoC designer and
not mandated by img. So is it part of the pvrsgx bindings or the
SoC?
Well we could add clocks and resets as optional but that would
allow to wrongly define omap.
Or delegate them to a parent "simple-pm-bus" node.
I have to study that material more to understand what you seem
to expect.
BR and thanks,
Nikolaus Schaller
On Wed, Apr 15, 2020 at 03:17:25PM +0200, H. Nikolaus Schaller wrote:
> Hi Neil,
>
> > Am 15.04.2020 um 14:54 schrieb Neil Armstrong <[email protected]>:
> >
> > Hi,
> >
> > On 15/04/2020 14:43, H. Nikolaus Schaller wrote:
> >>
> >>> Am 15.04.2020 um 12:12 schrieb Maxime Ripard <[email protected]>:
> >>>
> >>> Hi,
> >>>
> >>> On Wed, Apr 15, 2020 at 10:35:08AM +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, interrupt etc.).
> >>>>
> >>>> In most cases, Clock, Reset and power management is handled
> >>>> by a parent node or elsewhere (e.g. code in the driver).
> >>>
> >>> Wouldn't the "code in the driver" still require the clock / reset /
> >>> power domain to be set in the DT?
> >>
> >> Well, some SoC seem to use existing clocks and have no reset.
> >> Or, although not recommended, they may have the io-address range
> >> hard-coded.
> >
> > The possible clocks and resets should be added, even if optional.
> >
> > Please look at the arm utgard, midgard and bifrost bindings.
>
> Interesting to compare to. Maybe we should also add the
> $nodename: pattern: '^gpu@[a-f0-9]+$'
>
> But the sgx binding is difficult to grasp here. Some SoC like the
> omap series have their own ti,sysc based target modules and the
> gpu nodes is a child of it lacking any clock and reset references
> for purpose.
>
> The jz4780 and some other need a clocks definition, but no reset.
> Having a reset seems to be an option for the SoC designer and
> not mandated by img. So is it part of the pvrsgx bindings or the
> SoC?
>
> Well we could add clocks and resets as optional but that would
> allow to wrongly define omap.
>
> Or delegate them to a parent "simple-pm-bus" node.
>
> I have to study that material more to understand what you seem
> to expect.
The thing is, once that binding is in, it has to be backward
compatible. So every thing that you leave out is something that you'll
need to support in the driver eventually.
If you don't want it to be a complete nightmare, you'll want to figure
out as much as possible on how the GPU is integrated and make a
binding out of that. If OMAP is too much of a pain, you can also make
a separate binding for it, and a generic one for the rest of us.
I'd say that it's pretty unlikely that the clocks, interrupts (and
even regulators) are optional. It might be fixed on some SoCs, but
that's up to the DT to express that using fixed clocks / regulators,
not the GPU binding itself.
Maxime
Hi Maxime,
> Am 15.04.2020 um 16:21 schrieb Maxime Ripard <[email protected]>:
>
>>
>> Well we could add clocks and resets as optional but that would
>> allow to wrongly define omap.
>>
>> Or delegate them to a parent "simple-pm-bus" node.
>>
>> I have to study that material more to understand what you seem
>> to expect.
>
> The thing is, once that binding is in, it has to be backward
> compatible. So every thing that you leave out is something that you'll
> need to support in the driver eventually.
>
> If you don't want it to be a complete nightmare, you'll want to figure
> out as much as possible on how the GPU is integrated and make a
> binding out of that.
Hm. Yes. We know that there likely are clocks and maybe reset
but for some SoC this seems to be undocumented and the reset
line the VHDL of the sgx gpu provides may be permanently tied
to "inactive".
So if clocks are optional and not provided, a driver simply can assume
they are enabled somewhere else and does not have to care about. If
they are specified, the driver can enable/disable them.
> If OMAP is too much of a pain, you can also make
> a separate binding for it, and a generic one for the rest of us.
No, omap isn't any pain at all.
The pain is that some other SoC are most easily defined by clocks in
the gpu node which the omap doesn't need to explicitly specify.
I would expect a much bigger nightmare if we split this into two
bindings variants.
> I'd say that it's pretty unlikely that the clocks, interrupts (and
> even regulators) are optional. It might be fixed on some SoCs, but
> that's up to the DT to express that using fixed clocks / regulators,
> not the GPU binding itself.
omap already has these defined them not to be part of the GPU binding.
The reason seems to be that this needs special clock gating control
especially for idle states which is beyond simple clock-enable.
This sysc target-module@56000000 node is already merged and therefore
we are only adding the gpu child node. Without defining clocks.
For example:
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: gpu@0 {
compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
reg = <0x0 0x10000>;
interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
};
};
The jz4780 example will like this:
gpu: gpu@13040000 {
compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
reg = <0x13040000 0x4000>;
clocks = <&cgu JZ4780_CLK_GPU>;
clock-names = "gpu";
interrupt-parent = <&intc>;
interrupts = <63>;
};
So the question is which one is "generic for the rest of us"?
And how can we make a single binding for the sgx. Not one for each
special SoC variant that may exist.
IMHO the best answer is to make clocks an optional property.
Or if we do not want to define them explicitly, we use
additionalProperties: true.
An alternative could be to use a simple-pm-bus like:
sgx_module: sgx_module@13040000 {
compatible = "simple-pm-bus";
clocks = <&cgu JZ4780_CLK_GPU>;
clock-names = "gpu";
#address-cells = <1>;
#size-cells = <1>;
ranges = <0 0x13040000 0x10000>;
gpu: gpu@0 {
compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
reg = <0x0 0x4000>;
interrupt-parent = <&intc>;
interrupts = <63>;
};
};
This gets rid of any clock, reset and pm definitions for the sgx bindings.
But how this is done is outside this sgx bindings.
With such a scheme, the binding I propose here would be complete and fully
generic. We can even add additionalProperties: false.
BR,
Nikolaus
On Wed, Apr 15, 2020 at 05:09:45PM +0200, H. Nikolaus Schaller wrote:
> Hi Maxime,
>
> > Am 15.04.2020 um 16:21 schrieb Maxime Ripard <[email protected]>:
> >
> >>
> >> Well we could add clocks and resets as optional but that would
> >> allow to wrongly define omap.
> >>
> >> Or delegate them to a parent "simple-pm-bus" node.
> >>
> >> I have to study that material more to understand what you seem
> >> to expect.
> >
> > The thing is, once that binding is in, it has to be backward
> > compatible. So every thing that you leave out is something that you'll
> > need to support in the driver eventually.
>
> >
> > If you don't want it to be a complete nightmare, you'll want to figure
> > out as much as possible on how the GPU is integrated and make a
> > binding out of that.
>
> Hm. Yes. We know that there likely are clocks and maybe reset
> but for some SoC this seems to be undocumented and the reset
> line the VHDL of the sgx gpu provides may be permanently tied
> to "inactive".
>
> So if clocks are optional and not provided, a driver simply can assume
> they are enabled somewhere else and does not have to care about. If
> they are specified, the driver can enable/disable them.
Except that at the hardware level, the clock is always going to be
there. You can't control it, but it's there.
> > If OMAP is too much of a pain, you can also make
> > a separate binding for it, and a generic one for the rest of us.
>
> No, omap isn't any pain at all.
>
> The pain is that some other SoC are most easily defined by clocks in
> the gpu node which the omap doesn't need to explicitly specify.
>
> I would expect a much bigger nightmare if we split this into two
> bindings variants.
>
> > I'd say that it's pretty unlikely that the clocks, interrupts (and
> > even regulators) are optional. It might be fixed on some SoCs, but
> > that's up to the DT to express that using fixed clocks / regulators,
> > not the GPU binding itself.
>
> omap already has these defined them not to be part of the GPU binding.
> The reason seems to be that this needs special clock gating control
> especially for idle states which is beyond simple clock-enable.
>
> This sysc target-module@56000000 node is already merged and therefore
> we are only adding the gpu child node. Without defining clocks.
>
> For example:
>
> 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: gpu@0 {
> compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
> reg = <0x0 0x10000>;
> interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> };
> };
>
> The jz4780 example will like this:
>
> gpu: gpu@13040000 {
> compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
> reg = <0x13040000 0x4000>;
>
> clocks = <&cgu JZ4780_CLK_GPU>;
> clock-names = "gpu";
>
> interrupt-parent = <&intc>;
> interrupts = <63>;
> };
>
> So the question is which one is "generic for the rest of us"?
I'd say the latter.
> And how can we make a single binding for the sgx. Not one for each
> special SoC variant that may exist.
>
> IMHO the best answer is to make clocks an optional property.
> Or if we do not want to define them explicitly, we use
> additionalProperties: true.
If your clock is optional, then you define it but don't mandate
it. Not documenting it will only result in a mess where everyone will
put some clock into it, possibly with different semantics each and
every time.
> An alternative could be to use a simple-pm-bus like:
>
> sgx_module: sgx_module@13040000 {
> compatible = "simple-pm-bus";
>
> clocks = <&cgu JZ4780_CLK_GPU>;
> clock-names = "gpu";
>
> #address-cells = <1>;
> #size-cells = <1>;
> ranges = <0 0x13040000 0x10000>;
>
> gpu: gpu@0 {
> compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
> reg = <0x0 0x4000>;
>
> interrupt-parent = <&intc>;
> interrupts = <63>;
> };
> };
>
> This gets rid of any clock, reset and pm definitions for the sgx bindings.
> But how this is done is outside this sgx bindings.
>
> With such a scheme, the binding I propose here would be complete and fully
> generic. We can even add additionalProperties: false.
This has nothing to do with the binding being complete. And if you use
a binding like this one, you'll be severely limited when you'll want
to implement things like DVFS.
Maxime
Hi Maxime,
> Am 15.04.2020 um 18:21 schrieb Maxime Ripard <[email protected]>:
>
> On Wed, Apr 15, 2020 at 05:09:45PM +0200, H. Nikolaus Schaller wrote:
>> Hi Maxime,
>>
>> Hm. Yes. We know that there likely are clocks and maybe reset
>> but for some SoC this seems to be undocumented and the reset
>> line the VHDL of the sgx gpu provides may be permanently tied
>> to "inactive".
>>
>> So if clocks are optional and not provided, a driver simply can assume
>> they are enabled somewhere else and does not have to care about. If
>> they are specified, the driver can enable/disable them.
>
> Except that at the hardware level, the clock is always going to be
> there. You can't control it, but it's there.
Sure, we can deduce that from general hardware design knowledge.
But not every detail must be described in DT. Only the important
ones.
>
>>> If OMAP is too much of a pain, you can also make
>>> a separate binding for it, and a generic one for the rest of us.
>>
>> No, omap isn't any pain at all.
>>
>> The pain is that some other SoC are most easily defined by clocks in
>> the gpu node which the omap doesn't need to explicitly specify.
>>
>> I would expect a much bigger nightmare if we split this into two
>> bindings variants.
>>
>>> I'd say that it's pretty unlikely that the clocks, interrupts (and
>>> even regulators) are optional. It might be fixed on some SoCs, but
>>> that's up to the DT to express that using fixed clocks / regulators,
>>> not the GPU binding itself.
>>
>> omap already has these defined them not to be part of the GPU binding.
>> The reason seems to be that this needs special clock gating control
>> especially for idle states which is beyond simple clock-enable.
>>
>> This sysc target-module@56000000 node is already merged and therefore
>> we are only adding the gpu child node. Without defining clocks.
>>
>> For example:
>>
>> 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: gpu@0 {
>> compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
>> reg = <0x0 0x10000>;
>> interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
>> };
>> };
>>
>> The jz4780 example will like this:
>>
>> gpu: gpu@13040000 {
>> compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
>> reg = <0x13040000 0x4000>;
>>
>> clocks = <&cgu JZ4780_CLK_GPU>;
>> clock-names = "gpu";
>>
>> interrupt-parent = <&intc>;
>> interrupts = <63>;
>> };
>>
>> So the question is which one is "generic for the rest of us"?
>
> I'd say the latter.
Why?
TI SoC seem to be the broadest number of available users
of sgx5xx in the past and nowadays. Others are more the exception.
> If your clock is optional, then you define it but don't mandate
> it. Not documenting it will only result in a mess where everyone will
> put some clock into it, possibly with different semantics each and
> every time.
So you mean that we should require a dummy clock for the omap gpu node
or did I misunderstand that?
Well, yes there is of course a clock connection between the
omap target-module and the sgx but it is IMHO pointless to
describe it because it can't and does not need to be controlled
separately.
As said the target-module is already accepted and upstream and my
proposal is to get the gpu node described there. There is simply
no need for a clocks node for the omap.
What I also assume is that developers of DTS know what they do.
So the risk that there is different semantics is IMHO very low.
If you agree I can add the clocks/clock-names property as an
optional property. This should solve omap and all others.
>
> This has nothing to do with the binding being complete. And if you use
> a binding like this one, you'll be severely limited when you'll want
> to implement things like DVFS.
Now you have unhooked me... Nobody seems to know if and how DVFS can be
applied to SGX. IMHO we should bake small bread first and get initial
support into mainline.
BR,
Nikolaus
* H. Nikolaus Schaller <[email protected]> [200415 16:43]:
> If you agree I can add the clocks/clock-names property as an
> optional property. This should solve omap and all others.
Yes the clock can be optional property no problem. If we have
a clock, we just enable/disable it from the pvr_runtime_suspend()
and pvr_runtime_resume() we alaready have in pvr-drv.c.
Regards,
Tony
On Wed, 15 Apr 2020 10:35:08 +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, interrupt etc.).
>
> In most cases, Clock, Reset and power management is handled
> by a parent node or elsewhere (e.g. code in the driver).
>
> Tested by make dt_binding_check dtbs_check
>
> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> ---
> .../devicetree/bindings/gpu/img,pvrsgx.yaml | 122 ++++++++++++++++++
> 1 file changed, 122 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml
>
My bot found errors running 'make dt_binding_check' on your patch:
Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml: while parsing a block mapping
in "<unicode string>", line 74, column 13
did not find expected key
in "<unicode string>", line 117, column 21
Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/gpu/img,pvrsgx.example.dts' failed
make[1]: *** [Documentation/devicetree/bindings/gpu/img,pvrsgx.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
Makefile:1264: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2
See https://patchwork.ozlabs.org/patch/1270997
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:
pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade
Please check and re-submit.
Hi,
On Wed, Apr 15, 2020 at 06:42:18PM +0200, H. Nikolaus Schaller wrote:
> > Am 15.04.2020 um 18:21 schrieb Maxime Ripard <[email protected]>:
> >
> > On Wed, Apr 15, 2020 at 05:09:45PM +0200, H. Nikolaus Schaller wrote:
> >> Hi Maxime,
> >>
> >> Hm. Yes. We know that there likely are clocks and maybe reset
> >> but for some SoC this seems to be undocumented and the reset
> >> line the VHDL of the sgx gpu provides may be permanently tied
> >> to "inactive".
> >>
> >> So if clocks are optional and not provided, a driver simply can assume
> >> they are enabled somewhere else and does not have to care about. If
> >> they are specified, the driver can enable/disable them.
> >
> > Except that at the hardware level, the clock is always going to be
> > there. You can't control it, but it's there.
>
> Sure, we can deduce that from general hardware design knowledge.
> But not every detail must be described in DT. Only the important
> ones.
>
> >>> If OMAP is too much of a pain, you can also make
> >>> a separate binding for it, and a generic one for the rest of us.
> >>
> >> No, omap isn't any pain at all.
> >>
> >> The pain is that some other SoC are most easily defined by clocks in
> >> the gpu node which the omap doesn't need to explicitly specify.
> >>
> >> I would expect a much bigger nightmare if we split this into two
> >> bindings variants.
> >>
> >>> I'd say that it's pretty unlikely that the clocks, interrupts (and
> >>> even regulators) are optional. It might be fixed on some SoCs, but
> >>> that's up to the DT to express that using fixed clocks / regulators,
> >>> not the GPU binding itself.
> >>
> >> omap already has these defined them not to be part of the GPU binding.
> >> The reason seems to be that this needs special clock gating control
> >> especially for idle states which is beyond simple clock-enable.
> >>
> >> This sysc target-module@56000000 node is already merged and therefore
> >> we are only adding the gpu child node. Without defining clocks.
> >>
> >> For example:
> >>
> >> 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: gpu@0 {
> >> compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
> >> reg = <0x0 0x10000>;
> >> interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> >> };
> >> };
> >>
> >> The jz4780 example will like this:
> >>
> >> gpu: gpu@13040000 {
> >> compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
> >> reg = <0x13040000 0x4000>;
> >>
> >> clocks = <&cgu JZ4780_CLK_GPU>;
> >> clock-names = "gpu";
> >>
> >> interrupt-parent = <&intc>;
> >> interrupts = <63>;
> >> };
> >>
> >> So the question is which one is "generic for the rest of us"?
> >
> > I'd say the latter.
>
> Why?
>
> TI SoC seem to be the broadest number of available users
> of sgx5xx in the past and nowadays. Others are more the exception.
And maybe TI has some complicated stuff around the GPU that others don't have?
If I look quickly at the Allwinner stuff, I see nothing looking alike in the
SoC, so making the binding like that for everyone just because TI did something
doesn't really make much sense.
> > If your clock is optional, then you define it but don't mandate
> > it. Not documenting it will only result in a mess where everyone will
> > put some clock into it, possibly with different semantics each and
> > every time.
>
> So you mean that we should require a dummy clock for the omap gpu node
> or did I misunderstand that?
>
> Well, yes there is of course a clock connection between the
> omap target-module and the sgx but it is IMHO pointless to
> describe it because it can't and does not need to be controlled
> separately.
>
> As said the target-module is already accepted and upstream and my
> proposal is to get the gpu node described there. There is simply
> no need for a clocks node for the omap.
There is no need for a clocks property *currently* *on the OMAP*.
> What I also assume is that developers of DTS know what they do.
> So the risk that there is different semantics is IMHO very low.
Well, they know what they do if you document the binding. Let's say I have two
clocks now on my SoC, and you just document that you want a clocks property,
with a generic name in clock-names like "gpu".
> If you agree I can add the clocks/clock-names property as an
> optional property. This should solve omap and all others.
With the above example, what clock should I put in there? In which order? This
isn't some random example pulled out of nowhere. The Allwinner A31 has (at
least) 4 clocks for the GPU, 1 reset line and 1 regulator, so I can only assume
that the GPU actually needs at least that amount to be properly integrated into
an SoC.
This has nothing to do with being dumb or smart.
> > This has nothing to do with the binding being complete. And if you use
> > a binding like this one, you'll be severely limited when you'll want
> > to implement things like DVFS.
>
> Now you have unhooked me... Nobody seems to know if and how DVFS can be
> applied to SGX. IMHO we should bake small bread first and get initial
> support into mainline.
On the software side, yes, of course. But the discussion here doesn't have much
to do with software support, this is about the hardware. No matter if you enable
DVFS or not, you'll have those resources connected to the GPU.
And if you want to enable the strict minimum in DT for now and expand it later
as the software gains support for more stuff, then you'll have to deal with the
minimal stuff in software later-on to keep the backward compatibility.
But given that the current state on the Allwinner SoCs (at least) is that you
can't even read a register, it might be a good idea to delay the introduction of
that binding until you have something that works to avoid drowning under the
number of special cases to deal with backward compatibility.
Maxime
Hi Maxime,
> Am 17.04.2020 um 12:25 schrieb Maxime Ripard <[email protected]>:
>
> Hi,
>
> On Wed, Apr 15, 2020 at 06:42:18PM +0200, H. Nikolaus Schaller wrote:
>>> Am 15.04.2020 um 18:21 schrieb Maxime Ripard <[email protected]>:
>>>
>>> On Wed, Apr 15, 2020 at 05:09:45PM +0200, H. Nikolaus Schaller wrote:
>>>> Hi Maxime,
>>>>
>>>> Hm. Yes. We know that there likely are clocks and maybe reset
>>>> but for some SoC this seems to be undocumented and the reset
>>>> line the VHDL of the sgx gpu provides may be permanently tied
>>>> to "inactive".
>>>>
>>>> So if clocks are optional and not provided, a driver simply can assume
>>>> they are enabled somewhere else and does not have to care about. If
>>>> they are specified, the driver can enable/disable them.
>>>
>>> Except that at the hardware level, the clock is always going to be
>>> there. You can't control it, but it's there.
>>
>> Sure, we can deduce that from general hardware design knowledge.
>> But not every detail must be described in DT. Only the important
>> ones.
>>
>>>>> If OMAP is too much of a pain, you can also make
>>>>> a separate binding for it, and a generic one for the rest of us.
>>>>
>>>> No, omap isn't any pain at all.
>>>>
>>>> The pain is that some other SoC are most easily defined by clocks in
>>>> the gpu node which the omap doesn't need to explicitly specify.
>>>>
>>>> I would expect a much bigger nightmare if we split this into two
>>>> bindings variants.
>>>>
>>>>> I'd say that it's pretty unlikely that the clocks, interrupts (and
>>>>> even regulators) are optional. It might be fixed on some SoCs, but
>>>>> that's up to the DT to express that using fixed clocks / regulators,
>>>>> not the GPU binding itself.
>>>>
>>>> omap already has these defined them not to be part of the GPU binding.
>>>> The reason seems to be that this needs special clock gating control
>>>> especially for idle states which is beyond simple clock-enable.
>>>>
>>>> This sysc target-module@56000000 node is already merged and therefore
>>>> we are only adding the gpu child node. Without defining clocks.
>>>>
>>>> For example:
>>>>
>>>> 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: gpu@0 {
>>>> compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
>>>> reg = <0x0 0x10000>;
>>>> interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
>>>> };
>>>> };
>>>>
>>>> The jz4780 example will like this:
>>>>
>>>> gpu: gpu@13040000 {
>>>> compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
>>>> reg = <0x13040000 0x4000>;
>>>>
>>>> clocks = <&cgu JZ4780_CLK_GPU>;
>>>> clock-names = "gpu";
>>>>
>>>> interrupt-parent = <&intc>;
>>>> interrupts = <63>;
>>>> };
>>>>
>>>> So the question is which one is "generic for the rest of us"?
>>>
>>> I'd say the latter.
>>
>> Why?
>>
>> TI SoC seem to be the broadest number of available users
>> of sgx5xx in the past and nowadays. Others are more the exception.
>
> And maybe TI has some complicated stuff around the GPU that others don't have?
Looks so.
> If I look quickly at the Allwinner stuff, I see nothing looking alike in the
> SoC, so making the binding like that for everyone just because TI did something
> doesn't really make much sense.
That is why I propose to make the clocks optional. This solves both
cases in a simple and neat way.
>
>>> If your clock is optional, then you define it but don't mandate
>>> it. Not documenting it will only result in a mess where everyone will
>>> put some clock into it, possibly with different semantics each and
>>> every time.
>>
>> So you mean that we should require a dummy clock for the omap gpu node
>> or did I misunderstand that?
>>
>> Well, yes there is of course a clock connection between the
>> omap target-module and the sgx but it is IMHO pointless to
>> describe it because it can't and does not need to be controlled
>> separately.
>>
>> As said the target-module is already accepted and upstream and my
>> proposal is to get the gpu node described there. There is simply
>> no need for a clocks node for the omap.
>
> There is no need for a clocks property *currently* *on the OMAP*.
Yes. But why "currently"? Do you think the OMAPs we already have
defined and tested will change?
>
>> What I also assume is that developers of DTS know what they do.
>> So the risk that there is different semantics is IMHO very low.
>
> Well, they know what they do if you document the binding. Let's say I have two
> clocks now on my SoC, and you just document that you want a clocks property,
> with a generic name in clock-names like "gpu".
Yes, that is what I want to propose for v7:
clocks:
maxItems: 1
clock-names:
maxItems: 1
items:
- const: gpu
>
>> If you agree I can add the clocks/clock-names property as an
>> optional property. This should solve omap and all others.
>
> With the above example, what clock should I put in there? In which order? This
> isn't some random example pulled out of nowhere. The Allwinner A31 has (at
> least) 4 clocks for the GPU, 1 reset line and 1 regulator, so I can only assume
> that the GPU actually needs at least that amount to be properly integrated into
> an SoC.
Ah, now I understand your motivation: you have access and experience with
the A31 and you know that our proposal doesn't fit to it.
From what I know from your description is that the A31 is quite special with
4 GPU clocks... Are they all really for the GPU or 3 of them for the interface
logic (like on OMAP which separates between "functional clocks" and "interface
clocks")? Or are there 4 groups of GPU cores with a separate clock for each one?
So what would be your proposal for the A31 DT?
Then I get a chance to compare DT snippets and try to make a mixture for
the bindings.
>
>>> This has nothing to do with the binding being complete. And if you use
>>> a binding like this one, you'll be severely limited when you'll want
>>> to implement things like DVFS.
>>
>> Now you have unhooked me... Nobody seems to know if and how DVFS can be
>> applied to SGX. IMHO we should bake small bread first and get initial
>> support into mainline.
>
> On the software side, yes, of course. But the discussion here doesn't have much
> to do with software support, this is about the hardware. No matter if you enable
> DVFS or not, you'll have those resources connected to the GPU.
>
> And if you want to enable the strict minimum in DT for now and expand it later
> as the software gains support for more stuff, then you'll have to deal with the
> minimal stuff in software later-on to keep the backward compatibility.
That is IMHO common practise everywhere. Sometimes you even have to adapt
years old DT to new limitations of the drivers (this happened recently for
combination of SPI and GPIO).
And you can still write two different drivers for a single bindings document
or use the .data field of the compatibility table. And I think clocks and regulators
can also be referenced by name if they are not defined in DT. This is not a
"single variety" style, but a potential solution.
What I want to say: there are many roads to Rome.
> But given that the current state on the Allwinner SoCs (at least) is that you
> can't even read a register, it might be a good idea to delay the introduction of
> that binding until you have something that works to avoid drowning under the
> number of special cases to deal with backward compatibility.
Philipp has something minimal working on the A83 which works with the proposed
binding and enabled him to read out the sgx revision register.
So if you are a specialist for the A31 SGX, please make a proposal for a binding
that covers all the A31 needs and all other SoC we know. Or define a separate
bindings for the A31.
Thank you very much,
Nikolaus Schaller
Hi Nikolaus,
Hi Maxime,
>>> TI SoC seem to be the broadest number of available users
>>> of sgx5xx in the past and nowadays. Others are more the exception.
>>
>> And maybe TI has some complicated stuff around the GPU that others don't have?
>
> Looks so.
I can only agree on this.
>
>>
>>> What I also assume is that developers of DTS know what they do.
>>> So the risk that there is different semantics is IMHO very low.
>>
>> Well, they know what they do if you document the binding. Let's say I have two
>> clocks now on my SoC, and you just document that you want a clocks property,
>> with a generic name in clock-names like "gpu".
>
> Yes, that is what I want to propose for v7:
>
> clocks:
> maxItems: 1
>
> clock-names:
> maxItems: 1
> items:
> - const: gpu
>
>>
>>> If you agree I can add the clocks/clock-names property as an
>>> optional property. This should solve omap and all others.
>>
>> With the above example, what clock should I put in there? In which order? This
>> isn't some random example pulled out of nowhere. The Allwinner A31 has (at
>> least) 4 clocks for the GPU, 1 reset line and 1 regulator, so I can only assume
>> that the GPU actually needs at least that amount to be properly integrated into
>> an SoC.
>
> Ah, now I understand your motivation: you have access and experience with
> the A31 and you know that our proposal doesn't fit to it.
>
> From what I know from your description is that the A31 is quite special with
> 4 GPU clocks... Are they all really for the GPU or 3 of them for the interface
> logic (like on OMAP which separates between "functional clocks" and "interface
> clocks")? Or are there 4 groups of GPU cores with a separate clock for each one?
>
> So what would be your proposal for the A31 DT?
>
> Then I get a chance to compare DT snippets and try to make a mixture for
> the bindings.
>
This is my current binding for the A83T, the A31 looks similar but there
is still something missing, since some parts are not mainlined yet:
sun8i-a83t.dtsi:
gpu: gpu@1c400000 {
compatible = "allwinner,sun8i-a83t-sgx544-115",
"img,sgx544-115", "img,sgx544";
reg = <0x01c40000 0x10000>;
interrupts = <GIC_SPI 129 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&ccu CLK_BUS_GPU>, <&ccu CLK_GPU_CORE>,
<&ccu CLK_GPU_MEMORY>, <&ccu CLK_GPU_HYD>;
clock-names = "bus", "core", "memory", "hyd";
resets = <&ccu RST_BUS_GPU>;
};
sun8i-a83t-bananapi-m3.dts:
&gpu {
gpu-supply = <®_dcdc4>;
};
>
>> But given that the current state on the Allwinner SoCs (at least) is that you
>> can't even read a register, it might be a good idea to delay the introduction of
>> that binding until you have something that works to avoid drowning under the
>> number of special cases to deal with backward compatibility.
>
Maxime is right. Even if you enable the regulator, write 0x0 to the GPU
Power Off Gating Register, deassert the reset and enable the clocks you
are not able to read the register.
You must first run: pvrsrvctl --no-module --start (user space binaries)
to access registers otherwise the system will stuck with the following
message when accessing them:
./devmem2 0x01C40024
/dev/mem opened.
> Philipp has something minimal working on the A83 which works with the proposed
> binding and enabled him to read out the sgx revision register.
>
This is not correct. In the other mail I talked about my reference
system. This is an old 3.4.39 kernel, modified by allwinner to run on
their SOC's which don't use the common kernel techniques. So it's very
hacky, but they got the gpu running. I'm using this system for
comparison, to read out registers and for reverse engineering.
My current kernel module behaves similar like the reference design, but
right now I'm not able to run "pvrsrvctl --no-module --start" without
errors. So the initialization never get's finalized and I see the issue
described above.
> So if you are a specialist for the A31 SGX, please make a proposal for a binding
> that covers all the A31 needs and all other SoC we know. Or define a separate
> bindings for the A31.
The A31 and the A31s have some additional clocks mentioned in their
datasheet (@ System Control Register/SRAM Controler). Those are not
available in the A83T datasheet, but might be there since the memory map
looks similar and allwinner might use the same userspace binaries for
their devices.
From the knowledge I gained the last 3 days, we should delay the
patches for the A83T, A31 and A31s.
The idea of the placeholder patches was to show that from this binding
also other devices could benefit.
Cheers,
Philipp
On Fri, Apr 17, 2020 at 02:15:44PM +0200, H. Nikolaus Schaller wrote:
> > Am 17.04.2020 um 12:25 schrieb Maxime Ripard <[email protected]>:
> > On Wed, Apr 15, 2020 at 06:42:18PM +0200, H. Nikolaus Schaller wrote:
> >>> Am 15.04.2020 um 18:21 schrieb Maxime Ripard <[email protected]>:
> >>>
> >>> On Wed, Apr 15, 2020 at 05:09:45PM +0200, H. Nikolaus Schaller wrote:
> >>>> Hi Maxime,
> >>>>
> >>>> Hm. Yes. We know that there likely are clocks and maybe reset
> >>>> but for some SoC this seems to be undocumented and the reset
> >>>> line the VHDL of the sgx gpu provides may be permanently tied
> >>>> to "inactive".
> >>>>
> >>>> So if clocks are optional and not provided, a driver simply can assume
> >>>> they are enabled somewhere else and does not have to care about. If
> >>>> they are specified, the driver can enable/disable them.
> >>>
> >>> Except that at the hardware level, the clock is always going to be
> >>> there. You can't control it, but it's there.
> >>
> >> Sure, we can deduce that from general hardware design knowledge.
> >> But not every detail must be described in DT. Only the important
> >> ones.
> >>
> >>>>> If OMAP is too much of a pain, you can also make
> >>>>> a separate binding for it, and a generic one for the rest of us.
> >>>>
> >>>> No, omap isn't any pain at all.
> >>>>
> >>>> The pain is that some other SoC are most easily defined by clocks in
> >>>> the gpu node which the omap doesn't need to explicitly specify.
> >>>>
> >>>> I would expect a much bigger nightmare if we split this into two
> >>>> bindings variants.
> >>>>
> >>>>> I'd say that it's pretty unlikely that the clocks, interrupts (and
> >>>>> even regulators) are optional. It might be fixed on some SoCs, but
> >>>>> that's up to the DT to express that using fixed clocks / regulators,
> >>>>> not the GPU binding itself.
> >>>>
> >>>> omap already has these defined them not to be part of the GPU binding.
> >>>> The reason seems to be that this needs special clock gating control
> >>>> especially for idle states which is beyond simple clock-enable.
> >>>>
> >>>> This sysc target-module@56000000 node is already merged and therefore
> >>>> we are only adding the gpu child node. Without defining clocks.
> >>>>
> >>>> For example:
> >>>>
> >>>> 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: gpu@0 {
> >>>> compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544";
> >>>> reg = <0x0 0x10000>;
> >>>> interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> >>>> };
> >>>> };
> >>>>
> >>>> The jz4780 example will like this:
> >>>>
> >>>> gpu: gpu@13040000 {
> >>>> compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540";
> >>>> reg = <0x13040000 0x4000>;
> >>>>
> >>>> clocks = <&cgu JZ4780_CLK_GPU>;
> >>>> clock-names = "gpu";
> >>>>
> >>>> interrupt-parent = <&intc>;
> >>>> interrupts = <63>;
> >>>> };
> >>>>
> >>>> So the question is which one is "generic for the rest of us"?
> >>>
> >>> I'd say the latter.
> >>
> >> Why?
> >>
> >> TI SoC seem to be the broadest number of available users
> >> of sgx5xx in the past and nowadays. Others are more the exception.
> >
> > And maybe TI has some complicated stuff around the GPU that others don't have?
>
> Looks so.
>
> > If I look quickly at the Allwinner stuff, I see nothing looking alike in the
> > SoC, so making the binding like that for everyone just because TI did something
> > doesn't really make much sense.
>
> That is why I propose to make the clocks optional. This solves both
> cases in a simple and neat way.
>
> >
> >>> If your clock is optional, then you define it but don't mandate
> >>> it. Not documenting it will only result in a mess where everyone will
> >>> put some clock into it, possibly with different semantics each and
> >>> every time.
> >>
> >> So you mean that we should require a dummy clock for the omap gpu node
> >> or did I misunderstand that?
> >>
> >> Well, yes there is of course a clock connection between the
> >> omap target-module and the sgx but it is IMHO pointless to
> >> describe it because it can't and does not need to be controlled
> >> separately.
> >>
> >> As said the target-module is already accepted and upstream and my
> >> proposal is to get the gpu node described there. There is simply
> >> no need for a clocks node for the omap.
> >
> > There is no need for a clocks property *currently* *on the OMAP*.
>
> Yes. But why "currently"? Do you think the OMAPs we already have
> defined and tested will change?
Like I said, DVFS is likely to be one in the future.
> >> What I also assume is that developers of DTS know what they do.
> >> So the risk that there is different semantics is IMHO very low.
> >
> > Well, they know what they do if you document the binding. Let's say I have two
> > clocks now on my SoC, and you just document that you want a clocks property,
> > with a generic name in clock-names like "gpu".
>
> Yes, that is what I want to propose for v7:
>
> clocks:
> maxItems: 1
>
> clock-names:
> maxItems: 1
> items:
> - const: gpu
If you document what the "gpu" clock is supposed to be.
Is it the clock for the bus (clocking the register part of the GPU), the clock
for the GPU cores? Something else?
> >> If you agree I can add the clocks/clock-names property as an
> >> optional property. This should solve omap and all others.
> >
> > With the above example, what clock should I put in there? In which order? This
> > isn't some random example pulled out of nowhere. The Allwinner A31 has (at
> > least) 4 clocks for the GPU, 1 reset line and 1 regulator, so I can only assume
> > that the GPU actually needs at least that amount to be properly integrated into
> > an SoC.
>
> Ah, now I understand your motivation: you have access and experience with
> the A31 and you know that our proposal doesn't fit to it.
Not only the A31. If you don't document what your expectations are for a generic
component like that, every SoC will assume that your GPU clock is something
different and you won't be able to make any sense of it in your driver.
> From what I know from your description is that the A31 is quite special with
> 4 GPU clocks... Are they all really for the GPU or 3 of them for the interface
> logic (like on OMAP which separates between "functional clocks" and "interface
> clocks")? Or are there 4 groups of GPU cores with a separate clock for each one?
1 is the equivalent of the interface clock, the others seem to be for the
functional clocks.
> So what would be your proposal for the A31 DT?
>
> Then I get a chance to compare DT snippets and try to make a mixture for
> the bindings.
You'd have to know the GPU to do that, and I don't.
> >>> This has nothing to do with the binding being complete. And if you use
> >>> a binding like this one, you'll be severely limited when you'll want
> >>> to implement things like DVFS.
> >>
> >> Now you have unhooked me... Nobody seems to know if and how DVFS can be
> >> applied to SGX. IMHO we should bake small bread first and get initial
> >> support into mainline.
> >
> > On the software side, yes, of course. But the discussion here doesn't have much
> > to do with software support, this is about the hardware. No matter if you enable
> > DVFS or not, you'll have those resources connected to the GPU.
> >
> > And if you want to enable the strict minimum in DT for now and expand it later
> > as the software gains support for more stuff, then you'll have to deal with the
> > minimal stuff in software later-on to keep the backward compatibility.
>
> That is IMHO common practise everywhere. Sometimes you even have to adapt
> years old DT to new limitations of the drivers (this happened recently for
> combination of SPI and GPIO).
To some extent, yes. But those old bindings that turn out to be wrong at least
contain most infos about the hardware, even though it's incomplete or flawed.
Your proposal doesn't.
> And you can still write two different drivers for a single bindings document
> or use the .data field of the compatibility table. And I think clocks and regulators
> can also be referenced by name if they are not defined in DT. This is not a
> "single variety" style, but a potential solution.
>
> What I want to say: there are many roads to Rome.
What I want to say is: all the roads you listed above are going to be painful.
Take your time, have a generic driver running from your generic binding you want
to introduce on all the SoCs you want to support, and *then* start this
discussion again.
Maxime