Re: [RFC 1/3] arm64: dts: qcom: sdm845-cheza: add initial cheza dt

[Doug Anderson <dianders@chromium.org>]

Hi,

On Thu, May 9, 2019 at 11:44 AM Rob Clark <robdclark@gmail.com>wrote:

> From: Rob Clark <robdclark@chromium.org>
>
> This is essentialy a squash of a bunch of history of cheza dt updates
> from chromium kernel, some of which were themselves squashes of history
> from older chromium kernels.
>
> I don't claim any credit other than wanting to more easily boot upstream
> kernel on cheza to have an easier way to test upstream driver work ;-)
>
> I've added below in Cc tags all the original actual authors (apologies
> if I missed any).
>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Sibi Sankar <sibis@codeaurora.org>
> Cc: Evan Green <evgreen@chromium.org>
> Cc: Matthias Kaehlcke <mka@chromium.org>
> Cc: Abhinav Kumar <abhinavk@codeaurora.org>
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Venkat Gopalakrishnan <venkatg@codeaurora.org>
> Cc: Rajendra Nayak <rnayak@codeaurora.org>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  arch/arm64/boot/dts/qcom/Makefile            |    3 +
>  arch/arm64/boot/dts/qcom/sdm845-cheza-r1.dts |  238 ++++
>  arch/arm64/boot/dts/qcom/sdm845-cheza-r2.dts |  238 ++++
>  arch/arm64/boot/dts/qcom/sdm845-cheza-r3.dts |  180 +++
>  arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi   | 1317 ++++++++++++++++++
>  5 files changed, 1976 insertions(+)

In general I am supportive of getting cheza dts landed upstream.

One question is how much cleanup we want to do while landing upstream.
We have a few TODO / HACK comments currently.  Do we want to land what
we have and clean these up in separate commits, or should we try to do
cleanup beforehand.  Bjron / Andy: do you have any opinions?  I've
commented on a few below.


> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index b3fe72ff2955..7a038cf81543 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -8,6 +8,9 @@ dtb-$(CONFIG_ARCH_QCOM) += msm8994-angler-rev-101.dtb
>  dtb-$(CONFIG_ARCH_QCOM)        += msm8996-mtp.dtb
>  dtb-$(CONFIG_ARCH_QCOM)        += msm8998-mtp.dtb
> +dtb-$(CONFIG_ARCH_QCOM)        += sdm845-cheza-r1.dtb
> +dtb-$(CONFIG_ARCH_QCOM)        += sdm845-cheza-r2.dtb
> +dtb-$(CONFIG_ARCH_QCOM)        += sdm845-cheza-r3.dtb
>  dtb-$(CONFIG_ARCH_QCOM)        += sdm845-mtp.dtb
>  dtb-$(CONFIG_ARCH_QCOM)        += qcs404-evb-1000.dtb
>  dtb-$(CONFIG_ARCH_QCOM)        += qcs404-evb-4000.dtb

Something about the above makes "git am" unhappy and "patch -p1" think
this is a malformed patch.  When I delete the part touching the
Makefile then I can apply OK.


