lkml.org 
[lkml]   [2020]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
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

\
 
 \ /
  Last update: 2020-09-11 12:17    [W:0.165 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site