Re: [PATCH v6 08/12] arm: dts: s5pv210: Add G3D node

[Jonathan Bakker <xc-racer2@live.ca>]

Hi Nikolaus,

On 2020-04-15 5:50 a.m., H. Nikolaus Schaller wrote:
> 
>> Am 15.04.2020 um 13:49 schrieb Krzysztof Kozlowski <krzk@kernel.org>:
>>
>> On Wed, 15 Apr 2020 at 10:36, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>>
>>> From: Jonathan Bakker <xc-racer2@live.ca>
>>>
>>> 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.

>>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> ---
>>> arch/arm/boot/dts/s5pv210.dtsi | 15 +++++++++++++++
>>> 1 file changed, 15 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi
>>> index 2ad642f51fd9..e7fc709c0cca 100644
>>> --- a/arch/arm/boot/dts/s5pv210.dtsi
>>> +++ b/arch/arm/boot/dts/s5pv210.dtsi
>>> @@ -512,6 +512,21 @@ vic3: interrupt-controller@f2300000 {
>>>                        #interrupt-cells = <1>;
>>>                };
>>>
>>> +               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].
> 
>>
>>> +
>>> +                       power-domains = <&pd S5PV210_PD_G3D>;
>>
>> 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.

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

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.

> BR and thanks,
> Nikolaus
> 

Thanks,
Jonathan