[PATCH] ARM: rockchip: rk3188: enable pull-ups on UART inputs

[PATCH] ARM: rockchip: rk3188: enable pull-ups on UART inputs

[Max Schwarz]

Enable integrated pull-ups on the UART RX pins of Rockchip RK3188.

This fixes UART receive on the radxa Rock board, which has an input diode
preventing the RX pin from being raised from the outside.

Signed-off-by: Max Schwarz <max.schwarz@online.de>
---
 arch/arm/boot/dts/rk3188.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
index 412f4d0..2e10bd7 100644
--- a/arch/arm/boot/dts/rk3188.dtsi
+++ b/arch/arm/boot/dts/rk3188.dtsi
@@ -149,7 +149,7 @@
 
 			uart0 {
 				uart0_xfer: uart0-xfer {
-					rockchip,pins = <RK_GPIO1 0 RK_FUNC_1 &pcfg_pull_none>,
+					rockchip,pins = <RK_GPIO1 0 RK_FUNC_1 &pcfg_pull_up>,
 							<RK_GPIO1 1 RK_FUNC_1 &pcfg_pull_none>;
 				};
 
@@ -164,7 +164,7 @@
 
 			uart1 {
 				uart1_xfer: uart1-xfer {
-					rockchip,pins = <RK_GPIO1 4 RK_FUNC_1 &pcfg_pull_none>,
+					rockchip,pins = <RK_GPIO1 4 RK_FUNC_1 &pcfg_pull_up>,
 							<RK_GPIO1 5 RK_FUNC_1 &pcfg_pull_none>;
 				};
 
@@ -179,7 +179,7 @@
 
 			uart2 {
 				uart2_xfer: uart2-xfer {
-					rockchip,pins = <RK_GPIO1 8 RK_FUNC_1 &pcfg_pull_none>,
+					rockchip,pins = <RK_GPIO1 8 RK_FUNC_1 &pcfg_pull_up>,
 							<RK_GPIO1 9 RK_FUNC_1 &pcfg_pull_none>;
 				};
 				/* no rts / cts for uart2 */
@@ -187,7 +187,7 @@
 
 			uart3 {
 				uart3_xfer: uart3-xfer {
-					rockchip,pins = <RK_GPIO1 10 RK_FUNC_1 &pcfg_pull_none>,
+					rockchip,pins = <RK_GPIO1 10 RK_FUNC_1 &pcfg_pull_up>,
 							<RK_GPIO1 11 RK_FUNC_1 &pcfg_pull_none>;
 				};
 
-- 
1.8.3.2

[PATCH] ARM: rockchip: rk3188: enable pull-ups on UART inputs

[Heiko Stübner]

Hi Max,


Am Sonntag, 9. M?rz 2014, 20:43:11 schrieb Max Schwarz:
> Enable integrated pull-ups on the UART RX pins of Rockchip RK3188.
> 
> This fixes UART receive on the radxa Rock board, which has an input diode
> preventing the RX pin from being raised from the outside.
>
> Signed-off-by: Max Schwarz <max.schwarz@online.de>

Your patch changes the default behaviour for all rk3188 uarts, while the patch 
description suggests a Radxa Rock specific problem.
On my preproduction Radxa Rock the issue is not present, but I think I 
remember reading somewhere, that the diodes where added in the mass-production 
model.

So I've dug a bit through different sources:

- the schematics of another rk3188 based device, did not use diodes for the 
uarts like the Radxa, but

- the default values for pull configuration supports your observation:
	gpio1-0: pull-up
	gpio1-1: pull down [currently also none]
	gpio1-4: pull up
	gpio1-5: pull-down
		and so on for the two other pin-groups.

- and rockchip vendor kernels do not seem to change the uart pull configs

I've tested both your patch + enabling the pull down for the txd pin too. Both 
variants work on my radxa.

So I agree with you but would like to determine if we should also set the txd 
to pull down in one go, to restore the default pull setting of these pins or 
should leave them as is.
Thoughts?


Heiko


> ---
>  arch/arm/boot/dts/rk3188.dtsi | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
> index 412f4d0..2e10bd7 100644
> --- a/arch/arm/boot/dts/rk3188.dtsi
> +++ b/arch/arm/boot/dts/rk3188.dtsi
> @@ -149,7 +149,7 @@
> 
>  			uart0 {
>  				uart0_xfer: uart0-xfer {
> -					rockchip,pins = <RK_GPIO1 0 RK_FUNC_1 
&pcfg_pull_none>,
> +					rockchip,pins = <RK_GPIO1 0 RK_FUNC_1 &pcfg_pull_up>,
>  							<RK_GPIO1 1 RK_FUNC_1 &pcfg_pull_none>;
>  				};
> 
> @@ -164,7 +164,7 @@
> 
>  			uart1 {
>  				uart1_xfer: uart1-xfer {
> -					rockchip,pins = <RK_GPIO1 4 RK_FUNC_1 
&pcfg_pull_none>,
> +					rockchip,pins = <RK_GPIO1 4 RK_FUNC_1 &pcfg_pull_up>,
>  							<RK_GPIO1 5 RK_FUNC_1 &pcfg_pull_none>;
>  				};
> 
> @@ -179,7 +179,7 @@
> 
>  			uart2 {
>  				uart2_xfer: uart2-xfer {
> -					rockchip,pins = <RK_GPIO1 8 RK_FUNC_1 
&pcfg_pull_none>,
> +					rockchip,pins = <RK_GPIO1 8 RK_FUNC_1 &pcfg_pull_up>,
>  							<RK_GPIO1 9 RK_FUNC_1 &pcfg_pull_none>;
>  				};
>  				/* no rts / cts for uart2 */
> @@ -187,7 +187,7 @@
> 
>  			uart3 {
>  				uart3_xfer: uart3-xfer {
> -					rockchip,pins = <RK_GPIO1 10 RK_FUNC_1 
&pcfg_pull_none>,
> +					rockchip,pins = <RK_GPIO1 10 RK_FUNC_1 &pcfg_pull_up>,
>  							<RK_GPIO1 11 RK_FUNC_1 &pcfg_pull_none>;
>  				};

[PATCH] ARM: rockchip: rk3188: enable pull-ups on UART inputs

[Max Schwarz]

Hello Heiko,

> So I agree with you but would like to determine if we should also set the
> txd to pull down in one go, to restore the default pull setting of these
> pins or should leave them as is.
> Thoughts?
As soon as the UART is enabled, that TX pull-down does not do anything because 
the UART will actively drive the pin. I don't know what happens if the UART is 
suspended through runtime PM, though. I skimmed over the 8250_dw and saw 
support for that.

On the radxa board, there is even an external pull-up on the TX pin. The UART 
idle level is high, so that makes sense. If we wanted to pull the pin 
somewhere, I guess it should be up, not down. 

My vote would be to keep the patch as it is. In any case, it's an improvement 
of the status quo and does not change TX behavior.

Cheers,
  Max

[PATCH] ARM: rockchip: rk3188: enable pull-ups on UART inputs

[Heiko Stübner]

Hi Max,


Am Donnerstag, 13. M?rz 2014, 20:25:43 schrieb Max Schwarz:
> Hello Heiko,
> 
> > So I agree with you but would like to determine if we should also set the
> > txd to pull down in one go, to restore the default pull setting of these
> > pins or should leave them as is.
> > Thoughts?
> 
> As soon as the UART is enabled, that TX pull-down does not do anything
> because the UART will actively drive the pin. I don't know what happens if
> the UART is suspended through runtime PM, though. I skimmed over the
> 8250_dw and saw support for that.
> 
> On the radxa board, there is even an external pull-up on the TX pin. The
> UART idle level is high, so that makes sense. If we wanted to pull the pin
> somewhere, I guess it should be up, not down.
> 
> My vote would be to keep the patch as it is. In any case, it's an
> improvement of the status quo and does not change TX behavior.

ok, but as we change the default behaviour of the pin - and as we discussed 
above rightfully so, the commit message should reflect this and not describe a 
Radxa-Rock specific change. So would you be ok with something like:

---------- 8< --------------------
Subject: [PATCH] ARM: rockchip: rk3188: enable pull-ups on UART RX pins

The default behaviour of the uart-rx pins on the rk3188 is to be pulled up and 
a lot of designs use diodes to even prevent them from being raised from the 
outside.

Therefore change the rx-pin settings accordingly.

This also fixes a uart receive problem on mass production Radxa Rock boards.
---------- 8< --------------------


Heiko

[PATCH] ARM: rockchip: rk3188: enable pull-ups on UART inputs

[Max Schwarz]

Hi Heiko,

> ok, but as we change the default behaviour of the pin - and as we discussed
> above rightfully so, the commit message should reflect this and not describe
> a Radxa-Rock specific change. So would you be ok with something like:
> 
> ---------- 8< --------------------
> Subject: [PATCH] ARM: rockchip: rk3188: enable pull-ups on UART RX pins
> 
> The default behaviour of the uart-rx pins on the rk3188 is to be pulled up
> and a lot of designs use diodes to even prevent them from being raised from
> the outside.
> 
> Therefore change the rx-pin settings accordingly.
> 
> This also fixes a uart receive problem on mass production Radxa Rock boards.
> ---------- 8< --------------------

Yes, that sounds good to me. Thanks!

Cheers,
  Max