CHROMIUM: arm64: dts: qcom: Add sc7180-gelarshie

Message ID	20220330090947.9100-1-chenxiangrui@huaqin.corp-partner.google.com (mailing list archive)
State	Changes Requested, archived
Headers	show Return-Path: <linux-arm-msm-owner@kernel.org> From: Mars Chen <chenxiangrui@huaqin.corp-partner.google.com> To: agross@kernel.org Cc: Mars Chen <chenxiangrui@huaqin.corp-partner.google.com>, Bjorn Andersson <bjorn.andersson@linaro.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] CHROMIUM: arm64: dts: qcom: Add sc7180-gelarshie Date: Wed, 30 Mar 2022 17:09:46 +0800 Message-Id: <20220330090947.9100-1-chenxiangrui@huaqin.corp-partner.google.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	CHROMIUM: arm64: dts: qcom: Add sc7180-gelarshie \| expand CHROMIUM: arm64: dts: qcom: Add sc7180-gelarshie

Mars Chen March 30, 2022, 9:09 a.m. UTC

Initial attempt at Gelarshie device tree.

BUG=b:225756600
TEST=emerge-strongbad chromeos-kernel-5_4

Signed-off-by: Mars Chen <chenxiangrui@huaqin.corp-partner.google.com>
---
 arch/arm64/boot/dts/qcom/Makefile             |   1 +
 .../dts/qcom/sc7180-trogdor-gelarshie-r0.dts  |  15 +
 .../dts/qcom/sc7180-trogdor-gelarshie.dtsi    | 304 ++++++++++++++++++
 3 files changed, 320 insertions(+)
 create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts
 create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi

kernel test robot March 30, 2022, 2:21 p.m. UTC | #1

Hi Mars,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on v5.17 next-20220330]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Mars-Chen/CHROMIUM-arm64-dts-qcom-Add-sc7180-gelarshie/20220330-171139
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm64-randconfig-r032-20220330 (https://download.01.org/0day-ci/archive/20220330/202203302254.6kSD2Eo4-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 0f6d9501cf49ce02937099350d08f20c4af86f3d)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/18677c7abfdfc9a72daa7cfc3011314b098b361a
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Mars-Chen/CHROMIUM-arm64-dts-qcom-Add-sc7180-gelarshie/20220330-171139
        git checkout 18677c7abfdfc9a72daa7cfc3011314b098b361a
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts:10:
>> arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi:9:10: fatal error: 'sc7180-trogdor-mipi-camera.dtsi' file not found
   #include "sc7180-trogdor-mipi-camera.dtsi"
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 error generated.


vim +9 arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi

   > 9	#include "sc7180-trogdor-mipi-camera.dtsi"
    10

Matthias Kaehlcke March 30, 2022, 5:16 p.m. UTC | #2

On Wed, Mar 30, 2022 at 05:09:46PM +0800, Mars Chen wrote:

> Subject: CHROMIUM: arm64: dts: qcom: Add sc7180-gelarshie

No CHROMIUM tag for upstream posts.

> Initial attempt at Gelarshie device tree.

This is not very useful. If you don't want to reveal much information
about an unreleased device you could say something generic like
"Add device tree for Gelarshie, a trogdor variant".

> BUG=b:225756600
> TEST=emerge-strongbad chromeos-kernel-5_4

drop these

> Signed-off-by: Mars Chen <chenxiangrui@huaqin.corp-partner.google.com>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
>  arch/arm64/boot/dts/qcom/Makefile             |   1 +
>  .../dts/qcom/sc7180-trogdor-gelarshie-r0.dts  |  15 +
>  .../dts/qcom/sc7180-trogdor-gelarshie.dtsi    | 304 ++++++++++++++++++
>  3 files changed, 320 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi
> 
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index f9e6343acd03..cf8f88b065c3 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -57,6 +57,7 @@ dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-coachz-r1.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-coachz-r1-lte.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-coachz-r3.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-coachz-r3-lte.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-gelarshie-r0.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-homestar-r2.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-homestar-r3.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-homestar-r4.dtb
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts
> new file mode 100644
> index 000000000000..027d6d563a5f
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Google Gelarshie board device tree source
> + *
> + * Copyright 2022 Google LLC.
> + */
> +
> +/dts-v1/;
> +
> +#include "sc7180-trogdor-gelarshie.dtsi"
> +
> +/ {
> +	model = "Google Gelarshie (rev0+)";
> +	compatible = "google,gelarshie", "qcom,sc7180";
> +};
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi
> new file mode 100644
> index 000000000000..842f6cac6c27
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi
> @@ -0,0 +1,304 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Google Gelarshie board device tree source
> + *
> + * Copyright 2022 Google LLC.
> + */
> +
> +#include "sc7180.dtsi"
> +#include "sc7180-trogdor-mipi-camera.dtsi"

drop the mipi camera include, it is not upstream

> +
> +ap_ec_spi: &spi6 {};
> +ap_h1_spi: &spi0 {};
> +
> +#include "sc7180-trogdor.dtsi"
> +#include "sc7180-trogdor-ti-sn65dsi86.dtsi"
> +
> +/* Deleted nodes from trogdor.dtsi */
> +
> +/delete-node/ &alc5682;
> +/delete-node/ &pp3300_codec;
> +
> +/ {
> +	/* BOARD-SPECIFIC TOP LEVEL NODES */
> +
> +	adau7002: audio-codec-1 {
> +		compatible = "adi,adau7002";
> +		IOVDD-supply = <&pp1800_l15a>;
> +		wakeup-delay-ms = <80>;
> +		#sound-dai-cells = <0>;
> +	};
> +};
> +
> +&backlight {
> +	pwms = <&cros_ec_pwm 0>;
> +};
> +
> +&camcc {
> +	status = "okay";
> +};
> +
> +&cros_ec {
> +	cros_ec_proximity: proximity {
> +		compatible = "google,cros-ec-mkbp-proximity";
> +		label = "proximity-wifi";
> +	};
> +};
> +
> +ap_ts_pen_1v8: &i2c4 {
> +	status = "okay";
> +	clock-frequency = <400000>;
> +
> +	ap_ts: touchscreen@5d {
> +		compatible = "goodix,gt7375p";
> +		reg = <0x5d>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&ts_int_l>, <&ts_reset_l>;
> +
> +		interrupt-parent = <&tlmm>;
> +		interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
> +
> +		reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>;
> +
> +		vdd-supply = <&pp3300_ts>;
> +	};
> +};
> +
> +&i2c7 {
> +	status = "disabled";
> +};
> +
> +&i2c9 {
> +	status = "disabled";
> +};
> +
> +&mdp {
> +	chromium-enable-overlays;
> +};

I can't find documentation for 'chromium-enable-overlays', what is this
supposed to do?

> +
> +&panel {
> +	compatible = "edp-panel";
> +};
> +
> +&pm6150_adc {
> +	skin-temp-thermistor@4e {
> +		reg = <ADC5_AMUX_THM2_100K_PU>;
> +		qcom,ratiometric;
> +		qcom,hw-settle-time = <200>;
> +	};
> +};
> +
> +&pm6150_adc_tm {
> +	status = "okay";
> +
> +	skin-temp-thermistor@1 {
> +		reg = <1>;
> +		io-channels = <&pm6150_adc ADC5_AMUX_THM2_100K_PU>;
> +		qcom,ratiometric;
> +		qcom,hw-settle-time-us = <200>;
> +	};
> +};

The thermistor is currently unused, drop it and add it later when you
add the corresponding thermal zone.

> +
> +&pp1800_uf_cam {
> +	status = "okay";
> +};
> +
> +&pp1800_wf_cam {
> +	status = "okay";
> +};
> +
> +&pp2800_uf_cam {
> +	status = "okay";
> +};
> +
> +&pp2800_wf_cam {
> +	status = "okay";
> +};
> +
> +&pp3300_dx_edp {
> +	gpio = <&tlmm 67 GPIO_ACTIVE_HIGH>;
> +};
> +
> +&sdhc_2 {
> +	status = "okay";
> +};
> +
> +&sn65dsi86_out {
> +	data-lanes = <0 1 2 3>;
> +};
> +
> +&sound {
> +	compatible = "google,sc7180-coachz";

Is 'sc7180-coachz' intended because the config is the same as for
coachz or should this be 'sc7180-gelarshie'?

Krzysztof Kozlowski March 30, 2022, 5:23 p.m. UTC | #3

On 30/03/2022 19:16, Matthias Kaehlcke wrote:
> On Wed, Mar 30, 2022 at 05:09:46PM +0800, Mars Chen wrote:
> 
>> Subject: CHROMIUM: arm64: dts: qcom: Add sc7180-gelarshie
> 
> No CHROMIUM tag for upstream posts.
> 
>> Initial attempt at Gelarshie device tree.
> 
> This is not very useful. If you don't want to reveal much information
> about an unreleased device you could say something generic like
> "Add device tree for Gelarshie, a trogdor variant".
> 
>> BUG=b:225756600
>> TEST=emerge-strongbad chromeos-kernel-5_4
> 
> drop these
> 
>> Signed-off-by: Mars Chen <chenxiangrui@huaqin.corp-partner.google.com>
>> Reported-by: kernel test robot <lkp@intel.com>
>> ---
>>  arch/arm64/boot/dts/qcom/Makefile             |   1 +
>>  .../dts/qcom/sc7180-trogdor-gelarshie-r0.dts  |  15 +
>>  .../dts/qcom/sc7180-trogdor-gelarshie.dtsi    | 304 ++++++++++++++++++
>>  3 files changed, 320 insertions(+)
>>  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts
>>  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
>> index f9e6343acd03..cf8f88b065c3 100644
>> --- a/arch/arm64/boot/dts/qcom/Makefile
>> +++ b/arch/arm64/boot/dts/qcom/Makefile
>> @@ -57,6 +57,7 @@ dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-coachz-r1.dtb
>>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-coachz-r1-lte.dtb
>>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-coachz-r3.dtb
>>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-coachz-r3-lte.dtb
>> +dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-gelarshie-r0.dtb
>>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-homestar-r2.dtb
>>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-homestar-r3.dtb
>>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-homestar-r4.dtb
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts
>> new file mode 100644
>> index 000000000000..027d6d563a5f
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts
>> @@ -0,0 +1,15 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Google Gelarshie board device tree source
>> + *
>> + * Copyright 2022 Google LLC.
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include "sc7180-trogdor-gelarshie.dtsi"
>> +
>> +/ {
>> +	model = "Google Gelarshie (rev0+)";
>> +	compatible = "google,gelarshie", "qcom,sc7180";
>> +};
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi
>> new file mode 100644
>> index 000000000000..842f6cac6c27
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi
>> @@ -0,0 +1,304 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Google Gelarshie board device tree source
>> + *
>> + * Copyright 2022 Google LLC.
>> + */
>> +
>> +#include "sc7180.dtsi"
>> +#include "sc7180-trogdor-mipi-camera.dtsi"
> 
> drop the mipi camera include, it is not upstream

The file should not build in such form, so probably it was not tested on
mainline kernel... :-(

Best regards,
Krzysztof

Krzysztof Kozlowski March 30, 2022, 5:25 p.m. UTC | #4

On 30/03/2022 11:09, Mars Chen wrote:
> Initial attempt at Gelarshie device tree.
> 
> BUG=b:225756600
> TEST=emerge-strongbad chromeos-kernel-5_4
> 
> Signed-off-by: Mars Chen <chenxiangrui@huaqin.corp-partner.google.com>
> ---
>  arch/arm64/boot/dts/qcom/Makefile             |   1 +
>  .../dts/qcom/sc7180-trogdor-gelarshie-r0.dts  |  15 +
>  .../dts/qcom/sc7180-trogdor-gelarshie.dtsi    | 304 ++++++++++++++++++
>  3 files changed, 320 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi
> 
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index f9e6343acd03..cf8f88b065c3 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -57,6 +57,7 @@ dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-coachz-r1.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-coachz-r1-lte.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-coachz-r3.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-coachz-r3-lte.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-gelarshie-r0.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-homestar-r2.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-homestar-r3.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-homestar-r4.dtb
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts
> new file mode 100644
> index 000000000000..027d6d563a5f
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Google Gelarshie board device tree source
> + *
> + * Copyright 2022 Google LLC.
> + */
> +
> +/dts-v1/;
> +
> +#include "sc7180-trogdor-gelarshie.dtsi"
> +
> +/ {
> +	model = "Google Gelarshie (rev0+)";
> +	compatible = "google,gelarshie", "qcom,sc7180";

Missing bindings. Please document the compatible.

Best regards,
Krzysztof

kernel test robot March 30, 2022, 7:17 p.m. UTC | #5

Hi Mars,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on v5.17 next-20220330]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Mars-Chen/CHROMIUM-arm64-dts-qcom-Add-sc7180-gelarshie/20220330-171139
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm64-randconfig-r012-20220330 (https://download.01.org/0day-ci/archive/20220331/202203310354.EE0k3ev8-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/18677c7abfdfc9a72daa7cfc3011314b098b361a
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Mars-Chen/CHROMIUM-arm64-dts-qcom-Add-sc7180-gelarshie/20220330-171139
        git checkout 18677c7abfdfc9a72daa7cfc3011314b098b361a
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts:10:
>> arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi:9:10: fatal error: sc7180-trogdor-mipi-camera.dtsi: No such file or directory
       9 | #include "sc7180-trogdor-mipi-camera.dtsi"
         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   compilation terminated.


vim +9 arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi

   > 9	#include "sc7180-trogdor-mipi-camera.dtsi"
    10

Rob Clark March 30, 2022, 10:57 p.m. UTC | #6

On Wed, Mar 30, 2022 at 2:58 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Wed, Mar 30, 2022 at 05:09:46PM +0800, Mars Chen wrote:
>
> > Subject: CHROMIUM: arm64: dts: qcom: Add sc7180-gelarshie
>
> No CHROMIUM tag for upstream posts.
>
> > Initial attempt at Gelarshie device tree.
>
> This is not very useful. If you don't want to reveal much information
> about an unreleased device you could say something generic like
> "Add device tree for Gelarshie, a trogdor variant".
>
> > BUG=b:225756600
> > TEST=emerge-strongbad chromeos-kernel-5_4
>
> drop these
>
> > Signed-off-by: Mars Chen <chenxiangrui@huaqin.corp-partner.google.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> > ---
> >  arch/arm64/boot/dts/qcom/Makefile             |   1 +
> >  .../dts/qcom/sc7180-trogdor-gelarshie-r0.dts  |  15 +
> >  .../dts/qcom/sc7180-trogdor-gelarshie.dtsi    | 304 ++++++++++++++++++
> >  3 files changed, 320 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts
> >  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi
> >
> > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> > index f9e6343acd03..cf8f88b065c3 100644
> > --- a/arch/arm64/boot/dts/qcom/Makefile
> > +++ b/arch/arm64/boot/dts/qcom/Makefile
> > @@ -57,6 +57,7 @@ dtb-$(CONFIG_ARCH_QCOM)     += sc7180-trogdor-coachz-r1.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)      += sc7180-trogdor-coachz-r1-lte.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)      += sc7180-trogdor-coachz-r3.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)      += sc7180-trogdor-coachz-r3-lte.dtb
> > +dtb-$(CONFIG_ARCH_QCOM)      += sc7180-trogdor-gelarshie-r0.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)      += sc7180-trogdor-homestar-r2.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)      += sc7180-trogdor-homestar-r3.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)      += sc7180-trogdor-homestar-r4.dtb
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts
> > new file mode 100644
> > index 000000000000..027d6d563a5f
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts
> > @@ -0,0 +1,15 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Google Gelarshie board device tree source
> > + *
> > + * Copyright 2022 Google LLC.
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "sc7180-trogdor-gelarshie.dtsi"
> > +
> > +/ {
> > +     model = "Google Gelarshie (rev0+)";
> > +     compatible = "google,gelarshie", "qcom,sc7180";
> > +};
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi
> > new file mode 100644
> > index 000000000000..842f6cac6c27
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi
> > @@ -0,0 +1,304 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Google Gelarshie board device tree source
> > + *
> > + * Copyright 2022 Google LLC.
> > + */
> > +
> > +#include "sc7180.dtsi"
> > +#include "sc7180-trogdor-mipi-camera.dtsi"
>
> drop the mipi camera include, it is not upstream
>
> > +
> > +ap_ec_spi: &spi6 {};
> > +ap_h1_spi: &spi0 {};
> > +
> > +#include "sc7180-trogdor.dtsi"
> > +#include "sc7180-trogdor-ti-sn65dsi86.dtsi"
> > +
> > +/* Deleted nodes from trogdor.dtsi */
> > +
> > +/delete-node/ &alc5682;
> > +/delete-node/ &pp3300_codec;
> > +
> > +/ {
> > +     /* BOARD-SPECIFIC TOP LEVEL NODES */
> > +
> > +     adau7002: audio-codec-1 {
> > +             compatible = "adi,adau7002";
> > +             IOVDD-supply = <&pp1800_l15a>;
> > +             wakeup-delay-ms = <80>;
> > +             #sound-dai-cells = <0>;
> > +     };
> > +};
> > +
> > +&backlight {
> > +     pwms = <&cros_ec_pwm 0>;
> > +};
> > +
> > +&camcc {
> > +     status = "okay";
> > +};
> > +
> > +&cros_ec {
> > +     cros_ec_proximity: proximity {
> > +             compatible = "google,cros-ec-mkbp-proximity";
> > +             label = "proximity-wifi";
> > +     };
> > +};
> > +
> > +ap_ts_pen_1v8: &i2c4 {
> > +     status = "okay";
> > +     clock-frequency = <400000>;
> > +
> > +     ap_ts: touchscreen@5d {
> > +             compatible = "goodix,gt7375p";
> > +             reg = <0x5d>;
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&ts_int_l>, <&ts_reset_l>;
> > +
> > +             interrupt-parent = <&tlmm>;
> > +             interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
> > +
> > +             reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>;
> > +
> > +             vdd-supply = <&pp3300_ts>;
> > +     };
> > +};
> > +
> > +&i2c7 {
> > +     status = "disabled";
> > +};
> > +
> > +&i2c9 {
> > +     status = "disabled";
> > +};
> > +
> > +&mdp {
> > +     chromium-enable-overlays;
> > +};
>
> I can't find documentation for 'chromium-enable-overlays', what is this
> supposed to do?

