2014-07-18 17:21:49

by Andreas Färber

[permalink] [raw]
Subject: [PATCH v2 3/4] ARM: dts: exynos5250: Fold common ChromeOS parts into Snow

The remaining pieces are fairly minor.

Suggested-by: Doug Anderson <[email protected]>
Signed-off-by: Andreas Färber <[email protected]>
---
v2: New (Doug Anderson)

arch/arm/boot/dts/exynos5250-cros-common.dtsi | 164 -------------------------
arch/arm/boot/dts/exynos5250-snow.dts | 169 ++++++++++++++++++++++----
2 files changed, 148 insertions(+), 185 deletions(-)
delete mode 100644 arch/arm/boot/dts/exynos5250-cros-common.dtsi

diff --git a/arch/arm/boot/dts/exynos5250-cros-common.dtsi b/arch/arm/boot/dts/exynos5250-cros-common.dtsi
deleted file mode 100644
index e603e9c..0000000
--- a/arch/arm/boot/dts/exynos5250-cros-common.dtsi
+++ /dev/null
@@ -1,164 +0,0 @@
-/*
- * Common device tree include for all Exynos 5250 boards based off of Daisy.
- *
- * Copyright (c) 2012 Google, Inc
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
-*/
-
-/ {
- aliases {
- };
-
- memory {
- reg = <0x40000000 0x80000000>;
- };
-
- chosen {
- };
-
- pinctrl@11400000 {
- /*
- * Disabled pullups since external part has its own pullups and
- * double-pulling gets us out of spec in some cases.
- */
- i2c2_bus: i2c2-bus {
- samsung,pin-pud = <0>;
- };
- };
-
- i2c@12C60000 {
- status = "okay";
- samsung,i2c-sda-delay = <100>;
- samsung,i2c-max-bus-freq = <378000>;
- };
-
- i2c@12C70000 {
- status = "okay";
- samsung,i2c-sda-delay = <100>;
- samsung,i2c-max-bus-freq = <378000>;
- };
-
- i2c@12C80000 {
- status = "okay";
- samsung,i2c-sda-delay = <100>;
- samsung,i2c-max-bus-freq = <66000>;
-
- hdmiddc@50 {
- compatible = "samsung,exynos4210-hdmiddc";
- reg = <0x50>;
- };
- };
-
- i2c@12C90000 {
- status = "okay";
- samsung,i2c-sda-delay = <100>;
- samsung,i2c-max-bus-freq = <66000>;
- };
-
- i2c@12CA0000 {
- status = "okay";
- samsung,i2c-sda-delay = <100>;
- samsung,i2c-max-bus-freq = <66000>;
- };
-
- i2c@12CB0000 {
- status = "okay";
- samsung,i2c-sda-delay = <100>;
- samsung,i2c-max-bus-freq = <66000>;
- };
-
- i2c@12CD0000 {
- status = "okay";
- samsung,i2c-sda-delay = <100>;
- samsung,i2c-max-bus-freq = <66000>;
- };
-
- i2c@12CE0000 {
- status = "okay";
- samsung,i2c-sda-delay = <100>;
- samsung,i2c-max-bus-freq = <378000>;
-
- hdmiphy: hdmiphy@38 {
- compatible = "samsung,exynos4212-hdmiphy";
- reg = <0x38>;
- };
- };
-
- mmc@12200000 {
- num-slots = <1>;
- supports-highspeed;
- broken-cd;
- card-detect-delay = <200>;
- samsung,dw-mshc-ciu-div = <3>;
- samsung,dw-mshc-sdr-timing = <2 3>;
- samsung,dw-mshc-ddr-timing = <1 2>;
- pinctrl-names = "default";
- pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_cd &sd0_bus4 &sd0_bus8>;
-
- slot@0 {
- reg = <0>;
- bus-width = <8>;
- };
- };
-
- mmc@12220000 {
- num-slots = <1>;
- supports-highspeed;
- card-detect-delay = <200>;
- samsung,dw-mshc-ciu-div = <3>;
- samsung,dw-mshc-sdr-timing = <2 3>;
- samsung,dw-mshc-ddr-timing = <1 2>;
- pinctrl-names = "default";
- pinctrl-0 = <&sd2_clk &sd2_cmd &sd2_cd &sd2_bus4>;
-
- slot@0 {
- reg = <0>;
- bus-width = <4>;
- wp-gpios = <&gpc2 1 0>;
- };
- };
-
- mmc@12230000 {
- num-slots = <1>;
- supports-highspeed;
- broken-cd;
- card-detect-delay = <200>;
- samsung,dw-mshc-ciu-div = <3>;
- samsung,dw-mshc-sdr-timing = <2 3>;
- samsung,dw-mshc-ddr-timing = <1 2>;
- /* See board-specific dts files for pin setup */
-
- slot@0 {
- reg = <0>;
- bus-width = <4>;
- };
- };
-
- spi_1: spi@12d30000 {
- status = "okay";
- samsung,spi-src-clk = <0>;
- num-cs = <1>;
- };
-
- hdmi {
- hpd-gpio = <&gpx3 7 0>;
- pinctrl-names = "default";
- pinctrl-0 = <&hdmi_hpd_irq>;
- phy = <&hdmiphy>;
- ddc = <&i2c_2>;
- };
-
- gpio-keys {
- compatible = "gpio-keys";
-
- power {
- label = "Power";
- gpios = <&gpx1 3 1>;
- linux,code = <116>; /* KEY_POWER */
- gpio-key,wakeup;
- };
- };
-};
diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
index f2b8c41..cdf74c5 100644
--- a/arch/arm/boot/dts/exynos5250-snow.dts
+++ b/arch/arm/boot/dts/exynos5250-snow.dts
@@ -10,7 +10,6 @@

