| Message ID | 6599423.XddAUl3dCe@typ (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>; > };
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
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
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
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>; };
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(-)