it's a downstream workaround .. this can be dropped from upstream dt

BR,
-R

> > +
> > +&panel {
> > +     compatible = "edp-panel";
> > +};
> > +
> > +&pm6150_adc {
> > +     skin-temp-thermistor@4e {
> > +             reg = <ADC5_AMUX_THM2_100K_PU>;
> > +             qcom,ratiometric;
> > +             qcom,hw-settle-time = <200>;
> > +     };
> > +};
> > +
> > +&pm6150_adc_tm {
> > +     status = "okay";
> > +
> > +     skin-temp-thermistor@1 {
> > +             reg = <1>;
> > +             io-channels = <&pm6150_adc ADC5_AMUX_THM2_100K_PU>;
> > +             qcom,ratiometric;
> > +             qcom,hw-settle-time-us = <200>;
> > +     };
> > +};
>
> The thermistor is currently unused, drop it and add it later when you
> add the corresponding thermal zone.
>
> > +
> > +&pp1800_uf_cam {
> > +     status = "okay";
> > +};
> > +
> > +&pp1800_wf_cam {
> > +     status = "okay";
> > +};
> > +
> > +&pp2800_uf_cam {
> > +     status = "okay";
> > +};
> > +
> > +&pp2800_wf_cam {
> > +     status = "okay";
> > +};
> > +
> > +&pp3300_dx_edp {
> > +     gpio = <&tlmm 67 GPIO_ACTIVE_HIGH>;
> > +};
> > +
> > +&sdhc_2 {
> > +     status = "okay";
> > +};
> > +
> > +&sn65dsi86_out {
> > +     data-lanes = <0 1 2 3>;
> > +};
> > +
> > +&sound {
> > +     compatible = "google,sc7180-coachz";
>
> Is 'sc7180-coachz' intended because the config is the same as for
> coachz or should this be 'sc7180-gelarshie'?

Doug Anderson April 13, 2022, 9:48 p.m. UTC | #7

Hi,

On Wed, Mar 30, 2022 at 10:25 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 30/03/2022 11:09, Mars Chen wrote:
> > Initial attempt at Gelarshie device tree.
> >
> > BUG=b:225756600
> > TEST=emerge-strongbad chromeos-kernel-5_4
> >
> > Signed-off-by: Mars Chen <chenxiangrui@huaqin.corp-partner.google.com>
> > ---
> >  arch/arm64/boot/dts/qcom/Makefile             |   1 +
> >  .../dts/qcom/sc7180-trogdor-gelarshie-r0.dts  |  15 +
> >  .../dts/qcom/sc7180-trogdor-gelarshie.dtsi    | 304 ++++++++++++++++++
> >  3 files changed, 320 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts
> >  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie.dtsi
> >
> > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> > index f9e6343acd03..cf8f88b065c3 100644
> > --- a/arch/arm64/boot/dts/qcom/Makefile
> > +++ b/arch/arm64/boot/dts/qcom/Makefile
> > @@ -57,6 +57,7 @@ dtb-$(CONFIG_ARCH_QCOM)     += sc7180-trogdor-coachz-r1.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)      += sc7180-trogdor-coachz-r1-lte.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)      += sc7180-trogdor-coachz-r3.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)      += sc7180-trogdor-coachz-r3-lte.dtb
> > +dtb-$(CONFIG_ARCH_QCOM)      += sc7180-trogdor-gelarshie-r0.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)      += sc7180-trogdor-homestar-r2.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)      += sc7180-trogdor-homestar-r3.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)      += sc7180-trogdor-homestar-r4.dtb
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts
> > new file mode 100644
> > index 000000000000..027d6d563a5f
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-gelarshie-r0.dts
> > @@ -0,0 +1,15 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Google Gelarshie board device tree source
> > + *
> > + * Copyright 2022 Google LLC.
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "sc7180-trogdor-gelarshie.dtsi"
> > +
> > +/ {
> > +     model = "Google Gelarshie (rev0+)";
> > +     compatible = "google,gelarshie", "qcom,sc7180";
>
> Missing bindings. Please document the compatible.

I'm actually kinda curious: is there really a good reason for this? I
know I haven't been adding things to
`Documentation/devicetree/bindings/arm/qcom.yaml` for Qualcomm
Chromebooks.  Ironically, it turns out that the script I typically use
to invoke checkpatch happens to have "--no-tree" as an argument and
that seems to disable this check. Doh!

That being said, though, I do wonder a little bit about the value of
enumerating the top-level compatible like this in a yaml file.
Certainly the yaml schema validation in general can be quite useful,
but this top-level listing seems pure overhead. I guess it makes some
tools happy, but other than that it seems to provide very little
value...

-Doug

Krzysztof Kozlowski April 14, 2022, 7:10 a.m. UTC | #8

On 13/04/2022 23:48, Doug Anderson wrote:
> I'm actually kinda curious: is there really a good reason for this? I
> know I haven't been adding things to
> `Documentation/devicetree/bindings/arm/qcom.yaml` for Qualcomm
> Chromebooks.  Ironically, it turns out that the script I typically use
> to invoke checkpatch happens to have "--no-tree" as an argument and
> that seems to disable this check. Doh!
> 
> That being said, though, I do wonder a little bit about the value of
> enumerating the top-level compatible like this in a yaml file.
> Certainly the yaml schema validation in general can be quite useful,
> but this top-level listing seems pure overhead. I guess it makes some
> tools happy, but other than that it seems to provide very little
> value...

If compatible is not part of ABI, it is allowed to change in whatever
shape one wishes. In such case, how can anyone (e.g. user-space)
identify the board? Model name? Also not part of ABI (not documented)...


Best regards,
Krzysztof

Doug Anderson April 14, 2022, 5:36 p.m. UTC | #9

Hi,

On Thu, Apr 14, 2022 at 12:10 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 13/04/2022 23:48, Doug Anderson wrote:
> > I'm actually kinda curious: is there really a good reason for this? I
> > know I haven't been adding things to
> > `Documentation/devicetree/bindings/arm/qcom.yaml` for Qualcomm
> > Chromebooks.  Ironically, it turns out that the script I typically use
> > to invoke checkpatch happens to have "--no-tree" as an argument and
> > that seems to disable this check. Doh!
> >
> > That being said, though, I do wonder a little bit about the value of
> > enumerating the top-level compatible like this in a yaml file.
> > Certainly the yaml schema validation in general can be quite useful,
> > but this top-level listing seems pure overhead. I guess it makes some
> > tools happy, but other than that it seems to provide very little
> > value...
>
> If compatible is not part of ABI, it is allowed to change in whatever
> shape one wishes. In such case, how can anyone (e.g. user-space)
> identify the board? Model name? Also not part of ABI (not documented)...

Hmm, it is a good question. I guess one issue is that the way
Chromebooks interact with the bootloader it's not trivially easy to
enumerate what exactly the compatible will be in this hardcoded list.
It all has to do with the whole "revision" and "sku" scheme the
bootloader on ARM Chromebooks uses. For example, on one Chromebook I
have the bootloader prints out:

Compat preference: google,lazor-rev5-sku6 google,lazor-rev5
google,lazor-sku6 google,lazor

What that means is that:

1. The bootloader will first look for 'google,lazor-rev5-sku6'. If it
finds a dts with that compatible it will pick it.

2. The bootloader will then look for 'google,lazor-rev5'. If it finds
a dts with that compatible it will pick it.

3. The bootloader will then look for 'google,lazor-sku6'. If it finds
a dts with that compatible it will pick it.

4. Finally, the bootloader will look for 'google,lazor'.

There's a method to the madness. Among other things, this allows
revving the board revision for a change to the board even if it
_should_ be invisible to software. The rule is always that the
"newest" device tree that's in Linux is always listed _without_ a
board revision number. An example might help.

a) Assume '-rev5' is the newest revision available. In Linux this
would be the only dts that advertises "google,lazor" (with no -rev).
Previous dts file would advertise "-rev3" or "-rev4" or whatever.

