Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 15 Jun 2021 19:29:49 -0500
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Martin Botka <martin.botka@somainline.org>
Cc:     ~postmarketos/upstreaming@lists.sr.ht,
        konrad.dybcio@somainline.org,
        angelogioacchino.delregno@somainline.org,
        marijn.suijten@somainline.org, jamipkettunen@somainline.org,
        Andy Gross <agross@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 1/2] arch: arm64: dts: qcom: Add support for SM6125
Message-ID: <YMlF/aPn+253UIHn@builder.lan>
References: <20210613080522.25230-1-martin.botka@somainline.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210613080522.25230-1-martin.botka@somainline.org>
Precedence: bulk

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@somainline.org>

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.

> +			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?

> +				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.

> +				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.

> +			status = "disabled";
> +
> +			usb3_dwc3: dwc3@4e00000 {

These should be usb@ now.

> +				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