Hi Jonathan,
> Am 15.04.2020 um 20:17 schrieb Jonathan Bakker <xc-racer2@xxxxxxx>:
>
> 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@xxxxxxxxxx>:
>>>
>>> On Wed, 15 Apr 2020 at 10:36, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote:
>>>>
>>>> From: Jonathan Bakker <xc-racer2@xxxxxxx>
>>>>
>>>> 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.
Mine as well...
>
> 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@xxxxxxx>
>>>> Signed-off-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
>>>> ---
>>>> 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.
Yes, please.
>
>>>
>>>> +
>>>> + 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.
Ok, this means there is no need for regulators in the bindings.
BR,
Nikolaus