b) We need to spin the board for something that should be invisible to
software. Just in case, HW guys change the board strappings to -rev6.
This works _seamlessly_ because the newest dts file always advertises
just "google,lazor"

c) We spin the board for something that's _not_ invisible. It will be
"-rev7". Now, we go back and add "-rev5" and "-rev6" to the old board
dts file and remove the advertisement for "google,lazor". We create a
new dts file for -rev7 that advertises "google,lazor".

Now we can certainly argue back and forth above the above scheme and
how it's terrible and/or great, but it definitely works pretty well
and it's what we've been doing for a while now. Before that we used to
proactively add a whole bunch of "future" revisions "just in case".
That was definitely worse and had the same problem that we'd have to
shuffle compatibles. See, for instance `rk3288-veyron-jerry.dts`.

One thing we _definitely_ don't want to do is to give HW _any_
incentive to make board spins _without_ changing the revision. HW
sometimes makes spins without first involving software and if it
doesn't boot because they updated the board ID then someone in China
will just put the old ID in and ship it off. That's bad.

--

But I guess this doesn't answer your question: how can userspace
identify what board this is running? I don't have an answer to that,
but I guess I'd say that the top-level "compatible" isn't really it.
If nothing else, I think just from the definition it's not guaranteed
to be right, is it? From the spec: "Specifies a list of platform
architectures with which this platform is compatible." The key thing
is "a list". If this can be a list of things then how can you use it
to uniquely identify what one board you're on? If all of the things
that are different between two boards are things that are probable
(eDP panels, USB devices, PCIe devices) then two very different boards
could have the exact same device tree, right? ...and you could have
one device tree that lists the compatible of both boards?

That all being said, I think that on Chrome OS the userspace tools
_do_ some amount of parsing of the compatible strings here. For
Chromebooks they leverage the fact that they understand the above
scheme and thus can figure things out. I think they also use things
like `/proc/device-tree/firmware/coreboot/board-id` and
`/proc/device-tree/firmware/coreboot/sku-id`. That doesn't seem to be
documented, though. :(

I guess the question is, though, why do you need to know what board you're on?

-Doug

Krzysztof Kozlowski April 19, 2022, 3:47 p.m. UTC | #10

On 14/04/2022 19:36, Doug Anderson wrote:
> Hi,
> 
> On Thu, Apr 14, 2022 at 12:10 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 13/04/2022 23:48, Doug Anderson wrote:
>>> I'm actually kinda curious: is there really a good reason for this? I
>>> know I haven't been adding things to
>>> `Documentation/devicetree/bindings/arm/qcom.yaml` for Qualcomm
>>> Chromebooks.  Ironically, it turns out that the script I typically use
>>> to invoke checkpatch happens to have "--no-tree" as an argument and
>>> that seems to disable this check. Doh!
>>>
>>> That being said, though, I do wonder a little bit about the value of
>>> enumerating the top-level compatible like this in a yaml file.
>>> Certainly the yaml schema validation in general can be quite useful,
>>> but this top-level listing seems pure overhead. I guess it makes some
>>> tools happy, but other than that it seems to provide very little
>>> value...
>>
>> If compatible is not part of ABI, it is allowed to change in whatever
>> shape one wishes. In such case, how can anyone (e.g. user-space)
>> identify the board? Model name? Also not part of ABI (not documented)...
> 
> Hmm, it is a good question. I guess one issue is that the way
> Chromebooks interact with the bootloader it's not trivially easy to
> enumerate what exactly the compatible will be in this hardcoded list.
> It all has to do with the whole "revision" and "sku" scheme the
> bootloader on ARM Chromebooks uses. For example, on one Chromebook I
> have the bootloader prints out:
> 
> Compat preference: google,lazor-rev5-sku6 google,lazor-rev5
> google,lazor-sku6 google,lazor
> 
> What that means is that:
> 
> 1. The bootloader will first look for 'google,lazor-rev5-sku6'. If it
> finds a dts with that compatible it will pick it.
> 
> 2. The bootloader will then look for 'google,lazor-rev5'. If it finds
> a dts with that compatible it will pick it.
> 
> 3. The bootloader will then look for 'google,lazor-sku6'. If it finds
> a dts with that compatible it will pick it.
> 
> 4. Finally, the bootloader will look for 'google,lazor'.
> 
> There's a method to the madness. Among other things, this allows
> revving the board revision for a change to the board even if it
> _should_ be invisible to software. The rule is always that the
> "newest" device tree that's in Linux is always listed _without_ a
> board revision number. An example might help.
> 
> a) Assume '-rev5' is the newest revision available. In Linux this
> would be the only dts that advertises "google,lazor" (with no -rev).
> Previous dts file would advertise "-rev3" or "-rev4" or whatever.
> 
> b) We need to spin the board for something that should be invisible to
> software. Just in case, HW guys change the board strappings to -rev6.
> This works _seamlessly_ because the newest dts file always advertises
> just "google,lazor"
> 
> c) We spin the board for something that's _not_ invisible. It will be
> "-rev7". Now, we go back and add "-rev5" and "-rev6" to the old board
> dts file and remove the advertisement for "google,lazor". We create a
> new dts file for -rev7 that advertises "google,lazor".

Except shuffling the compatibles in bindings, you are changing the
meaning of final "google,lazor" compatible. The bootloader works as
expected - from most specific (rev5-sku6) to most generic compatible
(google,lazor) but why do you need to advertise the latest rev as
"google,lazor"? Why the bootloader on latest rev (e.g. rev7) cannot bind
to rev7 compatible?

> Now we can certainly argue back and forth above the above scheme and
> how it's terrible and/or great, but it definitely works pretty well
> and it's what we've been doing for a while now. Before that we used to
> proactively add a whole bunch of "future" revisions "just in case".
> That was definitely worse and had the same problem that we'd have to
> shuffle compatibles. See, for instance `rk3288-veyron-jerry.dts`.
> 
> One thing we _definitely_ don't want to do is to give HW _any_
> incentive to make board spins _without_ changing the revision. HW
> sometimes makes spins without first involving software and if it
> doesn't boot because they updated the board ID then someone in China
> will just put the old ID in and ship it off. That's bad.
> 
> --
> 
> But I guess this doesn't answer your question: how can userspace
> identify what board this is running? I don't have an answer to that,
> but I guess I'd say that the top-level "compatible" isn't really it.

It can, the same as bootloader, by looking at the most specific
compatible (rev7).

> If nothing else, I think just from the definition it's not guaranteed
> to be right, is it? From the spec: "Specifies a list of platform
> architectures with which this platform is compatible." The key thing
> is "a list". If this can be a list of things then how can you use it
> to uniquely identify what one board you're on? 

The most specific compatible identifies or, like recently Rob confirmed
in case of Renesas, the list of compatibles:
https://lore.kernel.org/linux-devicetree/Yk2%2F0Jf151gLuCGz@robh.at.kernel.org/

> If all of the things
> that are different between two boards are things that are probable
> (eDP panels, USB devices, PCIe devices) then two very different boards
> could have the exact same device tree, right? ...and you could have
> one device tree that lists the compatible of both boards?

What is the question here?

> 
> That all being said, I think that on Chrome OS the userspace tools
> _do_ some amount of parsing of the compatible strings here. For
> Chromebooks they leverage the fact that they understand the above
> scheme and thus can figure things out. I think they also use things
> like `/proc/device-tree/firmware/coreboot/board-id` and
> `/proc/device-tree/firmware/coreboot/sku-id`. That doesn't seem to be
> documented, though. :(
> 
> I guess the question is, though, why do you need to know what board you're on?

You might have (and I had in some previous job) single user-space
application working on different HW and responding slightly differently,
depending on the hardware it runs. Exactly the same as kernel behaves
differently, depending on DTB. The differences for example might be in
GPIOs or some other interfaces managed via user-space drivers, not in
presence of devices. Another example are system tests behaving
differently depending on the DUT, where again you run the tests in a
generic way so the DUT is autodetected based on board.

Of course you could say: different hardware, has different DTB, so
user-space should check entire tree, to figure out how to operate that
hardware. Yeah, that's one way, very complex and actually duplicating
kernel's work. Embedded apps are specialized, so it is much easier for
them to check board compatible and make assumptions on that.

Best regards,
Krzysztof

Doug Anderson April 19, 2022, 4:55 p.m. UTC | #11

Hi,

On Tue, Apr 19, 2022 at 8:47 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 14/04/2022 19:36, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Apr 14, 2022 at 12:10 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 13/04/2022 23:48, Doug Anderson wrote:
> >>> I'm actually kinda curious: is there really a good reason for this? I
> >>> know I haven't been adding things to
> >>> `Documentation/devicetree/bindings/arm/qcom.yaml` for Qualcomm
> >>> Chromebooks.  Ironically, it turns out that the script I typically use
> >>> to invoke checkpatch happens to have "--no-tree" as an argument and
> >>> that seems to disable this check. Doh!
> >>>
> >>> That being said, though, I do wonder a little bit about the value of
> >>> enumerating the top-level compatible like this in a yaml file.
> >>> Certainly the yaml schema validation in general can be quite useful,
> >>> but this top-level listing seems pure overhead. I guess it makes some
> >>> tools happy, but other than that it seems to provide very little
> >>> value...
> >>
> >> If compatible is not part of ABI, it is allowed to change in whatever
> >> shape one wishes. In such case, how can anyone (e.g. user-space)
> >> identify the board? Model name? Also not part of ABI (not documented)...
> >
> > Hmm, it is a good question. I guess one issue is that the way
> > Chromebooks interact with the bootloader it's not trivially easy to
> > enumerate what exactly the compatible will be in this hardcoded list.
> > It all has to do with the whole "revision" and "sku" scheme the
> > bootloader on ARM Chromebooks uses. For example, on one Chromebook I
> > have the bootloader prints out:
> >
> > Compat preference: google,lazor-rev5-sku6 google,lazor-rev5
> > google,lazor-sku6 google,lazor
> >
> > What that means is that:
> >
> > 1. The bootloader will first look for 'google,lazor-rev5-sku6'. If it
> > finds a dts with that compatible it will pick it.
> >
> > 2. The bootloader will then look for 'google,lazor-rev5'. If it finds
> > a dts with that compatible it will pick it.
> >
> > 3. The bootloader will then look for 'google,lazor-sku6'. If it finds
> > a dts with that compatible it will pick it.
> >
> > 4. Finally, the bootloader will look for 'google,lazor'.
> >
> > There's a method to the madness. Among other things, this allows
> > revving the board revision for a change to the board even if it
> > _should_ be invisible to software. The rule is always that the
> > "newest" device tree that's in Linux is always listed _without_ a
> > board revision number. An example might help.
> >
> > a) Assume '-rev5' is the newest revision available. In Linux this
> > would be the only dts that advertises "google,lazor" (with no -rev).
> > Previous dts file would advertise "-rev3" or "-rev4" or whatever.
> >
> > b) We need to spin the board for something that should be invisible to
> > software. Just in case, HW guys change the board strappings to -rev6.
> > This works _seamlessly_ because the newest dts file always advertises
> > just "google,lazor"
> >
> > c) We spin the board for something that's _not_ invisible. It will be
> > "-rev7". Now, we go back and add "-rev5" and "-rev6" to the old board
> > dts file and remove the advertisement for "google,lazor". We create a
> > new dts file for -rev7 that advertises "google,lazor".
>
> Except shuffling the compatibles in bindings, you are changing the
> meaning of final "google,lazor" compatible. The bootloader works as
> expected - from most specific (rev5-sku6) to most generic compatible
> (google,lazor) but why do you need to advertise the latest rev as
> "google,lazor"? Why the bootloader on latest rev (e.g. rev7) cannot bind
> to rev7 compatible?

