Message ID | 20230314210828.2049720-1-bhupesh.sharma@linaro.org |
---|---|
Headers | show |
Series | arm64: dts: qcom: Add Qualcomm RB2 board dts | expand |
On 14.03.2023 22:08, Bhupesh Sharma wrote: > Add DTS for Qualcomm qrb4210-rb2 board which uses SM4250 SoC. > > This adds debug uart, emmc, uSD and tlmm support along with > regulators found on this board. > > Also defines the 'xo_board' and 'sleep_clk' frequencies for > this board. > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > --- [...] > +#include <dt-bindings/regulator/qcom,rpmh-regulator.h> This SoC does not feature RPMh, drop. > +#include "sm4250.dtsi" > + > +/ { > + model = "Qualcomm Technologies, Inc. QRB4210 RB2"; > + compatible = "qcom,qrb4210-rb2", "qcom,sm4250"; Please add a qcom,qrb4210 between the board-specific and the common SoC compatibles so that we can address QRB-specific quirks if such ever arise. > + > + aliases { > + serial0 = &uart4; > + }; > + [...] > +&xo_board { > + clock-frequency = <19200000>; > +}; > + > +&sleep_clk { > + clock-frequency = <32000>; > +}; Out of alphanumerical order > + > +&qupv3_id_0 { > + status = "okay"; > +}; > + > +&uart4 { > + status = "okay"; > +}; > + > +&rpm_requests { Out of alphanumerical order > + regulators-0 { Will there be more PMICs under this node? If not, drop the -0. [...] > +&tlmm { > + gpio-reserved-ranges = <37 5>, <43 2>, <47 1>, > + <49 1>, <52 1>, <54 1>, > + <56 3>, <61 2>, <64 1>, > + <68 1>, <72 8>, <96 1>; > +}; Are there *really* so many? Does the board refuse to boot if you knock off any of these entries? If so, they probably don't belong here. > + > +&sdhc_1 { > + status = "okay"; Status should go last > + > + vmmc-supply = <&vreg_l24a_2p96>; /* emmc power line */ > + vqmmc-supply = <&vreg_l11a_1p8>; /* emmc vddq */ The comments are not very useful, drop please. > + bus-width = <8>; This is defined in the SoC dtsi already > + no-sdio; > + non-removable; > +}; > + > +&sdhc_2 { > + status = "okay"; > + > + cd-gpios = <&tlmm 88 GPIO_ACTIVE_HIGH>; /* card detect gpio */ > + vmmc-supply = <&vreg_l22a_2p96>; /* Card power line */ > + vqmmc-supply = <&vreg_l5a_2p96>; /* IO line power */ > + bus-width = <4>; > + no-sdio; > + no-emmc; Ditto Konrad > +};
On 3/15/23 2:55 AM, Konrad Dybcio wrote: > > > On 14.03.2023 22:08, Bhupesh Sharma wrote: >> Add DTS for Qualcomm qrb4210-rb2 board which uses SM4250 SoC. >> >> This adds debug uart, emmc, uSD and tlmm support along with >> regulators found on this board. >> >> Also defines the 'xo_board' and 'sleep_clk' frequencies for >> this board. >> >> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> >> --- > [...] > >> +#include <dt-bindings/regulator/qcom,rpmh-regulator.h> > This SoC does not feature RPMh, drop. Ok. >> +#include "sm4250.dtsi" >> + >> +/ { >> + model = "Qualcomm Technologies, Inc. QRB4210 RB2"; >> + compatible = "qcom,qrb4210-rb2", "qcom,sm4250"; > Please add a qcom,qrb4210 between the board-specific and the common SoC > compatibles so that we can address QRB-specific quirks if such ever arise. As per the available documentation there are no qrb specific quirks as of now, but let me add a qcom,qrb4210 for future compatibility. >> + >> + aliases { >> + serial0 = &uart4; >> + }; >> + > [...] > >> +&xo_board { >> + clock-frequency = <19200000>; >> +}; >> + >> +&sleep_clk { >> + clock-frequency = <32000>; >> +}; > Out of alphanumerical order Ok. >> + >> +&qupv3_id_0 { >> + status = "okay"; >> +}; >> + >> +&uart4 { >> + status = "okay"; >> +}; >> + >> +&rpm_requests { > Out of alphanumerical order Ok. >> + regulators-0 { > Will there be more PMICs under this node? If not, drop the -0. Ok. > [...] > >> +&tlmm { >> + gpio-reserved-ranges = <37 5>, <43 2>, <47 1>, >> + <49 1>, <52 1>, <54 1>, >> + <56 3>, <61 2>, <64 1>, >> + <68 1>, <72 8>, <96 1>; >> +}; > Are there *really* so many? Does the board refuse to boot if > you knock off any of these entries? If so, they probably > don't belong here. Yes, these are reserved / not-connected gpios as per latest version of the board schematics. >> + >> +&sdhc_1 { >> + status = "okay"; > Status should go last >> + >> + vmmc-supply = <&vreg_l24a_2p96>; /* emmc power line */ >> + vqmmc-supply = <&vreg_l11a_1p8>; /* emmc vddq */ > The comments are not very useful, drop please. > >> + bus-width = <8>; > This is defined in the SoC dtsi already Ok. >> + no-sdio; >> + non-removable; >> +}; >> + >> +&sdhc_2 { >> + status = "okay"; >> + >> + cd-gpios = <&tlmm 88 GPIO_ACTIVE_HIGH>; /* card detect gpio */ >> + vmmc-supply = <&vreg_l22a_2p96>; /* Card power line */ >> + vqmmc-supply = <&vreg_l5a_2p96>; /* IO line power */ >> + bus-width = <4>; >> + no-sdio; >> + no-emmc; > Ditto Ok. Will send updated v2 soon. Thanks.