| Message ID | 1646923503-28847-1-git-send-email-quic_c_sbhanu@quicinc.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [V2] arm64: dts: qcom: sc7280: Enable gcc HW reset | expand |
On Thu 10 Mar 06:45 PST 2022, Shaik Sajida Bhanu wrote: Without looking at what Krzysztof suggested, I think $subject should reflect the fact that it's the reset of the two SDCC controllers that you're adding. Something like "arm64: dts:qcom: Add resets for SDCC controllers" would allow me to grasp the full purpose of the patch by only reading the subject. > Enable gcc HW reset for eMMC and SD card. > > Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com> > --- > > Changes since V1: > - Updated commit message, subject and comments in dtsi file as > suggested by Krzysztof Kozlowski. > --- > arch/arm64/boot/dts/qcom/sc7280.dtsi | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index c07765d..721abf5 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -881,6 +881,9 @@ > mmc-hs400-1_8v; > mmc-hs400-enhanced-strobe; > > + /* Add gcc hw reset dt entry for eMMC */ When you read this comment 2 weeks from now it won't add any value. > + resets = <&gcc GCC_SDCC1_BCR>; > + reset-names = "core_reset"; Doesn't seem that this binding has been converted to YAML, but the .txt binding doesn't mention "resets" and I don't see a reason for having a reset-names for a single reset. And please keep the empty line before the opp-table. Regards, Bjorn > sdhc1_opp_table: opp-table { > compatible = "operating-points-v2"; > > @@ -2686,6 +2689,9 @@ > > qcom,dll-config = <0x0007642c>; > > + /* Add gcc hw reset dt entry for SD card */ > + resets = <&gcc GCC_SDCC2_BCR>; > + reset-names = "core_reset"; > sdhc2_opp_table: opp-table { > compatible = "operating-points-v2"; > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation >
Hi, Thanks for the review. Please find the inline comments. Thanks, Sajida > -----Original Message----- > From: Bjorn Andersson <bjorn.andersson@linaro.org> > Sent: Friday, March 11, 2022 9:26 AM > To: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com> > Cc: krzk+dt@kernel.org; agross@kernel.org; robh+dt@kernel.org; linux-arm- > msm@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; Asutosh Das (QUIC) <quic_asutoshd@quicinc.com>; > Ram Prakash Gupta (QUIC) <quic_rampraka@quicinc.com>; Pradeep > Pragallapati (QUIC) <quic_pragalla@quicinc.com>; Sarthak Garg (QUIC) > <quic_sartgarg@quicinc.com>; Nitin Rawat (QUIC) > <quic_nitirawa@quicinc.com>; Sayali Lokhande (QUIC) > <quic_sayalil@quicinc.com> > Subject: Re: [PATCH V2] arm64: dts: qcom: sc7280: Enable gcc HW reset > > On Thu 10 Mar 06:45 PST 2022, Shaik Sajida Bhanu wrote: > > Without looking at what Krzysztof suggested, I think $subject should reflect > the fact that it's the reset of the two SDCC controllers that you're adding. > > Something like "arm64: dts:qcom: Add resets for SDCC controllers" would > allow me to grasp the full purpose of the patch by only reading the subject. Ok Sure > > > Enable gcc HW reset for eMMC and SD card. > > > > Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com> > > --- > > > > Changes since V1: > > - Updated commit message, subject and comments in dtsi file as > > suggested by Krzysztof Kozlowski. > > --- > > arch/arm64/boot/dts/qcom/sc7280.dtsi | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi > > b/arch/arm64/boot/dts/qcom/sc7280.dtsi > > index c07765d..721abf5 100644 > > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > > @@ -881,6 +881,9 @@ > > mmc-hs400-1_8v; > > mmc-hs400-enhanced-strobe; > > > > + /* Add gcc hw reset dt entry for eMMC */ > > When you read this comment 2 weeks from now it won't add any value. Ok will update the comment. > > > + resets = <&gcc GCC_SDCC1_BCR>; > > + reset-names = "core_reset"; > > Doesn't seem that this binding has been converted to YAML, but the .txt > binding doesn't mention "resets" and I don't see a reason for having a reset- > names for a single reset. > Thanks for the pointing, I will add resets entry in dt-bindings in next patch version. Just followed existing node formats: Ex: resets = <&gcc GCC_PCIE_1_BCR>; reset-names = "pci"; > And please keep the empty line before the opp-table. Sure > > Regards, > Bjorn > > > sdhc1_opp_table: opp-table { > > compatible = "operating-points-v2"; > > > > @@ -2686,6 +2689,9 @@ > > > > qcom,dll-config = <0x0007642c>; > > > > + /* Add gcc hw reset dt entry for SD card */ > > + resets = <&gcc GCC_SDCC2_BCR>; > > + reset-names = "core_reset"; > > sdhc2_opp_table: opp-table { > > compatible = "operating-points-v2"; > > > > -- > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a > > member of Code Aurora Forum, hosted by The Linux Foundation > >
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi index c07765d..721abf5 100644 --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi @@ -881,6 +881,9 @@ mmc-hs400-1_8v; mmc-hs400-enhanced-strobe; + /* Add gcc hw reset dt entry for eMMC */ + resets = <&gcc GCC_SDCC1_BCR>; + reset-names = "core_reset"; sdhc1_opp_table: opp-table { compatible = "operating-points-v2"; @@ -2686,6 +2689,9 @@ qcom,dll-config = <0x0007642c>; + /* Add gcc hw reset dt entry for SD card */ + resets = <&gcc GCC_SDCC2_BCR>; + reset-names = "core_reset"; sdhc2_opp_table: opp-table { compatible = "operating-points-v2";
Enable gcc HW reset for eMMC and SD card. Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com> --- Changes since V1: - Updated commit message, subject and comments in dtsi file as suggested by Krzysztof Kozlowski. --- arch/arm64/boot/dts/qcom/sc7280.dtsi | 6 ++++++ 1 file changed, 6 insertions(+)