The problem really comes along when a board strapped as -rev8 comes
along that is a board spin (and thus a new revision) but "should" be
invisible to software. Since it should be invisible to software we
want it to boot without any software changes. As per my previous mail,
sometimes HW guys make these changes without first consulting software
(since it's invisible to SW!) and we want to make sure that they're
still going to strap as "-rev8".

So what happens with this -rev8 board? The bootloader will check and
it won't see any device tree that advertises "google,lazor-rev8",
right? If _all_ lazor revisions all include the "google,lazor"
compatible then the bootloader won't have any way to know which to
pick. The bootloader _doesn't_ have the smarts to know that "-rev7" is
closest to "-rev8". It'll just randomly pick one of the "google,lazor"
boards. :( This is why we only advertise "google,lazor" for the newest
device tree.

Yes, I agree it's not beautiful but it's what we ended up with. I
don't think we want to compromise on the ability to boot new revisions
without software changes because that will just incentivize people to
not increment the board revision. The only other option would be to
make the bootloader smart enough to pick the "next revision down" but
so far they haven't been willing to do that.


I guess the question, though, is what action should be taken. I guess
options are:

1. Say that the above requirement that new "invisible" HW revs can
boot w/ no software changes is not a worthy requirement. Personally, I
wouldn't accept this option.

2. Ignore. Don't try to document top level compatible for these devices.

3. Document the compatible and accept that it's going to shuffle around a lot.

4. Try again to get the bootloader to match earlier revisions as fallbacks.


> > Now we can certainly argue back and forth above the above scheme and
> > how it's terrible and/or great, but it definitely works pretty well
> > and it's what we've been doing for a while now. Before that we used to
> > proactively add a whole bunch of "future" revisions "just in case".
> > That was definitely worse and had the same problem that we'd have to
> > shuffle compatibles. See, for instance `rk3288-veyron-jerry.dts`.
> >
> > One thing we _definitely_ don't want to do is to give HW _any_
> > incentive to make board spins _without_ changing the revision. HW
> > sometimes makes spins without first involving software and if it
> > doesn't boot because they updated the board ID then someone in China
> > will just put the old ID in and ship it off. That's bad.
> >
> > --
> >
> > But I guess this doesn't answer your question: how can userspace
> > identify what board this is running? I don't have an answer to that,
> > but I guess I'd say that the top-level "compatible" isn't really it.
>
> It can, the same as bootloader, by looking at the most specific
> compatible (rev7).
>
> > If nothing else, I think just from the definition it's not guaranteed
> > to be right, is it? From the spec: "Specifies a list of platform
> > architectures with which this platform is compatible." The key thing
> > is "a list". If this can be a list of things then how can you use it
> > to uniquely identify what one board you're on?
>
> The most specific compatible identifies or, like recently Rob confirmed
> in case of Renesas, the list of compatibles:
> https://lore.kernel.org/linux-devicetree/Yk2%2F0Jf151gLuCGz@robh.at.kernel.org/

I'm confused. If the device tree contains the compatibles:

"google,lazor-rev4", "google,lazor-rev3", "google,lazor", "qualcomm,sc7180"

You want to know what board you're on and you look at the compatible,
right? You'll decide that you're on a "google,lazor-rev4" which is the
most specific compatible. ...but you could have booted a
"google,lazor-rev3". How do you know?

Further, imagine a case where two different HW manufacturers take some
reference design and each build hardware that's identical except for
what's plugged into PCIe / USB / eDP ports. We could have a single
device tree for these, right? So you could imagine a device tree with
compatibles these compatibles to support the imaginary CompUTown
CompUBox and the TheBestStuff BestBox computers:

"computown,compubox", "thebeststuff,bestbox"

Now you boot up. How do you know if you're on a CompUBox or a BestBox?


> > That all being said, I think that on Chrome OS the userspace tools
> > _do_ some amount of parsing of the compatible strings here. For
> > Chromebooks they leverage the fact that they understand the above
> > scheme and thus can figure things out. I think they also use things
> > like `/proc/device-tree/firmware/coreboot/board-id` and
> > `/proc/device-tree/firmware/coreboot/sku-id`. That doesn't seem to be
> > documented, though. :(
> >
> > I guess the question is, though, why do you need to know what board you're on?
>
> You might have (and I had in some previous job) single user-space
> application working on different HW and responding slightly differently,
> depending on the hardware it runs. Exactly the same as kernel behaves
> differently, depending on DTB. The differences for example might be in
> GPIOs or some other interfaces managed via user-space drivers, not in
> presence of devices. Another example are system tests behaving
> differently depending on the DUT, where again you run the tests in a
> generic way so the DUT is autodetected based on board.
>
> Of course you could say: different hardware, has different DTB, so
> user-space should check entire tree, to figure out how to operate that
> hardware. Yeah, that's one way, very complex and actually duplicating
> kernel's work. Embedded apps are specialized, so it is much easier for
> them to check board compatible and make assumptions on that.

I mean, it's fine if you want to make assumptions for the hardware
that you work on.

-Doug

Krzysztof Kozlowski May 3, 2022, 3:53 p.m. UTC | #12

On Tue, 19 Apr 2022 at 18:55, Doug Anderson <dianders@chromium.org> wrote:

> > Except shuffling the compatibles in bindings, you are changing the
> > meaning of final "google,lazor" compatible. The bootloader works as
> > expected - from most specific (rev5-sku6) to most generic compatible
> > (google,lazor) but why do you need to advertise the latest rev as
> > "google,lazor"? Why the bootloader on latest rev (e.g. rev7) cannot bind
> > to rev7 compatible?
>
> The problem really comes along when a board strapped as -rev8 comes
> along that is a board spin (and thus a new revision) but "should" be
> invisible to software. Since it should be invisible to software we
> want it to boot without any software changes. As per my previous mail,
> sometimes HW guys make these changes without first consulting software
> (since it's invisible to SW!) and we want to make sure that they're
> still going to strap as "-rev8".

If you want to boot it without any SW changes, do not change the SW.
Do not change the DTB. If you admit that you want to change DTB, so
the SW, sure, change it and accept the outcome - you have a new
compatible. This new compatible can be or might be not compatible with
rev7. Up to you.

>
> So what happens with this -rev8 board? The bootloader will check and
> it won't see any device tree that advertises "google,lazor-rev8",
> right?

Your bootloader looks for a specific rev8, which is not compatible
with rev7 (or is it? I lost the point of your example), and you ship
it with a DTB which has rev7, but not rev8. You control both pieces -
bootloader and DTB. You cannot put incompatible pieces of firmware
(one behaving entirely different than other) and expect proper output.
This is why you also have bindings.

> If _all_ lazor revisions all include the "google,lazor"
> compatible then the bootloader won't have any way to know which to
> pick. The bootloader _doesn't_ have the smarts to know that "-rev7" is
> closest to "-rev8".

rev7 the next in the compatible list, isn't it? So bootloader picks up
the fallback...

> It'll just randomly pick one of the "google,lazor"
> boards. :( This is why we only advertise "google,lazor" for the newest
> device tree.
>
> Yes, I agree it's not beautiful but it's what we ended up with. I
> don't think we want to compromise on the ability to boot new revisions
> without software changes because that will just incentivize people to
> not increment the board revision. The only other option would be to
> make the bootloader smart enough to pick the "next revision down" but
> so far they haven't been willing to do that.

Just choose the fallback and follow Devicetree spec...

> I guess the question, though, is what action should be taken. I guess
> options are:
>
> 1. Say that the above requirement that new "invisible" HW revs can
> boot w/ no software changes is not a worthy requirement. Personally, I
> wouldn't accept this option.
>
> 2. Ignore. Don't try to document top level compatible for these devices.
>
> 3. Document the compatible and accept that it's going to shuffle around a lot.
>
> 4. Try again to get the bootloader to match earlier revisions as fallbacks.
>
>
> > > Now we can certainly argue back and forth above the above scheme and
> > > how it's terrible and/or great, but it definitely works pretty well
> > > and it's what we've been doing for a while now. Before that we used to
> > > proactively add a whole bunch of "future" revisions "just in case".
> > > That was definitely worse and had the same problem that we'd have to
> > > shuffle compatibles. See, for instance `rk3288-veyron-jerry.dts`.
> > >
> > > One thing we _definitely_ don't want to do is to give HW _any_
> > > incentive to make board spins _without_ changing the revision. HW
> > > sometimes makes spins without first involving software and if it
> > > doesn't boot because they updated the board ID then someone in China
> > > will just put the old ID in and ship it off. That's bad.
> > >
> > > --
> > >
> > > But I guess this doesn't answer your question: how can userspace
> > > identify what board this is running? I don't have an answer to that,
> > > but I guess I'd say that the top-level "compatible" isn't really it.
> >
> > It can, the same as bootloader, by looking at the most specific
> > compatible (rev7).
> >
> > > If nothing else, I think just from the definition it's not guaranteed
> > > to be right, is it? From the spec: "Specifies a list of platform
> > > architectures with which this platform is compatible." The key thing
> > > is "a list". If this can be a list of things then how can you use it
> > > to uniquely identify what one board you're on?
> >
> > The most specific compatible identifies or, like recently Rob confirmed
> > in case of Renesas, the list of compatibles:
> > https://lore.kernel.org/linux-devicetree/Yk2%2F0Jf151gLuCGz@robh.at.kernel.org/
>
> I'm confused. If the device tree contains the compatibles:
>
> "google,lazor-rev4", "google,lazor-rev3", "google,lazor", "qualcomm,sc7180"
>
> You want to know what board you're on and you look at the compatible,
> right? You'll decide that you're on a "google,lazor-rev4" which is the
> most specific compatible. ...but you could have booted a
> "google,lazor-rev3". How do you know?

Applying the wrong DTB on the wrong device will always give you the
wrong answer. You can try too boot google,lazor-rev3 on x86 PC and it
does not make it a google,lazor-rev3...

Best regards,
Krzysztof

Doug Anderson May 3, 2022, 4:13 p.m. UTC | #13

Hi,

On Tue, May 3, 2022 at 8:54 AM Krzysztof Kozłowski
<k.kozlowski.k@gmail.com> wrote:
>
> On Tue, 19 Apr 2022 at 18:55, Doug Anderson <dianders@chromium.org> wrote:
>
> > > Except shuffling the compatibles in bindings, you are changing the
> > > meaning of final "google,lazor" compatible. The bootloader works as
> > > expected - from most specific (rev5-sku6) to most generic compatible
> > > (google,lazor) but why do you need to advertise the latest rev as
> > > "google,lazor"? Why the bootloader on latest rev (e.g. rev7) cannot bind
> > > to rev7 compatible?
> >
> > The problem really comes along when a board strapped as -rev8 comes
> > along that is a board spin (and thus a new revision) but "should" be
> > invisible to software. Since it should be invisible to software we
> > want it to boot without any software changes. As per my previous mail,
> > sometimes HW guys make these changes without first consulting software
> > (since it's invisible to SW!) and we want to make sure that they're
> > still going to strap as "-rev8".
>
> If you want to boot it without any SW changes, do not change the SW.
> Do not change the DTB. If you admit that you want to change DTB, so
> the SW, sure, change it and accept the outcome - you have a new
> compatible. This new compatible can be or might be not compatible with
> rev7. Up to you.
>
> >
> > So what happens with this -rev8 board? The bootloader will check and
> > it won't see any device tree that advertises "google,lazor-rev8",
> > right?
>
> Your bootloader looks for a specific rev8, which is not compatible
> with rev7 (or is it? I lost the point of your example)

Actually the whole point is that _we don't know_ if -rev7 and -rev8
are compatible.

Think of it this way. You've got component A on your board and you
power it up with 1.8 V. We run out of component A and we decide to
replace it with component B. The vendor promises that component B is a
drop-in replacement for component A. You boot up a few devices with
component B and everything looks good. You build a whole lot of
products.

Sometime down the line you start getting failure reports. It turns out
that products that have component B are sporadically failing in the
field. After talking to the vendor, they suggest that we need to power
component B with 1.85 V instead of 1.80 V. Luckily we can adjust the
voltage with the PMIC, but component A's vendor doesn't want you to
bump the voltage up to 1.85V.

Even though we originally thought that the two boards were 100%
compatible, it later turns out that they're not.

So as a general principle, if we make big changes to a product we
increment the board revision strappings even if we think it's
invisible to software. This can help us get out of sticky situations
in the future.

> and you ship
> it with a DTB which has rev7, but not rev8. You control both pieces -
> bootloader and DTB. You cannot put incompatible pieces of firmware
> (one behaving entirely different than other) and expect proper output.
> This is why you also have bindings.

...and by "you" in "*you* control both pieces" you mean some
collection of people spread across several companies and several
countries and who don't always communicate well with each other. If
they believe that a change should be invisible to software, folks
building the hardware in China don't always send me a heads up in
California, but I still want them to bump the revision number just in
case they messed up and we do need a software change down the road.

> > If _all_ lazor revisions all include the "google,lazor"
> > compatible then the bootloader won't have any way to know which to
> > pick. The bootloader _doesn't_ have the smarts to know that "-rev7" is
> > closest to "-rev8".
>
> rev7 the next in the compatible list, isn't it? So bootloader picks up
> the fallback...

No. The bootloader works like this (just looking at the revision
strappings and ignoring the SKU strappings):

1. Read board strappings and get and ID (like "8")

2. Look for "google,lazor-rev8".

3. If it's not there, look for "google,lazor"

4. If it's not there then that's bad.

...so "-rev7" is _not_ in the compatible list for "-rev8".

> > It'll just randomly pick one of the "google,lazor"
> > boards. :( This is why we only advertise "google,lazor" for the newest
> > device tree.
> >
> > Yes, I agree it's not beautiful but it's what we ended up with. I
> > don't think we want to compromise on the ability to boot new revisions
> > without software changes because that will just incentivize people to
> > not increment the board revision. The only other option would be to
> > make the bootloader smart enough to pick the "next revision down" but
> > so far they haven't been willing to do that.
>
> Just choose the fallback and follow Devicetree spec...

It does choose the fallback and follow the devicetree spec, but the
bootloader doesn't have rules to consider "-rev7" as a fallback for
"-rev8".

> > I guess the question, though, is what action should be taken. I guess
> > options are:
> >
> > 1. Say that the above requirement that new "invisible" HW revs can
> > boot w/ no software changes is not a worthy requirement. Personally, I
> > wouldn't accept this option.
> >
> > 2. Ignore. Don't try to document top level compatible for these devices.
> >
> > 3. Document the compatible and accept that it's going to shuffle around a lot.
> >
> > 4. Try again to get the bootloader to match earlier revisions as fallbacks.
> >
> >
> > > > Now we can certainly argue back and forth above the above scheme and
> > > > how it's terrible and/or great, but it definitely works pretty well
> > > > and it's what we've been doing for a while now. Before that we used to
> > > > proactively add a whole bunch of "future" revisions "just in case".
> > > > That was definitely worse and had the same problem that we'd have to
> > > > shuffle compatibles. See, for instance `rk3288-veyron-jerry.dts`.
> > > >
> > > > One thing we _definitely_ don't want to do is to give HW _any_
> > > > incentive to make board spins _without_ changing the revision. HW
> > > > sometimes makes spins without first involving software and if it
> > > > doesn't boot because they updated the board ID then someone in China
> > > > will just put the old ID in and ship it off. That's bad.
> > > >
> > > > --
> > > >
> > > > But I guess this doesn't answer your question: how can userspace
> > > > identify what board this is running? I don't have an answer to that,
> > > > but I guess I'd say that the top-level "compatible" isn't really it.
> > >
> > > It can, the same as bootloader, by looking at the most specific
> > > compatible (rev7).
> > >
> > > > If nothing else, I think just from the definition it's not guaranteed
> > > > to be right, is it? From the spec: "Specifies a list of platform
> > > > architectures with which this platform is compatible." The key thing
> > > > is "a list". If this can be a list of things then how can you use it
> > > > to uniquely identify what one board you're on?
> > >
> > > The most specific compatible identifies or, like recently Rob confirmed
> > > in case of Renesas, the list of compatibles:
> > > https://lore.kernel.org/linux-devicetree/Yk2%2F0Jf151gLuCGz@robh.at.kernel.org/
> >
> > I'm confused. If the device tree contains the compatibles:
> >
> > "google,lazor-rev4", "google,lazor-rev3", "google,lazor", "qualcomm,sc7180"
> >
> > You want to know what board you're on and you look at the compatible,
> > right? You'll decide that you're on a "google,lazor-rev4" which is the
> > most specific compatible. ...but you could have booted a
> > "google,lazor-rev3". How do you know?
>
> Applying the wrong DTB on the wrong device will always give you the
> wrong answer. You can try too boot google,lazor-rev3 on x86 PC and it
> does not make it a google,lazor-rev3...

I don't understand what you're saying here. If a device tree has the compatible:

"google,lazor-rev4", "google,lazor-rev3", "google,lazor", "qualcomm,sc7180"

You wouldn't expect to boot it on an x86 PC, but you would expect to
boot it on either a "google,lazor-rev4" _or_ a "google,lazor-rev3".
Correct? Now, after we've booted software wants to look at the
compatible of the device tree that was booted. The most specific entry
in that device tree is "google,lazor-rev4". ...but we could have
booted it on a "google,lazor-rev3". How can you know?

-Doug

Krzysztof Kozlowski May 4, 2022, 7:04 a.m. UTC | #14

On 03/05/2022 18:13, Doug Anderson wrote:
> Hi,
> 
> On Tue, May 3, 2022 at 8:54 AM Krzysztof Kozłowski
> <k.kozlowski.k@gmail.com> wrote:
>>
>> On Tue, 19 Apr 2022 at 18:55, Doug Anderson <dianders@chromium.org> wrote:
>>
>>>> Except shuffling the compatibles in bindings, you are changing the
>>>> meaning of final "google,lazor" compatible. The bootloader works as
>>>> expected - from most specific (rev5-sku6) to most generic compatible
>>>> (google,lazor) but why do you need to advertise the latest rev as
>>>> "google,lazor"? Why the bootloader on latest rev (e.g. rev7) cannot bind
>>>> to rev7 compatible?
>>>
>>> The problem really comes along when a board strapped as -rev8 comes
>>> along that is a board spin (and thus a new revision) but "should" be
>>> invisible to software. Since it should be invisible to software we
>>> want it to boot without any software changes. As per my previous mail,
>>> sometimes HW guys make these changes without first consulting software
>>> (since it's invisible to SW!) and we want to make sure that they're
>>> still going to strap as "-rev8".
>>
>> If you want to boot it without any SW changes, do not change the SW.
>> Do not change the DTB. If you admit that you want to change DTB, so
>> the SW, sure, change it and accept the outcome - you have a new
>> compatible. This new compatible can be or might be not compatible with
>> rev7. Up to you.
>>
>>>
>>> So what happens with this -rev8 board? The bootloader will check and
>>> it won't see any device tree that advertises "google,lazor-rev8",
>>> right?
>>
>> Your bootloader looks for a specific rev8, which is not compatible
>> with rev7 (or is it? I lost the point of your example)
> 
> Actually the whole point is that _we don't know_ if -rev7 and -rev8
> are compatible.
> 
> Think of it this way. You've got component A on your board and you
> power it up with 1.8 V. We run out of component A and we decide to
> replace it with component B. The vendor promises that component B is a
> drop-in replacement for component A. You boot up a few devices with
> component B and everything looks good. You build a whole lot of
> products.
> 
> Sometime down the line you start getting failure reports. It turns out
> that products that have component B are sporadically failing in the
> field. After talking to the vendor, they suggest that we need to power
> component B with 1.85 V instead of 1.80 V. Luckily we can adjust the
> voltage with the PMIC, but component A's vendor doesn't want you to
> bump the voltage up to 1.85V.
> 
> Even though we originally thought that the two boards were 100%
> compatible, it later turns out that they're not.
> 
> So as a general principle, if we make big changes to a product we
> increment the board revision strappings even if we think it's
> invisible to software. This can help us get out of sticky situations
> in the future.

Then assume boards are not really compatible, bump rev to rev8 and ship
it. Bootloader will know it is rev8 and use it.

> 
> 
>> and you ship
>> it with a DTB which has rev7, but not rev8. You control both pieces -
>> bootloader and DTB. You cannot put incompatible pieces of firmware
>> (one behaving entirely different than other) and expect proper output.
>> This is why you also have bindings.
> 
> ...and by "you" in "*you* control both pieces" you mean some
> collection of people spread across several companies and several
> countries and who don't always communicate well with each other. If
> they believe that a change should be invisible to software, folks
> building the hardware in China don't always send me a heads up in
> California, but I still want them to bump the revision number just in
> case they messed up and we do need a software change down the road.
> 
> 
>>> If _all_ lazor revisions all include the "google,lazor"
>>> compatible then the bootloader won't have any way to know which to
>>> pick. The bootloader _doesn't_ have the smarts to know that "-rev7" is
>>> closest to "-rev8".
>>
>> rev7 the next in the compatible list, isn't it? So bootloader picks up
>> the fallback...
> 
> No. The bootloader works like this (just looking at the revision
> strappings and ignoring the SKU strappings):
> 
> 1. Read board strappings and get and ID (like "8")
> 
> 2. Look for "google,lazor-rev8".
> 
> 3. If it's not there, look for "google,lazor"
> 
> 4. If it's not there then that's bad.
> 
> ...so "-rev7" is _not_ in the compatible list for "-rev8".

Everything looks fine then. You have a rev8 board, which is not
compatible with rev7, and bootloader looks for rev8. Finds it (since it
is physically there!), loads it.

You have a rev7 board so bootloader looks for rev7, finds it and loads it.

> 
> 
>>> It'll just randomly pick one of the "google,lazor"
>>> boards. :( This is why we only advertise "google,lazor" for the newest
>>> device tree.
>>>
>>> Yes, I agree it's not beautiful but it's what we ended up with. I
>>> don't think we want to compromise on the ability to boot new revisions
>>> without software changes because that will just incentivize people to
>>> not increment the board revision. The only other option would be to
>>> make the bootloader smart enough to pick the "next revision down" but
>>> so far they haven't been willing to do that.
>>
>> Just choose the fallback and follow Devicetree spec...
> 
> It does choose the fallback and follow the devicetree spec, but the
> bootloader doesn't have rules to consider "-rev7" as a fallback for
> "-rev8".

Sure, let's skip fallbacks and assume everything is not compatible with
else.

> 
> 
>>> I guess the question, though, is what action should be taken. I guess
>>> options are:
>>>
>>> 1. Say that the above requirement that new "invisible" HW revs can
>>> boot w/ no software changes is not a worthy requirement. Personally, I
>>> wouldn't accept this option.
>>>
>>> 2. Ignore. Don't try to document top level compatible for these devices.
>>>
>>> 3. Document the compatible and accept that it's going to shuffle around a lot.
>>>
>>> 4. Try again to get the bootloader to match earlier revisions as fallbacks.
>>>
>>>
>>>>> Now we can certainly argue back and forth above the above scheme and
>>>>> how it's terrible and/or great, but it definitely works pretty well
>>>>> and it's what we've been doing for a while now. Before that we used to
>>>>> proactively add a whole bunch of "future" revisions "just in case".
>>>>> That was definitely worse and had the same problem that we'd have to
>>>>> shuffle compatibles. See, for instance `rk3288-veyron-jerry.dts`.
>>>>>
>>>>> One thing we _definitely_ don't want to do is to give HW _any_
>>>>> incentive to make board spins _without_ changing the revision. HW
>>>>> sometimes makes spins without first involving software and if it
>>>>> doesn't boot because they updated the board ID then someone in China
>>>>> will just put the old ID in and ship it off. That's bad.
>>>>>
>>>>> --
>>>>>
>>>>> But I guess this doesn't answer your question: how can userspace
>>>>> identify what board this is running? I don't have an answer to that,
>>>>> but I guess I'd say that the top-level "compatible" isn't really it.
>>>>
>>>> It can, the same as bootloader, by looking at the most specific
>>>> compatible (rev7).
>>>>
>>>>> If nothing else, I think just from the definition it's not guaranteed
>>>>> to be right, is it? From the spec: "Specifies a list of platform
>>>>> architectures with which this platform is compatible." The key thing
>>>>> is "a list". If this can be a list of things then how can you use it
>>>>> to uniquely identify what one board you're on?
>>>>
>>>> The most specific compatible identifies or, like recently Rob confirmed
>>>> in case of Renesas, the list of compatibles:
>>>> https://lore.kernel.org/linux-devicetree/Yk2%2F0Jf151gLuCGz@robh.at.kernel.org/
>>>
>>> I'm confused. If the device tree contains the compatibles:
>>>
>>> "google,lazor-rev4", "google,lazor-rev3", "google,lazor", "qualcomm,sc7180"
>>>
>>> You want to know what board you're on and you look at the compatible,
>>> right? You'll decide that you're on a "google,lazor-rev4" which is the
>>> most specific compatible. ...but you could have booted a
>>> "google,lazor-rev3". How do you know?
>>
>> Applying the wrong DTB on the wrong device will always give you the
>> wrong answer. You can try too boot google,lazor-rev3 on x86 PC and it
>> does not make it a google,lazor-rev3...
> 
> I don't understand what you're saying here. If a device tree has the compatible:
> 
> "google,lazor-rev4", "google,lazor-rev3", "google,lazor", "qualcomm,sc7180"
> 
> You wouldn't expect to boot it on an x86 PC, but you would expect to
> boot it on either a "google,lazor-rev4" _or_ a "google,lazor-rev3".

Yes, but booting it does not mean that the hardware is rev3 or rev4.
Booting it means only that we are running DTB on a compatible hardware.
The DTB determines what is accessible to user-space, not what *really*
the hardware is. The user-space (since we are going now to original
question) reads it and can understand that it is running on hardware
compatible with rev3 - either rev3 or rev4 - and act accordingly.

> Correct? Now, after we've booted software wants to look at the
> compatible of the device tree that was booted. The most specific entry
> in that device tree is "google,lazor-rev4". ...but we could have
> booted it on a "google,lazor-rev3". How can you know?

No, providing and loading a rev4 DTB on a rev3 board is not correct and
does not make any sense. rev3 boards are not compatible with rev4, it's
the other way. Not every fruit is an apple, but every apple is a fruit.
This is why I used that example - if you load rev4 DTB on rev3 hardware
then you have totally wrong booting process.


Best regards,
Krzysztof

Julius Werner May 4, 2022, 11:56 p.m. UTC | #15

Trying to chime in here from the firmware development side of this
issue to help clarify what Doug is asking for. We have the fundamental
problem that we have several different board revisions that we _think_
should be 100% compatible for kernel and userspace, so for various
practical reasons (easier to maintain in the source, limiting kernel
image size by not having to bundle the same DTB multiple times under a
different name), we want to use the same DTB for all of them. Just
saying "well, you should treat each revision as incompatible to all
the others from the start then" doesn't scale (we have a lot of
revisions, and in the vast majority of cases they are just as
compatible as we initially expect them to be).

However, we also can't just exhaustively enumerate all revision
numbers that are compatible with this DTB, because we don't know in
advance how many we'll build. For again various practical reasons
(bundling, signing, testing), it would cost a lot of extra effort and
friction to rebuild a new kernel image just to add a new compatible
string to the list every time the factory updates the revision number.
An earlier hacky solution we had for this is to just define a bunch of
extra revision compatible strings in advance even though they don't
exist yet (e.g. you can still see this in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/tegra124-nyan-blaze.dts
-- I believe there were only actually ever 3 or 4 Blaze revisions, the
other compatible strings defined there never existed as physical
boards). This is cumbersome and hacky and also doesn't really scale
(particularly when we need to add SKU as another dimension into the
string), so we needed to come up with something more manageable.

Our solution is to use "google,lazor" as the stand-in compatible
string for "all Lazor boards compatible with the latest revision".
When a compatibility break happens at some point in the Lazor series
(say between rev3 and rev4), we'll change the compatible string in the
old rev3 DTB to no longer include "google,lazor" but to instead list
all specific revision numbers ("google,lazor-rev0", ...,
"google-lazor-rev3") exhaustively (at this point we can do it, because
at this point we know we're not going to build any new boards
compatible with that old layout in the future). The new rev4 DTB will
then list "google,lazor" and again remain valid for all future
revisions we build, until the next compatibility break.

You are correct that this ends up changing the meaning of
"google,lazor" at some point, but it's really the only good way I can
see how to solve this within our practical constraints. I also don't
really see a practical concern with that, since our firmware matching
algorithm (and I believe this is the standard other bootloaders like
U-Boot also follow) will look for the more specific string with the
explicit revision number first, before falling back to the generic
one. So whenever we decide to change the meaning of the latter in the
kernel, we also make sure to provide matches for all the specific
revisions that previously used the generic match, so that they will
pick up those instead and there's no chance of them getting confused
by the change in meaning. While it is perhaps a bit unorthodox, I
think it is practical, safe, and I don't really see a practical
alternative for our use case.

Doug Anderson May 6, 2022, 9:33 p.m. UTC | #16

Hi,

On Wed, May 4, 2022 at 12:04 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> >>>> The most specific compatible identifies or, like recently Rob confirmed
> >>>> in case of Renesas, the list of compatibles:
> >>>> https://lore.kernel.org/linux-devicetree/Yk2%2F0Jf151gLuCGz@robh.at.kernel.org/
> >>>
> >>> I'm confused. If the device tree contains the compatibles:
> >>>
> >>> "google,lazor-rev4", "google,lazor-rev3", "google,lazor", "qualcomm,sc7180"
> >>>
> >>> You want to know what board you're on and you look at the compatible,
> >>> right? You'll decide that you're on a "google,lazor-rev4" which is the
> >>> most specific compatible. ...but you could have booted a
> >>> "google,lazor-rev3". How do you know?
> >>
> >> Applying the wrong DTB on the wrong device will always give you the
> >> wrong answer. You can try too boot google,lazor-rev3 on x86 PC and it
> >> does not make it a google,lazor-rev3...
> >
> > I don't understand what you're saying here. If a device tree has the compatible:
> >
> > "google,lazor-rev4", "google,lazor-rev3", "google,lazor", "qualcomm,sc7180"
> >
> > You wouldn't expect to boot it on an x86 PC, but you would expect to
> > boot it on either a "google,lazor-rev4" _or_ a "google,lazor-rev3".
>
> Yes, but booting it does not mean that the hardware is rev3 or rev4.
> Booting it means only that we are running DTB on a compatible hardware.
> The DTB determines what is accessible to user-space, not what *really*
> the hardware is. The user-space (since we are going now to original
> question) reads it and can understand that it is running on hardware
> compatible with rev3 - either rev3 or rev4 - and act accordingly.
>
> > Correct? Now, after we've booted software wants to look at the
> > compatible of the device tree that was booted. The most specific entry
> > in that device tree is "google,lazor-rev4". ...but we could have
> > booted it on a "google,lazor-rev3". How can you know?
>
> No, providing and loading a rev4 DTB on a rev3 board is not correct and
> does not make any sense. rev3 boards are not compatible with rev4, it's
> the other way. Not every fruit is an apple, but every apple is a fruit.
> This is why I used that example - if you load rev4 DTB on rev3 hardware
> then you have totally wrong booting process.

I think this is the crux of the difference in opinion and there's no
reasonable way I'm aware of to do what you're asking. If -rev3 and
-rev4 are identical from a software point of view it would be silly
not to share a device tree for the two of them. The number of device
trees we'd have to land in the kernel tree would be multiplied by
several times and we'd have many that are identical except for this
compatible string. I see no benefit here and lots of downside.


-Doug

Krzysztof Kozlowski May 7, 2022, 5:04 p.m. UTC | #17

On 06/05/2022 23:33, Doug Anderson wrote:
> Hi,
> 
> On Wed, May 4, 2022 at 12:04 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>>>>>> The most specific compatible identifies or, like recently Rob confirmed
>>>>>> in case of Renesas, the list of compatibles:
>>>>>> https://lore.kernel.org/linux-devicetree/Yk2%2F0Jf151gLuCGz@robh.at.kernel.org/
>>>>>
>>>>> I'm confused. If the device tree contains the compatibles:
>>>>>
>>>>> "google,lazor-rev4", "google,lazor-rev3", "google,lazor", "qualcomm,sc7180"
>>>>>
>>>>> You want to know what board you're on and you look at the compatible,
>>>>> right? You'll decide that you're on a "google,lazor-rev4" which is the
>>>>> most specific compatible. ...but you could have booted a
>>>>> "google,lazor-rev3". How do you know?
>>>>
>>>> Applying the wrong DTB on the wrong device will always give you the
>>>> wrong answer. You can try too boot google,lazor-rev3 on x86 PC and it
>>>> does not make it a google,lazor-rev3...
>>>
>>> I don't understand what you're saying here. If a device tree has the compatible:
>>>
>>> "google,lazor-rev4", "google,lazor-rev3", "google,lazor", "qualcomm,sc7180"
>>>
>>> You wouldn't expect to boot it on an x86 PC, but you would expect to
>>> boot it on either a "google,lazor-rev4" _or_ a "google,lazor-rev3".
>>
>> Yes, but booting it does not mean that the hardware is rev3 or rev4.
>> Booting it means only that we are running DTB on a compatible hardware.
>> The DTB determines what is accessible to user-space, not what *really*
>> the hardware is. The user-space (since we are going now to original
>> question) reads it and can understand that it is running on hardware
>> compatible with rev3 - either rev3 or rev4 - and act accordingly.
>>
>>> Correct? Now, after we've booted software wants to look at the
>>> compatible of the device tree that was booted. The most specific entry
>>> in that device tree is "google,lazor-rev4". ...but we could have
>>> booted it on a "google,lazor-rev3". How can you know?
>>
>> No, providing and loading a rev4 DTB on a rev3 board is not correct and
>> does not make any sense. rev3 boards are not compatible with rev4, it's
>> the other way. Not every fruit is an apple, but every apple is a fruit.
>> This is why I used that example - if you load rev4 DTB on rev3 hardware
>> then you have totally wrong booting process.
> 
> I think this is the crux of the difference in opinion and there's no
> reasonable way I'm aware of to do what you're asking. If -rev3 and
> -rev4 are identical from a software point of view it would be silly
> not to share a device tree for the two of them. The number of device
> trees we'd have to land in the kernel tree would be multiplied by
> several times and we'd have many that are identical except for this
> compatible string. I see no benefit here and lots of downside.

Wait, we agreed that you don't consider them identical, didn't we? If
they are identical, you do not need rev4 at all. So they are not
identical...

If they are identical, just use rev3 and problem is gone.
If they are not identical or you need to assume there will be difference
(for future), then just go with rev3 without fallback to rev3 and also
problem is gone.

Right now it's not possible to validate QCOM DTSes against DT bindings
because they throw big fat warnings about undocumented top compatibles.
This is a downside for us.

Remember, you do not have to use Devicetree or Linux at all if it causes
you some downsides... No one is forced. :) If you choose to use it,
sorry, it comes with some requirements like being following Devicetree
specification or the binding guidelines.

Best regards,
Krzysztof

Krzysztof Kozlowski May 7, 2022, 5:11 p.m. UTC | #18

On 07/05/2022 19:04, Krzysztof Kozlowski wrote:
> On 06/05/2022 23:33, Doug Anderson wrote:
>> Hi,
>>
>> On Wed, May 4, 2022 at 12:04 AM Krzysztof Kozlowski
>> <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>>>>>> The most specific compatible identifies or, like recently Rob confirmed
>>>>>>> in case of Renesas, the list of compatibles:
>>>>>>> https://lore.kernel.org/linux-devicetree/Yk2%2F0Jf151gLuCGz@robh.at.kernel.org/
>>>>>>
>>>>>> I'm confused. If the device tree contains the compatibles:
>>>>>>
>>>>>> "google,lazor-rev4", "google,lazor-rev3", "google,lazor", "qualcomm,sc7180"
>>>>>>
>>>>>> You want to know what board you're on and you look at the compatible,
>>>>>> right? You'll decide that you're on a "google,lazor-rev4" which is the
>>>>>> most specific compatible. ...but you could have booted a
>>>>>> "google,lazor-rev3". How do you know?
>>>>>
>>>>> Applying the wrong DTB on the wrong device will always give you the
>>>>> wrong answer. You can try too boot google,lazor-rev3 on x86 PC and it
>>>>> does not make it a google,lazor-rev3...
>>>>
>>>> I don't understand what you're saying here. If a device tree has the compatible:
>>>>
>>>> "google,lazor-rev4", "google,lazor-rev3", "google,lazor", "qualcomm,sc7180"
>>>>
>>>> You wouldn't expect to boot it on an x86 PC, but you would expect to
>>>> boot it on either a "google,lazor-rev4" _or_ a "google,lazor-rev3".
>>>
>>> Yes, but booting it does not mean that the hardware is rev3 or rev4.
>>> Booting it means only that we are running DTB on a compatible hardware.
>>> The DTB determines what is accessible to user-space, not what *really*
>>> the hardware is. The user-space (since we are going now to original
>>> question) reads it and can understand that it is running on hardware
>>> compatible with rev3 - either rev3 or rev4 - and act accordingly.
>>>
>>>> Correct? Now, after we've booted software wants to look at the
>>>> compatible of the device tree that was booted. The most specific entry
>>>> in that device tree is "google,lazor-rev4". ...but we could have
>>>> booted it on a "google,lazor-rev3". How can you know?
>>>
>>> No, providing and loading a rev4 DTB on a rev3 board is not correct and
>>> does not make any sense. rev3 boards are not compatible with rev4, it's
>>> the other way. Not every fruit is an apple, but every apple is a fruit.
>>> This is why I used that example - if you load rev4 DTB on rev3 hardware
>>> then you have totally wrong booting process.
>>
>> I think this is the crux of the difference in opinion and there's no
>> reasonable way I'm aware of to do what you're asking. If -rev3 and
>> -rev4 are identical from a software point of view it would be silly
>> not to share a device tree for the two of them. The number of device
>> trees we'd have to land in the kernel tree would be multiplied by
>> several times and we'd have many that are identical except for this
>> compatible string. I see no benefit here and lots of downside.
> 
> Wait, we agreed that you don't consider them identical, didn't we? If
> they are identical, you do not need rev4 at all. So they are not
> identical...
> 
> If they are identical, just use rev3 and problem is gone.
> If they are not identical or you need to assume there will be difference
> (for future), then just go with rev3 without fallback to rev3 and also
> problem is gone.

This should be:
If they are not identical or you need to assume there will be difference
(for future), then just go with rev4 without fallback to rev3 and also
problem is gone.

> 
> Right now it's not possible to validate QCOM DTSes against DT bindings
> because they throw big fat warnings about undocumented top compatibles.
> This is a downside for us.
> 
> Remember, you do not have to use Devicetree or Linux at all if it causes
> you some downsides... No one is forced. :) If you choose to use it,
> sorry, it comes with some requirements like being following Devicetree
> specification or the binding guidelines.
> 
> Best regards,
> Krzysztof


Best regards,
Krzysztof

Julius Werner May 11, 2022, 2:39 a.m. UTC | #19

> Wait, we agreed that you don't consider them identical, didn't we? If
> they are identical, you do not need rev4 at all. So they are not
> identical...

Well, they are identical until they're not. We intend them to be
identical. But for practical purposes it does sometimes happen that
two board revisions which were meant to be indistinguishable by
software end up needing to be distinguished at a later point, when
both the hardware and firmware can no longer be changed. We need to
allow an escape hatch for that case. It does not happen often, so just
treating them all as separate boards from the start is not a scalable
solution. DTBs are not free when they all need to be packaged in the
same kernel image.

> Right now it's not possible to validate QCOM DTSes against DT bindings
> because they throw big fat warnings about undocumented top compatibles.
> This is a downside for us.

But that's a solvable problem, right? As I understand, what Doug was
initially just asking was whether it made _sense_ to document all of
these... not that we couldn't do it. Then this whole thread went down
a rabbit hole of whether our compatible assignments are allowed in the
first place. If we can compromise on this discussion by just doing
whatever needs to be done to make the tool happy, I think(?) we can
provide that.

> Remember, you do not have to use Devicetree or Linux at all if it causes
> you some downsides... No one is forced. :) If you choose to use it,
> sorry, it comes with some requirements like being following Devicetree
> specification or the binding guidelines.

Woah... that is maybe a bit extreme, don't you think? My understanding
was that Linux tries to support a wide variety of platforms and
devices and can make the necessary compromises where needed to stay
practical. I'm sure you are aware of the numerous hacks, workarounds
and special cases throughout the kernel that enthusiasts put in there
to get their favorite platform working, even if the original
manufacturer never bothered to test with anything but Windows and
blatantly violates common standards. Or how the USB stack has a file
listing custom quirks for hundreds of individual device IDs just to
make hardware work that didn't put any effort into following the spec.

We're not even asking for any of that -- we're here, engaging with you
and trying to find the best way for our platforms to fit cleanly into
your model. All we're asking is to please offer some way that makes
accommodations for the necessary practical concerns that come up when
building devices at our scale. We're open for new suggestions, but
they need to stay within the realm of what we can practically do (e.g.
not ship a wholly separate DTB for each board revision, because that
would grow the kernel image beyond what can fit in the kernel
partitions on our platforms, and would create a notable extra cost in
boot time for our users).

Besides, I don't actually see how this violates the Device Tree
specification? All I see it say about the toplevel compatible property
is that it

> Specifies a list of platform architectures with which this platform is compatible. This property can be used by operating systems in selecting platform specific code.

It doesn't say anything about having to uniquely identify the platform
architecture even if a more generic identifier is good enough to make
all necessary platform-specific code choices for the operating system.
In fact, about compatible properties in general the specification says

> The property value consists of a concatenated list of null terminated strings, from most specific to most general. They allow a device to express its compatibility with a family of similar devices, potentially allowing a single device driver to match against several devices.

Which implies that using a more generic string to cover multiple cases
at once is an intentionally allowed use case.

Krzysztof Kozlowski May 11, 2022, 7:20 a.m. UTC | #20

On 11/05/2022 04:39, Julius Werner wrote:
>> Wait, we agreed that you don't consider them identical, didn't we? If
>> they are identical, you do not need rev4 at all. So they are not
>> identical...
> 
> Well, they are identical until they're not. We intend them to be
> identical. But for practical purposes it does sometimes happen that
> two board revisions which were meant to be indistinguishable by
> software end up needing to be distinguished at a later point, when
> both the hardware and firmware can no longer be changed. We need to
> allow an escape hatch for that case. It does not happen often, so just
> treating them all as separate boards from the start is not a scalable
> solution. DTBs are not free when they all need to be packaged in the
> same kernel image.

You split more important part of my message, ignoring the point.

So you choose they are not identical, fine. Why insisting on adding
fallback compatible while not keeping bindings updated? Just don't add
the compatible and work on rev3 or rev4. Doug even once wrote "_we don't
know_ if -rev7 and -rev8 are compatible", so don't make them compatible.
Don't add fallbacks or some generic unspecified front-compatibles and
just work on revision.

> 
>> Right now it's not possible to validate QCOM DTSes against DT bindings
>> because they throw big fat warnings about undocumented top compatibles.
>> This is a downside for us.
> 
> But that's a solvable problem, right? As I understand, what Doug was
> initially just asking was whether it made _sense_ to document all of
> these... not that we couldn't do it. Then this whole thread went down
> a rabbit hole of whether our compatible assignments are allowed in the
> first place. If we can compromise on this discussion by just doing
> whatever needs to be done to make the tool happy, I think(?) we can
> provide that.

None of recent patches from Chromium were doing it, even after
complaining from my side, so why do you suddenly believe that it is
"doable"? If yes, please start doing it and fix the DTSes which you
already submitted without bindings.

To remind - entire discussion started with Doug saying it is pure
overhead for him.

> 
>> Remember, you do not have to use Devicetree or Linux at all if it causes
>> you some downsides... No one is forced. :) If you choose to use it,
>> sorry, it comes with some requirements like being following Devicetree
>> specification or the binding guidelines.
> 
> Woah... that is maybe a bit extreme, don't you think? 

Yes, it was sarcasting. :) But yeah, using Linux and DTS comes now with
DT schema. Please document the bindings in DT schema. That's the
drawback of using mainline...

Best regards,
Krzysztof

Doug Anderson May 11, 2022, 4:09 p.m. UTC | #21

Hi,

On Wed, May 11, 2022 at 12:20 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/05/2022 04:39, Julius Werner wrote:
> >> Wait, we agreed that you don't consider them identical, didn't we? If
> >> they are identical, you do not need rev4 at all. So they are not
> >> identical...
> >
> > Well, they are identical until they're not. We intend them to be
> > identical. But for practical purposes it does sometimes happen that
> > two board revisions which were meant to be indistinguishable by
> > software end up needing to be distinguished at a later point, when
> > both the hardware and firmware can no longer be changed. We need to
> > allow an escape hatch for that case. It does not happen often, so just
> > treating them all as separate boards from the start is not a scalable
> > solution. DTBs are not free when they all need to be packaged in the
> > same kernel image.
>
> You split more important part of my message, ignoring the point.
>
> So you choose they are not identical, fine. Why insisting on adding
> fallback compatible while not keeping bindings updated? Just don't add
> the compatible and work on rev3 or rev4. Doug even once wrote "_we don't
> know_ if -rev7 and -rev8 are compatible", so don't make them compatible.
> Don't add fallbacks or some generic unspecified front-compatibles and
> just work on revision.

Somehow, it seems like we keep talking past each other here and it
feels like folks are getting upset and we're not moving forward. Maybe
the right way to make progress is to find some face-to-face time at a
future conference and sit in front of a white board and hash it out.
That being said:

* Without changing our bootloader or having a big explosion in the
number of dts files, we really can't change our scheme. The best we
can do is document it.

* If we want to change our scheme, we'd need to sit down and come to
an agreement that satisfies everyone, if such a thing is possible.
That would only be able to affect future boards. We don't want to
change the bootloader dts loading scheme on old boards.

> >> Right now it's not possible to validate QCOM DTSes against DT bindings
> >> because they throw big fat warnings about undocumented top compatibles.
> >> This is a downside for us.
> >
> > But that's a solvable problem, right? As I understand, what Doug was
> > initially just asking was whether it made _sense_ to document all of
> > these... not that we couldn't do it. Then this whole thread went down
> > a rabbit hole of whether our compatible assignments are allowed in the
> > first place. If we can compromise on this discussion by just doing
> > whatever needs to be done to make the tool happy, I think(?) we can
> > provide that.
>
> None of recent patches from Chromium were doing it, even after
> complaining from my side, so why do you suddenly believe that it is
> "doable"? If yes, please start doing it and fix the DTSes which you
> already submitted without bindings.
>
> To remind - entire discussion started with Doug saying it is pure
> overhead for him.

I mean, to be fair I said it _seems_ pure overhead and then said that
we could do it if it makes some tools happy. ...but before doing that,
I wanted to make sure it was actually valuable. I still have doubts
about the assertion that the most specific compatible is guaranteed to
uniquely identify hardware. So if the whole reason for doing this is
to make the validation tools happy and there's no other value, then at
least it's plausible to argue that the tools could simply be fixed to
allow this and not shout about it. Now, certainly I'm not arguing that
yaml validation in general is useless. I'm in agreement that we want
dts files to be able to be formally validated because it catches
typos, missing properties, and bugs. I am _only_ saying that I still
haven't seen a good argument for why we need to validate the top-level
compatible string. Since there no properties associated with the
top-level compatible string, it's mostly just checking did some one
copy-paste the compatible string from one file (the dts file) to the
other file (the yaml file) correctly. To me, that does not feel like a
useful check.

The other thing I wanted to make sure was that we weren't just going
to get NAKed later if/when we had to adjust compatible strings on
existing dts files.

In any case, I guess I'll make an attempt to document the compatibles
for existing Chromebooks and we'll see what happens. I'm still not
convinced of the value, but as long as we're not going to get NAKed
for documenting reality it's fine.

-Doug

-Doug

Bjorn Andersson May 11, 2022, 4:57 p.m. UTC | #22

On Wed 11 May 09:09 PDT 2022, Doug Anderson wrote:

> Hi,
> 
> On Wed, May 11, 2022 at 12:20 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 11/05/2022 04:39, Julius Werner wrote:
> > >> Wait, we agreed that you don't consider them identical, didn't we? If
> > >> they are identical, you do not need rev4 at all. So they are not
> > >> identical...
> > >
> > > Well, they are identical until they're not. We intend them to be
> > > identical. But for practical purposes it does sometimes happen that
> > > two board revisions which were meant to be indistinguishable by
> > > software end up needing to be distinguished at a later point, when
> > > both the hardware and firmware can no longer be changed. We need to
> > > allow an escape hatch for that case. It does not happen often, so just
> > > treating them all as separate boards from the start is not a scalable
> > > solution. DTBs are not free when they all need to be packaged in the
> > > same kernel image.
> >
> > You split more important part of my message, ignoring the point.
> >
> > So you choose they are not identical, fine. Why insisting on adding
> > fallback compatible while not keeping bindings updated? Just don't add
> > the compatible and work on rev3 or rev4. Doug even once wrote "_we don't
> > know_ if -rev7 and -rev8 are compatible", so don't make them compatible.
> > Don't add fallbacks or some generic unspecified front-compatibles and
> > just work on revision.
> 
> Somehow, it seems like we keep talking past each other here and it
> feels like folks are getting upset and we're not moving forward. Maybe
> the right way to make progress is to find some face-to-face time at a
> future conference and sit in front of a white board and hash it out.
> That being said:
> 
> * Without changing our bootloader or having a big explosion in the
> number of dts files, we really can't change our scheme. The best we
> can do is document it.
> 
> * If we want to change our scheme, we'd need to sit down and come to
> an agreement that satisfies everyone, if such a thing is possible.
> That would only be able to affect future boards. We don't want to
> change the bootloader dts loading scheme on old boards.
> 

In particular we don't want to end up with a scheme that requires you to
spin new software for hardware that you think is compatible with the
existing description provided to the software, that would just cause
logistical overhead.

> 
> > >> Right now it's not possible to validate QCOM DTSes against DT bindings
> > >> because they throw big fat warnings about undocumented top compatibles.
> > >> This is a downside for us.
> > >
> > > But that's a solvable problem, right? As I understand, what Doug was
> > > initially just asking was whether it made _sense_ to document all of
> > > these... not that we couldn't do it. Then this whole thread went down
> > > a rabbit hole of whether our compatible assignments are allowed in the
> > > first place. If we can compromise on this discussion by just doing
> > > whatever needs to be done to make the tool happy, I think(?) we can
> > > provide that.
> >
> > None of recent patches from Chromium were doing it, even after
> > complaining from my side, so why do you suddenly believe that it is
> > "doable"? If yes, please start doing it and fix the DTSes which you
> > already submitted without bindings.
> >
> > To remind - entire discussion started with Doug saying it is pure
> > overhead for him.
> 
> I mean, to be fair I said it _seems_ pure overhead and then said that
> we could do it if it makes some tools happy. ...but before doing that,
> I wanted to make sure it was actually valuable. I still have doubts
> about the assertion that the most specific compatible is guaranteed to
> uniquely identify hardware. So if the whole reason for doing this is
> to make the validation tools happy and there's no other value, then at
> least it's plausible to argue that the tools could simply be fixed to
> allow this and not shout about it. Now, certainly I'm not arguing that
> yaml validation in general is useless. I'm in agreement that we want
> dts files to be able to be formally validated because it catches
> typos, missing properties, and bugs. I am _only_ saying that I still
> haven't seen a good argument for why we need to validate the top-level
> compatible string. Since there no properties associated with the
> top-level compatible string, it's mostly just checking did some one
> copy-paste the compatible string from one file (the dts file) to the
> other file (the yaml file) correctly. To me, that does not feel like a
> useful check.
> 
> The other thing I wanted to make sure was that we weren't just going
> to get NAKed later if/when we had to adjust compatible strings on
> existing dts files.
> 

I don't have a reason for rejecting such changes, as long as it doesn't
affect your ability to run off existing DTBs - but in the end you'd be
the ones suffering most from that...

Regards,
Bjorn

> In any case, I guess I'll make an attempt to document the compatibles
> for existing Chromebooks and we'll see what happens. I'm still not
> convinced of the value, but as long as we're not going to get NAKed
> for documenting reality it's fine.
> 
> -Doug
> 
> -Doug

Krzysztof Kozlowski May 11, 2022, 5:36 p.m. UTC | #23

On 11/05/2022 18:09, Doug Anderson wrote:
>>
>> So you choose they are not identical, fine. Why insisting on adding
>> fallback compatible while not keeping bindings updated? Just don't add
>> the compatible and work on rev3 or rev4. Doug even once wrote "_we don't
>> know_ if -rev7 and -rev8 are compatible", so don't make them compatible.
>> Don't add fallbacks or some generic unspecified front-compatibles and
>> just work on revision.
> 
> Somehow, it seems like we keep talking past each other here and it
> feels like folks are getting upset and we're not moving forward. Maybe
> the right way to make progress is to find some face-to-face time at a
> future conference and sit in front of a white board and hash it out.
> That being said:
> 
> * Without changing our bootloader or having a big explosion in the
> number of dts files, we really can't change our scheme. The best we
> can do is document it.

That's reasonable.

> 
> * If we want to change our scheme, we'd need to sit down and come to
> an agreement that satisfies everyone, if such a thing is possible.

There is open CFP for ELCE 2022 (in Ireland). Maybe we could organize
some session there? But we for sure would need Rob, so the arrangements
should rather focus on him, not on my availability.

> That would only be able to affect future boards.

I would like to say that if you had bindings, then obviously we would
not break them, but since there are no bindings... :)

> We don't want to
> change the bootloader dts loading scheme on old boards.

Understood.

>>>> Right now it's not possible to validate QCOM DTSes against DT bindings
>>>> because they throw big fat warnings about undocumented top compatibles.
>>>> This is a downside for us.
>>>
>>> But that's a solvable problem, right? As I understand, what Doug was
>>> initially just asking was whether it made _sense_ to document all of
>>> these... not that we couldn't do it. Then this whole thread went down
>>> a rabbit hole of whether our compatible assignments are allowed in the
>>> first place. If we can compromise on this discussion by just doing
>>> whatever needs to be done to make the tool happy, I think(?) we can
>>> provide that.
>>
>> None of recent patches from Chromium were doing it, even after
>> complaining from my side, so why do you suddenly believe that it is
>> "doable"? If yes, please start doing it and fix the DTSes which you
>> already submitted without bindings.
>>
>> To remind - entire discussion started with Doug saying it is pure
>> overhead for him.
> 
> I mean, to be fair I said it _seems_ pure overhead and then said that
> we could do it if it makes some tools happy. ...but before doing that,
> I wanted to make sure it was actually valuable. I still have doubts
> about the assertion that the most specific compatible is guaranteed to
> uniquely identify hardware. So if the whole reason for doing this is
> to make the validation tools happy and there's no other value, then at
> least it's plausible to argue that the tools could simply be fixed to
> allow this and not shout about it. 

Instead of adding bindings, you can indeed change/fix the tools. Go
ahead. :)

> Now, certainly I'm not arguing that
> yaml validation in general is useless. I'm in agreement that we want
> dts files to be able to be formally validated because it catches
> typos, missing properties, and bugs. I am _only_ saying that I still
> haven't seen a good argument for why we need to validate the top-level
> compatible string.

I don't feel expert enough on this topic to give you good answer. Which
does not prove that there isn't or there is such good answer.

> Since there no properties associated with the
> top-level compatible string, it's mostly just checking did some one
> copy-paste the compatible string from one file (the dts file) to the
> other file (the yaml file) correctly. To me, that does not feel like a
> useful check.

Still it can detect messing of SoC compatibles or not defining any
board-level compatible thus pretending that someone's board is just
SC7180. Imagine now user-space or bootloader trying to parse it...

BTW, the bindings validation of top-level compatible might actually help
you - to be sure that DTSes have proper compatibles matching what
bootloader expects.

> The other thing I wanted to make sure was that we weren't just going
> to get NAKed later if/when we had to adjust compatible strings on
> existing dts files.

Stable ABI is more of SoC maintainer decision and I see Bjorn responded
here.

> In any case, I guess I'll make an attempt to document the compatibles
> for existing Chromebooks and we'll see what happens. I'm still not
> convinced of the value, but as long as we're not going to get NAKed
> for documenting reality it's fine.


Best regards,
Krzysztof

Doug Anderson May 11, 2022, 5:49 p.m. UTC | #24

Hi,

On Wed, May 11, 2022 at 10:36 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> > * If we want to change our scheme, we'd need to sit down and come to
> > an agreement that satisfies everyone, if such a thing is possible.
>
> There is open CFP for ELCE 2022 (in Ireland). Maybe we could organize
> some session there? But we for sure would need Rob, so the arrangements
> should rather focus on him, not on my availability.

Looks plausible to me to make it.


> > I mean, to be fair I said it _seems_ pure overhead and then said that
> > we could do it if it makes some tools happy. ...but before doing that,
> > I wanted to make sure it was actually valuable. I still have doubts
> > about the assertion that the most specific compatible is guaranteed to
> > uniquely identify hardware. So if the whole reason for doing this is
> > to make the validation tools happy and there's no other value, then at
> > least it's plausible to argue that the tools could simply be fixed to
> > allow this and not shout about it.
>
> Instead of adding bindings, you can indeed change/fix the tools. Go
> ahead. :)

