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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Date:   Mon, 3 Oct 2022 08:45:26 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Dzmitry Sankouski <dsankouski@...il.com>
Cc:     linux-kernel@...r.kernel.org, Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 1/2] arm64: dts: qcom: sagit: add initial device tree
 for sagit

On 02/10/2022 20:36, Dzmitry Sankouski wrote:
>>> +
>>> +     bluetooth {
>>> +             compatible = "qcom,wcn3990-bt";
>>> +
>>> +             vddio-supply = <&vreg_s4a_1p8>;
>>> +             vddxo-supply = <&vreg_l7a_1p8>;
>>> +             vddrf-supply = <&vreg_l17a_1p3>;
>>> +             vddch0-supply = <&vreg_l25a_3p3>;
>>> +             max-speed = <3200000>;
>>> +     };
>>> +};
>>> +
>>> +&blsp1_uart3_on {
>>> +     rx {
>>
>> Missing suffix pins
>>
> 'rx' pin should be renamed with the corresponding pin in msm8998.dtsi file,
> since we're overriding the pin here, and rename 'rx' pins in other
> msm8998-based dts. Is that what you mean?
> What are the possible suffix names? AFAIU, it can be either 'active' or
> 'suspend', right?

Trim the replies. It takes time to scroll through unrelated context.

If you override node form msm8998.dtsi, then it is fine.

Otherwise it would be suffix "pins", as I wrote.


> 
>>
>>> +             /delete-property/ bias-disable;
>>> +             /*
>>> +              * Configure a pull-up on 46 (RX). This is needed to
>>> +              * avoid garbage data when the TX pin of the Bluetooth
>>> +              * module is in tri-state (module powered off or not
>>> +              * driving the signal yet).
>>> +              */
>>> +             bias-pull-up;
>>> +     };
>>> +
>>> +     cts {
>>
>> Missing suffix pins

This also can be skipped, since it is override.

>>
>>> +             /delete-property/ bias-disable;
>>> +             /*
>>> +              * Configure a pull-down on 47 (CTS) to match the pull
>>> +              * of the Bluetooth module.
>>> +              */
>>> +             bias-pull-down;
>>> +     };
>>> +};
>>> +
>>> +&blsp2_uart1 {
>>> +     status = "okay";
>>> +};
>>> +
>>> +&pm8005_lsid1 {
>>> +     pm8005-regulators {
>>
>> This is just "regulators", right?
>>
>>> +             compatible = "qcom,pm8005-regulators";
>>> +
>>> +             vdd_s1-supply = <&vph_pwr>;
>>> +
>>> +             pm8005_s1: s1 { /* VDD_GFX supply */
>>> +                     regulator-min-microvolt = <524000>;
>>> +                     regulator-max-microvolt = <1100000>;
>>> +                     regulator-enable-ramp-delay = <500>;
>>> +
>>> +                     /* hack until we rig up the gpu consumer */
>>> +                     regulator-always-on;
>>> +             };
>>> +     };
>>> +};
>>> +
>>> +&pm8998_gpio {
>>> +     vol_up_key_default: vol-up-key-default-state {
>>> +             pins = "gpio6";
>>> +             function = "normal";
>>> +             bias-pull-up;
>>> +             input-enable;
>>> +             qcom,drive-strength = <PMIC_GPIO_STRENGTH_NO>;
>>> +     };
>>> +
>>> +     audio_mclk_pin: audio-mclk-pin-active-state {
>>> +             pins = "gpio13";
>>> +             function = "func2";
>>> +             power-source = <0>;
>>> +     };
>>> +};
>>> +
>>> +&qusb2phy {
>>> +     status = "okay";
>>> +
>>> +     vdda-pll-supply = <&vreg_l12a_1p8>;
>>> +     vdda-phy-dpdm-supply = <&vreg_l24a_3p075>;
>>> +};
>>> +
>>> +&rpm_requests {
>>> +     pm8998-regulators {
>>
>> This is for sure now regulators (and since you have two: regulators-0).
>>
> There's no 'regulators-0' occurrences in 6.0.0-rc6 kernel, i.e. it's a new
> convention. Why do we need this new convention? I mean, other msm8998
> boards regulators  are named by driver name, and changing that seems like
> rename refactoring.

Do you see "pm8998-regulators" supported? If not, then how it is "rename
refactoring"?

https://lore.kernel.org/all/20220901093243.134288-1-krzysztof.kozlowski@linaro.org/

> 
>>
>>> +             compatible = "qcom,rpm-pm8998-regulators";
>>> +
>>> +             vdd_s1-supply = <&vph_pwr>;
>>> +             vdd_s2-supply = <&vph_pwr>;
>>> +             vdd_s3-supply = <&vph_pwr>;
>>> +             vdd_s4-supply = <&vph_pwr>;
>>> +             vdd_s5-supply = <&vph_pwr>;
>>> +             vdd_s6-supply = <&vph_pwr>;
>>> +             vdd_s7-supply = <&vph_pwr>;
>>> +             vdd_s8-supply = <&vph_pwr>;
>>> +             vdd_s9-supply = <&vph_pwr>;
>>> +             vdd_s10-supply = <&vph_pwr>;
>>> +             vdd_s11-supply = <&vph_pwr>;
>>> +             vdd_s12-supply = <&vph_pwr>;
>>> +             vdd_s13-supply = <&vph_pwr>;
>>> +             vdd_l1_l27-supply = <&vreg_s7a_1p025>;
>>> +             vdd_l2_l8_l17-supply = <&vreg_s3a_1p35>;
>>> +             vdd_l3_l11-supply = <&vreg_s7a_1p025>;
>>> +             vdd_l4_l5-supply = <&vreg_s7a_1p025>;
>>> +             vdd_l6-supply = <&vreg_s5a_2p04>;
>>> +             vdd_l7_l12_l14_l15-supply = <&vreg_s5a_2p04>;
>>> +             vdd_l9-supply = <&vreg_bob>;
>>> +             vdd_l10_l23_l25-supply = <&vreg_bob>;
>>> +             vdd_l13_l19_l21-supply = <&vreg_bob>;
>>> +             vdd_l16_l28-supply = <&vreg_bob>;
>>> +             vdd_l18_l22-supply = <&vreg_bob>;
>>> +             vdd_l20_l24-supply = <&vreg_bob>;
>>> +             vdd_l26-supply = <&vreg_s3a_1p35>;
>>> +             vdd_lvs1_lvs2-supply = <&vreg_s4a_1p8>;
>>> +
>>> +             vreg_s3a_1p35: s3 {
>>> +                     regulator-min-microvolt = <1352000>;
>>> +                     regulator-max-microvolt = <1352000>;
>>> +             };
>>> +
>>> +             vreg_s4a_1p8: s4 {
>>> +                     regulator-min-microvolt = <1800000>;
>>> +                     regulator-max-microvolt = <1800000>;
>>> +                     regulator-allow-set-load;
>>> +             };
>>> +
>>> +             vreg_s5a_2p04: s5 {
>>> +                     regulator-min-microvolt = <1904000>;
>>> +                     regulator-max-microvolt = <2040000>;
>>> +             };
>>> +
>>> +             vreg_s7a_1p025: s7 {
>>> +                     regulator-min-microvolt = <900000>;
>>> +                     regulator-max-microvolt = <1028000>;
>>> +             };
>>> +
>>> +             vreg_l1a_0p875: l1 {
>>> +                     regulator-min-microvolt = <880000>;
>>> +                     regulator-max-microvolt = <880000>;
>>> +             };
>>> +
>>> +             vreg_l2a_1p2: l2 {
>>> +                     regulator-min-microvolt = <1200000>;
>>> +                     regulator-max-microvolt = <1200000>;
>>> +             };
>>> +
>>> +             vreg_l3a_1p0: l3 {
>>> +                     regulator-min-microvolt = <1000000>;
>>> +                     regulator-max-microvolt = <1000000>;
>>> +             };
>>> +
>>> +             vreg_l5a_0p8: l5 {
>>> +                     regulator-min-microvolt = <800000>;
>>> +                     regulator-max-microvolt = <800000>;
>>> +             };
>>> +
>>> +             vreg_l6a_1p8: l6 {
>>> +                     regulator-min-microvolt = <1800000>;
>>> +                     regulator-max-microvolt = <1800000>;
>>> +             };
>>> +
>>> +             vreg_l7a_1p8: l7 {
>>> +                     regulator-min-microvolt = <1800000>;
>>> +                     regulator-max-microvolt = <1800000>;
>>> +             };
>>> +
>>> +             vreg_l8a_1p2: l8 {
>>> +                     regulator-min-microvolt = <1200000>;
>>> +                     regulator-max-microvolt = <1200000>;
>>> +             };
>>> +
>>> +             vreg_l9a_1p8: l9 {
>>> +                     regulator-min-microvolt = <1808000>;
>>> +                     regulator-max-microvolt = <2960000>;
>>> +             };
>>> +
>>> +             vreg_l10a_1p8: l10 {
>>> +                     regulator-min-microvolt = <1808000>;
>>> +                     regulator-max-microvolt = <2960000>;
>>> +             };
>>> +
>>> +             vreg_l11a_1p0: l11 {
>>> +                     regulator-min-microvolt = <1000000>;
>>> +                     regulator-max-microvolt = <1000000>;
>>> +             };
>>> +
>>> +             vreg_l12a_1p8: l12 {
>>> +                     regulator-min-microvolt = <1800000>;
>>> +                     regulator-max-microvolt = <1800000>;
>>> +             };
>>> +
>>> +             vreg_l13a_2p95: l13 {
>>> +                     regulator-min-microvolt = <1808000>;
>>> +                     regulator-max-microvolt = <2960000>;
>>> +             };
>>> +
>>> +             vreg_l14a_1p8: l14 {
>>> +                     regulator-min-microvolt = <1800000>;
>>> +                     regulator-max-microvolt = <1800000>;
>>> +             };
>>> +
>>> +             vreg_l15a_1p8: l15 {
>>> +                     regulator-min-microvolt = <1800000>;
>>> +                     regulator-max-microvolt = <1800000>;
>>> +             };
>>> +
>>> +             vreg_l16a_2p7: l16 {
>>> +                     regulator-min-microvolt = <2704000>;
>>> +                     regulator-max-microvolt = <2704000>;
>>> +             };
>>> +
>>> +             vreg_l17a_1p3: l17 {
>>> +                     regulator-min-microvolt = <1304000>;
>>> +                     regulator-max-microvolt = <1304000>;
>>> +             };
>>> +
>>> +             vreg_l18a_2p7: l18 {
>>> +                     regulator-min-microvolt = <2704000>;
>>> +                     regulator-max-microvolt = <2704000>;
>>> +             };
>>> +
>>> +             vreg_l19a_3p0: l19 {
>>> +                     regulator-min-microvolt = <3008000>;
>>> +                     regulator-max-microvolt = <3008000>;
>>> +             };
>>> +
>>> +             vreg_l20a_2p95: l20 {
>>> +                     regulator-min-microvolt = <2960000>;
>>> +                     regulator-max-microvolt = <2960000>;
>>> +                     regulator-allow-set-load;
>>> +             };
>>> +
>>> +             vreg_l21a_2p95: l21 {
>>> +                     regulator-min-microvolt = <2960000>;
>>> +                     regulator-max-microvolt = <2960000>;
>>> +                     regulator-system-load = <800000>;
>>> +                     regulator-allow-set-load;
>>> +             };
>>> +
>>> +             vreg_l22a_2p85: l22 {
>>> +                     regulator-min-microvolt = <2864000>;
>>> +                     regulator-max-microvolt = <2864000>;
>>> +             };
>>> +
>>> +             vreg_l23a_3p3: l23 {
>>> +                     regulator-min-microvolt = <3312000>;
>>> +                     regulator-max-microvolt = <3312000>;
>>> +             };
>>> +
>>> +             vreg_l24a_3p075: l24 {
>>> +                     regulator-min-microvolt = <3088000>;
>>> +                     regulator-max-microvolt = <3088000>;
>>> +             };
>>> +
>>> +             vreg_l25a_3p3: l25 {
>>> +                     regulator-min-microvolt = <3104000>;
>>> +                     regulator-max-microvolt = <3312000>;
>>> +             };
>>> +
>>> +             vreg_l26a_1p2: l26 {
>>> +                     regulator-min-microvolt = <1200000>;
>>> +                     regulator-max-microvolt = <1200000>;
>>> +                     regulator-allow-set-load;
>>> +             };
>>> +
>>> +             vreg_l28_3p0: l28 {
>>> +                     regulator-min-microvolt = <3008000>;
>>> +                     regulator-max-microvolt = <3008000>;
>>> +             };
>>> +
>>> +             vreg_lvs1a_1p8: lvs1 { };
>>> +
>>> +             vreg_lvs2a_1p8: lvs2 { };
>>> +     };
>>> +
>>> +     pmi8998-regulators {
>>
>> regulators-1
>>
>>> +             compatible = "qcom,rpm-pmi8998-regulators";
>>> +
>>> +             vdd_bob-supply = <&vph_pwr>;
>>> +
>>> +             vreg_bob: bob {
>>> +                     regulator-min-microvolt = <3312000>;
>>> +                     regulator-max-microvolt = <3600000>;
>>> +             };
>>> +     };
>>> +};
>>> +
>>> +&tlmm {
>>> +     gpio-reserved-ranges = <0 4>, <81 4>;
>>> +
>>> +     cci1_default: cci1-default {
>>
>> Missing suffix state. The same in all other places.
>>
>>> +             pins = "gpio18", "gpio19";
>>> +             function = "cci_i2c";
>>> +             bias-disable;
>>> +             drive-strength = <2>;
>>> +     };
>>> +
>>
>> (...)
>>
>>> +
>>> +&wifi {
>>> +     status = "okay";
>>> +     vdd-0.8-cx-mx-supply = <&vreg_l5a_0p8>;
>>> +     vdd-1.8-xo-supply = <&vreg_l7a_1p8>;
>>> +     vdd-1.3-rfa-supply = <&vreg_l17a_1p3>;
>>> +     vdd-3.3-ch0-supply = <&vreg_l25a_3p3>;
>>> +};
>>> diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi
>> b/arch/arm64/boot/dts/qcom/pm8998.dtsi
>>> index d09f2954b6f9..4551af463081 100644
>>> --- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
>>> @@ -52,6 +52,12 @@ pm8998_pwrkey: pwrkey {
>>>                               bias-pull-up;
>>>                               linux,code = <KEY_POWER>;
>>>                       };
>>> +
>>> +                     pm8998_resin: resin {
>>
>> Missing suffix state

This comment is probably wrong - it's resin, not pin control.

>>
>>> +                             compatible = "qcom,pm8941-resin";
>>> +                             bias-pull-up;
>>> +                             interrupts = <GIC_SPI 0x8 1
>> IRQ_TYPE_EDGE_BOTH>;


Best regards,
Krzysztof

Powered by blists - more mailing lists