/dts-v1/;
#include "exynos5250.dtsi"
-#include "exynos5250-cros-common.dtsi"

/ {
model = "Google Snow";
@@ -20,6 +19,13 @@
i2c104 = &i2c_104;
};

+ memory {
+ reg = <0x40000000 0x80000000>;
+ };
+
+ chosen {
+ };
+
rtc@101E0000 {
status = "okay";
};
@@ -93,6 +99,13 @@
gpio-keys {
compatible = "gpio-keys";

+ power {
+ label = "Power";
+ gpios = <&gpx1 3 1>;
+ linux,code = <116>; /* KEY_POWER */
+ gpio-key,wakeup;
+ };
+
lid-switch {
label = "Lid";
gpios = <&gpx3 5 1>;
@@ -226,26 +239,6 @@
};
};

- mmc@12200000 {
- status = "okay";
- };
-
- mmc@12220000 {
- status = "okay";
- };
-
- /*
- * On Snow we've got SIP WiFi and so can keep drive strengths low to
- * reduce EMI.
- */
- mmc@12230000 {
- status = "okay";
- slot@0 {
- pinctrl-names = "default";
- pinctrl-0 = <&sd3_clk &sd3_cmd &sd3_bus4>;
- };
- };
-
i2c@12CD0000 {
max98095: codec@11 {
compatible = "maxim,max98095";
@@ -314,6 +307,14 @@
samsung,invert-vclk;
};

+ hdmi {
+ hpd-gpio = <&gpx3 7 0>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&hdmi_hpd_irq>;
+ phy = <&hdmiphy>;
+ ddc = <&i2c_2>;
+ };
+
dp-controller@145B0000 {
status = "okay";
pinctrl-names = "default";
@@ -345,6 +346,10 @@
};

&i2c_0 {
+ status = "okay";
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <378000>;
+
max77686@09 {
compatible = "maxim,max77686";
interrupt-parent = <&gpx3>;
@@ -491,6 +496,10 @@
};

&i2c_1 {
+ status = "okay";
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <378000>;
+
trackpad {
reg = <0x67>;
compatible = "cypress,cyapa";
@@ -500,7 +509,119 @@
};
};

+&i2c_2 {
+ status = "okay";
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <66000>;
+
+ hdmiddc@50 {
+ compatible = "samsung,exynos4210-hdmiddc";
+ reg = <0x50>;
+ };
+};
+
+&i2c_3 {
+ status = "okay";
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <66000>;
+};
+
+&i2c_4 {
+ status = "okay";
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <66000>;
+};
+
+&i2c_5 {
+ status = "okay";
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <66000>;
+};
+
+&i2c_7 {
+ status = "okay";
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <66000>;
+};
+
+&i2c_8 {
+ status = "okay";
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <378000>;
+
+ hdmiphy: hdmiphy@38 {
+ compatible = "samsung,exynos4212-hdmiphy";
+ reg = <0x38>;
+ };
+};
+
+&mmc_0 {
+ status = "okay";
+ num-slots = <1>;
+ supports-highspeed;
+ broken-cd;
+ card-detect-delay = <200>;
+ samsung,dw-mshc-ciu-div = <3>;
+ samsung,dw-mshc-sdr-timing = <2 3>;
+ samsung,dw-mshc-ddr-timing = <1 2>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_cd &sd0_bus4 &sd0_bus8>;
+
+ slot@0 {
+ reg = <0>;
+ bus-width = <8>;
+ };
+};
+
+&mmc_2 {
+ status = "okay";
+ num-slots = <1>;
+ supports-highspeed;
+ card-detect-delay = <200>;
+ samsung,dw-mshc-ciu-div = <3>;
+ samsung,dw-mshc-sdr-timing = <2 3>;
+ samsung,dw-mshc-ddr-timing = <1 2>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&sd2_clk &sd2_cmd &sd2_cd &sd2_bus4>;
+
+ slot@0 {
+ reg = <0>;
+ bus-width = <4>;
+ wp-gpios = <&gpc2 1 0>;
+ };
+};
+
+/*
+ * On Snow we've got SIP WiFi and so can keep drive strengths low to
+ * reduce EMI.
+ */
+&mmc_3 {
+ status = "okay";
+ num-slots = <1>;
+ supports-highspeed;
+ broken-cd;
+ card-detect-delay = <200>;
+ samsung,dw-mshc-ciu-div = <3>;
+ samsung,dw-mshc-sdr-timing = <2 3>;
+ samsung,dw-mshc-ddr-timing = <1 2>;
+
+ slot@0 {
+ reg = <0>;
+ bus-width = <4>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&sd3_clk &sd3_cmd &sd3_bus4>;
+ };
+};
+
&pinctrl_0 {
+ /*
+ * Disabled pullups since external part has its own pullups and
+ * double-pulling gets us out of spec in some cases.
+ */
+ i2c2_bus: i2c2-bus {
+ samsung,pin-pud = <0>;
+ };
+
max77686_irq: max77686-irq {
samsung,pins = "gpx3-2";
samsung,pin-function = <0>;
@@ -509,4 +630,10 @@
};
};

+&spi_1 {
+ status = "okay";
+ samsung,spi-src-clk = <0>;
+ num-cs = <1>;
+};
+
#include "cros-ec-keyboard.dtsi"
--
1.9.3


2014-07-25 16:02:59

by Doug Anderson

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] ARM: dts: exynos5250: Fold common ChromeOS parts into Snow

Andreas,

On Fri, Jul 18, 2014 at 10:20 AM, Andreas Färber <[email protected]> wrote:
> The remaining pieces are fairly minor.
>
> Suggested-by: Doug Anderson <[email protected]>
> Signed-off-by: Andreas Färber <[email protected]>
> ---
> v2: New (Doug Anderson)
>
> arch/arm/boot/dts/exynos5250-cros-common.dtsi | 164 -------------------------
> arch/arm/boot/dts/exynos5250-snow.dts | 169 ++++++++++++++++++++++----
> 2 files changed, 148 insertions(+), 185 deletions(-)
> delete mode 100644 arch/arm/boot/dts/exynos5250-cros-common.dtsi
>
> diff --git a/arch/arm/boot/dts/exynos5250-cros-common.dtsi b/arch/arm/boot/dts/exynos5250-cros-common.dtsi
> deleted file mode 100644
> index e603e9c..0000000
> --- a/arch/arm/boot/dts/exynos5250-cros-common.dtsi
> +++ /dev/null
> @@ -1,164 +0,0 @@
> -/*
> - * Common device tree include for all Exynos 5250 boards based off of Daisy.
> - *
> - * Copyright (c) 2012 Google, Inc
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> -*/
> -
> -/ {
> - aliases {
> - };
> -
> - memory {
> - reg = <0x40000000 0x80000000>;
> - };
> -
> - chosen {
> - };
> -
> - pinctrl@11400000 {
> - /*
> - * Disabled pullups since external part has its own pullups and
> - * double-pulling gets us out of spec in some cases.
> - */
> - i2c2_bus: i2c2-bus {
> - samsung,pin-pud = <0>;
> - };
> - };
> -
> - i2c@12C60000 {
> - status = "okay";
> - samsung,i2c-sda-delay = <100>;
> - samsung,i2c-max-bus-freq = <378000>;
> - };
> -
> - i2c@12C70000 {
> - status = "okay";
> - samsung,i2c-sda-delay = <100>;
> - samsung,i2c-max-bus-freq = <378000>;
> - };
> -
> - i2c@12C80000 {
> - status = "okay";
> - samsung,i2c-sda-delay = <100>;
> - samsung,i2c-max-bus-freq = <66000>;
> -
> - hdmiddc@50 {
> - compatible = "samsung,exynos4210-hdmiddc";
> - reg = <0x50>;
> - };
> - };
> -
> - i2c@12C90000 {
> - status = "okay";
> - samsung,i2c-sda-delay = <100>;
> - samsung,i2c-max-bus-freq = <66000>;
> - };
> -
> - i2c@12CA0000 {
> - status = "okay";
> - samsung,i2c-sda-delay = <100>;
> - samsung,i2c-max-bus-freq = <66000>;
> - };
> -
> - i2c@12CB0000 {
> - status = "okay";
> - samsung,i2c-sda-delay = <100>;
> - samsung,i2c-max-bus-freq = <66000>;
> - };
> -
> - i2c@12CD0000 {
> - status = "okay";
> - samsung,i2c-sda-delay = <100>;
> - samsung,i2c-max-bus-freq = <66000>;
> - };
> -
> - i2c@12CE0000 {
> - status = "okay";
> - samsung,i2c-sda-delay = <100>;
> - samsung,i2c-max-bus-freq = <378000>;
> -
> - hdmiphy: hdmiphy@38 {
> - compatible = "samsung,exynos4212-hdmiphy";
> - reg = <0x38>;
> - };
> - };
> -
> - mmc@12200000 {
> - num-slots = <1>;
> - supports-highspeed;
> - broken-cd;
> - card-detect-delay = <200>;
> - samsung,dw-mshc-ciu-div = <3>;
> - samsung,dw-mshc-sdr-timing = <2 3>;
> - samsung,dw-mshc-ddr-timing = <1 2>;
> - pinctrl-names = "default";
> - pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_cd &sd0_bus4 &sd0_bus8>;
> -
> - slot@0 {
> - reg = <0>;
> - bus-width = <8>;
> - };
> - };
> -
> - mmc@12220000 {
> - num-slots = <1>;
> - supports-highspeed;
> - card-detect-delay = <200>;
> - samsung,dw-mshc-ciu-div = <3>;
> - samsung,dw-mshc-sdr-timing = <2 3>;
> - samsung,dw-mshc-ddr-timing = <1 2>;
> - pinctrl-names = "default";
> - pinctrl-0 = <&sd2_clk &sd2_cmd &sd2_cd &sd2_bus4>;
> -
> - slot@0 {
> - reg = <0>;
> - bus-width = <4>;
> - wp-gpios = <&gpc2 1 0>;
> - };
> - };
> -
> - mmc@12230000 {
> - num-slots = <1>;
> - supports-highspeed;
> - broken-cd;
> - card-detect-delay = <200>;
> - samsung,dw-mshc-ciu-div = <3>;
> - samsung,dw-mshc-sdr-timing = <2 3>;
> - samsung,dw-mshc-ddr-timing = <1 2>;
> - /* See board-specific dts files for pin setup */
> -
> - slot@0 {
> - reg = <0>;
> - bus-width = <4>;
> - };
> - };
> -
> - spi_1: spi@12d30000 {
> - status = "okay";
> - samsung,spi-src-clk = <0>;
> - num-cs = <1>;
> - };
> -
> - hdmi {
> - hpd-gpio = <&gpx3 7 0>;
> - pinctrl-names = "default";
> - pinctrl-0 = <&hdmi_hpd_irq>;
> - phy = <&hdmiphy>;
> - ddc = <&i2c_2>;
> - };
> -
> - gpio-keys {
> - compatible = "gpio-keys";
> -
> - power {
> - label = "Power";
> - gpios = <&gpx1 3 1>;
> - linux,code = <116>; /* KEY_POWER */
> - gpio-key,wakeup;
> - };
> - };
> -};
> diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
> index f2b8c41..cdf74c5 100644
> --- a/arch/arm/boot/dts/exynos5250-snow.dts
> +++ b/arch/arm/boot/dts/exynos5250-snow.dts
> @@ -10,7 +10,6 @@
>
> /dts-v1/;
> #include "exynos5250.dtsi"
> -#include "exynos5250-cros-common.dtsi"
>
> / {
> model = "Google Snow";
> @@ -20,6 +19,13 @@
> i2c104 = &i2c_104;
> };
>
> + memory {
> + reg = <0x40000000 0x80000000>;
> + };
> +
> + chosen {
> + };
> +
> rtc@101E0000 {
> status = "okay";
> };
> @@ -93,6 +99,13 @@
> gpio-keys {
> compatible = "gpio-keys";
>
> + power {
> + label = "Power";
> + gpios = <&gpx1 3 1>;
> + linux,code = <116>; /* KEY_POWER */
> + gpio-key,wakeup;
> + };
> +
> lid-switch {
> label = "Lid";
> gpios = <&gpx3 5 1>;
> @@ -226,26 +239,6 @@
> };
> };
>
> - mmc@12200000 {
> - status = "okay";
> - };
> -
> - mmc@12220000 {
> - status = "okay";
> - };
> -
> - /*
> - * On Snow we've got SIP WiFi and so can keep drive strengths low to
> - * reduce EMI.
> - */
> - mmc@12230000 {
> - status = "okay";
> - slot@0 {
> - pinctrl-names = "default";
> - pinctrl-0 = <&sd3_clk &sd3_cmd &sd3_bus4>;
> - };
> - };
> -
> i2c@12CD0000 {
> max98095: codec@11 {
> compatible = "maxim,max98095";
> @@ -314,6 +307,14 @@
> samsung,invert-vclk;
> };
>
> + hdmi {
> + hpd-gpio = <&gpx3 7 0>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&hdmi_hpd_irq>;
> + phy = <&hdmiphy>;
> + ddc = <&i2c_2>;
> + };
> +
> dp-controller@145B0000 {
> status = "okay";
> pinctrl-names = "default";
> @@ -345,6 +346,10 @@
> };
>
> &i2c_0 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <378000>;
> +
> max77686@09 {
> compatible = "maxim,max77686";
> interrupt-parent = <&gpx3>;
> @@ -491,6 +496,10 @@
> };
>
> &i2c_1 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <378000>;
> +
> trackpad {
> reg = <0x67>;
> compatible = "cypress,cyapa";
> @@ -500,7 +509,119 @@
> };
> };
>
> +&i2c_2 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <66000>;
> +
> + hdmiddc@50 {
> + compatible = "samsung,exynos4210-hdmiddc";
> + reg = <0x50>;
> + };
> +};
> +
> +&i2c_3 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <66000>;
> +};
> +
> +&i2c_4 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <66000>;
> +};
> +
> +&i2c_5 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <66000>;
> +};
> +
> +&i2c_7 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <66000>;
> +};
> +
> +&i2c_8 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <378000>;
> +
> + hdmiphy: hdmiphy@38 {
> + compatible = "samsung,exynos4212-hdmiphy";
> + reg = <0x38>;
> + };
> +};
> +
> +&mmc_0 {
> + status = "okay";
> + num-slots = <1>;
> + supports-highspeed;
> + broken-cd;
> + card-detect-delay = <200>;
> + samsung,dw-mshc-ciu-div = <3>;
> + samsung,dw-mshc-sdr-timing = <2 3>;
> + samsung,dw-mshc-ddr-timing = <1 2>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_cd &sd0_bus4 &sd0_bus8>;
> +
> + slot@0 {
> + reg = <0>;
> + bus-width = <8>;
> + };
> +};
> +
> +&mmc_2 {
> + status = "okay";
> + num-slots = <1>;
> + supports-highspeed;
> + card-detect-delay = <200>;
> + samsung,dw-mshc-ciu-div = <3>;
> + samsung,dw-mshc-sdr-timing = <2 3>;
> + samsung,dw-mshc-ddr-timing = <1 2>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&sd2_clk &sd2_cmd &sd2_cd &sd2_bus4>;
> +
> + slot@0 {
> + reg = <0>;
> + bus-width = <4>;
> + wp-gpios = <&gpc2 1 0>;
> + };
> +};
> +
> +/*
> + * On Snow we've got SIP WiFi and so can keep drive strengths low to
> + * reduce EMI.
> + */
> +&mmc_3 {
> + status = "okay";
> + num-slots = <1>;
> + supports-highspeed;
> + broken-cd;
> + card-detect-delay = <200>;
> + samsung,dw-mshc-ciu-div = <3>;
> + samsung,dw-mshc-sdr-timing = <2 3>;
> + samsung,dw-mshc-ddr-timing = <1 2>;
> +
> + slot@0 {
> + reg = <0>;
> + bus-width = <4>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&sd3_clk &sd3_cmd &sd3_bus4>;

This actually looks like a bug on snow. I don't think these pinctrl
statements are taking effect and I think they belong at the mmc level,
not the slot level. That's not a new bug introduced by you, though.
Mind fixing it? You've already got it right for Spring mmc_1.


> + };
> +};
> +
> &pinctrl_0 {
> + /*
> + * Disabled pullups since external part has its own pullups and
> + * double-pulling gets us out of spec in some cases.
> + */
> + i2c2_bus: i2c2-bus {
> + samsung,pin-pud = <0>;
> + };
> +
> max77686_irq: max77686-irq {
> samsung,pins = "gpx3-2";
> samsung,pin-function = <0>;
> @@ -509,4 +630,10 @@
> };
> };
>
> +&spi_1 {
> + status = "okay";
> + samsung,spi-src-clk = <0>;
> + num-cs = <1>;
> +};
> +
> #include "cros-ec-keyboard.dtsi"
> --
> 1.9.3
>