I will try to take a quick look to see what this would look like.


> > Since there no properties associated with the
> > top-level compatible string, it's mostly just checking did some one
> > copy-paste the compatible string from one file (the dts file) to the
> > other file (the yaml file) correctly. To me, that does not feel like a
> > useful check.
>
> Still it can detect messing of SoC compatibles or not defining any
> board-level compatible thus pretending that someone's board is just
> SC7180. Imagine now user-space or bootloader trying to parse it...
>
> BTW, the bindings validation of top-level compatible might actually help
> you - to be sure that DTSes have proper compatibles matching what
> bootloader expects.

I'm still not seeing the help here. Is it somehow going to be easier
for someone to sneak in a dts file to the kernel tree that is just
"sc7180" than it will be to sneak an entry into the bindings that is
just "sc7180"? The people reviewing the dts and the list of allowed
boards in the bindings are the same people, right? Every entry in the
bindings gets used to match exactly one board, so, as I said, it's
pretty much just a question of whether you copy-pasted properly...

-Doug

Doug Anderson May 12, 2022, 4:08 p.m. UTC | #25

Hi,

On Wed, May 11, 2022 at 10:49 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, May 11, 2022 at 10:36 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > > * If we want to change our scheme, we'd need to sit down and come to
> > > an agreement that satisfies everyone, if such a thing is possible.
> >
> > There is open CFP for ELCE 2022 (in Ireland). Maybe we could organize
> > some session there? But we for sure would need Rob, so the arrangements
> > should rather focus on him, not on my availability.
>
> Looks plausible to me to make it.
>
>
> > > I mean, to be fair I said it _seems_ pure overhead and then said that
> > > we could do it if it makes some tools happy. ...but before doing that,
> > > I wanted to make sure it was actually valuable. I still have doubts
> > > about the assertion that the most specific compatible is guaranteed to
> > > uniquely identify hardware. So if the whole reason for doing this is
> > > to make the validation tools happy and there's no other value, then at
> > > least it's plausible to argue that the tools could simply be fixed to
> > > allow this and not shout about it.
> >
> > Instead of adding bindings, you can indeed change/fix the tools. Go
> > ahead. :)
>
> I will try to take a quick look to see what this would look like.

