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

[skakit]

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@xxxxxxxxxxxxxx>
> > Reviewed-by: Akash Asthana <akashast@xxxxxxxxxxxxxx>
> > ---
> > 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