From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <40159140.kOHBSj8v8b@phil> References: <1484791919-4665-1-git-send-email-eddie.cai@rock-chips.com> <1484791919-4665-3-git-send-email-eddie.cai@rock-chips.com> <40159140.kOHBSj8v8b@phil> From: Eddie Cai Date: Fri, 20 Jan 2017 14:42:53 +0800 Message-ID: Subject: Re: [PATCH 2/2] ARM: dts: rockchip: add dts for RK3288-Tinker board Content-Type: multipart/alternative; boundary=001a114ac30e995c1a054680f599 To: Heiko Stuebner Cc: robh+dt@kernel.org, mark.rutland@arm.com, linux@armlinux.org.uk, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org, Eddie Cai List-ID: --001a114ac30e995c1a054680f599 Content-Type: text/plain; charset=UTF-8 2017-01-19 17:58 GMT+08:00 Heiko Stuebner : > Hi Eddie, > > Am Donnerstag, 19. Januar 2017, 10:11:59 CET schrieb Eddie Cai: > > This patch add basic support for RK3288-Tinker board. We can boot in to > > rootfs with this patch. > > > > Signed-off-by: Eddie Cai > > looks good in general, just some small question down below. > > [...] > > > + /* > > + * NOTE: vcc_sd isn't hooked up on v1.0 boards where power comes > from > > + * vcc_io directly. Those boards won't be able to power cycle SD > cards > > + * but it shouldn't hurt to toggle this pin there anyway. > > + */ > > just to clarify, later board will have that pin connected, right? > Copy from rk3288-evb.dtsi. forgot to delete it. I will remove it next version > > > + vcc_sd: sdmmc-regulator { > > + compatible = "regulator-fixed"; > > + gpio = <&gpio7 11 GPIO_ACTIVE_LOW>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&sdmmc_pwr>; > > + regulator-name = "vcc_sd"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + startup-delay-us = <100000>; > > + vin-supply = <&vcc_io>; > > + }; > > +}; > > [...] > > > +&hdmi { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + #sound-dai-cells = <0>; > > + ddc-i2c-bus = <&i2c5>; > > + status = "okay"; > > + /* Don't use vopl for HDMI */ > > + ports { > > + hdmi_in: port { > > + /delete-node/ endpoint@1; > > + }; > > what is the reason for this? You enable both VOPs below and the linux > display > subsystem should be able to select an appropriate VOP for output just fine > on > its own. So there should be no reason for capping the hdmi's connection to > one > of the vops. > The VOP big support 4k display. is designed for HDMI 4K display. VOP little is for other display(eDP, LVDS, Mipi etc) > > > + }; > > +}; > > [...] > > > +&usb_host0_ehci { > > + no-relinquish-port; > > This seems like an unused/undocumented property > I will remove it next version > > > + status = "okay"; > > +}; > > [...] > > > +&vopl { > > + status = "okay"; > > + /* Don't use vopl for HDMI */ > > + vopl_out: port { > > + /delete-node/ endpoint@0; > > + }; > > see comment at the hdmi node > > > +}; > > > Thanks > Heiko > --001a114ac30e995c1a054680f599 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


2017-01-19 17:58 GMT+08:00 Heiko Stuebner <heiko@sntech.de>:
Hi Eddie,

Am Donnerstag, 19. Januar 2017, 10:11:59 CET schrieb Eddie Cai:
> This patch add basic support for RK3288-Tinker board. We can boot in t= o
> rootfs with this patch.
>
> Signed-off-by: Eddie Cai <eddie.cai@rock-chips.com>

looks good in general, just some small question down below.

[...]

> +=C2=A0 =C2=A0 =C2=A0/*
> +=C2=A0 =C2=A0 =C2=A0 * NOTE: vcc_sd isn't hooked up on v1.0 board= s where power comes from
> +=C2=A0 =C2=A0 =C2=A0 * vcc_io directly.=C2=A0 Those boards won't = be able to power cycle SD cards
> +=C2=A0 =C2=A0 =C2=A0 * but it shouldn't hurt to toggle this pin t= here anyway.
> +=C2=A0 =C2=A0 =C2=A0 */

just to clarify, later board will have that pin connected, right?
Copy from rk3288-evb.dtsi. forgot to delete it. I will r= emove it next version =C2=A0=C2=A0

> +=C2=A0 =C2=A0 =C2=A0vcc_sd: sdmmc-regulator {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0compatible =3D "= regulator-fixed";
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0gpio =3D <&gpi= o7 11 GPIO_ACTIVE_LOW>;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pinctrl-names =3D &qu= ot;default";
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pinctrl-0 =3D <&am= p;sdmmc_pwr>;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0regulator-name =3D &q= uot;vcc_sd";
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0regulator-min-microvo= lt =3D <3300000>;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0regulator-max-microvo= lt =3D <3300000>;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0startup-delay-us =3D = <100000>;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0vin-supply =3D <&a= mp;vcc_io>;
> +=C2=A0 =C2=A0 =C2=A0};
> +};

[...]

> +&hdmi {
> +=C2=A0 =C2=A0 =C2=A0#address-cells =3D <1>;
> +=C2=A0 =C2=A0 =C2=A0#size-cells =3D <0>;
> +=C2=A0 =C2=A0 =C2=A0#sound-dai-cells =3D <0>;
> +=C2=A0 =C2=A0 =C2=A0ddc-i2c-bus =3D <&i2c5>;
> +=C2=A0 =C2=A0 =C2=A0status =3D "okay";
> +=C2=A0 =C2=A0 =C2=A0/* Don't use vopl for HDMI */
> +=C2=A0 =C2=A0 =C2=A0ports {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0hdmi_in: port {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0/delete-node/ endpoint@1;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0};

what is the reason for this? You enable both VOPs below and the linu= x display
subsystem should be able to select an appropriate VOP for output just fine = on
its own. So there should be no reason for capping the hdmi's connection= to one
of the vops.
The VOP big support 4k display. is design= ed for HDMI =C2=A04K display. VOP little is for other display(eDP, LVDS, Mi= pi etc)

> +=C2=A0 =C2=A0 =C2=A0};
> +};

[...]

> +&usb_host0_ehci {
> +=C2=A0 =C2=A0 =C2=A0no-relinquish-port;

This seems like an unused/undocumented property
I will= remove it next version=C2=A0

> +=C2=A0 =C2=A0 =C2=A0status =3D "okay";
> +};

[...]

> +&vopl {
> +=C2=A0 =C2=A0 =C2=A0status =3D "okay";
> +=C2=A0 =C2=A0 =C2=A0/* Don't use vopl for HDMI */
> +=C2=A0 =C2=A0 =C2=A0vopl_out: port {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/delete-node/ endpoin= t@0;
> +=C2=A0 =C2=A0 =C2=A0};

see comment at the hdmi node

> +};


Thanks
Heiko

--001a114ac30e995c1a054680f599--