Re: [PATCH V4 2/4] arm64: dts: sc7280: Add QUPv3 wrapper_0 nodes

[rajpat@codeaurora.org]

On 2021-07-26 21:32, Matthias Kaehlcke wrote:
> On Mon, Jul 26, 2021 at 07:10:45PM +0530, Rajesh Patil wrote:
>> From: Roja Rani Yarubandi <rojay@codeaurora.org>
>> 
>> Add QUPv3 wrapper_0 DT nodes for SC7280 SoC.
>> 
>> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
>> Signed-off-by: Rajesh Patil <rajpat@codeaurora.org>
>> ---
>> Changes in V4:
>>  - As per Bjorn's comment, added QUP Wrapper_0 nodes
>>    other than debug-uart node
>>  - Dropped interconnect votes for wrapper_0 node
>> 
>> Changes in V3:
>>  - Broken the huge V2 patch into 3 smaller patches.
>>    1. QSPI DT nodes
>>    2. QUP wrapper_0 DT nodes
>>    3. QUP wrapper_1 DT nodes
>> 
>> Changes in V2:
>>  - As per Doug's comments removed pinmux/pinconf subnodes.
>>  - As per Doug's comments split of SPI, UART nodes has been done.
>>  - Moved QSPI node before aps_smmu as per the order.
>> 
>>  arch/arm64/boot/dts/qcom/sc7280-idp.dts |  84 ++++
>>  arch/arm64/boot/dts/qcom/sc7280.dtsi    | 720 
>> ++++++++++++++++++++++++++++++++
>>  2 files changed, 804 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts 
>> b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> index b0bfd8e..f63cf51 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> @@ -358,6 +358,16 @@
>>  	vdda18-supply = <&vreg_l1c_1p8>;
>>  };
>> 
>> +&uart7 {
>> +	status = "okay";
>> +
>> +	/delete-property/interrupts;
>> +	interrupts-extended = <&intc GIC_SPI 608 IRQ_TYPE_LEVEL_HIGH>,
>> +				<&tlmm 31 IRQ_TYPE_EDGE_FALLING>;
>> +	pinctrl-names = "default", "sleep";
>> +	pinctrl-1 = <&qup_uart7_sleep_cts>, <&qup_uart7_sleep_rts>, 
>> <&qup_uart7_sleep_tx>, <&qup_uart7_sleep_rx>;
>> +};
>> +
>>  /* PINCTRL - additions to nodes defined in sc7280.dtsi */
>> 
>>  &qspi_cs0 {
>> @@ -428,3 +438,77 @@
>>  		bias-pull-up;
>>  	};
>>  };
>> +&qup_uart7_cts {
>> +	/*
>> +	 * Configure a pull-down on CTS to match the pull of
>> +	 * the Bluetooth module.
>> +	 */
>> +	bias-pull-down;
>> +};
>> +
>> +&qup_uart7_rts {
>> +	/* We'll drive RTS, so no pull */
>> +	drive-strength = <2>;
>> +	bias-disable;
>> +};
>> +
>> +&qup_uart7_tx {
>> +	/* We'll drive TX, so no pull */
>> +	drive-strength = <2>;
>> +	bias-disable;
>> +};
>> +
>> +&qup_uart7_rx {
>> +	/*
>> +	 * Configure a pull-up on RX. This is needed to avoid
>> +	 * garbage data when the TX pin of the Bluetooth module is
>> +	 * in tri-state (module powered off or not driving the
>> +	 * signal yet).
>> +	 */
>> +	bias-pull-up;
>> +};
>> +
>> +&tlmm {
>> +	qup_uart7_sleep_cts: qup-uart7-sleep-cts {
>> +		pins = "gpio28";
>> +		function = "gpio";
>> +		/*
>> +		 * Configure a pull-down on CTS to match the pull of
>> +		 * the Bluetooth module.
>> +		 */
>> +		bias-pull-down;
>> +	};
>> +
>> +	qup_uart7_sleep_rts: qup-uart7-sleep-rts {
>> +		pins = "gpio29";
>> +		function = "gpio";
>> +		/*
>> +		 * Configure pull-down on RTS. As RTS is active low
>> +		 * signal, pull it low to indicate the BT SoC that it
>> +		 * can wakeup the system anytime from suspend state by
>> +		 * pulling RX low (by sending wakeup bytes).
>> +		 */
>> +		bias-pull-down;
>> +	};
>> +
>> +	qup_uart7_sleep_tx: qup-uart7-sleep-tx {
>> +		pins = "gpio30";
>> +		function = "gpio";
>> +		/*
>> +		 * Configure pull-up on TX when it isn't actively driven
>> +		 * to prevent BT SoC from receiving garbage during sleep.
>> +		 */
>> +		bias-pull-up;
>> +	};
>> 
>> +	qup_uart7_sleep_rx: qup-uart7-sleep-rx {
>> +		pins = "gpio31";
>> +		function = "gpio";
>> +		/*
>> +		 * Configure a pull-up on RX. This is needed to avoid
>> +		 * garbage data when the TX pin of the Bluetooth module
>> +		 * is floating which may cause spurious wakeups.
>> +		 */
>> +		bias-pull-up;
>> +	};
>> +};
> 
> How the patches of this series are split strikes me as a bit odd. 
> Supposedly
> this patch adds the QUPv3 wrapper_0 DT nodes for the SC7280, however 
> the
> above is the pin configuration for the Bluetooth UART of the SC7280 IDP 
> board.
> I don't see a good reason why that should be part of this patch. It 
> should be
> a separate change whose subject indicates that it configures the 
> Bluetooth UART
> of the SC7280 IDP.
> 

Okay will split this up.

> Without this conflation of SoC and board DT it would seem perfectly 
> reasonable
> to squash this patch and '[4/4] arm64: dts: sc7280: Add QUPv3 wrapper_1 
> nodes'
> into a single one, they are essentially doing the same thing, I see no 
> need to
> have different patches for the wrapper 0 and 1 nodes.

Previously when QUP wrapper 0 and wrapper 1 nodes were added in single 
patch, we faced some git issues as the patch was huge. Hence we split it 
up.
https://partnerissuetracker.corp.google.com/issues/177045897#comment12