> diff --git a/arch/arm64/boot/dts/qcom/sdm845-cheza-r1.dts b/arch/arm64/boot/dts/qcom/sdm845-cheza-r1.dts
> new file mode 100644
> index 000000000000..35bb4629edd4
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sdm845-cheza-r1.dts
> @@ -0,0 +1,238 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Google Cheza board device tree source
> + *
> + * Copyright 2018 Google LLC.
> + */
> +
> +/dts-v1/;
> +
> +#include "sdm845-cheza.dtsi"
> +
> +/ {
> +       model = "Google Cheza (rev1)";
> +       compatible = "google,cheza-rev1", "google,cheza", "qcom,sdm845";

See below for rev 3, but I think this might be better as:

compatible = "google,cheza-rev1", "qcom,sdm845";


> diff --git a/arch/arm64/boot/dts/qcom/sdm845-cheza-r2.dts b/arch/arm64/boot/dts/qcom/sdm845-cheza-r2.dts
> new file mode 100644
> index 000000000000..4359a82d4bb4
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sdm845-cheza-r2.dts
> @@ -0,0 +1,238 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Google Cheza board device tree source
> + *
> + * Copyright 2018 Google LLC.
> + */
> +
> +/dts-v1/;
> +
> +#include "sdm845-cheza.dtsi"
> +
> +/ {
> +       model = "Google Cheza (rev2)";
> +       compatible = "google,cheza-rev2", "google,cheza", "qcom,sdm845";

See above and below, but probably:

compatible = "google,cheza-rev2", "qcom,sdm845";


> diff --git a/arch/arm64/boot/dts/qcom/sdm845-cheza-r3.dts b/arch/arm64/boot/dts/qcom/sdm845-cheza-r3.dts
> new file mode 100644
> index 000000000000..2c3f815b90c1
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sdm845-cheza-r3.dts
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Google Cheza board device tree source
> + *
> + * Copyright 2018 Google LLC.
> + */
> +
> +/dts-v1/;
> +
> +#include "sdm845-cheza.dtsi"
> +
> +/ {
> +       model = "Google Cheza (rev3+)";
> +       compatible = "google,cheza-rev15", "google,cheza-rev14",
> +                    "google,cheza-rev13", "google,cheza-rev12",
> +                    "google,cheza-rev11", "google,cheza-rev10",
> +                    "google,cheza-rev9", "google,cheza-rev8",
> +                    "google,cheza-rev7", "google,cheza-rev6",
> +                    "google,cheza-rev5", "google,cheza-rev4",
> +                    "google,cheza-rev3", "google,cheza", "qcom,sdm845";

Julius Werner suggested that a better solution is that the newest dts
should specify no revision.  Thus this should be just:

compatible = "google,cheza", "qcom,sdm845";


> diff --git a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
> new file mode 100644
> index 000000000000..338c92ddd1c3
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
> @@ -0,0 +1,1317 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Google Cheza device tree source (common between revisions)
> + *
> + * Copyright 2018 Google LLC.
> + */
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
> +#include "sdm845.dtsi"
> +
> +/* PMICs depend on spmi_bus label and so must come after SoC */
> +#include "pm8005.dtsi"
> +#include "pm8998.dtsi"
> +
> +/ {
> +       aliases {
> +               bluetooth0 = &bluetooth;
> +               hsuart0 = &uart6;
> +               serial0 = &uart9;
> +               wifi0 = &wifi;
> +       };
> +
> +       chosen {
> +               stdout-path = "serial0:115200n8";
> +       };
> +
> +       backlight: backlight {
> +               compatible = "pwm-backlight";
> +               pwms = <&cros_ec_pwm 0>;
> +               enable-gpios = <&tlmm 37 GPIO_ACTIVE_HIGH>;
> +               power-supply = <&ppvar_sys>;
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&ap_edp_bklten>;
> +       };
> +
> +       reserved-memory {
> +               rmtfs@88f00000 {
> +                       compatible = "qcom,rmtfs-mem";
> +                       reg = <0x0 0x88f00000 0x0 0x800000>;
> +                       no-map;
> +
> +                       qcom,client-id = <1>;
> +               };
> +       };

You missed part of:

https://crrev.com/c/1588964 - CHROMIUM: arm64: dts: qcom:
sdm845-cheza: Temporarily delete reserved-mem changes

...that should make the above "reserved-memory" section go away.


> +&spi0 {
> +       status = "okay";
> +       spi-max-frequency = <3000000>;  /* TODO: Drop this? */

Drop "spi-max-frequency".  That was part of original bindings but
shouldn't be there anymore.


> +};
> +
> +&spi5 {
> +       status = "okay";
> +       spi-max-frequency = <800000>; /* TODO: Drop this */
> +       cr50@0 {
> +               compatible = "google,cr50";
> +               reg = <0>;
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&h1_ap_int_odl>;
> +               spi-max-frequency = <800000>;
> +               interrupt-parent = <&tlmm>;
> +               interrupts = <129 IRQ_TYPE_EDGE_RISING>;
> +       };
> +};

Presumably the above needs to be dropped since "google,cr50" isn't
actually upstream.


> +&spi10 {
> +       status = "okay";
> +       spi-max-frequency = <3000000>;  /* TODO: Drop this? */

Drop "spi-max-frequency".  That was part of original bindings but
shouldn't be there anymore.


-Doug