I looked a bit and decided that unless maintainers agreed that we
should do this that it would just be a waste of time. I guess I'll
save it for the next time I see Rob...


> > > Since there no properties associated with the
> > > top-level compatible string, it's mostly just checking did some one
> > > copy-paste the compatible string from one file (the dts file) to the
> > > other file (the yaml file) correctly. To me, that does not feel like a
> > > useful check.
> >
> > Still it can detect messing of SoC compatibles or not defining any
> > board-level compatible thus pretending that someone's board is just
> > SC7180. Imagine now user-space or bootloader trying to parse it...
> >
> > BTW, the bindings validation of top-level compatible might actually help
> > you - to be sure that DTSes have proper compatibles matching what
> > bootloader expects.
>
> I'm still not seeing the help here. Is it somehow going to be easier
> for someone to sneak in a dts file to the kernel tree that is just
> "sc7180" than it will be to sneak an entry into the bindings that is
> just "sc7180"? The people reviewing the dts and the list of allowed
> boards in the bindings are the same people, right? Every entry in the
> bindings gets used to match exactly one board, so, as I said, it's
> pretty much just a question of whether you copy-pasted properly...

After a bit of time of copy pasting, we now have a 3-patch series that
starts with:

https://lore.kernel.org/r/20220512090429.1.I9804fcd5d6c8552ab25f598dd7a3ea71b15b55f0@changeid

-Doug

CHROMIUM: arm64: dts: qcom: Add sc7180-gelarshie

Commit Message

Comments

Patch