2014-07-25 16:35:46

by Javier Martinez Canillas

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] ARM: dts: exynos5250: Fold common ChromeOS parts into Snow

Hello Andreas,

On Fri, Jul 18, 2014 at 7:20 PM, Andreas Färber <[email protected]> wrote:
>
> + memory {
> + reg = <0x40000000 0x80000000>;
> + };
> +
> + chosen {
> + };
> +

Is there a reason for an empty chosen node? Same for the Spring DTS.

Best regards,
Javier

2014-07-25 16:43:52

by Doug Anderson

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] ARM: dts: exynos5250: Fold common ChromeOS parts into Snow

Javier,

On Fri, Jul 25, 2014 at 9:35 AM, Javier Martinez Canillas
<[email protected]> wrote:
> Hello Andreas,
>
> On Fri, Jul 18, 2014 at 7:20 PM, Andreas Färber <[email protected]> wrote:
>>
>> + memory {
>> + reg = <0x40000000 0x80000000>;
>> + };
>> +
>> + chosen {
>> + };
>> +
>
> Is there a reason for an empty chosen node? Same for the Spring DTS.

I know that the bootloader (U-Boot) fills this in. I'd have to double
check how the bootloader handles the node not being there to begin
with.

2014-07-25 17:02:37

by Javier Martinez Canillas

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] ARM: dts: exynos5250: Fold common ChromeOS parts into Snow

