[Openpvrsgx-devgroup] S5PV210 Patches

[Jonathan Bakker]

Hi Nikolaus,

On 2020-04-20 1:59 p.m., H. Nikolaus Schaller wrote:
> 
> Ok, I see. This roughly corresponds to the functional clock "fclk" and interface
> clock "iclk" concept of OMAP devices, although those are no longer explicitly
> controlled by the sgx driver.
> 
> This is what may have confused Maxime for the A31, because he knows that the
> driver should control 4 clocks there.
> 
>>  I have no real preference on what they are named.  "gpu" seem a little generic.
> 
> What about "core"?

Sure, sounds fine to me.

> 
> Or could you please try if this works (or can be made working?):
> 
> 		gpu-module at f3000000 {
> 			compatible = "simple-pm-bus";
> 			reg = <0xf3000000 0x10000>;
>      			#address-cells = <1>;
> 			#size-cells = <1>;
> 			ranges;
> 
> 			clock-names = "sclk";
> 			clocks = <&clocks CLK_G3D>;
> 
> 			assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>;
> 			assigned-clock-rates = <0>, <66700000>;
> 			assigned-clock-parents = <&clocks MOUT_MPLL>;
> 
> 			gpu: gpu at 0 {
> 				compatible = "samsung,s5pv210-sgx540-120";
> 				reg = <0x0 0x10000>;
> 				interrupt-parent = <&vic2>;
> 				interrupts = <10>;
> 			};
> 		};
> 
> This would allow to keep the clocks and their number completely outside
> of the sgx node. Basically it looks to me that you have 3 clocks.

Nope, the MOUT_G3D and the DOUT_G3D are the muxes and dividers for the controllable "core" clock.  The CLK_G3D is the only gate that is connected (I believe it may actually be connected to both the core and the bus, but the datasheet is a little unclear).

> 
> I am not exactly sure about ranges and reg entries, but here
> it is described as exactly what we would need> 
> https://elixir.bootlin.com/linux/v5.7-rc2/source/Documentation/devicetree/bindings/bus/simple-pm-bus.yaml
> 

Close, but I think it should look like:

		bus {
			compatible = "simple-pm-bus";
			#address-cells = <1>;
			#size-cells = <1>;
		        ranges;

			clock-names = "sclk";
			clocks = <&clocks CLK_G3D>;

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

			gpu: gpu at f3000000 {
				compatible = "samsung,s5pv210-sgx540-120";
				reg = <0xf3000000 0x10000>;
				interrupt-parent = <&vic2>;
				interrupts = <10>;
			};
		};

Unfortunately, looking at the actual simple-pm-bus driver (drivers/bus/simple-pm-bus.c) it doesn't appear to get or enable any clock.  Running this results in a freeze when trying to use the SGX (same as if the clock wasn't enabled).

https://lwn.net/Articles/632058/ notes that:

  1. Without a device driver bound to the bus device, this device is
     not attached to the PM domain. And although a child device is
     present and active, the PM domain may be powered down, as it's
     considered unused by the PM domain core.
  2. Without a device driver calling pm_runtime_enable(), its
     functional clock is not enabled. Once runtime PM is enabled, the
     R-Mobile PM domain platform driver manages the functional clock
     using runtime PM.

But I have no idea what this means :)


> But this would allow to use Tony's pvr_drv driver to work without
> any explicit clock and power gating since it does the required pm
> calls.
> 

Yeah, the only other "issue" for s5pv210 is that the SGX supposedly requires the DRAM frequency (which is tied to the CPU frequency) to be kept up.
This was done via a cpufreq notifier and setting the minimal frequency, but is now done with pm_qos_request.  Based on the vendor kernel commit
that introduced it at https://github.com/xc-racer99/android_kernel_samsung_aries/commit/ed00c113883db367669b0c129aa60b01ca6124ed it is a "nice to have",
not a "requirement".

> The more I think about, Maxime hasn't understood the concept we
> are following here and it could easily solve his A31 definition
> (but I am not sure if I have - and I haven't explained well :).
> 
> My goal is to separate the clock/pm definitions from the sgx core itself.
> 
> So we should take the chance to try/compare this approach on real
> hardware before we submit a version with optional clocks for
> the sgx bindings. Because this forces us to have a driver that
> is capable of getting this clock. And then we have to agree on
> a clock name.

Thanks,
Jonathan