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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).