Hello Doug,

On Fri, Jul 25, 2014 at 6:43 PM, Doug Anderson <[email protected]> wrote:
> Javier,
>
> On Fri, Jul 25, 2014 at 9:35 AM, Javier Martinez Canillas
> <[email protected]> wrote:
>> Hello Andreas,
>>
>> On Fri, Jul 18, 2014 at 7:20 PM, Andreas Färber <[email protected]> wrote:
>>>
>>> + memory {
>>> + reg = <0x40000000 0x80000000>;
>>> + };
>>> +
>>> + chosen {
>>> + };
>>> +
>>
>> Is there a reason for an empty chosen node? Same for the Spring DTS.
>
> I know that the bootloader (U-Boot) fills this in. I'd have to double
> check how the bootloader handles the node not being there to begin
> with.

Yes, U-Boot fills this but if the node does not exist in the FDT, it
creates one. Take a look at fdt_chosen() in common/fdt_support.c [0].

So I think it's better to just remove it since is empty.

Best regards,
Javier

[0]: https://chromium.googlesource.com/chromiumos/third_party/u-boot/+/master/common/fdt_support.c#215

2014-07-29 12:45:43

by Andreas Färber

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] ARM: dts: exynos5250: Fold common ChromeOS parts into Snow

Doug,

Am 25.07.2014 18:02, schrieb Doug Anderson:
> On Fri, Jul 18, 2014 at 10:20 AM, Andreas Färber <[email protected]> wrote:
>> +/*
>> + * On Snow we've got SIP WiFi and so can keep drive strengths low to
>> + * reduce EMI.
>> + */
>> +&mmc_3 {
>> + status = "okay";
>> + num-slots = <1>;
>> + supports-highspeed;
>> + broken-cd;
>> + card-detect-delay = <200>;
>> + samsung,dw-mshc-ciu-div = <3>;
>> + samsung,dw-mshc-sdr-timing = <2 3>;
>> + samsung,dw-mshc-ddr-timing = <1 2>;
>> +
>> + slot@0 {
>> + reg = <0>;
>> + bus-width = <4>;
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&sd3_clk &sd3_cmd &sd3_bus4>;
>
> This actually looks like a bug on snow. I don't think these pinctrl
> statements are taking effect and I think they belong at the mmc level,
> not the slot level. That's not a new bug introduced by you, though.
> Mind fixing it? You've already got it right for Spring mmc_1.

I was taught never to mix code movements with functional changes, as it
hides them even if mentioned in the commit message. Would you like me to
fix it pre- or post-move? Post-move would be easiest for me. ;)

Regards,
Andreas

--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

2014-07-29 13:00:11

by Andreas Färber

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] ARM: dts: exynos5250: Fold common ChromeOS parts into Snow

