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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <D44C023E-C8B3-4974-B800-1DBEC7B4F82C@goldelico.com>
Date:   Fri, 17 Apr 2020 14:15:36 +0200
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Jonathan Bakker <xc-racer2@...e.ca>
Cc:     Krzysztof Kozlowski <krzk@...nel.org>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Benoît Cousson <bcousson@...libre.com>,
        Tony Lindgren <tony@...mide.com>,
        Paul Cercueil <paul@...pouillou.net>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Paul Burton <paulburton@...nel.org>,
        James Hogan <jhogan@...nel.org>, Kukjin Kim <kgene@...nel.org>,
        Maxime Ripard <mripard@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Philipp Rossak <embed3d@...il.com>,
        dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux-omap@...r.kernel.org, openpvrsgx-devgroup@...ux.org,
        letux-kernel@...nphoenux.org, kernel@...a-handheld.com,
        linux-mips@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        "linux-samsung-soc@...r.kernel.org" 
        <linux-samsung-soc@...r.kernel.org>
Subject: Re: [PATCH v6 08/12] arm: dts: s5pv210: Add G3D node

Hi Jonathan,

> Am 15.04.2020 um 20:17 schrieb Jonathan Bakker <xc-racer2@...e.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@...nel.org>:
>>> 
>>> On Wed, 15 Apr 2020 at 10:36, H. Nikolaus Schaller <hns@...delico.com> wrote:
>>>> 
>>>> From: Jonathan Bakker <xc-racer2@...e.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.

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@...e.ca>
>>>> Signed-off-by: H. Nikolaus Schaller <hns@...delico.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@...00000 {
>>>>                       #interrupt-cells = <1>;
>>>>               };
>>>> 
>>>> +               g3d: g3d@...00000 {
>>>> +                       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