linux-kernel - Re: [PATCH 2/2] arm64: dts: rockchip: Add basic support for orion-r68

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]
Date:	Fri, 15 Jan 2016 12:31:22 -0300
From:	Javier Martinez Canillas <javier@...hile0.org>
To:	Heiko Stuebner <heiko@...ech.de>
Cc:	Matthias Brugger <matthias.bgg@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Thierry Reding <treding@...dia.com>,
	Alexandru Stan <amstan@...omium.org>,
	Arnd Bergmann <arnd@...db.de>,
	Brian Norris <briannorris@...omium.org>,
	Douglas Anderson <dianders@...omium.org>,
	Naoki FUKAUMI <naobsd@...il.com>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	linux-rockchip@...ts.infradead.org,
	Sjoerd Simons <sjoerd.simons@...labora.co.uk>,
	mbrugger@...e.com, Dmitry Torokhov <dmitry.torokhov@...il.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>, p.zabel@...gutronix.de,
	Olof Johansson <olof@...om.net>, andy.yan@...k-chips.com,
	Romain Perier <romain.perier@...il.com>, jic23@...nel.org
Subject: Re: [PATCH 2/2] arm64: dts: rockchip: Add basic support for orion-r68

Hello Heiko,

On Fri, Jan 15, 2016 at 11:26 AM, Heiko Stuebner <heiko@...ech.de> wrote:
> Am Freitag, 15. Januar 2016, 11:22:52 schrieb Javier Martinez Canillas:
>> Hello Heiko,
>>
>> On Fri, Jan 15, 2016 at 11:03 AM, Heiko Stuebner <heiko@...ech.de> wrote:
>> > Hi Javier,
>> >
>> > Am Freitag, 15. Januar 2016, 10:28:44 schrieb Javier Martinez Canillas:
>> >> This is not a complete review but I just wanted to comment on two
>> >
>> >> things that I noticed:
>> > same for me for now :-)
>> >
>> >> On Fri, Jan 15, 2016 at 10:06 AM, Matthias Brugger
>> >> <matthias.bgg@...il.com> wrote:
>> >>
>> >> [snip]
>> >>
>> >> > +       };
>> >> > +
>> >> > +       vcc_18: vcc18-regulator {
>> >> > +               compatible = "regulator-fixed";
>> >> > +               regulator-name = "vcc_18";
>> >> > +               regulator-min-microvolt = <1800000>;
>> >> > +               regulator-max-microvolt = <1800000>;
>> >> > +               regulator-always-on;
>> >> > +               regulator-boot-on;
>> >> > +               vin-supply = <&vcc_sys>;
>> >> > +       };
>> >> > +
>> >> > +       /* supplies both host and otg */
>> >> > +       vcc_host: vcc-host-regulator {
>> >> > +               compatible = "regulator-fixed";
>> >> > +               gpio = <&gpio0 4 GPIO_ACTIVE_LOW>;
>> >> > +               pinctrl-names = "default";
>> >> > +               pinctrl-0 = <&host_vbus_drv>;
>> >> > +               regulator-name = "vcc_host";
>> >> > +               regulator-always-on;
>> >> > +               regulator-boot-on;
>> >> > +               vin-supply = <&vcc_sys>;
>> >> > +       };
>> >> > +
>> >> > +       vccio_sd: vcc-io-sd-regulator {
>> >> > +               regulator-name= "vccio_sd";
>> >> > +               gpio = <&gpio0 9 GPIO_ACTIVE_LOW>;
>> >> > +               regulator-min-microvolt = <3300000>;
>> >> > +               regulator-max-microvolt = <3300000>;
>
> that seems to be missing essential regulator properties
> like a compatible.
>

Oh right, I missed that when first reading the patch.

>> >> > +       };
>> >> > +
>> >> > +       vcc_sd: vcc-sd-regulator {
>> >> > +               compatible = "regulator-fixed";
>> >> > +               regulator-name = "vcc_sd";
>> >> > +               gpio = <&gpio3 11 GPIO_ACTIVE_LOW>;
>> >> > +               regulator-min-microvolt = <3300000>;
>> >> > +               regulator-max-microvolt = <3300000>;
>> >> > +               regulator-always-on;
>> >> > +               regulator-boot-on;
>> >> > +               vin-supply = <&vcc_io>;
>> >> > +       };
>> >> > +
>> >> > +       vcc_io: vcc-io-regulator {
>> >> > +               compatible = "regulator-fixed";
>> >> > +               regulator-name = "vcc_io";
>> >> > +               regulator-min-microvolt = <3300000>;
>> >> > +               regulator-max-microvolt = <3300000>;
>> >> > +               regulator-always-on;
>> >> > +               regulator-boot-on;
>> >> > +               vin-supply = <&vcc_sys>;
>> >> > +       };
>> >> > +
>> >> > +       vcc_lan: vcc-lan-regulator {
>> >> > +               compatible = "regulator-fixed";
>> >> > +               regulator-name = "vcc_lan";
>> >> > +               regulator-min-microvolt = <3300000>;
>> >> > +               regulator-max-microvolt = <3300000>;
>> >> > +               regulator-always-on;
>> >> > +               regulator-boot-on;
>> >> > +               vin-supply = <&vcc_io>;
>> >> > +       };
>> >> > +
>> >> > +       vcc_sys: vcc-sys-regulator {
>> >> > +               compatible = "regulator-fixed";
>> >> > +               regulator-name = "vcc_sys";
>> >> > +               regulator-min-microvolt = <5000000>;
>> >> > +               regulator-max-microvolt = <5000000>;
>> >> > +               regulator-always-on;
>> >> > +               regulator-boot-on;
>> >> > +       };
>> >> > +
>> >> > +       vccio_wl: vccio-wl-regulator {
>> >> > +               compatible = "regulator-fixed";
>> >> > +               regulator-name = "vccio_wl";
>> >> > +               regulator-min-microvolt = <3300000>;
>> >> > +               regulator-max-microvolt = <3300000>;
>> >> > +               regulator-always-on;
>> >> > +               regulator-boot-on;
>> >> > +               vin-supply = <&vcc_io>;
>> >> > +       };
>> >> > +
>> >> > +       vdd_10: vdd-10-regulator {
>> >> > +               compatible = "regulator-fixed";
>> >> > +               regulator-name = "vdd_10";
>> >> > +               regulator-min-microvolt = <1000000>;
>> >> > +               regulator-max-microvolt = <1000000>;
>> >> > +               regulator-always-on;
>> >> > +               regulator-boot-on;
>> >> > +               vin-supply = <&vcc_sys>;
>> >> > +       };
>> >> > +};
>> >>
>> >> There is only one regulator that is not marked as always-on. This will
>> >> prevent the regulator subsystem to disable unused regulators. Do you
>> >> really need all of them to be always-on?
>> >
>> > I do believe the regulators should be more or less correct. Sadly there
>> > are no real schematics for that specific device available, but the
>> > reference design for rk3368-based tv-boxes uses a hirarchy of
>> > hard-wired individual regulators without any ability to control for
>> > most of them [aka always on].
>> >
>> > And due to the r68 not sporting any different real pmic, I'd believe the
>> > Orion will most likely also follow that scheme.
>>
>> I see, thanks a lot for the clarification. But then in that case I
>> wonder what's the point of defining the gpio property in the fixed
>> regulator device nodes?
>
> the ones with gpio properties actually can be turned off - here they are the
> different supplies for usb and sd-card only, so leaf-regulators without
> further children.
>

Ok, sorry I misunderstood and thought you were referring to all
regulators. I of course agree that regulators that can't be turned of
should be marked as always-on. I just asked this to be revisit and
only mark as always-on the ones that really should be.

>
> Heiko

Best regards,
Javier