Hi Javier and Doug,

Am 25.07.2014 19:02, schrieb Javier Martinez Canillas:
> On Fri, Jul 25, 2014 at 6:43 PM, Doug Anderson <[email protected]> wrote:
>> On Fri, Jul 25, 2014 at 9:35 AM, Javier Martinez Canillas
>> <[email protected]> wrote:
>>> Hello Andreas,
>>>
>>> On Fri, Jul 18, 2014 at 7:20 PM, Andreas Färber <[email protected]> wrote:
>>>>
>>>> + memory {
>>>> + reg = <0x40000000 0x80000000>;
>>>> + };
>>>> +
>>>> + chosen {
>>>> + };
>>>> +
>>>
>>> Is there a reason for an empty chosen node? Same for the Spring DTS.
>>
>> I know that the bootloader (U-Boot) fills this in. I'd have to double
>> check how the bootloader handles the node not being there to begin
>> with.
>
> Yes, U-Boot fills this but if the node does not exist in the FDT, it
> creates one. Take a look at fdt_chosen() in common/fdt_support.c [0].
>
> So I think it's better to just remove it since is empty.

Hm, in different context it was stated that device trees shouldn't rely
on bootloader behavior (for a /memory node on Zynq we ended up
specifying the size cell with the real value despite U-Boot overriding
it to a lesser value).

