Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1583-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 2 Feb 2024 10:47:29 -0600
From: Bjorn Andersson <andersson@kernel.org>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Konrad Dybcio <konrad.dybcio@linaro.org>, 
	Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, 
	Conor Dooley <conor+dt@kernel.org>, Marcel Holtmann <marcel@holtmann.org>, 
	Luiz Augusto von Dentz <luiz.dentz@gmail.com>, Bjorn Helgaas <bhelgaas@google.com>, 
	Neil Armstrong <neil.armstrong@linaro.org>, Alex Elder <elder@linaro.org>, 
	Srini Kandagatla <srinivas.kandagatla@linaro.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
	Arnd Bergmann <arnd@arndb.de>, Abel Vesa <abel.vesa@linaro.org>, 
	Manivannan Sadhasivam <mani@kernel.org>, Lukas Wunner <lukas@wunner.de>, linux-arm-msm@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-bluetooth@vger.kernel.org, 
	linux-pci@vger.kernel.org, Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Subject: Re: [RFC 2/9] arm64: dts: qcom: qrb5165-rb5: model the PMU of the
 QCA6391
Message-ID: <zyiimnkpifo3mb54e2oyqmw6xcjtzkwodxuq5y7vac3slizfkm@mtiijlxeahuk>
References: <20240201155532.49707-1-brgl@bgdev.pl>
 <20240201155532.49707-3-brgl@bgdev.pl>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240201155532.49707-3-brgl@bgdev.pl>

On Thu, Feb 01, 2024 at 04:55:25PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Add a node for the PMU module of the QCA6391 present on the RB5 board.
> Assign its LDO power outputs to the existing Bluetooth module. Add a
> node for the PCIe port to sm8250.dtsi and define the WLAN node on it in
> the board's .dts and also make it consume the power outputs of the PMU.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++--
>  arch/arm64/boot/dts/qcom/sm8250.dtsi     |  10 ++
>  2 files changed, 127 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> index cd0db4f31d4a..fab5bebafbad 100644
> --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
> @@ -108,6 +108,87 @@ lt9611_3v3: lt9611-3v3 {
>  		regulator-always-on;
>  	};
>  
> +	qca6390_pmu: pmu@0 {

The node doesn't have an address, so why does it have a unit address?
Also, the node isn't referenced, so please skip the label.

> +		compatible = "qcom,qca6390-pmu";
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&bt_en_state>, <&wlan_en_state>;
> +
> +		vddaon-supply = <&vreg_s6a_0p95>;
> +		vddpmu-supply = <&vreg_s2f_0p95>;
> +		vddrfa1-supply = <&vreg_s2f_0p95>;
> +		vddrfa2-supply = <&vreg_s8c_1p3>;
> +		vddrfa3-supply = <&vreg_s5a_1p9>;
> +		vddpcie1-supply = <&vreg_s8c_1p3>;
> +		vddpcie2-supply = <&vreg_s5a_1p9>;
> +		vddio-supply = <&vreg_s4a_1p8>;

So, after studying the datasheet for this. The names of the pins seems
to come from the existing binding(s). As you're introducing a new
binding (and driver) for qcom,qca6390-pmu, please use the pad names from
qca6390.

> +
> +		wlan-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>;
> +		bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
> +
> +		regulators {
> +			vreg_pmu_rfa_cmn: ldo0 {
> +				regulator-name = "vreg_pmu_rfa_cmn";
> +				regulator-min-microvolt = <760000>;
> +				regulator-max-microvolt = <840000>;

The min/max operating range of the regulator is something we provide in
the implementation, in DeviceTree you should specify the expected
operating voltage. Based on the TYP value this should be
regulator-min-microvolt = regulator-max-microvolt = 0.8V.

> +			};
> +
> +			vreg_pmu_aon_0p59: ldo1 {
> +				regulator-name = "vreg_pmu_aon_0p59";
> +				regulator-min-microvolt = <540000>;
> +				regulator-max-microvolt = <840000>;
> +			};
[..]
> @@ -1303,6 +1402,14 @@ sdc2_card_det_n: sd-card-det-n-state {
>  		function = "gpio";
>  		bias-pull-up;
>  	};
> +
> +	wlan_en_state: wlan-default-state {
> +		pins = "gpio20";
> +		function = "gpio";
> +		drive-strength = <16>;
> +		output-low;

Please omit output-low here.

> +		bias-pull-up;

Why do you drive it low and pull it high? bias-disable sounds more
appropriate.

Regards,
Bjorn

> +	};
>  };
>  
>  &uart6 {
> @@ -1311,17 +1418,16 @@ &uart6 {
>  	bluetooth {
>  		compatible = "qcom,qca6390-bt";
>  
> -		pinctrl-names = "default";
> -		pinctrl-0 = <&bt_en_state>;
> -
> -		enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
> -
> -		vddio-supply = <&vreg_s4a_1p8>;
> -		vddpmu-supply = <&vreg_s2f_0p95>;
> -		vddaon-supply = <&vreg_s6a_0p95>;
> -		vddrfa0p9-supply = <&vreg_s2f_0p95>;
> -		vddrfa1p3-supply = <&vreg_s8c_1p3>;
> -		vddrfa1p9-supply = <&vreg_s5a_1p9>;
> +		vddrfacmn-supply = <&vreg_pmu_rfa_cmn>;
> +		vddaon-supply = <&vreg_pmu_aon_0p59>;
> +		vddwlcx-supply = <&vreg_pmu_wlcx_0p8>;
> +		vddwlmx-supply = <&vreg_pmu_wlmx_0p85>;
> +		vddbtcmx-supply = <&vreg_pmu_btcmx_0p85>;
> +		vddrfa0-supply = <&vreg_pmu_rfa_0p8>;
> +		vddrfa1-supply = <&vreg_pmu_rfa_1p2>;
> +		vddrfa2-supply = <&vreg_pmu_rfa_1p7>;
> +		vddpcie0-supply = <&vreg_pmu_pcie_0p9>;
> +		vddpcie1-supply = <&vreg_pmu_pcie_1p8>;
>  	};
>  };
>  
> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index 4d849e98bf9b..7cd21d4e7278 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> @@ -2203,6 +2203,16 @@ pcie0: pcie@1c00000 {
>  			dma-coherent;
>  
>  			status = "disabled";
> +
> +			pcieport0: pcie@0 {
> +				device_type = "pci";
> +				reg = <0x0 0x0 0x0 0x0 0x0>;
> +				#address-cells = <3>;
> +				#size-cells = <2>;
> +				ranges;
> +
> +				bus-range = <0x01 0xff>;
> +			};
>  		};
>  
>  		pcie0_phy: phy@1c06000 {
> -- 
> 2.40.1
>