Re: [RFC 4/4] ARM: dts: exynos5250: Add Spring device tree

From: Doug Anderson
Date: Mon Jun 23 2014 - 15:48:10 EST

Andreas,

Thanks for posting! A first pass on this is below...


On Sun, Jun 22, 2014 at 6:21 PM, Andreas FÃrber <afaerber@xxxxxxx> wrote:
> Adds initial support for the HP Chromebook 11.
>
> Cc: Vincent Palatin <vpalatin@xxxxxxxxxxxx>
> Cc: Doug Anderson <dianders@xxxxxxxxxxxx>
> Cc: Stephan van Schaik <stephan@xxxxxxxxxxxxxx>
> Signed-off-by: Andreas FÃrber <afaerber@xxxxxxx>
> ---
> arch/arm/boot/dts/Makefile | 1 +
> arch/arm/boot/dts/exynos5250-spring.dts | 556 ++++++++++++++++++++++++++++++++
> 2 files changed, 557 insertions(+)
> create mode 100644 arch/arm/boot/dts/exynos5250-spring.dts
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 5986ff6..dc2c5aa 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -74,6 +74,7 @@ dtb-$(CONFIG_ARCH_EXYNOS) += exynos4210-origen.dtb \
> exynos5250-arndale.dtb \
> exynos5250-smdk5250.dtb \
> exynos5250-snow.dtb \
> + exynos5250-spring.dtb \
> exynos5260-xyref5260.dtb \
> exynos5410-smdk5410.dtb \
> exynos5420-arndale-octa.dtb \
> diff --git a/arch/arm/boot/dts/exynos5250-spring.dts b/arch/arm/boot/dts/exynos5250-spring.dts
> new file mode 100644
> index 0000000..e857d44
> --- /dev/null
> +++ b/arch/arm/boot/dts/exynos5250-spring.dts
> @@ -0,0 +1,556 @@
> +/*
> + * Google Spring board device tree source
> + *
> + * Copyright (c) 2013 Google, Inc
> + * Copyright (c) 2014 SUSE LINUX Products GmbH
> + *
> + * 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.
> + */
> +
> +/dts-v1/;
> +#include "exynos5250.dtsi"
> +#include "exynos5250-cros-common.dtsi"

It is possible we may want to backpedal on the use of
"exynos5250-cros-common.dtsi". I know that Olof (now CCed) said he
wasn't a fan of how it turned out.

The original idea was that it should include the arbitrary set of
things that are common between a chunk of Chrome OS boards. As more
boards were introduced things would need to migrate from the "common"
file to the board files.

At the moment the current conventional wisdom is that some duplication
is better than the confusing movement of everything back and forth.
See exynos5420-peach-pit and exynos5800-peach-pi in ToT linux-next.


