Re: [PATCH v2] arm64: dts: qcom: starqltechn: add initial device tree for starqltechn

[Krzysztof Kozlowski]

On 17/06/2022 04:01, Dzmitry Sankouski wrote:
> New device support - Samsung S9 (SM-G9600) phone
> What works:
> - simple framebuffer
> - storage (both main and sdcard)
> - ramoops
> 
> Signed-off-by: Dzmitry Sankouski <dsankouski@xxxxxxxxx>
> ---
> Changes for v2:
> - rebase on Linus's master branch
> 
>  arch/arm64/boot/dts/qcom/Makefile             |   1 +
>  .../dts/qcom/sdm845-samsung-starqltechn.dts   |  45 ++
>  .../dts/qcom/sdm845-samsung-starqltechn.dtsi  | 448 ++++++++++++++++++
>  3 files changed, 494 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/qcom/sdm845-samsung-starqltechn.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/sdm845-samsung-starqltechn.dtsi
> 
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index 2f8aec2cc6db..85140db507b6 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -103,6 +103,7 @@ dtb-$(CONFIG_ARCH_QCOM)	+= sdm845-db845c.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sdm845-mtp.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sdm845-oneplus-enchilada.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sdm845-oneplus-fajita.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= sdm845-samsung-starqltechn.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sdm845-sony-xperia-tama-akari.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sdm845-sony-xperia-tama-akatsuki.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sdm845-sony-xperia-tama-apollo.dtb
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-samsung-starqltechn.dts b/arch/arm64/boot/dts/qcom/sdm845-samsung-starqltechn.dts
> new file mode 100644
> index 000000000000..53bb2ebf0061
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sdm845-samsung-starqltechn.dts
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SDM845 Samsung S9 (SM-G9600) (starqltechn) specific device tree
> + *
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#include "sdm845-samsung-starqltechn.dtsi"
> +
> +/ {
> +	model = "Samsung S9 SM-G9600";
> +	compatible = "samsung,starqltechn", "qcom,sdm845";

Missing documentation. I guess you should rebase on my series adding
compatibles for sdm845.

Bjorn, can we get my series merged? It's really more and more DTs
needing it.

https://lore.kernel.org/all/20220521164550.91115-7-krzysztof.kozlowski@xxxxxxxxxx/

> +
> +	chosen {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +		bootargs = "console=tty1 loglevel=15 clk_ignore_unused PMOS_NO_OUTPUT_REDIRECT";

This should not be in DTS. Especially debugging stuff... console should
be selected by stdout-path.

> +		framebuffer: framebuffer@9D400000 {

lowercase addresses everywhere

> +			compatible = "simple-framebuffer";
> +			reg = <0 0x9D400000 0 (2960 * 1440 * 4)>;//2400000
> +			width = <1440>;
> +			height = <2960>;
> +			stride = <(1440 * 4)>;
> +			format = "a8r8g8b8";
> +		};
> +	};
> +
> +

Just one blank line.

> +	reserved-memory {
> +		framebuffer_region@9d400000 {
> +			reg = <0x0 0x9d400000 0x0 0x02400000>;
> +			no-map;
> +		};
> +
> +		ramoops@A1300000 {

lowercase hex. In entire file.

> +			compatible = "ramoops";
> +			reg = <0x0 0xa1300000 0x0 0x100000>;
> +			record-size = <0x40000>;
> +			console-size = <0x40000>;
> +			ftrace-size = <0x40000>;
> +			pmsg-size = <0x40000>;
> +		};
> +	};
> +};
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-samsung-starqltechn.dtsi b/arch/arm64/boot/dts/qcom/sdm845-samsung-starqltechn.dtsi
> new file mode 100644
> index 000000000000..1d640d88c61a
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sdm845-samsung-starqltechn.dtsi

Why do you have DTSI for one DTS?


> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SDM845 Samsung S9 (SM-G9600) (starqltechn / star2qltechn) common device tree source
> + *
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
> +#include "sdm845.dtsi"
> +
> +/ {
> +	chassis-type = "handset";
> +
> +	vph_pwr: vph-pwr-regulator {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vph_pwr";
> +		regulator-min-microvolt = <3700000>;
> +		regulator-max-microvolt = <3700000>;
> +	};
> +
> +	/*
> +	 * Apparently RPMh does not provide support for PM8998 S4 because it
> +	 * is always-on; model it as a fixed regulator.
> +	 */
> +	vreg_s4a_1p8: pm8998-smps4 {

Consistent naming. Please apply all comments from your other patchset...

> +		compatible = "regulator-fixed";
> +		regulator-name = "vreg_s4a_1p8";
> +
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +
> +		regulator-always-on;
> +		regulator-boot-on;
> +
> +		vin-supply = <&vph_pwr>;
> +	};
> +};
> +
> +

One blank line.

> +&apps_rsc {
> +	pm8998-rpmh-regulators {
> +		compatible = "qcom,pm8998-rpmh-regulators";
> +		qcom,pmic-id = "a";
> +



Best regards,
Krzysztof