Can we instead settle on filling this node with defaults? :)
bootargs = "console=tty1"; would be my choice for Spring. Would that be
applicable for Snow as well?
Not sure how to specify that via linux,stdout-path property.

I believe this would be a follow-up patch either way.

Since there's at least two series out there trying to fiddle with
-cros-common.dtsi, including one that drops the slot sub-node of the MMC
and one that adds CPU power supply, it would be nice if we can get the
-cros-common parts sorted out and applied. Can you ack/review 1-2?
Should I squash them in a v3?

Thanks,
Andreas

--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

2014-07-29 14:39:24

by Javier Martinez Canillas

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] ARM: dts: exynos5250: Fold common ChromeOS parts into Snow

Hello Andreas,

On Tue, Jul 29, 2014 at 3:00 PM, Andreas Färber <[email protected]> wrote:
> Hi Javier and Doug,
>
>
> Hm, in different context it was stated that device trees shouldn't rely
> on bootloader behavior (for a /memory node on Zynq we ended up
> specifying the size cell with the real value despite U-Boot overriding
> it to a lesser value).
>

Well is not strictly the same, the memory node is a hardware
description while the chosen node is one of those exceptional nodes
that don't represent a real device. Also the usage for the memory node
is marked as required in ePAR while the later is marked as optional.

