| Message ID | 1637650813-16654-4-git-send-email-rnayak@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | arm64: dts: qcom: Add support for the sc7280 CRD board | expand |
On Tue, Nov 23, 2021 at 12:30:12PM +0530, Rajendra Nayak wrote: > Subject: arm64: dts: qcom: sc7280: Define EC and H1 nodes that seems to suggest that EC and H1 nodes are something generic of the sc7280, however these two chips are only present on systems that target Chrome OS, and the specific nodes are added are only used by the QCA sc7280 IDP and CRD, not other sc7280 boards using Chrome OS, like herobrine. I suggest to change it to "arm64: dts: qcom: sc7280: Define EC and H1 nodes for IDP/CRD". > From: Kshitiz Godara <kgodara@codeaurora.org> > > The IDP2 and CRD boards share the EC and H1 parts, so define > all related device nodes into a common file and include them > in the idp2 and crd dts files to avoid duplication. > > Signed-off-by: Kshitiz Godara <kgodara@codeaurora.org> > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > --- > arch/arm64/boot/dts/qcom/sc7280-crd.dts | 1 + > arch/arm64/boot/dts/qcom/sc7280-ec-h1.dtsi | 110 +++++++++++++++++++++++++++++ > arch/arm64/boot/dts/qcom/sc7280-idp2.dts | 1 + > 3 files changed, 112 insertions(+) > create mode 100644 arch/arm64/boot/dts/qcom/sc7280-ec-h1.dtsi > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-crd.dts > index 09d02c2..8c2aee6 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts > +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts > @@ -8,6 +8,7 @@ > /dts-v1/; > > #include "sc7280-idp.dtsi" > +#include "sc7280-ec-h1.dtsi" > > / { > model = "Qualcomm Technologies, Inc. sc7280 CRD platform"; > diff --git a/arch/arm64/boot/dts/qcom/sc7280-ec-h1.dtsi b/arch/arm64/boot/dts/qcom/sc7280-ec-h1.dtsi > new file mode 100644 > index 0000000..78fb5eb > --- /dev/null > +++ b/arch/arm64/boot/dts/qcom/sc7280-ec-h1.dtsi Similar comment as for the subject, the file name seems to imply that the include could be useful for any board with an EC and H1, however it will be only used by the IDP and CRD. Maybe name it 'sc7280-idp-ec-h1.dtsi', from the CRD DT file it is alreay clear that it is related with the IDP, so it shouldn't be too confusing that the file name only says IDP. Also a birdie told me that the EC and H1 configuration is going to change in future revisions of the CRD, which is another reason for being more specific with the file name (a sc7280-crd-ec-h1.dtsi might be needed at that point, or the new not-any-longer-shared config goes directly into the sc7280-crd-revN.dts. > @@ -0,0 +1,110 @@ > +// SPDX-License-Identifier: BSD-3-Clause > +/* > + * sc7280 EC/H1 over SPI (common between IDP2 and CRD) > + * > + * Copyright (c) 2021, The Linux Foundation. All rights reserved. > + */ > + > +ap_ec_spi: &spi10 { > + status = "okay"; > + > + pinctrl-0 = <&qup_spi10_cs_gpio_init_high>, <&qup_spi10_cs_gpio>; Shouldn't this also have <&qup_spi10_data_clk>? > + cs-gpios = <&tlmm 43 GPIO_ACTIVE_LOW>; > + > + cros_ec: ec@0 { > + compatible = "google,cros-ec-spi"; > + reg = <0>; > + interrupt-parent = <&tlmm>; > + interrupts = <18 IRQ_TYPE_LEVEL_LOW>; > + pinctrl-names = "default"; > + pinctrl-0 = <&ap_ec_int_l>; > + spi-max-frequency = <3000000>; > + > + cros_ec_pwm: ec-pwm { > + compatible = "google,cros-ec-pwm"; > + #pwm-cells = <1>; > + }; > + > + i2c_tunnel: i2c-tunnel { > + compatible = "google,cros-ec-i2c-tunnel"; > + google,remote-bus = <0>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + typec { > + compatible = "google,cros-ec-typec"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + usb_c0: connector@0 { > + compatible = "usb-c-connector"; > + reg = <0>; > + label = "left"; > + power-role = "dual"; > + data-role = "host"; > + try-power-role = "source"; > + }; > + > + usb_c1: connector@1 { > + compatible = "usb-c-connector"; > + reg = <1>; > + label = "right"; > + power-role = "dual"; > + data-role = "host"; > + try-power-role = "source"; > + }; > + }; > + }; > +}; > + > +#include <arm/cros-ec-keyboard.dtsi> > +#include <arm/cros-ec-sbs.dtsi> > + > +ap_h1_spi: &spi14 { > + status = "okay"; > + > + pinctrl-0 = <&qup_spi14_cs_gpio_init_high>, <&qup_spi14_cs_gpio>; <&qup_spi14_data_clk> missing? > + cs-gpios = <&tlmm 59 GPIO_ACTIVE_LOW>; > + > + cr50: tpm@0 { > + compatible = "google,cr50"; > + reg = <0>; > + pinctrl-names = "default"; > + pinctrl-0 = <&h1_ap_int_odl>; > + spi-max-frequency = <800000>; > + interrupt-parent = <&tlmm>; > + interrupts = <104 IRQ_TYPE_EDGE_RISING>; > + }; > +}; > + > +&tlmm { > + ap_ec_int_l: ap-ec-int-l { > + pins = "gpio18"; > + function = "gpio"; > + input-enable; > + bias-pull-up; > + drive-strength = <2>; Is the explicit drive-strength setting actually needed? Documentation/devicetree/bindings/pinctrl/qcom,sc7280-pinctrl.yaml: drive-strength: enum: [2, 4, 6, 8, 10, 12, 14, 16] default: 2 <= description: Selects the drive strength for the specified pins, in mA. The default is 2, hence it shouldn't be necessary it set it explicitly. > + }; > + > + h1_ap_int_odl: h1-ap-int-odl { > + pins = "gpio104"; > + function = "gpio"; > + input-enable; > + bias-pull-up; > + drive-strength = <2>; see above > + }; > + > + qup_spi10_cs_gpio_init_high: qup-spi10-cs-gpio-init-high { > + pins = "gpio43"; > + output-high; > + drive-strength = <2>; see above > + }; > + > + qup_spi14_cs_gpio_init_high: qup-spi14-cs-gpio-init-high { > + pins = "gpio59"; > + output-high; > + drive-strength = <2>; see above > + }; > +}; > + > diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp2.dts b/arch/arm64/boot/dts/qcom/sc7280-idp2.dts > index 3ae9969..208ca69 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-idp2.dts > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp2.dts > @@ -8,6 +8,7 @@ > /dts-v1/; > > #include "sc7280-idp.dtsi" > +#include "sc7280-ec-h1.dtsi" > > / { > model = "Qualcomm Technologies, Inc. sc7280 IDP SKU2 platform";
On 11/23/2021 11:10 PM, Matthias Kaehlcke wrote: > On Tue, Nov 23, 2021 at 12:30:12PM +0530, Rajendra Nayak wrote: > >> Subject: arm64: dts: qcom: sc7280: Define EC and H1 nodes > > that seems to suggest that EC and H1 nodes are something generic of the > sc7280, however these two chips are only present on systems that target > Chrome OS, and the specific nodes are added are only used by the QCA > sc7280 IDP and CRD, not other sc7280 boards using Chrome OS, like > herobrine. I suggest to change it to "arm64: dts: qcom: sc7280: Define > EC and H1 nodes for IDP/CRD". sure makes sense > >> From: Kshitiz Godara <kgodara@codeaurora.org> >> >> The IDP2 and CRD boards share the EC and H1 parts, so define >> all related device nodes into a common file and include them >> in the idp2 and crd dts files to avoid duplication. >> >> Signed-off-by: Kshitiz Godara <kgodara@codeaurora.org> >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >> --- >> arch/arm64/boot/dts/qcom/sc7280-crd.dts | 1 + >> arch/arm64/boot/dts/qcom/sc7280-ec-h1.dtsi | 110 +++++++++++++++++++++++++++++ >> arch/arm64/boot/dts/qcom/sc7280-idp2.dts | 1 + >> 3 files changed, 112 insertions(+) >> create mode 100644 arch/arm64/boot/dts/qcom/sc7280-ec-h1.dtsi >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-crd.dts >> index 09d02c2..8c2aee6 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts >> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts >> @@ -8,6 +8,7 @@ >> /dts-v1/; >> >> #include "sc7280-idp.dtsi" >> +#include "sc7280-ec-h1.dtsi" >> >> / { >> model = "Qualcomm Technologies, Inc. sc7280 CRD platform"; >> diff --git a/arch/arm64/boot/dts/qcom/sc7280-ec-h1.dtsi b/arch/arm64/boot/dts/qcom/sc7280-ec-h1.dtsi >> new file mode 100644 >> index 0000000..78fb5eb >> --- /dev/null >> +++ b/arch/arm64/boot/dts/qcom/sc7280-ec-h1.dtsi > > Similar comment as for the subject, the file name seems to imply > that the include could be useful for any board with an EC and H1, > however it will be only used by the IDP and CRD. Maybe name it > 'sc7280-idp-ec-h1.dtsi', from the CRD DT file it is alreay clear > that it is related with the IDP, so it shouldn't be too confusing > that the file name only says IDP. > > Also a birdie told me that the EC and H1 configuration is going to > change in future revisions of the CRD, which is another reason for > being more specific with the file name (a sc7280-crd-ec-h1.dtsi > might be needed at that point, or the new not-any-longer-shared > config goes directly into the sc7280-crd-revN.dts. right I will rename it to sc7280-idp-ec-h1.dtsi > >> @@ -0,0 +1,110 @@ >> +// SPDX-License-Identifier: BSD-3-Clause >> +/* >> + * sc7280 EC/H1 over SPI (common between IDP2 and CRD) >> + * >> + * Copyright (c) 2021, The Linux Foundation. All rights reserved. >> + */ >> + >> +ap_ec_spi: &spi10 { >> + status = "okay"; >> + >> + pinctrl-0 = <&qup_spi10_cs_gpio_init_high>, <&qup_spi10_cs_gpio>; > > Shouldn't this also have <&qup_spi10_data_clk>? looks like it does, I'll add > >> + cs-gpios = <&tlmm 43 GPIO_ACTIVE_LOW>; >> + >> + cros_ec: ec@0 { >> + compatible = "google,cros-ec-spi"; >> + reg = <0>; >> + interrupt-parent = <&tlmm>; >> + interrupts = <18 IRQ_TYPE_LEVEL_LOW>; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&ap_ec_int_l>; >> + spi-max-frequency = <3000000>; >> + >> + cros_ec_pwm: ec-pwm { >> + compatible = "google,cros-ec-pwm"; >> + #pwm-cells = <1>; >> + }; >> + >> + i2c_tunnel: i2c-tunnel { >> + compatible = "google,cros-ec-i2c-tunnel"; >> + google,remote-bus = <0>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + }; >> + >> + typec { >> + compatible = "google,cros-ec-typec"; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + usb_c0: connector@0 { >> + compatible = "usb-c-connector"; >> + reg = <0>; >> + label = "left"; >> + power-role = "dual"; >> + data-role = "host"; >> + try-power-role = "source"; >> + }; >> + >> + usb_c1: connector@1 { >> + compatible = "usb-c-connector"; >> + reg = <1>; >> + label = "right"; >> + power-role = "dual"; >> + data-role = "host"; >> + try-power-role = "source"; >> + }; >> + }; >> + }; >> +}; >> + >> +#include <arm/cros-ec-keyboard.dtsi> >> +#include <arm/cros-ec-sbs.dtsi> >> + >> +ap_h1_spi: &spi14 { >> + status = "okay"; >> + >> + pinctrl-0 = <&qup_spi14_cs_gpio_init_high>, <&qup_spi14_cs_gpio>; > > <&qup_spi14_data_clk> missing? > >> + cs-gpios = <&tlmm 59 GPIO_ACTIVE_LOW>; >> + >> + cr50: tpm@0 { >> + compatible = "google,cr50"; >> + reg = <0>; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&h1_ap_int_odl>; >> + spi-max-frequency = <800000>; >> + interrupt-parent = <&tlmm>; >> + interrupts = <104 IRQ_TYPE_EDGE_RISING>; >> + }; >> +}; >> + >> +&tlmm { >> + ap_ec_int_l: ap-ec-int-l { >> + pins = "gpio18"; >> + function = "gpio"; >> + input-enable; >> + bias-pull-up; >> + drive-strength = <2>; > > Is the explicit drive-strength setting actually needed? > > Documentation/devicetree/bindings/pinctrl/qcom,sc7280-pinctrl.yaml: > > drive-strength: > enum: [2, 4, 6, 8, 10, 12, 14, 16] > default: 2 <= > description: > Selects the drive strength for the specified pins, in mA. > > The default is 2, hence it shouldn't be necessary it set it explicitly. right, will remove when i respin thanks for the review > >> + }; >> + >> + h1_ap_int_odl: h1-ap-int-odl { >> + pins = "gpio104"; >> + function = "gpio"; >> + input-enable; >> + bias-pull-up; >> + drive-strength = <2>; > > see above > >> + }; >> + >> + qup_spi10_cs_gpio_init_high: qup-spi10-cs-gpio-init-high { >> + pins = "gpio43"; >> + output-high; >> + drive-strength = <2>; > > see above > >> + }; >> + >> + qup_spi14_cs_gpio_init_high: qup-spi14-cs-gpio-init-high { >> + pins = "gpio59"; >> + output-high; >> + drive-strength = <2>; > > see above > >> + }; >> +}; >> + >> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp2.dts b/arch/arm64/boot/dts/qcom/sc7280-idp2.dts >> index 3ae9969..208ca69 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7280-idp2.dts >> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp2.dts >> @@ -8,6 +8,7 @@ >> /dts-v1/; >> >> #include "sc7280-idp.dtsi" >> +#include "sc7280-ec-h1.dtsi" >> >> / { >> model = "Qualcomm Technologies, Inc. sc7280 IDP SKU2 platform";
diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-crd.dts index 09d02c2..8c2aee6 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts @@ -8,6 +8,7 @@ /dts-v1/; #include "sc7280-idp.dtsi" +#include "sc7280-ec-h1.dtsi" / { model = "Qualcomm Technologies, Inc. sc7280 CRD platform"; diff --git a/arch/arm64/boot/dts/qcom/sc7280-ec-h1.dtsi b/arch/arm64/boot/dts/qcom/sc7280-ec-h1.dtsi new file mode 100644 index 0000000..78fb5eb --- /dev/null +++ b/arch/arm64/boot/dts/qcom/sc7280-ec-h1.dtsi @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: BSD-3-Clause +/* + * sc7280 EC/H1 over SPI (common between IDP2 and CRD) + * + * Copyright (c) 2021, The Linux Foundation. All rights reserved. + */ + +ap_ec_spi: &spi10 { + status = "okay"; + + pinctrl-0 = <&qup_spi10_cs_gpio_init_high>, <&qup_spi10_cs_gpio>; + cs-gpios = <&tlmm 43 GPIO_ACTIVE_LOW>; + + cros_ec: ec@0 { + compatible = "google,cros-ec-spi"; + reg = <0>; + interrupt-parent = <&tlmm>; + interrupts = <18 IRQ_TYPE_LEVEL_LOW>; + pinctrl-names = "default"; + pinctrl-0 = <&ap_ec_int_l>; + spi-max-frequency = <3000000>; + + cros_ec_pwm: ec-pwm { + compatible = "google,cros-ec-pwm"; + #pwm-cells = <1>; + }; + + i2c_tunnel: i2c-tunnel { + compatible = "google,cros-ec-i2c-tunnel"; + google,remote-bus = <0>; + #address-cells = <1>; + #size-cells = <0>; + }; + + typec { + compatible = "google,cros-ec-typec"; + #address-cells = <1>; + #size-cells = <0>; + + usb_c0: connector@0 { + compatible = "usb-c-connector"; + reg = <0>; + label = "left"; + power-role = "dual"; + data-role = "host"; + try-power-role = "source"; + }; + + usb_c1: connector@1 { + compatible = "usb-c-connector"; + reg = <1>; + label = "right"; + power-role = "dual"; + data-role = "host"; + try-power-role = "source"; + }; + }; + }; +}; + +#include <arm/cros-ec-keyboard.dtsi> +#include <arm/cros-ec-sbs.dtsi> + +ap_h1_spi: &spi14 { + status = "okay"; + + pinctrl-0 = <&qup_spi14_cs_gpio_init_high>, <&qup_spi14_cs_gpio>; + cs-gpios = <&tlmm 59 GPIO_ACTIVE_LOW>; + + cr50: tpm@0 { + compatible = "google,cr50"; + reg = <0>; + pinctrl-names = "default"; + pinctrl-0 = <&h1_ap_int_odl>; + spi-max-frequency = <800000>; + interrupt-parent = <&tlmm>; + interrupts = <104 IRQ_TYPE_EDGE_RISING>; + }; +}; + +&tlmm { + ap_ec_int_l: ap-ec-int-l { + pins = "gpio18"; + function = "gpio"; + input-enable; + bias-pull-up; + drive-strength = <2>; + }; + + h1_ap_int_odl: h1-ap-int-odl { + pins = "gpio104"; + function = "gpio"; + input-enable; + bias-pull-up; + drive-strength = <2>; + }; + + qup_spi10_cs_gpio_init_high: qup-spi10-cs-gpio-init-high { + pins = "gpio43"; + output-high; + drive-strength = <2>; + }; + + qup_spi14_cs_gpio_init_high: qup-spi14-cs-gpio-init-high { + pins = "gpio59"; + output-high; + drive-strength = <2>; + }; +}; + diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp2.dts b/arch/arm64/boot/dts/qcom/sc7280-idp2.dts index 3ae9969..208ca69 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-idp2.dts +++ b/arch/arm64/boot/dts/qcom/sc7280-idp2.dts @@ -8,6 +8,7 @@ /dts-v1/; #include "sc7280-idp.dtsi" +#include "sc7280-ec-h1.dtsi" / { model = "Qualcomm Technologies, Inc. sc7280 IDP SKU2 platform";