From: Rajendra Nayak <rnayak@codeaurora.org>
To: Konrad Dybcio <konrad.dybcio@somainline.org>,
agross@kernel.org, bjorn.andersson@linaro.org,
robh+dt@kernel.org
Cc: linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, skakit@codeaurora.org
Subject: Re: [PATCH 2/2] arm64: dts: qcom: sc7280-idp: Add device tree files for IDP2
Date: Mon, 2 Aug 2021 18:06:27 +0530 [thread overview]
Message-ID: <bb608936-65e0-ed68-9410-44a40faf2f61@codeaurora.org> (raw)
In-Reply-To: <aedc1f03-59ef-ca17-59e6-a6464367990d@somainline.org>
On 8/2/2021 5:08 PM, Konrad Dybcio wrote:
>
> Hi!
>> Move all the common device tree bits for both sc7280 IDPs into a
>> sc7280-idp.dtsi and create 2 different dts files (sc7280-idp.dts
>> and sc7280-idp2.dts) in order to manage differences across the
>> IDP SKU1 and SKU2 Boards.
>> PMR735A is present on IDP board only and is not present on IDP2.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>> arch/arm64/boot/dts/qcom/Makefile | 1 +
>> arch/arm64/boot/dts/qcom/sc7280-idp.dts | 352 +--------------------
>
> At this point, wouldn't it be more clear to rename it to sc7280-idp-sku1.dts and the other one to sc7280-idp-sku2.dts?
I wasn't really sure if its a good idea to change an existing board dts name and if that's
going to break something someplace with build scripts etc, but if none of that's a concern,
then yes, perhaps I could name them idp1 and idp2 instead of the current idp and idp2 which
I agree is a little confusing.
>> .../dts/qcom/{sc7280-idp.dts => sc7280-idp.dtsi} | 48 +--
>> arch/arm64/boot/dts/qcom/sc7280-idp2.dts | 24 ++
>> 4 files changed, 29 insertions(+), 396 deletions(-)
>> copy arch/arm64/boot/dts/qcom/{sc7280-idp.dts => sc7280-idp.dtsi} (87%)
>> create mode 100644 arch/arm64/boot/dts/qcom/sc7280-idp2.dts
>>
>> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
>> index a571bfdd..50cf2cf 100644
>> --- a/arch/arm64/boot/dts/qcom/Makefile
>> +++ b/arch/arm64/boot/dts/qcom/Makefile
>> @@ -61,6 +61,7 @@ dtb-$(CONFIG_ARCH_QCOM) += sc7180-trogdor-pompom-r3-lte.dtb
>> dtb-$(CONFIG_ARCH_QCOM) += sc7180-trogdor-r1.dtb
>> dtb-$(CONFIG_ARCH_QCOM) += sc7180-trogdor-r1-lte.dtb
>> dtb-$(CONFIG_ARCH_QCOM) += sc7280-idp.dtb
>> +dtb-$(CONFIG_ARCH_QCOM) += sc7280-idp2.dtb
>> dtb-$(CONFIG_ARCH_QCOM) += sdm630-sony-xperia-ganges-kirin.dtb
>> dtb-$(CONFIG_ARCH_QCOM) += sdm630-sony-xperia-nile-discovery.dtb
>> dtb-$(CONFIG_ARCH_QCOM) += sdm630-sony-xperia-nile-pioneer.dtb
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> index 73225e3..f014cda 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> @@ -7,19 +7,12 @@
>>
>> /dts-v1/;
>>
>> -#include <dt-bindings/gpio/gpio.h>
>> -#include <dt-bindings/iio/qcom,spmi-adc7-pmr735a.h>
>> -#include <dt-bindings/iio/qcom,spmi-adc7-pmr735b.h>
>
> Don't you want pmr735a include to stay here in SKU1 and not be included in the common one, as the hw is not present on sku2?
[]..
>> #include "pmr735a.dtsi"
Right, that's why this ^^^ include stays here and is not part of the common dtsi.
>> -#include "pm8350c.dtsi"
>> -#include "pmk8350.dtsi"
>>
>> / {
>> - model = "Qualcomm Technologies, Inc. sc7280 IDP platform";
>> + model = "Qualcomm Technologies, Inc. sc7280 IDP SKU1 platform";
>> compatible = "qcom,sc7280-idp", "google,senor", "qcom,sc7280";
>>
>> aliases {
>> @@ -31,187 +24,7 @@
>> };
>> };
>>
>> -&apps_rsc {
>> - pm7325-regulators {
>> - compatible = "qcom,pm7325-rpmh-regulators";
>> - qcom,pmic-id = "b";
>> -
>> - vreg_s1b_1p8: smps1 {
>> - regulator-min-microvolt = <1856000>;
>> - regulator-max-microvolt = <2040000>;
>> - };
>> -
>> - vreg_s7b_0p9: smps7 {
>> - regulator-min-microvolt = <535000>;
>> - regulator-max-microvolt = <1120000>;
>> - };
>> -
>> - vreg_s8b_1p2: smps8 {
>> - regulator-min-microvolt = <1256000>;
>> - regulator-max-microvolt = <1500000>;
>> - };
>> -
>> - vreg_l1b_0p8: ldo1 {
>> - regulator-min-microvolt = <825000>;
>> - regulator-max-microvolt = <925000>;
>> - };
>> -
>> - vreg_l2b_3p0: ldo2 {
>> - regulator-min-microvolt = <2700000>;
>> - regulator-max-microvolt = <3544000>;
>> - };
>> -
>> - vreg_l6b_1p2: ldo6 {
>> - regulator-min-microvolt = <1140000>;
>> - regulator-max-microvolt = <1260000>;
>> - };
>> -
>> - vreg_l7b_2p9: ldo7 {
>> - regulator-min-microvolt = <2960000>;
>> - regulator-max-microvolt = <2960000>;
>> - };
>> -
>> - vreg_l8b_0p9: ldo8 {
>> - regulator-min-microvolt = <870000>;
>> - regulator-max-microvolt = <970000>;
>> - };
>> -
>> - vreg_l9b_1p2: ldo9 {
>> - regulator-min-microvolt = <1080000>;
>> - regulator-max-microvolt = <1304000>;
>> - };
>> -
>> - vreg_l11b_1p7: ldo11 {
>> - regulator-min-microvolt = <1504000>;
>> - regulator-max-microvolt = <2000000>;
>> - };
>> -
>> - vreg_l12b_0p8: ldo12 {
>> - regulator-min-microvolt = <751000>;
>> - regulator-max-microvolt = <824000>;
>> - };
>> -
>> - vreg_l13b_0p8: ldo13 {
>> - regulator-min-microvolt = <530000>;
>> - regulator-max-microvolt = <824000>;
>> - };
>> -
>> - vreg_l14b_1p2: ldo14 {
>> - regulator-min-microvolt = <1080000>;
>> - regulator-max-microvolt = <1304000>;
>> - };
>> -
>> - vreg_l15b_0p8: ldo15 {
>> - regulator-min-microvolt = <765000>;
>> - regulator-max-microvolt = <1020000>;
>> - };
>> -
>> - vreg_l16b_1p2: ldo16 {
>> - regulator-min-microvolt = <1100000>;
>> - regulator-max-microvolt = <1300000>;
>> - };
>> -
>> - vreg_l17b_1p8: ldo17 {
>> - regulator-min-microvolt = <1700000>;
>> - regulator-max-microvolt = <1900000>;
>> - };
>> -
>> - vreg_l18b_1p8: ldo18 {
>> - regulator-min-microvolt = <1800000>;
>> - regulator-max-microvolt = <2000000>;
>> - };
>> -
>> - vreg_l19b_1p8: ldo19 {
>> - regulator-min-microvolt = <1800000>;
>> - regulator-max-microvolt = <1800000>;
>> - };
>> - };
>> -
>> - pm8350c-regulators {
>> - compatible = "qcom,pm8350c-rpmh-regulators";
>> - qcom,pmic-id = "c";
>> -
>> - vreg_s1c_2p2: smps1 {
>> - regulator-min-microvolt = <2190000>;
>> - regulator-max-microvolt = <2210000>;
>> - };
>> -
>> - vreg_s9c_1p0: smps9 {
>> - regulator-min-microvolt = <1010000>;
>> - regulator-max-microvolt = <1170000>;
>> - };
>> -
>> - vreg_l1c_1p8: ldo1 {
>> - regulator-min-microvolt = <1800000>;
>> - regulator-max-microvolt = <1980000>;
>> - };
>> -
>> - vreg_l2c_1p8: ldo2 {
>> - regulator-min-microvolt = <1620000>;
>> - regulator-max-microvolt = <1980000>;
>> - };
>> -
>> - vreg_l3c_3p0: ldo3 {
>> - regulator-min-microvolt = <2800000>;
>> - regulator-max-microvolt = <3540000>;
>> - };
>> -
>> - vreg_l4c_1p8: ldo4 {
>> - regulator-min-microvolt = <1620000>;
>> - regulator-max-microvolt = <3300000>;
>> - };
>> -
>> - vreg_l5c_1p8: ldo5 {
>> - regulator-min-microvolt = <1620000>;
>> - regulator-max-microvolt = <3300000>;
>> - };
>> -
>> - vreg_l6c_2p9: ldo6 {
>> - regulator-min-microvolt = <1800000>;
>> - regulator-max-microvolt = <2950000>;
>> - };
>> -
>> - vreg_l7c_3p0: ldo7 {
>> - regulator-min-microvolt = <3000000>;
>> - regulator-max-microvolt = <3544000>;
>> - };
>> -
>> - vreg_l8c_1p8: ldo8 {
>> - regulator-min-microvolt = <1620000>;
>> - regulator-max-microvolt = <2000000>;
>> - };
>> -
>> - vreg_l9c_2p9: ldo9 {
>> - regulator-min-microvolt = <2960000>;
>> - regulator-max-microvolt = <2960000>;
>> - };
>> -
>> - vreg_l10c_0p8: ldo10 {
>> - regulator-min-microvolt = <720000>;
>> - regulator-max-microvolt = <1050000>;
>> - };
>> -
>> - vreg_l11c_2p8: ldo11 {
>> - regulator-min-microvolt = <2800000>;
>> - regulator-max-microvolt = <3544000>;
>> - };
>> -
>> - vreg_l12c_1p8: ldo12 {
>> - regulator-min-microvolt = <1650000>;
>> - regulator-max-microvolt = <2000000>;
>> - };
>> -
>> - vreg_l13c_3p0: ldo13 {
>> - regulator-min-microvolt = <2700000>;
>> - regulator-max-microvolt = <3544000>;
>> - };
>> -
>> - vreg_bob: bob {
>> - regulator-min-microvolt = <3008000>;
>> - regulator-max-microvolt = <3960000>;
>> - };
>> - };
>> -
>> +&apps_rsc{
>> pmr735a-regulators {
>> compatible = "qcom,pmr735a-rpmh-regulators";
>> qcom,pmic-id = "e";
>> @@ -242,162 +55,3 @@
>> };
>> };
>> };
>> -
>> -&pmk8350_vadc {
>> - pm8350_die_temp {
>> - reg = <PM8350_ADC7_DIE_TEMP>;
>> - label = "pm8350_die_temp";
>> - qcom,pre-scaling = <1 1>;
>> - };
>> -
>> - pmk8350_die_temp {
>> - reg = <PMK8350_ADC7_DIE_TEMP>;
>> - label = "pmk8350_die_temp";
>> - qcom,pre-scaling = <1 1>;
>> - };
>> -
>> - pmr735a_die_temp {
>> - reg = <PMR735A_ADC7_DIE_TEMP>;
>> - label = "pmr735a_die_temp";
>> - qcom,pre-scaling = <1 1>;
>> - };
>> -
>> - pmr735b_die_temp {
>> - reg = <PMR735B_ADC7_DIE_TEMP>;
>> - label = "pmr735b_die_temp";
>> - qcom,pre-scaling = <1 1>;
>> - };
>> -};
>
> It may be Thunderbird messing with me again, but it looks like there's a tab too many in this block.
Its not your mailer, the existing file seems to have the extra tab.
>> -
>> -&qupv3_id_0 {
>> - status = "okay";
>> -};
>> -
>> -&sdhc_1 {
>> - status = "okay";
>> -
>> - pinctrl-names = "default", "sleep";
>> - pinctrl-0 = <&sdc1_on>;
>> - pinctrl-1 = <&sdc1_off>;
>> -
>> - non-removable;
>> - no-sd;
>> - no-sdio;
>> -
>> - vmmc-supply = <&vreg_l7b_2p9>;
>> - vqmmc-supply = <&vreg_l19b_1p8>;
>> -};
>> -
>> -&sdhc_2 {
>> - status = "okay";
>> -
>> - pinctrl-names = "default", "sleep";
>> - pinctrl-0 = <&sdc2_on>;
>> - pinctrl-1 = <&sdc2_off>;
>> -
>> - vmmc-supply = <&vreg_l9c_2p9>;
>> - vqmmc-supply = <&vreg_l6c_2p9>;
>> -
>> - cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;
>> -};
>> -
>> -&uart5 {
>> - status = "okay";
>> -};
>> -
>> -&usb_1 {
>> - status = "okay";
>> -};
>> -
>> -&usb_1_dwc3 {
>> - dr_mode = "host";
>> -};
>> -
>> -&usb_1_hsphy {
>> - status = "okay";
>> -
>> - vdda-pll-supply = <&vreg_l10c_0p8>;
>> - vdda33-supply = <&vreg_l2b_3p0>;
>> - vdda18-supply = <&vreg_l1c_1p8>;
>> -};
>> -
>> -&usb_1_qmpphy {
>> - status = "okay";
>> -
>> - vdda-phy-supply = <&vreg_l6b_1p2>;
>> - vdda-pll-supply = <&vreg_l1b_0p8>;
>> -};
>> -
>> -&usb_2 {
>> - status = "okay";
>> -};
>> -
>> -&usb_2_dwc3 {
>> - dr_mode = "peripheral";
>> -};
>> -
>> -&usb_2_hsphy {
>> - status = "okay";
>> -
>> - vdda-pll-supply = <&vreg_l10c_0p8>;
>> - vdda33-supply = <&vreg_l2b_3p0>;
>> - vdda18-supply = <&vreg_l1c_1p8>;
>> -};
>> -
>> -/* PINCTRL - additions to nodes defined in sc7280.dtsi */
>> -
>> -&qup_uart5_default {
>> - tx {
>> - pins = "gpio46";
>> - drive-strength = <2>;
>> - bias-disable;
>> - };
>> -
>> - rx {
>> - pins = "gpio47";
>> - drive-strength = <2>;
>> - bias-pull-up;
>> - };
>> -};
>> -
>> -&sdc1_on {
>> - clk {
>> - bias-disable;
>> - drive-strength = <16>;
>> - };
>> -
>> - cmd {
>> - bias-pull-up;
>> - drive-strength = <10>;
>> - };
>> -
>> - data {
>> - bias-pull-up;
>> - drive-strength = <10>;
>> - };
>> -
>> - rclk {
>> - bias-pull-down;
>> - };
>> -};
>> -
>> -&sdc2_on {
>> - clk {
>> - bias-disable;
>> - drive-strength = <16>;
>> - };
>> -
>> - cmd {
>> - bias-pull-up;
>> - drive-strength = <10>;
>> - };
>> -
>> - data {
>> - bias-pull-up;
>> - drive-strength = <10>;
>> - };
>> -
>> - sd-cd {
>> - bias-pull-up;
>> - };
>> -};
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> similarity index 87%
>> copy from arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> copy to arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> index 73225e3..295d88a 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> @@ -1,12 +1,10 @@
>> // SPDX-License-Identifier: BSD-3-Clause
>> /*
>> - * sc7280 IDP board device tree source
>> + * sc7280 IDP board device tree source (common between SKU1 and SKU2)
>> *
>> * Copyright (c) 2021, The Linux Foundation. All rights reserved.
>> */
>>
>> -/dts-v1/;
>> -
>> #include <dt-bindings/gpio/gpio.h>
>> #include <dt-bindings/iio/qcom,spmi-adc7-pmr735a.h>
>> #include <dt-bindings/iio/qcom,spmi-adc7-pmr735b.h>
>> @@ -14,23 +12,9 @@
>> #include <dt-bindings/iio/qcom,spmi-adc7-pmk8350.h>
>> #include "sc7280.dtsi"
>> #include "pm7325.dtsi"
>> -#include "pmr735a.dtsi"
>> #include "pm8350c.dtsi"
>> #include "pmk8350.dtsi"
>>
>> -/ {
>> - model = "Qualcomm Technologies, Inc. sc7280 IDP platform";
>> - compatible = "qcom,sc7280-idp", "google,senor", "qcom,sc7280";
>> -
>> - aliases {
>> - serial0 = &uart5;
>> - };
>> -
>> - chosen {
>> - stdout-path = "serial0:115200n8";
>> - };
>> -};
>
> Shouldn't these be common if they were there before on SKU1 and now are there on SKU2?
I was expecting other things inside of these nodes to be different as more patches get
added on top so left these in the resp. dts files.
>> -
>> &apps_rsc {
>> pm7325-regulators {
>> compatible = "qcom,pm7325-rpmh-regulators";
>> @@ -211,36 +195,6 @@
>> regulator-max-microvolt = <3960000>;
>> };
>> };
>> -
>> - pmr735a-regulators {
>> - compatible = "qcom,pmr735a-rpmh-regulators";
>> - qcom,pmic-id = "e";
>> -
>> - vreg_l2e_1p2: ldo2 {
>> - regulator-min-microvolt = <1200000>;
>> - regulator-max-microvolt = <1200000>;
>> - };
>> -
>> - vreg_l3e_0p9: ldo3 {
>> - regulator-min-microvolt = <912000>;
>> - regulator-max-microvolt = <1020000>;
>> - };
>> -
>> - vreg_l4e_1p7: ldo4 {
>> - regulator-min-microvolt = <1776000>;
>> - regulator-max-microvolt = <1890000>;
>> - };
>> -
>> - vreg_l5e_0p8: ldo5 {
>> - regulator-min-microvolt = <800000>;
>> - regulator-max-microvolt = <800000>;
>> - };
>> -
>> - vreg_l6e_0p8: ldo6 {
>> - regulator-min-microvolt = <480000>;
>> - regulator-max-microvolt = <904000>;
>> - };
>> - };
>> };
>>
>> &pmk8350_vadc {
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp2.dts b/arch/arm64/boot/dts/qcom/sc7280-idp2.dts
>> new file mode 100644
>> index 0000000..c842da6
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp2.dts
>> @@ -0,0 +1,24 @@
>> +// SPDX-License-Identifier: BSD-3-Clause
>> +/*
>> + * sc7280 IDP2 board device tree source
>> + *
>> + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include "sc7280.dtsi"
>
> sc7280.dtsi is already included in the sc7280-idp.dtsi file.
ah yes, I will drop this.
>
>> +#include "sc7280-idp.dtsi"
>> +
>> +/ {
>> + model = "Qualcomm Technologies, Inc. sc7280 IDP SKU2 platform";
>> + compatible = "qcom,sc7280-idp2", "qcom,sc7280";
>
> No "google,something" compatible on SKU2?
Thats a good point, I need to go back and check with how this would
be handled by the bootloader, thanks for catching it.
Appreciate you taking time out to review.
>
>> +
>> + aliases {
>> + serial0 = &uart5;
>> + };
>> +
>> + chosen {
>> + stdout-path = "serial0:115200n8";
>> + };
>> +};
>>
>
> Konrad
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
next prev parent reply other threads:[~2021-08-02 12:36 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-02 9:39 [PATCH 0/2] Add DT files for sc7280 IDP2 board Rajendra Nayak
2021-08-02 9:39 ` [PATCH 1/2] dt-bindings: arm: qcom: Document qcom,sc7280-idp2 board Rajendra Nayak
2021-08-03 8:30 ` Stephen Boyd
2021-08-02 9:39 ` [PATCH 2/2] arm64: dts: qcom: sc7280-idp: Add device tree files for IDP2 Rajendra Nayak
2021-08-02 11:38 ` Konrad Dybcio
2021-08-02 12:36 ` Rajendra Nayak [this message]
2021-08-03 4:33 ` Rajendra Nayak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bb608936-65e0-ed68-9410-44a40faf2f61@codeaurora.org \
--to=rnayak@codeaurora.org \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=konrad.dybcio@somainline.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=skakit@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.