> Can we instead settle on filling this node with defaults? :)
> bootargs = "console=tty1"; would be my choice for Spring. Would that be
> applicable for Snow as well?
> Not sure how to specify that via linux,stdout-path property.
>
> I believe this would be a follow-up patch either way.
>
> Since there's at least two series out there trying to fiddle with
> -cros-common.dtsi, including one that drops the slot sub-node of the MMC
> and one that adds CPU power supply, it would be nice if we can get the
> -cros-common parts sorted out and applied. Can you ack/review 1-2?
> Should I squash them in a v3?
>

A sensible default makes sense to me and yes it should be a follow-up
patch anyways.

> Thanks,
> Andreas
>
> --

Best regards,
Javier

2014-07-29 15:12:04

by Doug Anderson

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] ARM: dts: exynos5250: Fold common ChromeOS parts into Snow

Andreas,

On Tue, Jul 29, 2014 at 5:45 AM, Andreas Färber <[email protected]> wrote:
> Doug,
>
> Am 25.07.2014 18:02, schrieb Doug Anderson:
>> On Fri, Jul 18, 2014 at 10:20 AM, Andreas Färber <[email protected]> wrote:
>>> +/*
>>> + * On Snow we've got SIP WiFi and so can keep drive strengths low to
>>> + * reduce EMI.
>>> + */
>>> +&mmc_3 {
>>> + status = "okay";
>>> + num-slots = <1>;
>>> + supports-highspeed;
>>> + broken-cd;
>>> + card-detect-delay = <200>;
>>> + samsung,dw-mshc-ciu-div = <3>;
>>> + samsung,dw-mshc-sdr-timing = <2 3>;
>>> + samsung,dw-mshc-ddr-timing = <1 2>;
>>> +
>>> + slot@0 {
>>> + reg = <0>;
>>> + bus-width = <4>;
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&sd3_clk &sd3_cmd &sd3_bus4>;
>>
>> This actually looks like a bug on snow. I don't think these pinctrl
>> statements are taking effect and I think they belong at the mmc level,
>> not the slot level. That's not a new bug introduced by you, though.
>> Mind fixing it? You've already got it right for Spring mmc_1.
>
> I was taught never to mix code movements with functional changes, as it
> hides them even if mentioned in the commit message. Would you like me to
> fix it pre- or post-move? Post-move would be easiest for me. ;)

Agreed. Sorry for implying that it belonged as part of this patch.
I'd be happy if it was somewhere in the series, ideally before you
introduce the Spring device tree so diffs are cleaner for it. I don't
care if it's pre-move or post-move.

-Doug

2014-07-29 15:27:18

by Doug Anderson

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] ARM: dts: exynos5250: Fold common ChromeOS parts into Snow

