ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

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).

Am 15.04.2020 um 12:12 schrieb Maxime Ripard <[email protected]>:

Hi,

Quoted 4 lines · H. Nikolaus Schaller

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.

Quoted 5 lines

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?

Quoted 16 lines

Am 15.04.2020 um 12:12 schrieb Maxime Ripard <[email protected]>:

Hi,

Quoted 9 lines · H. Nikolaus Schaller

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).

Quoted 2 lines

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

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Am 15.04.2020 um 14:54 schrieb Neil Armstrong <[email protected]>:

Hi,

Quoted 19 lines · H. Nikolaus Schaller

Quoted 18 lines

Am 15.04.2020 um 12:12 schrieb Maxime Ripard <[email protected]>:

Hi,

Quoted 9 lines · H. Nikolaus Schaller

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?

Quoted 3 lines

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.

Hi Neil,

Quoted 24 lines

Am 15.04.2020 um 14:54 schrieb Neil Armstrong <[email protected]>:

Hi,

Quoted 19 lines · H. Nikolaus Schaller

Quoted 18 lines

Am 15.04.2020 um 12:12 schrieb Maxime Ripard <[email protected]>:

Hi,

Quoted 9 lines · H. Nikolaus Schaller

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?

Quoted 7 lines

Quoted 3 lines

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.

Am 15.04.2020 um 16:21 schrieb Maxime Ripard <[email protected]>:

Quoted 8 lines

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.

Hi Maxime,

Quoted 11 lines

Am 15.04.2020 um 16:21 schrieb Maxime Ripard <[email protected]>:

Quoted 8 lines

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.

Quoted 8 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Am 15.04.2020 um 18:21 schrieb Maxime Ripard <[email protected]>:

Quoted 7 lines · H. Nikolaus Schaller

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".

Quoted 3 lines

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.

Quoted 59 lines

Quoted 3 lines

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.

Quoted 4 lines

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";

Quoted 5 lines

		interrupt-parent = <&intc>;
		interrupts = <63>;
	};

So the question is which one is "generic for the rest of us"?

I'd say the latter.

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.

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.

If you agree I can add the clocks/clock-names property as an optional property. This should solve omap and all others.

Quoted 10 lines

Am 15.04.2020 um 18:21 schrieb Maxime Ripard <[email protected]>:

Quoted 7 lines · H. Nikolaus Schaller

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".

Quoted 6 lines

Quoted 3 lines

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.

Quoted 62 lines

Quoted 62 lines

Quoted 2 lines

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.

Quoted 4 lines

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>;
	};

Quoted 3 lines

Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Am 17.04.2020 um 12:25 schrieb Maxime Ripard <[email protected]>:

Hi,

Quoted 85 lines · H. Nikolaus Schaller

Quoted 10 lines

Am 15.04.2020 um 18:21 schrieb Maxime Ripard <[email protected]>:

Quoted 7 lines · H. Nikolaus Schaller

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".

Quoted 6 lines

Quoted 3 lines

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.

Quoted 64 lines

Quoted 63 lines

Quoted 2 lines

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.

Quoted 4 lines

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"?

Quoted 6 lines

Quoted 1 line

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.

Quoted 14 lines

Quoted 5 lines

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.

Quoted 3 lines

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*.

Quoted 3 lines

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".

Quoted 3 lines

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.

Quoted 9 lines

Quoted 5 lines

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 4 lines

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

Quoted 9 lines

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 87 lines

Am 17.04.2020 um 12:25 schrieb Maxime Ripard <[email protected]>:

Quoted 85 lines · H. Nikolaus Schaller

Quoted 10 lines

Am 15.04.2020 um 18:21 schrieb Maxime Ripard <[email protected]>:

Quoted 7 lines · H. Nikolaus Schaller

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".

Quoted 6 lines

Quoted 3 lines

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.

Quoted 64 lines

Quoted 63 lines

Quoted 2 lines

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.

Quoted 4 lines

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"?

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 14 lines

Quoted 14 lines

Quoted 5 lines

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.

Quoted 5 lines

Quoted 3 lines

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?

Quoted 6 lines

Quoted 2 lines

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

Quoted 8 lines

Quoted 2 lines

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.

Quoted 12 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

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

Am 16.04.2020 um 22:41 schrieb Rob Herring <[email protected]>:

Quoted 10 lines · H. Nikolaus Schaller

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).

Quoted 8 lines

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 Rob,

Quoted 24 lines

Am 16.04.2020 um 22:41 schrieb Rob Herring <[email protected]>:

Quoted 10 lines · H. Nikolaus Schaller

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).

Quoted 8 lines

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:

Quoted 2 lines

Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml: while parsing a block mapping
in "<unicode string>", line 74, column 13

It turned out that there was a stray " in line 74 from the last editing phase. Will be fixed in v7.

Would it be possible to make dt_binding_check not only report line/column but the contents of the line instead of "<unicode string>"?

and add interrupt.

Tested-by H. Nikolaus Schaller <[email protected]> # Pyra-Handheld.

Am 15.04.2020 um 13:38 schrieb Krzysztof Kozlowski <[email protected]>:

Quoted 4 lines · H. Nikolaus Schaller

and add interrupt.

Tested-by H. Nikolaus Schaller <[email protected]> # Pyra-Handheld.

Don't add your own Tested-by tags. These are implied by authorship, otherwise all patches people make should have such tag.

Hi Krzysztof,

Quoted 10 lines

Am 15.04.2020 um 13:38 schrieb Krzysztof Kozlowski <[email protected]>:

Quoted 4 lines · H. Nikolaus Schaller

and add interrupt.

Tested-by H. Nikolaus Schaller <[email protected]> # Pyra-Handheld.

Don't add your own Tested-by tags. These are implied by authorship, otherwise all patches people make should have such tag.

Ok I see. AFAIR it originates in several phases of editing to report on which device it was tested.

Is there a canonical way of writing "tested-on: ${HARDWARE}"?

E.g. would this be ok?

Signed-off: H. Nikolaus Schaller <[email protected]> # tested on Pyra-Handheld

Am 15.04.2020 um 15:47 schrieb Krzysztof Kozlowski <[email protected]>:

Quoted 13 lines · H. Nikolaus Schaller

Hi Krzysztof,

Quoted 10 lines

Am 15.04.2020 um 13:38 schrieb Krzysztof Kozlowski <[email protected]>:

Quoted 4 lines · H. Nikolaus Schaller

and add interrupt.

Tested-by H. Nikolaus Schaller <[email protected]> # Pyra-Handheld.

Don't add your own Tested-by tags. These are implied by authorship, otherwise all patches people make should have such tag.

Quoted 7 lines

Ok I see. AFAIR it originates in several phases of editing to report on which device it was tested.

Is there a canonical way of writing "tested-on: ${HARDWARE}"?

E.g. would this be ok?

Signed-off: H. Nikolaus Schaller <[email protected]> # tested on Pyra-Handheld

If you think tested platform is worth mentioning in the commit msg (it will stay there forever, ever, ever) then just add a line like:

"Add SGX GPU node. Tested on Pyra-Handheld."

From time to time we add such information to note that only one platform was actually tested.

I am not sure what benefit it brings to most
cases... but your commit msg is so short that adding one more sentence seems reasonable. :)

Best regards,
Krzysztof

From: Jonathan Bakker <[email protected]>

to add support for SGX540 GPU.

Signed-off-by Jonathan Bakker <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
  arch/arm/boot/dts/s5pv210.dtsi | 15 +++++++++++++++
  1 file changed, 15 insertions(+)

arch/arm/boot/dts/s5pv210.dtsi +0 -0

index 2ad642f51fd9..e7fc709c0cca 100644

Am 15.04.2020 um 11:15 schrieb Sergei Shtylyov <[email protected]>:

