| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3b89ed5e-52b6-f450-7504-4bf33a42c9b6@codeaurora.org>
Date: Wed, 24 Nov 2021 17:18:49 +0530
From: Rajendra Nayak <rnayak@...eaurora.org>
To: Matthias Kaehlcke <mka@...omium.org>
Cc: agross@...nel.org, bjorn.andersson@...aro.org, robh+dt@...nel.org,
linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, sboyd@...eaurora.org,
dianders@...omium.org, kgodara@...eaurora.org
Subject: Re: [PATCH 3/4] arm64: dts: qcom: sc7280: Define EC and H1 nodes
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@...eaurora.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@...eaurora.org>
>> Signed-off-by: Rajendra Nayak <rnayak@...eaurora.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";
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
Powered by blists - more mailing lists