> +/ {
> + model = "Google Spring";
> + compatible = "google,spring", "samsung,exynos5250", "samsung,exynos5";
> +
> + pinctrl@11400000 {

The new best way to do things is to put this down at the bottom. See
exynos5420-peach-pit as an example:

&pinctrl_0 {
...
}

Note that I believe it was decided that top-level references like that
should be sorted alphabetically.

If you wanted to apply that run to exynos5250-snow I don't think it
would be a terrible idea.


> + s5m8767_dvs: s5m8767-dvs {
> + samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2";
> + samsung,pin-function = <0>;
> + samsung,pin-pud = <1>;
> + samsung,pin-drv = <0>;
> + };
> +
> + s5m8767_ds: s5m8767-ds {
> + samsung,pins = "gpx2-3", "gpx2-4", "gpx2-5";
> + samsung,pin-function = <0>;
> + samsung,pin-pud = <1>;
> + samsung,pin-drv = <0>;
> + };
> +
> + tps65090_irq: tps65090-irq {
> + samsung,pins = "gpx2-6";
> + samsung,pin-function = <0>;
> + samsung,pin-pud = <0>;
> + samsung,pin-drv = <0>;
> + };
> +
> + s5m8767_irq: s5m8767-irq {
> + samsung,pins = "gpx3-2";
> + samsung,pin-function = <0>;
> + samsung,pin-pud = <0>;
> + samsung,pin-drv = <0>;
> + };
> +
> + hdmi_hpd_irq: hdmi-hpd-irq {
> + samsung,pins = "gpx3-7";
> + samsung,pin-function = <0>;
> + samsung,pin-pud = <1>;
> + samsung,pin-drv = <0>;
> + };
> + };
> +
> + pinctrl@13400000 {
> + hsic_reset: hsic-reset {
> + samsung,pins = "gpe1-0";
> + samsung,pin-function = <1>;
> + samsung,pin-pud = <0>;
> + samsung,pin-drv = <0>;
> + };

I'm pretty sure that the HSIC reset needed some funky code to make it
work and I'm not sure what the status of that is upstream


> + };
> +
> + vbat: vbat-fixed-regulator {
> + compatible = "regulator-fixed";
> + regulator-name = "vbat-supply";
> + regulator-boot-on;
> + };
> +
> + usb@12000000 {
> + status = "okay";
> + };
> +
> + usb3_vbus_reg: regulator-usb3 {
> + compatible = "regulator-fixed";
> + regulator-name = "P5.0V_USB3CON";
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + gpio = <&gpe1 0 1>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&hsic_reset>;
> + enable-active-high;
> + };
> +
> + phy@12100000 {
> + vbus-supply = <&usb3_vbus_reg>;
> + };
> +
> + usb@12110000 {
> + samsung,vbus-gpio = <&gpx1 1 0>;
> + status = "okay";
> + };
> +
> + usb@12120000 {
> + status = "okay";
> + };
> +
> + mmc@12220000 {
> + /* MMC2 pins are used as GPIO for eDP bridge control. */
> + status = "disabled";
> + };
> +
> + mmc@12230000 {
> + status = "disabled";
> + };
> +
> + i2c@12C60000 {
> + max77686@09 {

There is no max77686 on spring. It uses s5m8767 in the place of max77686.

...you've got "status = disabled", just remove it.


> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> +
> + rtc {
> + reg = <0x6>;
> + };
> + };
> +
> + s5m8767_pmic@66 {
> + compatible = "samsung,s5m8767-pmic";
> + reg = <0x66>;
> + interrupt-parent = <&gpx3>;
> + interrupts = <2 0>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&s5m8767_irq &s5m8767_dvs &s5m8767_ds>;
> + wakeup-source;
> +
> + s5m8767,pmic-buck-dvs-gpios = <&gpd1 0 1>, /* DVS1 */
> + <&gpd1 1 1>, /* DVS2 */
> + <&gpd1 2 1>; /* DVS3 */
> +
> + s5m8767,pmic-buck-ds-gpios = <&gpx2 3 1>, /* SET1 */
> + <&gpx2 4 1>, /* SET2 */
> + <&gpx2 5 1>; /* SET3 */

The final "1" in each of the GPIO specifiers above should be GPIO_ACTIVE_LOW.


> +
> + /*
> + * The following arrays of DVS voltages are not used, since we are
> + * not using GPIOs to control PMIC bucks, but they must be defined
> + * to please the driver.
> + */
> + s5m8767,pmic-buck2-dvs-voltage = <1350000>, <1300000>,
> + <1250000>, <1200000>,
> + <1150000>, <1100000>,
> + <1000000>, <950000>;
> +
> + s5m8767,pmic-buck3-dvs-voltage = <1100000>, <1100000>,
> + <1100000>, <1100000>,
> + <1000000>, <1000000>,
> + <1000000>, <1000000>;
> +
> + s5m8767,pmic-buck4-dvs-voltage = <1200000>, <1200000>,
> + <1200000>, <1200000>,
> + <1200000>, <1200000>,
> + <1200000>, <1200000>;
> +
> + clocks {
> + compatible = "samsung,s5m8767-clk";
> + #clock-cells = <1>;
> + clock-output-names = "en32khz_ap",
> + "en32khz_cp",
> + "en32khz_bt";
> + };
> +
> + regulators {
> + s5m_ldo4_reg: LDO4 {
> + regulator-name = "P1.0V_LDO_OUT4";
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1000000>;
> + regulator-always-on;
> + op_mode = <0>;

I think that "op_mode" here is questionable. Adding Javier to the
thread who was looking at this for max77802 and possibly max77686.


> + };
> +
> + s5m_ldo5_reg: LDO5 {
> + regulator-name = "P1.0V_LDO_OUT5";
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1000000>;
> + regulator-always-on;
> + op_mode = <0>;
> + };
> +
> + s5m_ldo6_reg: LDO6 {
> + regulator-name = "vdd_mydp";
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1000000>;
> + regulator-always-on;
> + op_mode = <3>;
> + };
> +
> + s5m_ldo7_reg: LDO7 {
> + regulator-name = "P1.1V_LDO_OUT7";
> + regulator-min-microvolt = <1100000>;
> + regulator-max-microvolt = <1100000>;
> + regulator-always-on;
> + op_mode = <3>;
> + };
> +
> + s5m_ldo8_reg: LDO8 {
> + regulator-name = "P1.0V_LDO_OUT8";
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1000000>;
> + regulator-always-on;
> + op_mode = <3>;
> + };
> +
> + s5m_ldo10_reg: LDO10 {
> + regulator-name = "P1.8V_LDO_OUT10";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-always-on;
> + op_mode = <3>;
> + };
> +
> + s5m_ldo11_reg: LDO11 {
> + regulator-name = "P1.8V_LDO_OUT11";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-always-on;
> + op_mode = <0>;
> + };
> +
> + s5m_ldo12_reg: LDO12 {
> + regulator-name = "P3.0V_LDO_OUT12";
> + regulator-min-microvolt = <3000000>;
> + regulator-max-microvolt = <3000000>;
> + regulator-always-on;
> + op_mode = <3>;
> + };
> +
> + s5m_ldo13_reg: LDO13 {
> + regulator-name = "P1.8V_LDO_OUT13";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-always-on;
> + op_mode = <0>;
> + };
> +
> + s5m_ldo14_reg: LDO14 {
> + regulator-name = "P1.8V_LDO_OUT14";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-always-on;
> + op_mode = <3>;
> + };
> +
> + s5m_ldo15_reg: LDO15 {
> + regulator-name = "P1.0V_LDO_OUT15";
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1000000>;
> + regulator-always-on;
> + op_mode = <3>;
> + };
> +
> + s5m_ldo16_reg: LDO16 {
> + regulator-name = "P1.8V_LDO_OUT16";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-always-on;
> + op_mode = <3>;
> + };
> +
> + s5m_ldo17_reg: LDO17 {
> + regulator-name = "P2.8V_LDO_OUT17";
> + regulator-min-microvolt = <2800000>;
> + regulator-max-microvolt = <2800000>;
> + regulator-always-on;
> + op_mode = <0>;
> + };
> +
> + s5m_ldo25_reg: LDO25 {
> + regulator-name = "vdd_bridge";
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <1200000>;
> + regulator-always-on;
> + op_mode = <1>;
> + };
> +
> + BUCK1 {
> + regulator-name = "vdd_mif";
> + regulator-min-microvolt = <950000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-always-on;
> + regulator-boot-on;
> + op_mode = <3>;
> + };
> +
> + BUCK2 {
> + regulator-name = "vdd_arm";
> + regulator-min-microvolt = <850000>;
> + regulator-max-microvolt = <1350000>;
> + regulator-always-on;
> + regulator-boot-on;
> + op_mode = <3>;
> + };
> +
> + BUCK3 {
> + regulator-name = "vdd_int";
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <1200000>;
> + regulator-always-on;
> + regulator-boot-on;
> + op_mode = <3>;
> + };
> +
> + BUCK4 {
> + regulator-name = "vdd_g3d";
> + regulator-min-microvolt = <850000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-boot-on;
> + op_mode = <3>;
> + };
> +
> + BUCK5 {
> + regulator-name = "P1.8V_BUCK_OUT5";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-always-on;
> + regulator-boot-on;
> + op_mode = <1>;
> + };
> +
> + BUCK6 {
> + regulator-name = "P1.2V_BUCK_OUT6";
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <1200000>;
> + regulator-always-on;
> + regulator-boot-on;
> + op_mode = <0>;
> + };
> +
> + BUCK9 {
> + regulator-name = "vdd_ummc";
> + regulator-min-microvolt = <950000>;
> + regulator-max-microvolt = <3000000>;
> + regulator-always-on;
> + regulator-boot-on;
> + op_mode = <3>;
> + };
> + };
> + };
> + };
> +
> + i2c@12C70000 {
> + trackpad {
> + status = "disabled";

Having this bogus entry here doesn't add anything. Remove it until
the trackpad should be added. See http://crbug.com/371114 for a
slightly stale bug about trackpad.


> + };
> + };
> +
> + i2c@12CA0000 {
> + embedded-controller {

Add "cros_ec" alias like snow.


> + compatible = "google,cros-ec-i2c";
> + reg = <0x1e>;
> + interrupts = <6 0>;
> + interrupt-parent = <&gpx1>;
> + wakeup-source;
> +
> + keyboard-controller {

Don't include keyboard-controller here. Add:

#include "cros-ec-keyboard.dtsi"

...at the bottom. Note that I think that the spring EC has a special
"charger" key that it uses to indicate when a charger was plugged in
and unplugged. I'm not sure how that will end up getting supported
upstream but you could just leave it out for now.

> + compatible = "google,cros-ec-keyb";
> + keypad,num-rows = <8>;
> + keypad,num-columns = <13>;

Don't you need pinctrl here?


> + google,needs-ghost-filter;
> + linux,keymap = <
> + 0x0001007d /* L_META */
> + 0x0002003b /* F1 */
> + 0x00030030 /* B */
> + 0x00040044 /* F10 */
> + 0x00060031 /* N */
> + 0x0008000d /* = */
> + 0x000a0064 /* R_ALT */
> +
> + 0x01010001 /* ESC */
> + 0x0102003e /* F4 */
> + 0x01030022 /* G */
> + 0x01040041 /* F7 */
> + 0x01060023 /* H */
> + 0x01080028 /* ' */
> + 0x01090043 /* F9 */
> + 0x010b000e /* BKSPACE */
> +
> + 0x0200001d /* L_CTRL */
> + 0x0201000f /* TAB */
> + 0x0202003d /* F3 */
> + 0x02030014 /* T */
> + 0x02040040 /* F6 */
> + 0x0205001b /* ] */
> + 0x02060015 /* Y */
> + 0x02070056 /* 102ND */
> + 0x0208001a /* [ */
> + 0x02090042 /* F8 */
> +
> + 0x03010029 /* GRAVE */
> + 0x0302003c /* F2 */
> + 0x03030006 /* 5 */
> + 0x0304003f /* F5 */
> + 0x03060007 /* 6 */
> + 0x0308000c /* - */
> + 0x030b002b /* \ */
> +
> + 0x04000061 /* R_CTRL */
> + 0x0401001e /* A */
> + 0x04020020 /* D */
> + 0x04030021 /* F */
> + 0x0404001f /* S */
> + 0x04050025 /* K */
> + 0x04060024 /* J */
> + 0x04080027 /* ; */
> + 0x04090026 /* L */
> + 0x040a002b /* \ */
> + 0x040b001c /* ENTER */
> +
> + 0x0501002c /* Z */
> + 0x0502002e /* C */
> + 0x0503002f /* V */
> + 0x0504002d /* X */
> + 0x05050033 /* , */
> + 0x05060032 /* M */
> + 0x0507002a /* L_SHIFT */
> + 0x05080035 /* / */
> + 0x05090034 /* . */
> + 0x050B0039 /* SPACE */
> +
> + 0x06010002 /* 1 */
> + 0x06020004 /* 3 */
> + 0x06030005 /* 4 */
> + 0x06040003 /* 2 */
> + 0x06050009 /* 8 */
> + 0x06060008 /* 7 */
> + 0x0608000b /* 0 */
> + 0x0609000a /* 9 */
> + 0x060a0038 /* L_ALT */
> + 0x060b006c /* DOWN */
> + 0x060c006a /* RIGHT */
> +
> + 0x07010010 /* Q */
> + 0x07020012 /* E */
> + 0x07030013 /* R */
> + 0x07040011 /* W */
> + 0x07050017 /* I */
> + 0x07060016 /* U */
> + 0x07070036 /* R_SHIFT */
> + 0x07080019 /* P */
> + 0x07090018 /* O */
> + 0x070b0067 /* UP */
> + 0x070c0069 /* LEFT */
> + >;
> + };
> + };
> +
> + power-regulator {
> + compatible = "ti,tps65090";

I doubt tps65090 will "just work". Does it?

On spring the tps65090 is not directly on the same i2c bus as the EC.
It's actually hidden behind the EC.

Locally in the ChromeOS tree there appears to be a forked copy of the
65090 regulator driver that's in use just for spring. See this from
the ChromeOS 3.8 tree:

./drivers/regulator/tps65090-regulator.c
./drivers/regulator/cros_ec-tps65090.c

The Spring version of the driver sends EC commands directly to access
the tps65090.

It is possible (untested) that you could also talk to tps65090 over an
i2c tunnel. On exynos5420-peach-pit we have a full fledged i2c
tunnel, but you may be able to extend the tunnel to export an i2c
tunnel for spring using something like
<https://chromium-review.googlesource.com/66116>


> + reg = <0x48>;
> +
> + /*
> + * Config irq to disable internal pulls
> + * even though we run in polling mode.
> + */
> + pinctrl-names = "default";
> + pinctrl-0 = <&tps65090_irq>;
> +
> + vsys1-supply = <&vbat>;
> + vsys2-supply = <&vbat>;
> + vsys3-supply = <&vbat>;
> + infet1-supply = <&vbat>;
> + infet2-supply = <&vbat>;
> + infet3-supply = <&vbat>;
> + infet4-supply = <&vbat>;
> + infet5-supply = <&vbat>;
> + infet6-supply = <&vbat>;
> + infet7-supply = <&vbat>;
> + vsys-l1-supply = <&vbat>;
> + vsys-l2-supply = <&vbat>;
> +
> + regulators {
> + fet1 {
> + regulator-name = "vcd_led";
> + regulator-min-microvolt = <12000000>;
> + regulator-max-microvolt = <12000000>;
> + };
> + fet3 {
> + regulator-name = "wwan_r";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-always-on;
> + };
> + fet5 {
> + regulator-name = "cam";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-always-on;
> + };
> + fet6 {
> + regulator-name = "lcd_vdd";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + };
> + fet7 {
> + regulator-name = "ts";
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + };
> + };
> +
> + charger {
> + compatible = "ti,tps65090-charger";
> + };
> + };
> + };
> +
> + hdmi {
> + hdmi-en-supply = <&s5m_ldo8_reg>;
> + vdd-supply = <&s5m_ldo8_reg>;
> + vdd_osc-supply = <&s5m_ldo10_reg>;
> + vdd_pll-supply = <&s5m_ldo8_reg>;
> + };
> +
> + fimd@14400000 {
> + status = "okay";
> + samsung,invert-vclk;
> + };
> +
> + dp-controller@145B0000 {
> + status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <&dp_hpd>;

This is probably not right. It looks as if spring uses gpc3-0 for
display port HPD (as a GPIO). The upstream has this in the
exynos5250-pinctrl.dtsi as a different pin.

I think you'll need to define your own pinctrl here.


> + samsung,color-space = <0>;
> + samsung,dynamic-range = <0>;
> + samsung,ycbcr-coeff = <0>;
> + samsung,color-depth = <1>;
> + samsung,link-rate = <0x0a>;
> + samsung,lane-count = <1>;
> + samsung,hpd-gpio = <&gpc3 0 0>;
> +
> + display-timings {
> + native-mode = <&timing1>;
> +
> + timing1: timing@1 {
> + clock-frequency = <70589280>;
> + hactive = <1366>;
> + vactive = <768>;
> + hfront-porch = <40>;
> + hback-porch = <40>;
> + hsync-len = <32>;
> + vback-porch = <10>;
> + vfront-porch = <12>;
> + vsync-len = <6>;
> + };
> + };
> + };
> +
> + fixed-rate-clocks {
> + xxti {
> + compatible = "samsung,clock-xxti";
> + clock-frequency = <24000000>;
> + };
> + };
> +};

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/