Hello!

Quoted 9 lines · H. Nikolaus Schaller

From: Jonathan Bakker <[email protected]> to add support for SGX540 GPU.

Signed-off-by Jonathan Bakker <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 arch/arm/boot/dts/s5pv210.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

arch/arm/boot/dts/s5pv210.dtsi +0 -0

index 2ad642f51fd9..e7fc709c0cca 100644

arch/arm/boot/dts/s5pv210.dtsi vic3: interrupt-controller@f2300000 { · line 514

Quoted 6 lines

arch/arm/boot/dts/s5pv210.dtsi +0 -0

@@ -512,6 +512,21 @@ vic3: interrupt-controller@f2300000 {
 			#interrupt-cells = <1>;
 		};
 +		g3d: g3d@f3000000 {

Should be named generically, "gpu@f3000000", according to the DT spec 0.2, section 2.2.2. It's either "gpu" or "display" TTBOMK...

[...]

MBR, Sergei

From: Jonathan Bakker <[email protected]>

to add support for SGX540 GPU.

Signed-off-by Jonathan Bakker <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 arch/arm/boot/dts/s5pv210.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

arch/arm/boot/dts/s5pv210.dtsi +0 -0

index 2ad642f51fd9..e7fc709c0cca 100644

Am 15.04.2020 um 13:49 schrieb Krzysztof Kozlowski <[email protected]>:

Quoted 4 lines · H. Nikolaus Schaller

From: Jonathan Bakker <[email protected]>

to add support for SGX540 GPU.

Do not continue the subject in commit msg like it is one sentence. These are two separate sentences, so commit msg starts with capital letter and it is sentence by itself.

Quoted 14 lines

Signed-off-by Jonathan Bakker <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
arch/arm/boot/dts/s5pv210.dtsi | 15 +++++++++++++++

1 file changed, 15 insertions(+)

arch/arm/boot/dts/s5pv210.dtsi +0 -0

index 2ad642f51fd9..e7fc709c0cca 100644
@@ -512,6 +512,21 @@ vic3: interrupt-controller@f2300000 {
                       #interrupt-cells = <1>;
               };
 

arch/arm/boot/dts/s5pv210.dtsi vic3: interrupt-controller@f2300000 { · line 521

Quoted 7 lines

arch/arm/boot/dts/s5pv210.dtsi +7 -0

+               g3d: g3d@f3000000 {
+                       compatible = "samsung,s5pv210-sgx540-120";
+                       reg = <0xf3000000 0x10000>;
+                       interrupt-parent = <&vic2>;
+                       interrupts = <10>;
+                       clock-names = "sclk";
+                       clocks = <&clocks CLK_G3D>;

Not part of bindings, please remove or add to the bindings.

arch/arm/boot/dts/s5pv210.dtsi vic3: interrupt-controller@f2300000 { · line 523

Quoted 3 lines

arch/arm/boot/dts/s5pv210.dtsi +2 -0

+
+                       power-domains = <&pd S5PV210_PD_G3D>;

Ditto

arch/arm/boot/dts/s5pv210.dtsi vic3: interrupt-controller@f2300000 { · line 527

Quoted 5 lines

arch/arm/boot/dts/s5pv210.dtsi +4 -0

+
+                       assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
+                       assigned-clock-rates = <0>, <66700000>;
+                       assigned-clock-parents = <&clocks MOUT_MPLL>;

Probably this should have status disabled because you do not set regulator supply.

Best regards,
Krzysztof

Quoted 27 lines

Am 15.04.2020 um 13:49 schrieb Krzysztof Kozlowski <[email protected]>:

Quoted 4 lines · H. Nikolaus Schaller

From: Jonathan Bakker <[email protected]>

to add support for SGX540 GPU.

Do not continue the subject in commit msg like it is one sentence. These are two separate sentences, so commit msg starts with capital letter and it is sentence by itself.

Quoted 14 lines

Signed-off-by Jonathan Bakker <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
arch/arm/boot/dts/s5pv210.dtsi | 15 +++++++++++++++

1 file changed, 15 insertions(+)

arch/arm/boot/dts/s5pv210.dtsi +0 -0

index 2ad642f51fd9..e7fc709c0cca 100644
@@ -512,6 +512,21 @@ vic3: interrupt-controller@f2300000 {
                       #interrupt-cells = <1>;
               };
 

Quoted 9 lines

arch/arm/boot/dts/s5pv210.dtsi vic3: interrupt-controller@f2300000 { · line 521

Quoted 7 lines

arch/arm/boot/dts/s5pv210.dtsi +7 -0

+               g3d: g3d@f3000000 {
+                       compatible = "samsung,s5pv210-sgx540-120";
+                       reg = <0xf3000000 0x10000>;
+                       interrupt-parent = <&vic2>;
+                       interrupts = <10>;
+                       clock-names = "sclk";
+                       clocks = <&clocks CLK_G3D>;

Not part of bindings, please remove or add to the bindings.

Well, the bindings should describe what is common for all SoC and they are quite different in what they need in addition.

Thererfore we have no "additionalProperties: false" in the bindings [PATCH v6 01/12].

Quoted 5 lines

Am 15.04.2020 um 13:49 schrieb Krzysztof Kozlowski <[email protected]>:

Quoted 1 line · H. Nikolaus Schaller

Quoted 8 lines

Quoted 3 lines

From: Jonathan Bakker <[email protected]>

to add support for SGX540 GPU.

Do not continue the subject in commit msg like it is one sentence. These are two separate sentences, so commit msg starts with capital letter and it is sentence by itself.

Quoted 14 lines

Quoted 14 lines

Signed-off-by Jonathan Bakker <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
arch/arm/boot/dts/s5pv210.dtsi | 15 +++++++++++++++

1 file changed, 15 insertions(+)

arch/arm/boot/dts/s5pv210.dtsi +0 -0

index 2ad642f51fd9..e7fc709c0cca 100644
@@ -512,6 +512,21 @@ vic3: interrupt-controller@f2300000 {
                       #interrupt-cells = <1>;
               };
 

Quoted 9 lines

arch/arm/boot/dts/s5pv210.dtsi vic3: interrupt-controller@f2300000 { · line 521

Quoted 7 lines

arch/arm/boot/dts/s5pv210.dtsi +7 -0

+               g3d: g3d@f3000000 {
+                       compatible = "samsung,s5pv210-sgx540-120";
+                       reg = <0xf3000000 0x10000>;
+                       interrupt-parent = <&vic2>;
+                       interrupts = <10>;
+                       clock-names = "sclk";
+                       clocks = <&clocks CLK_G3D>;

Not part of bindings, please remove or add to the bindings.

Well, the bindings should describe what is common for all SoC and they are quite different in what they need in addition.

Thererfore we have no "additionalProperties: false" in the bindings [PATCH v6 01/12].

Quoted 4 lines

Quoted 3 lines

arch/arm/boot/dts/s5pv210.dtsi +2 -0

+
+                       power-domains = <&pd S5PV210_PD_G3D>;

Quoted 1 line

Ditto

In this case it might be possible to add the clock/power-domains etc. to a wrapper node compatible to "simple-pm-bus" or similar and make the gpu a child of it.

@Jontahan: can you please give it a try?

Quoted 8 lines

arch/arm/boot/dts/s5pv210.dtsi vic3: interrupt-controller@f2300000 { · line 527

Quoted 5 lines

arch/arm/boot/dts/s5pv210.dtsi +4 -0

+
+                       assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
+                       assigned-clock-rates = <0>, <66700000>;
+                       assigned-clock-parents = <&clocks MOUT_MPLL>;

Probably this should have status disabled because you do not set regulator supply.

BR and thanks,
Nikolaus

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

arch/arm/boot/dts/s5pv210.dtsi +4 -0

+
+                       assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
+                       assigned-clock-rates = <0>, <66700000>;
+                       assigned-clock-parents = <&clocks MOUT_MPLL>;

Quoted 2 lines

Quoted 2 lines

Probably this should have status disabled because you do not set regulator supply.

I don't believe there is a regulator on s5pv210, if there is, then it is a fixed regulator with no control on both s5pv210 devices that I have.

The vendor driver did use the regulator framework for its power domain implementation, but that definitely shouldn't be upstreamed.

Am 15.04.2020 um 20:17 schrieb Jonathan Bakker <[email protected]>:

Hi Nikolaus,

Quoted 7 lines · H. Nikolaus Schaller

Quoted 7 lines

Am 15.04.2020 um 13:49 schrieb Krzysztof Kozlowski <[email protected]>:

Quoted 3 lines · H. Nikolaus Schaller

From: Jonathan Bakker <[email protected]>

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

to add support for SGX540 GPU.

Do not continue the subject in commit msg like it is one sentence. These are two separate sentences, so commit msg starts with capital letter and it is sentence by itself.

Sorry, that's my fault, I should know better.

Nikolaus took this from my testing tree and I apparently didn't have it in as good as state as I should have.

Quoted 34 lines

Quoted 14 lines

Quoted 14 lines

Signed-off-by Jonathan Bakker <[email protected]>

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
arch/arm/boot/dts/s5pv210.dtsi | 15 +++++++++++++++

1 file changed, 15 insertions(+)

arch/arm/boot/dts/s5pv210.dtsi +0 -0

index 2ad642f51fd9..e7fc709c0cca 100644
@@ -512,6 +512,21 @@ vic3: interrupt-controller@f2300000 {
                      #interrupt-cells = <1>;
              };
 

Quoted 9 lines

arch/arm/boot/dts/s5pv210.dtsi vic3: interrupt-controller@f2300000 { · line 521

Quoted 7 lines

arch/arm/boot/dts/s5pv210.dtsi +7 -0

+               g3d: g3d@f3000000 {
+                       compatible = "samsung,s5pv210-sgx540-120";
+                       reg = <0xf3000000 0x10000>;
+                       interrupt-parent = <&vic2>;
+                       interrupts = <10>;
+                       clock-names = "sclk";
+                       clocks = <&clocks CLK_G3D>;

Not part of bindings, please remove or add to the bindings.

Well, the bindings should describe what is common for all SoC and they are quite different in what they need in addition.

Thererfore we have no "additionalProperties: false" in the bindings [PATCH v6 01/12].

Quoted 4 lines

Quoted 3 lines

arch/arm/boot/dts/s5pv210.dtsi +2 -0

+
+                       power-domains = <&pd S5PV210_PD_G3D>;

Quoted 9 lines

Quoted 1 line

Ditto

In this case it might be possible to add the clock/power-domains etc. to a wrapper node compatible to "simple-pm-bus" or similar and make the gpu a child of it.

@Jontahan: can you please give it a try?

The power-domains comes from a (so far) non-upstreamed power domain driver for s5pv210 that I've been playing around with. It's not necessary for proper operation as it's on by default.

Looking at simple-pm-bus, I don't really understand its purpose. Is it a way of separating out a power domain from a main device's node? Or is it designed for when you have multiple devices under the same power domain?

Nikolaus, I can regenerate a proper patch for you if you want that's not based on my testing tree.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

arch/arm/boot/dts/s5pv210.dtsi +4 -0

+
+                       assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
+                       assigned-clock-rates = <0>, <66700000>;
+                       assigned-clock-parents = <&clocks MOUT_MPLL>;

Quoted 2 lines

Quoted 2 lines

Probably this should have status disabled because you do not set regulator supply.

I don't believe there is a regulator on s5pv210, if there is, then it is a fixed regulator with no control on both s5pv210 devices that I have.

The vendor driver did use the regulator framework for its power domain implementation, but that definitely shouldn't be upstreamed.

* rebased to v5.7-rc1
* added DTS for for a31, a31s, a83t - by Philipp Rossak <[email protected]>
* added DTS for "samsung,s5pv210-sgx540-120" - by Jonathan Bakker <[email protected]>
* bindings.yaml fixes:
- added a31, a31
- fixes for omap4470
- jz4780 contains an sgx540-130 and not -120
- a83t contains an sgx544-115 and not -116
- removed "additionalProperties: false" because some SoC may need additional properties

PATCH V5 2020-03-29 19:38:32:
* reworked YAML bindings to pass dt_binding_check and be better grouped
* rename all nodes to "gpu: gpu@<address>"
* removed "img,sgx5" from example - suggested by Rob Herring <[email protected]>

PATCH V4 2019-12-17 19:02:11:
* MIPS: DTS: jz4780: removed "img,sgx5" from bindings
* YAML bindings: updated according to suggestions by Rob Herring
* MIPS: DTS: jz4780: insert-sorted gpu node by register address - suggested by Paul Cercueil

PATCH V3 2019-11-24 12:40:33:
* reworked YAML format with help by Rob Herring
* removed .txt binding document
* change compatible "ti,am335x-sgx" to "ti,am3352-sgx" - suggested by Tony Lindgren

PATCH V2 2019-11-07 12:06:17:
* tried to convert bindings to YAML format - suggested by Rob Herring
* added JZ4780 DTS node (proven to load the driver)
* removed timer and img,cores properties until we know we really need them - suggested by Rob Herring

PATCH V1 2019-10-18 20:46:35:

This patch series defines child nodes for the SGX5xx interface inside different SoC so that a driver can be found and probed by the compatible strings and can retrieve information about the SGX revision that is included in a specific SoC. It also defines the interrupt number to be used by the SGX driver.

There is currently no mainline driver for these GPUs, but a project
[1] is ongoing with the goal to get the open-source part as provided by TI/IMG and others into drivers/gpu/drm/pvrsgx.

The kernel modules built from this project have successfully demonstrated to work with the DTS definitions from this patch set on AM335x BeagleBone Black, DM3730 and OMAP5 Pyra and Droid 4. They partially work on OMAP3530 and PandaBoard ES but that is likely a problem in the kernel driver or the (non-free) user-space libraries and binaries.

Wotk for JZ4780 (CI20 board) is in progress and there is potential to extend this work to e.g. BananaPi-M3 (A83) and some Intel Poulsbo and CedarView devices.

Am 15.04.2020 um 12:10 schrieb Maxime Ripard <[email protected]>:

Hi,

Quoted 38 lines · H. Nikolaus Schaller

* rebased to v5.7-rc1
* added DTS for for a31, a31s, a83t - by Philipp Rossak <[email protected]>
* added DTS for "samsung,s5pv210-sgx540-120" - by Jonathan Bakker <[email protected]>
* bindings.yaml fixes:
- added a31, a31
- fixes for omap4470
- jz4780 contains an sgx540-130 and not -120
- a83t contains an sgx544-115 and not -116
- removed "additionalProperties: false" because some SoC may need additional properties

PATCH V5 2020-03-29 19:38:32:
* reworked YAML bindings to pass dt_binding_check and be better grouped
* rename all nodes to "gpu: gpu@<address>"
* removed "img,sgx5" from example - suggested by Rob Herring <[email protected]>

PATCH V4 2019-12-17 19:02:11:
* MIPS: DTS: jz4780: removed "img,sgx5" from bindings
* YAML bindings: updated according to suggestions by Rob Herring
* MIPS: DTS: jz4780: insert-sorted gpu node by register address - suggested by Paul Cercueil

PATCH V3 2019-11-24 12:40:33:
* reworked YAML format with help by Rob Herring
* removed .txt binding document
* change compatible "ti,am335x-sgx" to "ti,am3352-sgx" - suggested by Tony Lindgren

PATCH V2 2019-11-07 12:06:17:
* tried to convert bindings to YAML format - suggested by Rob Herring
* added JZ4780 DTS node (proven to load the driver)
* removed timer and img,cores properties until we know we really need them - suggested by Rob Herring

PATCH V1 2019-10-18 20:46:35:

This patch series defines child nodes for the SGX5xx interface inside different SoC so that a driver can be found and probed by the compatible strings and can retrieve information about the SGX revision that is included in a specific SoC. It also defines the interrupt number to be used by the SGX driver.

Quoted 3 lines

There is currently no mainline driver for these GPUs, but a project
[1] is ongoing with the goal to get the open-source part as provided by TI/IMG and others into drivers/gpu/drm/pvrsgx.

Just a heads up, DRM requires an open-source user-space, so if your plan is to move the open-source kernel driver while using the closed-source library (as that page seem to suggest), that might change a few things.

Quoted 8 lines

The kernel modules built from this project have successfully demonstrated to work with the DTS definitions from this patch set on AM335x BeagleBone Black, DM3730 and OMAP5 Pyra and Droid 4. They partially work on OMAP3530 and PandaBoard ES but that is likely a problem in the kernel driver or the (non-free) user-space libraries and binaries.

Quoted 3 lines

Wotk for JZ4780 (CI20 board) is in progress and there is potential to extend this work to e.g. BananaPi-M3 (A83) and some Intel Poulsbo and CedarView devices.

If it's not been tested on any Allwinner board yet, I'll leave it aside until it's been properly shown to work.

Hi Maxime,

Quoted 43 lines

Am 15.04.2020 um 12:10 schrieb Maxime Ripard <[email protected]>:

Hi,

Quoted 38 lines · H. Nikolaus Schaller

* rebased to v5.7-rc1
* added DTS for for a31, a31s, a83t - by Philipp Rossak <[email protected]>
* added DTS for "samsung,s5pv210-sgx540-120" - by Jonathan Bakker <[email protected]>
* bindings.yaml fixes:
- added a31, a31
- fixes for omap4470
- jz4780 contains an sgx540-130 and not -120
- a83t contains an sgx544-115 and not -116
- removed "additionalProperties: false" because some SoC may need additional properties

PATCH V5 2020-03-29 19:38:32:
* reworked YAML bindings to pass dt_binding_check and be better grouped
* rename all nodes to "gpu: gpu@<address>"
* removed "img,sgx5" from example - suggested by Rob Herring <[email protected]>

PATCH V4 2019-12-17 19:02:11:
* MIPS: DTS: jz4780: removed "img,sgx5" from bindings
* YAML bindings: updated according to suggestions by Rob Herring
* MIPS: DTS: jz4780: insert-sorted gpu node by register address - suggested by Paul Cercueil

PATCH V3 2019-11-24 12:40:33:
* reworked YAML format with help by Rob Herring
* removed .txt binding document
* change compatible "ti,am335x-sgx" to "ti,am3352-sgx" - suggested by Tony Lindgren

PATCH V2 2019-11-07 12:06:17:
* tried to convert bindings to YAML format - suggested by Rob Herring
* added JZ4780 DTS node (proven to load the driver)
* removed timer and img,cores properties until we know we really need them - suggested by Rob Herring

PATCH V1 2019-10-18 20:46:35:

This patch series defines child nodes for the SGX5xx interface inside different SoC so that a driver can be found and probed by the compatible strings and can retrieve information about the SGX revision that is included in a specific SoC. It also defines the interrupt number to be used by the SGX driver.

Quoted 8 lines

Quoted 3 lines

There is currently no mainline driver for these GPUs, but a project
[1] is ongoing with the goal to get the open-source part as provided by TI/IMG and others into drivers/gpu/drm/pvrsgx.

Just a heads up, DRM requires an open-source user-space, so if your plan is to move the open-source kernel driver while using the closed-source library (as that page seem to suggest), that might change a few things.

The far future goal is to arrive at a completely open implementation, but nobody knows how to get there. Therefore we bake smaller bread :)

step 1: get SoC integration right and stable (this is what this series is for) step 2: make the open source kernel driver work with closed-source libs step 3: write open-source replacements for user-space

Quoted 9 lines

Quoted 9 lines

The kernel modules built from this project have successfully demonstrated to work with the DTS definitions from this patch set on AM335x BeagleBone Black, DM3730 and OMAP5 Pyra and Droid 4. They partially work on OMAP3530 and PandaBoard ES but that is likely a problem in the kernel driver or the (non-free) user-space libraries and binaries.

Quoted 6 lines

Quoted 3 lines

Wotk for JZ4780 (CI20 board) is in progress and there is potential to extend this work to e.g. BananaPi-M3 (A83) and some Intel Poulsbo and CedarView devices.

If it's not been tested on any Allwinner board yet, I'll leave it aside until it's been properly shown to work.

Phillip has testes something on a83.

BR and thanks,
Nikolaus

Quoted 7 lines

Quoted 7 lines

The kernel modules built from this project have successfully demonstrated to work with the DTS definitions from this patch set on AM335x BeagleBone Black, DM3730 and OMAP5 Pyra and Droid 4. They partially work on OMAP3530 and PandaBoard ES but that is likely a problem in the kernel driver or the (non-free) user-space libraries and binaries.

Quoted 6 lines

Quoted 3 lines

Wotk for JZ4780 (CI20 board) is in progress and there is potential to extend this work to e.g. BananaPi-M3 (A83) and some Intel Poulsbo and CedarView devices.

If it's not been tested on any Allwinner board yet, I'll leave it aside until it's been properly shown to work.

Phillip has testes something on a83.

Am 15.04.2020 um 15:02 schrieb Maxime Ripard <[email protected]>:

Quoted 11 lines · H. Nikolaus Schaller

Quoted 11 lines

Quoted 10 lines

The kernel modules built from this project have successfully demonstrated to work with the DTS definitions from this patch set on AM335x BeagleBone Black, DM3730 and OMAP5 Pyra and Droid 4. They partially work on OMAP3530 and PandaBoard ES but that is likely a problem in the kernel driver or the (non-free) user-space libraries and binaries.

Wotk for JZ4780 (CI20 board) is in progress and there is potential to extend this work to e.g. BananaPi-M3 (A83) and some Intel Poulsbo and CedarView devices.

Quoted 4 lines

Quoted 2 lines

If it's not been tested on any Allwinner board yet, I'll leave it aside until it's been properly shown to work.

Phillip has tested something on a83.

I'm a bit skeptical on that one since it doesn't even list the interrupts connected to the GPU that the binding mandates.

Quoted 15 lines

Am 15.04.2020 um 15:02 schrieb Maxime Ripard <[email protected]>:

Quoted 11 lines · H. Nikolaus Schaller

Quoted 11 lines

Quoted 10 lines

The kernel modules built from this project have successfully demonstrated to work with the DTS definitions from this patch set on AM335x BeagleBone Black, DM3730 and OMAP5 Pyra and Droid 4. They partially work on OMAP3530 and PandaBoard ES but that is likely a problem in the kernel driver or the (non-free) user-space libraries and binaries.

Wotk for JZ4780 (CI20 board) is in progress and there is potential to extend this work to e.g. BananaPi-M3 (A83) and some Intel Poulsbo and CedarView devices.

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

If it's not been tested on any Allwinner board yet, I'll leave it aside until it's been properly shown to work.

Phillip has tested something on a83.

Quoted 2 lines

I'm a bit skeptical on that one since it doesn't even list the interrupts connected to the GPU that the binding mandates.

I think he left it out for a future update.
But best he comments himself.

Quoted 5 lines

Quoted 2 lines

I'm a bit skeptical on that one since it doesn't even list the interrupts connected to the GPU that the binding mandates.

I think he left it out for a future update.
But best he comments himself.

I'm currently working on those bindings. They are now 90% done, but they are not finished till now. Currently there is some mainline support missing to add the full binding. The A83T and also the A31/A31s have a GPU Power Off Gating Register in the R_PRCM module, that is not supported right now in Mainline. The Register need to be written when the GPU is powered on and off.

@Maxime: I totally agree on your point that a demo needs to be provided before the related DTS patches should be provided. That's the reason why I added the gpu placeholder patches.
Do you have an idea how a driver for the R_PRCM stuff can look like? I'm not that experienced with the clock driver framework.

The big question is right now how to proceed with the A83T and A31s patches. I see there three options, which one do you prefer?:

1. Provide now placeholder patches and send new patches, if everything is clear and other things are mainlined
2. Provide now patches as complete as possible and provide later patches to complete them when the R_PRCM things are mainlined
3. Leave them out, till the related work is mainlined and the bindings are final.

Since this GPU IP core is very flexible and the SOC manufactures can configure it on their needs, I think the binding will extend in the future. For example the SGX544 GPU is available in different configurations: there is a SGX544 core and SGX544MPx core. The x stands for the count of the USSE (Universal Scalable Shader Engine) cores. For example the GPU in the A83T is a MP1 and the A31/A31s a MP2.

In addition to that some of the GPU's have also a 2D engine.

There might be even more differences in the GPU's that we don't know right now and should be described in the Devicetree, but that's a different topic that we should keep in mind.

Hi,

Quoted 13 lines · Philipp Rossak

Quoted 5 lines

Quoted 2 lines

I'm a bit skeptical on that one since it doesn't even list the interrupts connected to the GPU that the binding mandates.

I think he left it out for a future update.
But best he comments himself.

I'm currently working on those bindings. They are now 90% done, but they are not finished till now. Currently there is some mainline support missing to add the full binding. The A83T and also the A31/A31s have a GPU Power Off Gating Register in the R_PRCM module, that is not supported right now in Mainline. The Register need to be written when the GPU is powered on and off.

Quoted 5 lines

@Maxime: I totally agree on your point that a demo needs to be provided before the related DTS patches should be provided. That's the reason why I added the gpu placeholder patches.
Do you have an idea how a driver for the R_PRCM stuff can look like? I'm not that experienced with the clock driver framework.

It looks like a power-domain to me, so you'd rather plug that into the genpd framework.

Quoted 9 lines

The big question is right now how to proceed with the A83T and A31s patches. I see there three options, which one do you prefer?:

1. Provide now placeholder patches and send new patches, if everything is clear and other things are mainlined
2. Provide now patches as complete as possible and provide later patches to complete them when the R_PRCM things are mainlined
3. Leave them out, till the related work is mainlined and the bindings are final.

Like I said, the DT *has* to be backward-compatible, so for any DT patch that you are asking to be merged, you should be prepared to support it indefinitely and be able to run from it, and you won't be able to change the bindings later on.

Quoted 6 lines

Since this GPU IP core is very flexible and the SOC manufactures can configure it on their needs, I think the binding will extend in the future. For example the SGX544 GPU is available in different configurations: there is a SGX544 core and SGX544MPx core. The x stands for the count of the USSE (Universal Scalable Shader Engine) cores. For example the GPU in the A83T is a MP1 and the A31/A31s a MP2.

Mali is in the same situation and it didn't cause much trouble.

Quoted 1 line

In addition to that some of the GPU's have also a 2D engine.

In the same memory region, running from the same interrupts, or is it a completely separate IP that happens to be sold by the same vendor?

Quoted 22 lines · Maxime Ripard

Hi,

Quoted 18 lines · Philipp Rossak

Quoted 5 lines

Quoted 2 lines

I'm a bit skeptical on that one since it doesn't even list the interrupts connected to the GPU that the binding mandates.

I think he left it out for a future update.
But best he comments himself.

I'm currently working on those bindings. They are now 90% done, but they are not finished till now. Currently there is some mainline support missing to add the full binding. The A83T and also the A31/A31s have a GPU Power Off Gating Register in the R_PRCM module, that is not supported right now in Mainline. The Register need to be written when the GPU is powered on and off.

@Maxime: I totally agree on your point that a demo needs to be provided before the related DTS patches should be provided. That's the reason why I added the gpu placeholder patches.
Do you have an idea how a driver for the R_PRCM stuff can look like? I'm not that experienced with the clock driver framework.

Quoted 2 lines

It looks like a power-domain to me, so you'd rather plug that into the genpd framework.

I had a look on genpd and I'm not really sure if that fits.

It is basically some bit that verify that the clocks should be enabled or disabled.

I think this is better placed somewhere in the clocking framework. I see there more similarities to the gating stuff.
Do you think it is suitable to implement it like the clock gating?

Quoted 10 lines

Quoted 9 lines

The big question is right now how to proceed with the A83T and A31s patches. I see there three options, which one do you prefer?:

1. Provide now placeholder patches and send new patches, if everything is clear and other things are mainlined
2. Provide now patches as complete as possible and provide later patches to complete them when the R_PRCM things are mainlined
3. Leave them out, till the related work is mainlined and the bindings are final.

Quoted 4 lines

Like I said, the DT *has* to be backward-compatible, so for any DT patch that you are asking to be merged, you should be prepared to support it indefinitely and be able to run from it, and you won't be able to change the bindings later on.

I agree on your points. But is this also suitable to drivers that are currently off tree and might be merged in one or two years?

Quoted 4 lines · Philipp Rossak

I had a look on genpd and I'm not really sure if that fits.

It is basically some bit that verify that the clocks should be enabled or disabled.

No, it can do much more than that. It's a framework to control the SoCs power domains, so clocks might be a part of it, but most of the time it's going to be about powering up a particular device.

Am 21.04.2020 um 16:15 schrieb Tony Lindgren <[email protected]>:

Quoted 3 lines · Maxime Ripard

Quoted 2 lines · Philipp Rossak

I had a look on genpd and I'm not really sure if that fits.

Quoted 6 lines

Quoted 2 lines

It is basically some bit that verify that the clocks should be enabled or disabled.

No, it can do much more than that. It's a framework to control the SoCs power domains, so clocks might be a part of it, but most of the time it's going to be about powering up a particular device.

Note that on omaps there are actually SoC module specific registers.

And there can be multiple devices within a single target module on omaps. So the extra dts node and device is justified there.

For other SoCs, the SGX clocks are probably best handled directly in pvr-drv.c PM runtime functions unless a custom hardware wrapper with SoC specific registers exists.

Regards,

Tony

Quoted 2 lines

Am 21.04.2020 um 16:15 schrieb Tony Lindgren <[email protected]>:
Note that on omaps there are actually SoC module specific registers.

Ah, I see. This is of course a difference that the TI glue logic has its own registers in the same address range as the sgx and this can't be easily handled by a common sgx driver.

This indeed seems to be unique with omap.

Quoted 6 lines

And there can be multiple devices within a single target module on omaps. So the extra dts node and device is justified there.

For other SoCs, the SGX clocks are probably best handled directly in pvr-drv.c PM runtime functions unless a custom hardware wrapper with SoC specific registers exists.

That is why we need to evaluate what the better strategy is.

So we have
a) omap which has a custom wrapper around the sgx
b) others without, i.e. an empty (or pass-through) wrapper

Which one do we make the "standard" and which one the "exception"? What are good reasons for either one?

It also allows to handle different number of clocks (A31 seems to need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings or making big lists of conditionals. This variance would be handled outside the sgx core bindings and driver.

See for example the standard 8250 uart for am335x with:

$ git grep -B20 -A10 uart0 arch/arm/boot/dts/am33xx-l4.dtsi

The 8250 device configuration is described in the standard 8250 dts binding, and the am335x module in the ti-sysc binding. The are separate devices :)

Quoted 7 lines

Am 21.04.2020 um 16:15 schrieb Tony Lindgren <[email protected]>:

Quoted 3 lines · Maxime Ripard

Quoted 2 lines · Philipp Rossak

I had a look on genpd and I'm not really sure if that fits.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

It is basically some bit that verify that the clocks should be enabled or disabled.

No, it can do much more than that. It's a framework to control the SoCs power domains, so clocks might be a part of it, but most of the time it's going to be about powering up a particular device.

Note that on omaps there are actually SoC module specific registers.

Ah, I see. This is of course a difference that the TI glue logic has its own registers in the same address range as the sgx and this can't be easily handled by a common sgx driver.

This indeed seems to be unique with omap.

Quoted 6 lines

And there can be multiple devices within a single target module on omaps. So the extra dts node and device is justified there.

For other SoCs, the SGX clocks are probably best handled directly in pvr-drv.c PM runtime functions unless a custom hardware wrapper with SoC specific registers exists.

That is why we need to evaluate what the better strategy is.

So we have
a) omap which has a custom wrapper around the sgx
b) others without, i.e. an empty (or pass-through) wrapper

Which one do we make the "standard" and which one the "exception"? What are good reasons for either one?

I am currently in strong favour of a) being standard because it makes the pvr-drv.c simpler and really generic (independent of wrapping into any SoC).

This will likely avoid problems if we find more SoC with yet another scheme how the SGX clocks are wrapped.

It also allows to handle different number of clocks (A31 seems to need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings or making big lists of conditionals. This variance would be handled outside the sgx core bindings and driver.

Am 22.04.2020 um 08:58 schrieb Maxime Ripard <[email protected]>:

Quoted 46 lines · H. Nikolaus Schaller

Quoted 7 lines

Am 21.04.2020 um 16:15 schrieb Tony Lindgren <[email protected]>:

Quoted 3 lines · Maxime Ripard

Quoted 2 lines · Philipp Rossak

I had a look on genpd and I'm not really sure if that fits.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

It is basically some bit that verify that the clocks should be enabled or disabled.

No, it can do much more than that. It's a framework to control the SoCs power domains, so clocks might be a part of it, but most of the time it's going to be about powering up a particular device.

Note that on omaps there are actually SoC module specific registers.

Ah, I see. This is of course a difference that the TI glue logic has its own registers in the same address range as the sgx and this can't be easily handled by a common sgx driver.

This indeed seems to be unique with omap.

Quoted 6 lines

And there can be multiple devices within a single target module on omaps. So the extra dts node and device is justified there.

For other SoCs, the SGX clocks are probably best handled directly in pvr-drv.c PM runtime functions unless a custom hardware wrapper with SoC specific registers exists.

That is why we need to evaluate what the better strategy is.

So we have
a) omap which has a custom wrapper around the sgx
b) others without, i.e. an empty (or pass-through) wrapper

Which one do we make the "standard" and which one the "exception"? What are good reasons for either one?

I am currently in strong favour of a) being standard because it makes the pvr-drv.c simpler and really generic (independent of wrapping into any SoC).

This will likely avoid problems if we find more SoC with yet another scheme how the SGX clocks are wrapped.

Quoted 4 lines

It also allows to handle different number of clocks (A31 seems to need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings or making big lists of conditionals. This variance would be handled outside the sgx core bindings and driver.

I disagree. Every other GPU binding and driver is handling that just fine, and the SGX is not special in any case here.

Quoted 54 lines

Am 22.04.2020 um 08:58 schrieb Maxime Ripard <[email protected]>:

Quoted 50 lines · H. Nikolaus Schaller

Quoted 7 lines

Am 21.04.2020 um 16:15 schrieb Tony Lindgren <[email protected]>:

Quoted 3 lines · Maxime Ripard

Quoted 2 lines · Philipp Rossak

I had a look on genpd and I'm not really sure if that fits.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

It is basically some bit that verify that the clocks should be enabled or disabled.

No, it can do much more than that. It's a framework to control the SoCs power domains, so clocks might be a part of it, but most of the time it's going to be about powering up a particular device.

Note that on omaps there are actually SoC module specific registers.

Ah, I see. This is of course a difference that the TI glue logic has its own registers in the same address range as the sgx and this can't be easily handled by a common sgx driver.

This indeed seems to be unique with omap.

Quoted 6 lines

And there can be multiple devices within a single target module on omaps. So the extra dts node and device is justified there.

For other SoCs, the SGX clocks are probably best handled directly in pvr-drv.c PM runtime functions unless a custom hardware wrapper with SoC specific registers exists.

That is why we need to evaluate what the better strategy is.

So we have
a) omap which has a custom wrapper around the sgx
b) others without, i.e. an empty (or pass-through) wrapper

Which one do we make the "standard" and which one the "exception"? What are good reasons for either one?

I am currently in strong favour of a) being standard because it makes the pvr-drv.c simpler and really generic (independent of wrapping into any SoC).

This will likely avoid problems if we find more SoC with yet another scheme how the SGX clocks are wrapped.

It also allows to handle different number of clocks (A31 seems to need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings or making big lists of conditionals. This variance would be handled outside the sgx core bindings and driver.

Quoted 2 lines

I disagree. Every other GPU binding and driver is handling that just fine, and the SGX is not special in any case here.

Can you please better explain this? With example or a description or a proposal?

I simply do not have your experience with "every other GPU" as you have. And I admit that I can't read from your statement what we should do to bring this topic forward.

So please make a proposal how it should be in your view.

Am 22.04.2020 um 17:13 schrieb Maxime Ripard <[email protected]>:

Quoted 7 lines · H. Nikolaus Schaller

Quoted 7 lines

Am 22.04.2020 um 08:58 schrieb Maxime Ripard <[email protected]>:

Quoted 5 lines

It also allows to handle different number of clocks (A31 seems to need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings or making big lists of conditionals. This variance would be handled outside the sgx core bindings and driver.

Quoted 5 lines

Quoted 2 lines

I disagree. Every other GPU binding and driver is handling that just fine, and the SGX is not special in any case here.

Can you please better explain this? With example or a description or a proposal?

I can't, I don't have any knowledge about this GPU.

Quoted 6 lines

I simply do not have your experience with "every other GPU" as you have. And I admit that I can't read from your statement what we should do to bring this topic forward.

So please make a proposal how it should be in your view.

If you need some inspiration, I guess you could look at the mali and vivante bindings once you have an idea of what the GPU needs across the SoCs it's integrated in.

Am 22.04.2020 um 18:55 schrieb Paul Cercueil <[email protected]>:

Hi Nikolaus,

Quoted 24 lines · H. Nikolaus Schaller

Hi Maxime,

Quoted 7 lines

Am 22.04.2020 um 17:13 schrieb Maxime Ripard <[email protected]>:

Quoted 5 lines · H. Nikolaus Schaller

Quoted 5 lines

Am 22.04.2020 um 08:58 schrieb Maxime Ripard <[email protected]>:

Quoted 4 lines

It also allows to handle different number of clocks (A31 seems to need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings or making big lists of conditionals. This variance would be handled outside the sgx core bindings and driver.

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

I disagree. Every other GPU binding and driver is handling that just fine, and the SGX is not special in any case here.

Can you please better explain this? With example or a description or a proposal?

I can't, I don't have any knowledge about this GPU.

Hm. Now I am fully puzzled.
You have no knowledge about this GPU but disagree with our proposal? Is it just gut feeling?
Anyways, we need to find a solution. Together.

Quoted 7 lines

Quoted 4 lines

I simply do not have your experience with "every other GPU" as you have. And I admit that I can't read from your statement what we should do to bring this topic forward.
So please make a proposal how it should be in your view.

If you need some inspiration, I guess you could look at the mali and vivante bindings once you have an idea of what the GPU needs across the SoCs it's integrated in.

Quoted 9 lines

Well, I do not need inspiration, we need to come to an agreement about img,pvrsgx.yaml and we need some maintainer to finally pick it up. I wonder how we can come to this stage.
If I look at vivante,gc.yaml or arm,mali-utgard.yaml I don't see big differences to what we propose and those I see seem to come from technical differences between sgx, vivante, mali etc. So there is no single scheme that fits all different gpu types.
One thing we can learn is that "core" seems to be a de facto standard for the core clock-name. An alternative "gpu" is used by nvidia,gk20a.txt.

The Vivante GPU binding requires "bus", "core" and "shader" clocks. But if your SoC only has one clock for the GPU, there's nothing that prevents you from passing the very same clock as "bus", "core" and "shader". This is what we do on the Ingenic JZ4770 SoC.

Hi Paul,

Quoted 40 lines

Am 22.04.2020 um 18:55 schrieb Paul Cercueil <[email protected]>:

Hi Nikolaus,

Quoted 24 lines · H. Nikolaus Schaller

Hi Maxime,

Quoted 7 lines

Am 22.04.2020 um 17:13 schrieb Maxime Ripard <[email protected]>:

Quoted 5 lines · H. Nikolaus Schaller

Quoted 5 lines

Am 22.04.2020 um 08:58 schrieb Maxime Ripard <[email protected]>:

Quoted 4 lines

It also allows to handle different number of clocks (A31 seems to need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings or making big lists of conditionals. This variance would be handled outside the sgx core bindings and driver.

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

I disagree. Every other GPU binding and driver is handling that just fine, and the SGX is not special in any case here.

Can you please better explain this? With example or a description or a proposal?

I can't, I don't have any knowledge about this GPU.

Hm. Now I am fully puzzled.
You have no knowledge about this GPU but disagree with our proposal? Is it just gut feeling?
Anyways, we need to find a solution. Together.

Quoted 7 lines

Quoted 4 lines

I simply do not have your experience with "every other GPU" as you have. And I admit that I can't read from your statement what we should do to bring this topic forward.
So please make a proposal how it should be in your view.

If you need some inspiration, I guess you could look at the mali and vivante bindings once you have an idea of what the GPU needs across the SoCs it's integrated in.

Quoted 9 lines

Well, I do not need inspiration, we need to come to an agreement about img,pvrsgx.yaml and we need some maintainer to finally pick it up. I wonder how we can come to this stage.
If I look at vivante,gc.yaml or arm,mali-utgard.yaml I don't see big differences to what we propose and those I see seem to come from technical differences between sgx, vivante, mali etc. So there is no single scheme that fits all different gpu types.
One thing we can learn is that "core" seems to be a de facto standard for the core clock-name. An alternative "gpu" is used by nvidia,gk20a.txt.

Quoted 1 line

The Vivante GPU binding requires "bus", "core" and "shader" clocks. But if your SoC only has one clock for the GPU, there's nothing that prevents you from passing the very same clock as "bus", "core" and "shader". This is what we do on the Ingenic JZ4770 SoC.

Fine and good to know.

Well, for the SGX we so far only know a single "core" clock (with different names). Only the A31 seems to be different.

Fortunately I finally found a little time to scan through the a31 user manual: http://dl.linux-sunxi.org/A31/A31%20User%20Manual%20V1.20.pdf

There are 3 clock dividers. And there is a single clock PLL8 dedicated to the gpu. The clock dividers are called "gpu core", "gpu mem", "gpu hyd".

Then, there are dedicated clock gating registers. And idle/power status registers.

Unfortunately, chapter "5.1. GPU" is almost empty and has no block diagram. So I have no idea what "HYD" stands for. And if the memory and HYD clocks are needed and how they should be initialized. If they are different ones or can all be driven by PLL8 in parallel.

That scarce information makes it difficult to form a "proper" bindings document out of it. Any can fit or be false.

At least there is something common with all other SGX implementations I am aware of: there is a "core" clock.

So I'd suggest to get things moving forwards:
* we add a "core" clock-names to the bindings
* this can't be wrong for the A31 since it is defined in the data sheet
* we make it optional since the omap chips have a clock wrapper
* "core" is a name I think all architectures/drivers can live with and can add later "shader", "bus" etc. if needed
* any additions for the A31 will be additions

If that sounds ok and nobody objects to it, I can submit a new patch version for further review.

BR and thanks,
Nikolaus

A few years back, I did a big research on the PowerVR GPUs. Back then I found an interesting TI datasheet. I forgot about this till I have seen the right buzz words. Sorry that I remembered it that late.

Back then I came to the conclusion that all PowerVR GPU's have in general 3 Clocks.

A system clock, a memory clock and a core clock. [1].

The hyd_clk at sunxi devices seems to be the system clock.

With those additional information it should be very easy to get a proper binding.

[1]: https://github.com/embed-3d/PVRSGX_hwdoc/blob/master/sources/pdfs/Spruh73c_chapter_SGX_Graphics_Accelerator.pdf

Am 22.04.2020 um 21:33 schrieb Tony Lindgren <[email protected]>:

Quoted 8 lines · Philipp Rossak

A few years back, I did a big research on the PowerVR GPUs. Back then I found an interesting TI datasheet. I forgot about this till I have seen the right buzz words. Sorry that I remembered it that late.

Back then I came to the conclusion that all PowerVR GPU's have in general 3 Clocks.

A system clock, a memory clock and a core clock. [1].

Hmm I'm not sure if those names are sgx or SoC specific.

Anyways, the sgx clocks for omap variants are already handled by the ti-sysc module as "fck" and "ick" so nothing to do there.

Quoted 5 lines

The hyd_clk at sunxi devices seems to be the system clock.

With those additional information it should be very easy to get a proper binding.

It would be best to find the clock(s) name used in the sgx docs to avoid using SoC specific naming :)

But yeah "sysclk" "memclk" and "coreclk" seem just fine for me for the optional clocks if that works for other SoCs.

Regards,

Tony

Quoted 1 line

[1]: https://github.com/embed-3d/PVRSGX_hwdoc/blob/master/sources/pdfs/Spruh73c_chapter_SGX_Graphics_Accelerator.pdf

Hi Maxime,

Quoted 10 lines

Am 22.04.2020 um 17:13 schrieb Maxime Ripard <[email protected]>:

Quoted 7 lines · H. Nikolaus Schaller

Quoted 7 lines

Am 22.04.2020 um 08:58 schrieb Maxime Ripard <[email protected]>:

Quoted 5 lines

It also allows to handle different number of clocks (A31 seems to need 4, Samsung, A83 and JZ4780 one) without changing the sgx bindings or making big lists of conditionals. This variance would be handled outside the sgx core bindings and driver.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

I disagree. Every other GPU binding and driver is handling that just fine, and the SGX is not special in any case here.

Can you please better explain this? With example or a description or a proposal?

I can't, I don't have any knowledge about this GPU.

Hm. Now I am fully puzzled.
You have no knowledge about this GPU but disagree with our proposal? Is it just gut feeling?

Anyways, we need to find a solution. Together.

Quoted 10 lines

Quoted 6 lines

I simply do not have your experience with "every other GPU" as you have. And I admit that I can't read from your statement what we should do to bring this topic forward.

So please make a proposal how it should be in your view.

If you need some inspiration, I guess you could look at the mali and vivante bindings once you have an idea of what the GPU needs across the SoCs it's integrated in.

Well, I do not need inspiration, we need to come to an agreement about img,pvrsgx.yaml and we need some maintainer to finally pick it up.

I wonder how we can come to this stage.

If I look at vivante,gc.yaml or arm,mali-utgard.yaml I don't see big differences to what we propose and those I see seem to come from technical differences between sgx, vivante, mali etc. So there is no single scheme that fits all different gpu types.

One thing we can learn is that "core" seems to be a de facto standard for the core clock-name. An alternative "gpu" is used by nvidia,gk20a.txt.

BR and thanks,
Nikolaus

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Am 23.04.2020 um 17:00 schrieb Neil Armstrong <[email protected]>:

Quoted 2 lines

One thing we can learn is that "core" seems to be a de facto standard for the core clock-name. An alternative "gpu" is used by nvidia,gk20a.txt.

Usually IPs needs a few clocks:
- pclk or apb or reg: the clock clocking the "slave" bus to serve the registers
- axi or bus or ahb: the bus clocking the the "master" bus to get data from system memory
- core: the actual clock feeding the GPU logic

Sometimes you have a single clock for slave and master bus.

But you can also have separate clocks for shader cores, .. this depends on the IP and it's architecture. The IP can also have memories with separate clocks, etc...

But all these clocks can be source by an unique clock on a SoC, but different on another SoC, this is why it's important to list them all, even optional.

You'll certainly have at least a reset signal, and a power domain, these should exist and be optional.

Quoted 8 lines

Am 23.04.2020 um 17:00 schrieb Neil Armstrong <[email protected]>:

Quoted 2 lines

One thing we can learn is that "core" seems to be a de facto standard for the core clock-name. An alternative "gpu" is used by nvidia,gk20a.txt.

Usually IPs needs a few clocks:
- pclk or apb or reg: the clock clocking the "slave" bus to serve the registers
- axi or bus or ahb: the bus clocking the the "master" bus to get data from system memory
- core: the actual clock feeding the GPU logic

And the sgx544 seems to have two such clocks.

Quoted 4 lines

Sometimes you have a single clock for slave and master bus.

But you can also have separate clocks for shader cores, .. this depends on the IP and it's architecture. The IP can also have memories with separate clocks, etc...

Indeed.

Quoted 4 lines

But all these clocks can be source by an unique clock on a SoC, but different on another SoC, this is why it's important to list them all, even optional.

You'll certainly have at least a reset signal, and a power domain, these should exist and be optional.

Well, they exist only as hints in block diagrams of some SoC data sheets (so we do not know if they represent the imagination definitions) and the current driver code doesn't make use of it. Still the gpu core works.

So I do not see any urgent need to add a complete list to the bindings now.

Unless some special SoC integration makes use of them. Then it is IMHO easier to extend the bindings by a follow-up patch than now thinking about all potential options and bloating the bindings with things we (the open source community) doesn't and can't know.

My goal is to keep the bindings as minimalistic as possible. And reset lines and power domains are (at least for those we have in the works) not needed to make working systems.

Therefore, for clocks I also would start with a minimalistic approach for a single optional GPU core clock and leave out reset and power completely.

Am 23.04.2020 um 22:36 schrieb Maxime Ripard <[email protected]>:

Quoted 4 lines

My goal is to keep the bindings as minimalistic as possible. And reset lines and power domains are (at least for those we have in the works) not needed to make working systems.

Quoted 3 lines

Therefore, for clocks I also would start with a minimalistic approach for a single optional GPU core clock and leave out reset and power completely.

Like I said above, the DT is considered an ABI and you'll have to maintain backward compatibility (ie, newer kernel running with older DT).

Therefore, you won't be able to require a new clock, reset or power-domain later on for example.

I guess the question I'm really asking is: since you don't really know how the hardware is integrated at the moment,

why should we have that
discussion *now*. It's really not suprising that you don't know yet, so I'm not sure why we need to rush in the bindings.

Hi,

Quoted 26 lines · Philipp Rossak

Quoted 24 lines · Maxime Ripard

Hi,

Quoted 13 lines · Philipp Rossak

Quoted 5 lines

Quoted 2 lines

I'm a bit skeptical on that one since it doesn't even list the interrupts connected to the GPU that the binding mandates.

I think he left it out for a future update.
But best he comments himself.

I'm currently working on those bindings. They are now 90% done, but they are not finished till now. Currently there is some mainline support missing to add the full binding. The A83T and also the A31/A31s have a GPU Power Off Gating Register in the R_PRCM module, that is not supported right now in Mainline. The Register need to be written when the GPU is powered on and off.

Quoted 5 lines

@Maxime: I totally agree on your point that a demo needs to be provided before the related DTS patches should be provided. That's the reason why I added the gpu placeholder patches.
Do you have an idea how a driver for the R_PRCM stuff can look like? I'm not that experienced with the clock driver framework.

It looks like a power-domain to me, so you'd rather plug that into the genpd framework.

Quoted 4 lines

I had a look on genpd and I'm not really sure if that fits.

It is basically some bit that verify that the clocks should be enabled or disabled.

No, it can do much more than that. It's a framework to control the SoCs power domains, so clocks might be a part of it, but most of the time it's going to be about powering up a particular device.

Quoted 3 lines

I think this is better placed somewhere in the clocking framework. I see there more similarities to the gating stuff.
Do you think it is suitable to implement it like the clock gating?

I'm really not sure what makes you think that this should be modelled as a clock?

Quoted 15 lines

Quoted 14 lines

Quoted 9 lines

The big question is right now how to proceed with the A83T and A31s patches. I see there three options, which one do you prefer?:

1. Provide now placeholder patches and send new patches, if everything is clear and other things are mainlined
2. Provide now patches as complete as possible and provide later patches to complete them when the R_PRCM things are mainlined
3. Leave them out, till the related work is mainlined and the bindings are final.

Like I said, the DT *has* to be backward-compatible, so for any DT patch that you are asking to be merged, you should be prepared to support it indefinitely and be able to run from it, and you won't be able to change the bindings later on.

Quoted 2 lines

I agree on your points. But is this also suitable to drivers that are currently off tree and might be merged in one or two years?

This is what we done for the Mali. The devicetree binding was first done for the out-of-tree driver, and then lima/panfrost reused it.

The key thing here is to have enough confidence about how the hardware works so that you can accurately describe it.

Hi,

Quoted 29 lines · Maxime Ripard

Hi,

Quoted 26 lines · Philipp Rossak

Quoted 24 lines · Maxime Ripard

Hi,

Quoted 13 lines · Philipp Rossak

Quoted 5 lines

Quoted 2 lines

I'm a bit skeptical on that one since it doesn't even list the interrupts connected to the GPU that the binding mandates.

I think he left it out for a future update.
But best he comments himself.

I'm currently working on those bindings. They are now 90% done, but they are not finished till now. Currently there is some mainline support missing to add the full binding. The A83T and also the A31/A31s have a GPU Power Off Gating Register in the R_PRCM module, that is not supported right now in Mainline. The Register need to be written when the GPU is powered on and off.

Quoted 5 lines

@Maxime: I totally agree on your point that a demo needs to be provided before the related DTS patches should be provided. That's the reason why I added the gpu placeholder patches.
Do you have an idea how a driver for the R_PRCM stuff can look like? I'm not that experienced with the clock driver framework.

It looks like a power-domain to me, so you'd rather plug that into the genpd framework.

Quoted 9 lines

Quoted 4 lines

I had a look on genpd and I'm not really sure if that fits.

It is basically some bit that verify that the clocks should be enabled or disabled.

No, it can do much more than that. It's a framework to control the SoCs power domains, so clocks might be a part of it, but most of the time it's going to be about powering up a particular device.

So I think I've found now the right piece of documentation and a driver that implements something similar [1].

So I will write a similar driver like linked above that only sets the right bits for A83T and A31/A31s.
Do you think this is the right approach?