[V4,2/4] arm64: dts: qcom: sc7180: Improve the pin config settings for CTS and TX

Configure no-pull for CTS, as this is driven by BT do not specify any pull
in order to not conflict with BT pulls.

Remove output-high from CTS and TX as this is not really required. During
bringup to fix transfer failures this was added to match with console uart
settings. Probably some boot loader config was missing then. As it is
working fine now, remove it.

Signed-off-by: satya priya <skakit@codeaurora.org>
Reviewed-by: Akash Asthana <akashast@codeaurora.org>
---
Changes in V4:
 - This is newly added in V4 to separate the improvements in pin settings
   and wakeup related changes.

 arch/arm64/boot/dts/qcom/sc7180-idp.dts | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

On Thu, Sep 03, 2020 at 08:34:56PM +0530, satya priya wrote:
> Configure no-pull for CTS, as this is driven by BT do not specify any pull
> in order to not conflict with BT pulls.
> 
> Remove output-high from CTS and TX as this is not really required. During
> bringup to fix transfer failures this was added to match with console uart
> settings. Probably some boot loader config was missing then. As it is
> working fine now, remove it.

You might want to revisit the 'output-high' settings for the IDP console
uart too. I still think this shouldn't do anything on an input pin that
isn't configured as GPIO. Specifically this combination seems silly:

  bias-pull-down;
  output-high;

> Signed-off-by: satya priya <skakit@codeaurora.org>
> Reviewed-by: Akash Asthana <akashast@codeaurora.org>
> ---
> Changes in V4:
>  - This is newly added in V4 to separate the improvements in pin settings
>    and wakeup related changes.
> 
>  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> index d8b5507..cecac3e 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> @@ -474,32 +474,30 @@
>  &qup_uart3_default {
>  	pinconf-cts {
>  		/*
> -		 * Configure a pull-down on 38 (CTS) to match the pull of
> -		 * the Bluetooth module.
> +		 * Configure no-pull on CTS. As this is driven by BT, do not
> +		 * specify any pull in order to not conflict with BT pulls.
>  		 */
>  		pins = "gpio38";
> -		bias-pull-down;
> -		output-high;
> +		bias-disable;
>  	};
>  
>  	pinconf-rts {
> -		/* We'll drive 39 (RTS), so no pull */
> +		/* We'll drive RTS, so no pull */
>  		pins = "gpio39";
>  		drive-strength = <2>;
>  		bias-disable;
>  	};
>  
>  	pinconf-tx {
> -		/* We'll drive 40 (TX), so no pull */
> +		/* We'll drive TX, so no pull */
>  		pins = "gpio40";
>  		drive-strength = <2>;
>  		bias-disable;
> -		output-high;
>  	};
>  
>  	pinconf-rx {
>  		/*
> -		 * Configure a pull-up on 41 (RX). This is needed to avoid
> +		 * 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).

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Hi,

On Thu, Sep 3, 2020 at 8:07 AM satya priya <skakit@codeaurora.org> wrote:
>
> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> @@ -474,32 +474,30 @@
>  &qup_uart3_default {
>         pinconf-cts {
>                 /*
> -                * Configure a pull-down on 38 (CTS) to match the pull of
> -                * the Bluetooth module.
> +                * Configure no-pull on CTS. As this is driven by BT, do not
> +                * specify any pull in order to not conflict with BT pulls.
>                  */
>                 pins = "gpio38";
> -               bias-pull-down;
> -               output-high;

Weird, how did that output-high sneak in there?  Glad it's going away.


> +               bias-disable;

I'm not convinced that the removal of the pul is the correct thing
here.  Specifically for the rx line the comment makes the argument
that if we power off the Bluetooth module then it will stop driving
this pin.  In that case if we remove the pull here then the line will
be floating and that can cause some extra power consumption as the
line floats between different logic levels.  Do you really need to
remove this pull?

Same comment for the next patch where you add the sleep settings.


-Doug

Hi Doug,

On 2020-09-10 02:58, Doug Anderson wrote:
> Hi,
> 
> On Thu, Sep 3, 2020 at 8:07 AM satya priya <skakit@codeaurora.org> 
> wrote:
>> 
>> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> @@ -474,32 +474,30 @@
>>  &qup_uart3_default {
>>         pinconf-cts {
>>                 /*
>> -                * Configure a pull-down on 38 (CTS) to match the pull 
>> of
>> -                * the Bluetooth module.
>> +                * Configure no-pull on CTS. As this is driven by BT, 
>> do not
>> +                * specify any pull in order to not conflict with BT 
>> pulls.
>>                  */
>>                 pins = "gpio38";
>> -               bias-pull-down;
>> -               output-high;
> 
> Weird, how did that output-high sneak in there?  Glad it's going away.
> 
> 
>> +               bias-disable;
> 
> I'm not convinced that the removal of the pul is the correct thing
> here.  Specifically for the rx line the comment makes the argument
> that if we power off the Bluetooth module then it will stop driving
> this pin.  In that case if we remove the pull here then the line will
> be floating and that can cause some extra power consumption as the
> line floats between different logic levels.  Do you really need to
> remove this pull?
> 

Okay, will keep the pull-down back for CTS.

> Same comment for the next patch where you add the sleep settings.
> 
> 
> -Doug