Andreas,

On Tue, Jul 29, 2014 at 6:00 AM, Andreas Färber <[email protected]> wrote:
> Hi Javier and Doug,
>
> Am 25.07.2014 19:02, schrieb Javier Martinez Canillas:
>> On Fri, Jul 25, 2014 at 6:43 PM, Doug Anderson <[email protected]> wrote:
>>> On Fri, Jul 25, 2014 at 9:35 AM, Javier Martinez Canillas
>>> <[email protected]> wrote:
>>>> Hello Andreas,
>>>>
>>>> On Fri, Jul 18, 2014 at 7:20 PM, Andreas Färber <[email protected]> wrote:
>>>>>
>>>>> + memory {
>>>>> + reg = <0x40000000 0x80000000>;
>>>>> + };
>>>>> +
>>>>> + chosen {
>>>>> + };
>>>>> +
>>>>
>>>> Is there a reason for an empty chosen node? Same for the Spring DTS.
>>>
>>> I know that the bootloader (U-Boot) fills this in. I'd have to double
>>> check how the bootloader handles the node not being there to begin
>>> with.
>>
>> Yes, U-Boot fills this but if the node does not exist in the FDT, it
>> creates one. Take a look at fdt_chosen() in common/fdt_support.c [0].
>>
>> So I think it's better to just remove it since is empty.
>
> Hm, in different context it was stated that device trees shouldn't rely
> on bootloader behavior (for a /memory node on Zynq we ended up
> specifying the size cell with the real value despite U-Boot overriding
> it to a lesser value).
>
> Can we instead settle on filling this node with defaults? :)
> bootargs = "console=tty1"; would be my choice for Spring. Would that be
> applicable for Snow as well?

Personally I think that there's not a lot of use for including
bootargs. Every single person will need different bootargs since
everyone will have a different boot source (eMMC partitioin 0, eMMC
partition3, SD card, USB, etc). You have to rely on the bootloader
for _something_.


> Not sure how to specify that via linux,stdout-path property.
>
> I believe this would be a follow-up patch either way.

Right


> Since there's at least two series out there trying to fiddle with
> -cros-common.dtsi, including one that drops the slot sub-node of the MMC
> and one that adds CPU power supply, it would be nice if we can get the
> -cros-common parts sorted out and applied. Can you ack/review 1-2?
> Should I squash them in a v3?

Yeah, it will be a fight to see who gets landed first... :-P

I was thinking you were going to spin the patches and I'd do another
batch of review, but I think I can mark the first two as Reveiwed-by
if it's helpful to you. Let me do one more double-check when I get to
my desk.

-Doug