Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART

[skakit@codeaurora.org]

Hi Bjorn,

Thanks for reviewing the patches.

On 2020-09-11 05:15, Bjorn Andersson wrote:
> On Thu 10 Sep 12:53 UTC 2020, satya priya wrote:
> 
>> Add a suitable sleep configuration for uart3 to support Bluetooth 
>> wakeup.
>> 
>> If QUP function is selected in sleep state, UART RTS/RFR is pulled 
>> high
>> during suspend and BT SoC not able to send wakeup bytes. So, configure
>> GPIO mode in sleep state to keep it low during suspend.
>> 
> 
> But patch 4 says that you change this behavior, is that patch really
> needed if we switch the pins to GPIO, or if this patch really needed if
> we merge patch 4?
> 
> Could it be that in lower power states we drop the power to the uart
> block and rely on the PDC to wait for the BT chip to start sending the
> wakeup bytes on the rx pin?
> 

As discussed on V4 patch 
https://patchwork.kernel.org/patch/11753971/#23602723
The patch 4 is good to have, to make sure the UART_MANUAL_RFR is in 
ready state to receive the wakeup bytes.

> 
> This commit will become the reference for all other platforms where we
> enable the same functionality, so better document it properly.
> 

Okay.

>> Signed-off-by: satya priya <skakit@codeaurora.org>
>> Reviewed-by: Akash Asthana <akashast@codeaurora.org>
>> ---
>> Changes in V2:
>>  - This patch adds sleep state for BT UART. Newly added in V2.
>> 
>> Changes in V3:
>>  - Remove "output-high" for TX from both sleep and default states
>>    as it is not required. Configure pull-up for TX in sleep state.
>> 
>> Changes in V4:
>>  - As per Matthias's comment, removed drive-strength for sleep state
>>    and fixed nit-pick.
>> 
>> Changes in V5:
>>  - As per Matthias's comments, moved pinmux change for sleep state,
>>    pinctrl and interrupt config to the board specific file.
>> 
>>  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 
>> +++++++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts 
>> b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> index 04888df..e529a41 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> @@ -344,6 +344,10 @@
>>  };
>> 
>>  &uart3 {
>> +	pinctrl-names = "default", "sleep";
>> +	pinctrl-1 = <&qup_uart3_sleep>;
>> +	interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
>> +				<&tlmm 41 IRQ_TYPE_EDGE_FALLING>;
>>  	status = "okay";
>> 
>>  	bluetooth: wcn3990-bt {
>> @@ -545,3 +549,47 @@
>>  	};
>>  };
>> 
>> +&tlmm {
>> +	qup_uart3_sleep: qup-uart3-sleep {
>> +		pinmux {
>> +			pins = "gpio38", "gpio39",
>> +			       "gpio40", "gpio41";
>> +			function = "gpio";
>> +		};
>> +
>> +		pinconf-cts {
>> +			/*
>> +			 * Configure a pull-down on CTS to match the pull of
>> +			 * the Bluetooth module.
>> +			 */
>> +			pins = "gpio38";
>> +			bias-pull-down;
>> +		};
>> +
>> +		pinconf-rts {
>> +			/*
>> +			 * Configure pull-down on RTS to make sure that the BT SoC can
>> +			 * wake up the system by sending wakeup bytes during suspend.
> 
> So "request to send" is active low and pulling it low will indicate to
> the BT chip that it's allowed to wake us up by pulling rx low?
> 
> I would like this comment to really describe what's actually going on.
> 

Ok, will modify the rationale.

>> +			 */
>> +			 pins = "gpio39";
>> +			 bias-pull-down;
>> +		};
>> +
>> +		pinconf-tx {
>> +			/* Configure pull-up on TX when it isn't actively driven */
> 
> Sure, but why? Wouldn't that be to prevent the BT chip from receiving
> garbage while the SoC is asleep?
> 

yes, this is to prevent the BT chip from receiving garbage, will mention 
the same.

>> +			pins = "gpio40";
>> +			bias-pull-up;
>> +		};
>> +
>> +		pinconf-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).
>> +			 */
> 
> It's nice to avoid "garbage data", but isn't the real reason that the
> floating pin on the other side would cause spurious wakeups?
> 

yes, we need pull-up on RX to prevent spurious wakeups, will modify this 
comment to mention it.

> Regards,
> Bjorn
> 
>> +			pins = "gpio41";
>> +			bias-pull-up;
>> +		};
>> +	};
>> +};
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
>> member
>> of Code Aurora Forum, hosted by The Linux Foundation
>> 

Thanks,
Satya Priya