Re: [PATCH v3 1/2] arch: arm64: dts: qcom: Add support for SM6125

[Martin Botka]

On Tue, Jun 15 2021 at 07:29:49 PM -0500, Bjorn Andersson 
<bjorn.andersson@xxxxxxxxxx> wrote:
> On Sun 13 Jun 03:05 CDT 2021, Martin Botka wrote:
>
> >  This commits adds the Device tree file for SM6125 SoC.
> >
> >  Signed-off-by: Martin Botka <martin.botka@xxxxxxxxxxxxxx>
>
> Thanks for your work on this Martin, just spotted a few minor 
> finishing
> touches below.

:)

> >  ---
> >  Changes in V2:
> >  Update compatibles for mailbox & pinctrl
> >  Changes in V3:
> >  Fix reg for sdhci1
> >  Replace hc_mem with hc and core_mem with core
> >   arch/arm64/boot/dts/qcom/sm6125.dtsi | 603 
> > +++++++++++++++++++++++++++
> >   1 file changed, 603 insertions(+)
> >   create mode 100644 arch/arm64/boot/dts/qcom/sm6125.dtsi
> >
> >  diff --git a/arch/arm64/boot/dts/qcom/sm6125.dtsi 
> > b/arch/arm64/boot/dts/qcom/sm6125.dtsi
> [..]
> >  +	soc {
> >  +		#address-cells = <1>;
> >  +		#size-cells = <1>;
> >  +		ranges = <0x00 0x00 0x00 0xffffffff>;
> >  +		compatible = "simple-bus";
> >  +
> >  +		tcsr_mutex_regs: syscon@340000 {
>
> It's no longer valid to have a stray syscon like this, so please 
> update
> this in accordance with the tcsr mutex binding.
>
> If this platform needs to poke at the registers at the end of the 
> memory
> region, you can do compatible = "qcom,tcsr-mutex", "syscon"; to make 
> it
> represent both things.

ack

> >  +			compatible = "syscon";
> >  +			reg = <0x00340000 0x20000>;
> >  +		};
> >  +
> >  +		tlmm: pinctrl@500000 {
> >  +			compatible = "qcom,sm6125-tlmm";
> >  +			reg = <0x00500000 0x400000>,
> >  +				<0x00900000 0x400000>,
> >  +				<0x00d00000 0x400000>;
> >  +			reg-names = "west", "south", "east";
> >  +			interrupts = <GIC_SPI 227 IRQ_TYPE_LEVEL_HIGH>;
> >  +			gpio-controller;
> >  +			gpio-ranges = <&tlmm 0 0 134>;
> >  +			#gpio-cells = <2>;
> >  +			interrupt-controller;
> >  +			#interrupt-cells = <2>;
> >  +
> >  +			sdc2_state_on: sdc2-on {
>
> As I just forced Konrad to move the on-state definition to the board
> file, can you please do the same?

ofc

> >  +				clk {
> >  +					pins = "sdc2_clk";
> >  +					bias-disable;
> >  +					drive-strength = <16>;
> >  +				};
> >  +
> >  +				cmd {
> >  +					pins = "sdc2_cmd";
> >  +					bias-pull-up;
> >  +					drive-strength = <10>;
> >  +				};
> >  +
> >  +				data {
> >  +					pins = "sdc2_data";
> >  +					bias-pull-up;
> >  +					drive-strength = <10>;
> >  +				};
> >  +
> >  +				sd-cd {
> >  +					pins = "gpio98";
> >  +					bias-pull-up;
> >  +					drive-strength = <2>;
> >  +				};
> >  +			};
> >  +
> >  +			sdc2_state_off: sdc2-off {
>
> This should be common between all boards (except possibly the cd 
> line),
> so this is okay to share here.

Do you want me to move the cd as well or
do you want it to stay in here?

> >  +				clk {
> >  +					pins = "sdc2_clk";
> >  +					bias-disable;
> >  +					drive-strength = <2>;
> >  +				};
> >  +
> >  +				cmd {
> >  +					pins = "sdc2_cmd";
> >  +					bias-pull-up;
> >  +					drive-strength = <2>;
> >  +				};
> >  +
> >  +				data {
> >  +					pins = "sdc2_data";
> >  +					bias-pull-up;
> >  +					drive-strength = <2>;
> >  +				};
> >  +
> >  +				sd-cd {
> >  +					pins = "gpio98";
> >  +					bias-disable;
> >  +					drive-strength = <2>;
> >  +				};
> >  +			};
> >  +		};
> >  +
> [..]
> >  +
> >  +		usb3: usb@4ef8800 {
> >  +			compatible = "qcom,msm8996-dwc3", "qcom,dwc3";
> >  +			reg = <0x04ef8800 0x400>;
> >  +			#address-cells = <1>;
> >  +			#size-cells = <1>;
> >  +			ranges;
> >  +
> >  +			clocks = <&gcc GCC_USB30_PRIM_MASTER_CLK>,
> >  +				<&gcc GCC_SYS_NOC_USB3_PRIM_AXI_CLK>,
> >  +				<&gcc GCC_CFG_NOC_USB3_PRIM_AXI_CLK>,
> >  +				<&gcc GCC_USB3_PRIM_CLKREF_CLK>,
> >  +				<&gcc GCC_USB30_PRIM_SLEEP_CLK>,
> >  +				<&gcc GCC_USB30_PRIM_MOCK_UTMI_CLK>;
> >  +
> >  +			assigned-clocks = <&gcc GCC_USB30_PRIM_MOCK_UTMI_CLK>,
> >  +					  <&gcc GCC_USB30_PRIM_MASTER_CLK>;
> >  +			assigned-clock-rates = <19200000>, <66666667>;
> >  +
> >  +			power-domains = <&gcc USB30_PRIM_GDSC>;
> >  +			qcom,select-utmi-as-pipe-clk;
>
> Stray space at the end of this line.

Oopsie.

> >  +			status = "disabled";
> >  +
> >  +			usb3_dwc3: dwc3@4e00000 {
>
> These should be usb@ now.

ack

> >  +				compatible = "snps,dwc3";
> >  +				reg = <0x04e00000 0xcd00>;
> >  +				interrupts = <GIC_SPI 255 IRQ_TYPE_LEVEL_HIGH>;
> >  +				phys = <&hsusb_phy1>;
> >  +				phy-names = "usb2-phy";
> >  +				snps,dis_u2_susphy_quirk;
> >  +				snps,dis_enblslpm_quirk;
> >  +				maximum-speed = "high-speed";
> >  +				dr_mode = "peripheral";
> >  +			};
> >  +		};
>
> Thanks,
> Bjorn