[PATCH v2] ARM: dts: rockchip: use internal pull-up resistors on I2C busses

[PATCH v2] ARM: dts: rockchip: use internal pull-up resistors on I2C busses

[Julien CHAUVEAU]

According to the I2C bus specification, it is required to use pull-up resistors
on the clock and data lines. Probing the I2C busses with i2cdetect results in
bad results when no devices are connected and no external resistors are used.

This patch configures the I2C busses to use the internal pull-up resistors
on the RK3066, RK3188 and RK3288 Rockchip processors. It should also be noted
that these default pull settings match the original configuration on these SoCs.

Below is the results of using i2cdetect on a I2C bus with no devices connected
before (1) and after (2) applying this patch.

(1) root@localhost:~# i2cdetect -y 3
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f
20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
30: -- -- -- -- -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x81, state: 2
-- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 2
-- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 2
-- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
-- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
-- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
...

(2) root@localhost:~# i2cdetect -y 3
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          -- -- -- -- -- -- -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
70: -- -- -- -- -- -- -- --

Signed-off-by: Julien CHAUVEAU <julien.chauveau@neo-technologies.fr>
---
Changes since v1:
- fix the rk3066a pull settings (only available is pull_none/pull_default)
- remove the warnings in the results of i2cdetect

 arch/arm/boot/dts/rk3066a.dtsi | 20 ++++++++++----------
 arch/arm/boot/dts/rk3188.dtsi  | 20 ++++++++++----------
 arch/arm/boot/dts/rk3288.dtsi  | 24 ++++++++++++------------
 3 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/arch/arm/boot/dts/rk3066a.dtsi b/arch/arm/boot/dts/rk3066a.dtsi
index ad9c2db..dbc1a0b 100644
--- a/arch/arm/boot/dts/rk3066a.dtsi
+++ b/arch/arm/boot/dts/rk3066a.dtsi
@@ -202,36 +202,36 @@
 
 		i2c0 {
 			i2c0_xfer: i2c0-xfer {
-				rockchip,pins = <RK_GPIO2 28 RK_FUNC_1 &pcfg_pull_none>,
-						<RK_GPIO2 29 RK_FUNC_1 &pcfg_pull_none>;
+				rockchip,pins = <RK_GPIO2 28 RK_FUNC_1 &pcfg_pull_default>,
+						<RK_GPIO2 29 RK_FUNC_1 &pcfg_pull_default>;
 			};
 		};
 
 		i2c1 {
 			i2c1_xfer: i2c1-xfer {
-				rockchip,pins = <RK_GPIO2 30 RK_FUNC_1 &pcfg_pull_none>,
-						<RK_GPIO2 31 RK_FUNC_1 &pcfg_pull_none>;
+				rockchip,pins = <RK_GPIO2 30 RK_FUNC_1 &pcfg_pull_default>,
+						<RK_GPIO2 31 RK_FUNC_1 &pcfg_pull_default>;
 			};
 		};
 
 		i2c2 {
 			i2c2_xfer: i2c2-xfer {
-				rockchip,pins = <RK_GPIO3 0 RK_FUNC_1 &pcfg_pull_none>,
-						<RK_GPIO3 1 RK_FUNC_1 &pcfg_pull_none>;
+				rockchip,pins = <RK_GPIO3 0 RK_FUNC_1 &pcfg_pull_default>,
+						<RK_GPIO3 1 RK_FUNC_1 &pcfg_pull_default>;
 			};
 		};
 
 		i2c3 {
 			i2c3_xfer: i2c3-xfer {
-				rockchip,pins = <RK_GPIO3 2 RK_FUNC_2 &pcfg_pull_none>,
-						<RK_GPIO3 3 RK_FUNC_2 &pcfg_pull_none>;
+				rockchip,pins = <RK_GPIO3 2 RK_FUNC_2 &pcfg_pull_default>,
+						<RK_GPIO3 3 RK_FUNC_2 &pcfg_pull_default>;
 			};
 		};
 
 		i2c4 {
 			i2c4_xfer: i2c4-xfer {
-				rockchip,pins = <RK_GPIO3 4 RK_FUNC_1 &pcfg_pull_none>,
-						<RK_GPIO3 5 RK_FUNC_1 &pcfg_pull_none>;
+				rockchip,pins = <RK_GPIO3 4 RK_FUNC_1 &pcfg_pull_default>,
+						<RK_GPIO3 5 RK_FUNC_1 &pcfg_pull_default>;
 			};
 		};
 
diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
index ddaada7..ee2865f 100644
--- a/arch/arm/boot/dts/rk3188.dtsi
+++ b/arch/arm/boot/dts/rk3188.dtsi
@@ -188,36 +188,36 @@
 
 		i2c0 {
 			i2c0_xfer: i2c0-xfer {
-				rockchip,pins = <RK_GPIO1 24 RK_FUNC_1 &pcfg_pull_none>,
-						<RK_GPIO1 25 RK_FUNC_1 &pcfg_pull_none>;
+				rockchip,pins = <RK_GPIO1 24 RK_FUNC_1 &pcfg_pull_up>,
+						<RK_GPIO1 25 RK_FUNC_1 &pcfg_pull_up>;
 			};
 		};
 
 		i2c1 {
 			i2c1_xfer: i2c1-xfer {
-				rockchip,pins = <RK_GPIO1 26 RK_FUNC_1 &pcfg_pull_none>,
-						<RK_GPIO1 27 RK_FUNC_1 &pcfg_pull_none>;
+				rockchip,pins = <RK_GPIO1 26 RK_FUNC_1 &pcfg_pull_up>,
+						<RK_GPIO1 27 RK_FUNC_1 &pcfg_pull_up>;
 			};
 		};
 
 		i2c2 {
 			i2c2_xfer: i2c2-xfer {
-				rockchip,pins = <RK_GPIO1 28 RK_FUNC_1 &pcfg_pull_none>,
-						<RK_GPIO1 29 RK_FUNC_1 &pcfg_pull_none>;
+				rockchip,pins = <RK_GPIO1 28 RK_FUNC_1 &pcfg_pull_up>,
+						<RK_GPIO1 29 RK_FUNC_1 &pcfg_pull_up>;
 			};
 		};
 
 		i2c3 {
 			i2c3_xfer: i2c3-xfer {
-				rockchip,pins = <RK_GPIO3 14 RK_FUNC_2 &pcfg_pull_none>,
-						<RK_GPIO3 15 RK_FUNC_2 &pcfg_pull_none>;
+				rockchip,pins = <RK_GPIO3 14 RK_FUNC_2 &pcfg_pull_up>,
+						<RK_GPIO3 15 RK_FUNC_2 &pcfg_pull_up>;
 			};
 		};
 
 		i2c4 {
 			i2c4_xfer: i2c4-xfer {
-				rockchip,pins = <RK_GPIO1 30 RK_FUNC_1 &pcfg_pull_none>,
-						<RK_GPIO1 31 RK_FUNC_1 &pcfg_pull_none>;
+				rockchip,pins = <RK_GPIO1 30 RK_FUNC_1 &pcfg_pull_up>,
+						<RK_GPIO1 31 RK_FUNC_1 &pcfg_pull_up>;
 			};
 		};
 
diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 874e66d..9a4b1f7 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -636,43 +636,43 @@
 
 		i2c0 {
 			i2c0_xfer: i2c0-xfer {
-				rockchip,pins = <0 15 RK_FUNC_1 &pcfg_pull_none>,
-						<0 16 RK_FUNC_1 &pcfg_pull_none>;
+				rockchip,pins = <0 15 RK_FUNC_1 &pcfg_pull_up>,
+						<0 16 RK_FUNC_1 &pcfg_pull_up>;
 			};
 		};
 
 		i2c1 {
 			i2c1_xfer: i2c1-xfer {
-				rockchip,pins = <8 4 RK_FUNC_1 &pcfg_pull_none>,
-						<8 5 RK_FUNC_1 &pcfg_pull_none>;
+				rockchip,pins = <8 4 RK_FUNC_1 &pcfg_pull_up>,
+						<8 5 RK_FUNC_1 &pcfg_pull_up>;
 			};
 		};
 
 		i2c2 {
 			i2c2_xfer: i2c2-xfer {
-				rockchip,pins = <6 9 RK_FUNC_1 &pcfg_pull_none>,
-						<6 10 RK_FUNC_1 &pcfg_pull_none>;
+				rockchip,pins = <6 9 RK_FUNC_1 &pcfg_pull_up>,
+						<6 10 RK_FUNC_1 &pcfg_pull_up>;
 			};
 		};
 
 		i2c3 {
 			i2c3_xfer: i2c3-xfer {
-				rockchip,pins = <2 16 RK_FUNC_1 &pcfg_pull_none>,
-						<2 17 RK_FUNC_1 &pcfg_pull_none>;
+				rockchip,pins = <2 16 RK_FUNC_1 &pcfg_pull_up>,
+						<2 17 RK_FUNC_1 &pcfg_pull_up>;
 			};
 		};
 
 		i2c4 {
 			i2c4_xfer: i2c4-xfer {
-				rockchip,pins = <7 17 RK_FUNC_1 &pcfg_pull_none>,
-						<7 18 RK_FUNC_1 &pcfg_pull_none>;
+				rockchip,pins = <7 17 RK_FUNC_1 &pcfg_pull_up>,
+						<7 18 RK_FUNC_1 &pcfg_pull_up>;
 			};
 		};
 
 		i2c5 {
 			i2c5_xfer: i2c5-xfer {
-				rockchip,pins = <7 19 RK_FUNC_1 &pcfg_pull_none>,
-						<7 20 RK_FUNC_1 &pcfg_pull_none>;
+				rockchip,pins = <7 19 RK_FUNC_1 &pcfg_pull_up>,
+						<7 20 RK_FUNC_1 &pcfg_pull_up>;
 			};
 		};
 
-- 
1.9.1

Re: [PATCH v2] ARM: dts: rockchip: use internal pull-up resistors on I2C busses

[Doug Anderson]

Julien,

On Tue, Oct 28, 2014 at 3:36 AM, Julien CHAUVEAU
<julien.chauveau-MwaZihP3oV2HdZQakrT3ElaPQRlvutdw@public.gmane.org> wrote:
> According to the I2C bus specification, it is required to use pull-up resistors
> on the clock and data lines. Probing the I2C busses with i2cdetect results in
> bad results when no devices are connected and no external resistors are used.
>
> This patch configures the I2C busses to use the internal pull-up resistors
> on the RK3066, RK3188 and RK3288 Rockchip processors. It should also be noted
> that these default pull settings match the original configuration on these SoCs.
>
> Below is the results of using i2cdetect on a I2C bus with no devices connected
> before (1) and after (2) applying this patch.
>
> (1) root@localhost:~# i2cdetect -y 3
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00:          03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
> 10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f
> 20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
> 30: -- -- -- -- -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x81, state: 2
> -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 2
> -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 2
> -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
> -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
> -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
> ...
>
> (2) root@localhost:~# i2cdetect -y 3
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00:          -- -- -- -- -- -- -- -- -- -- -- -- --
> 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 70: -- -- -- -- -- -- -- --
>
> Signed-off-by: Julien CHAUVEAU <julien.chauveau-MwaZihP3oV2HdZQakrT3ElaPQRlvutdw@public.gmane.org>
> ---
> Changes since v1:
> - fix the rk3066a pull settings (only available is pull_none/pull_default)
> - remove the warnings in the results of i2cdetect
>
>  arch/arm/boot/dts/rk3066a.dtsi | 20 ++++++++++----------
>  arch/arm/boot/dts/rk3188.dtsi  | 20 ++++++++++----------
>  arch/arm/boot/dts/rk3288.dtsi  | 24 ++++++++++++------------
>  3 files changed, 32 insertions(+), 32 deletions(-)

I won't NAK this change myself, but I have to say I'm not a huge fan.
On exynos boards the i2c has pulls by default, so there is precedent
for what you're proposing at least.

...but I'll also say that most sane boards have external pulls on i2c
and don't rely on the internal pulls.  If you've got a board where the
internal pull works well enough (lucky you!) then I think you should
override the pinctrl for just that board.

You are arguing that the internal pull "can't hurt".  I have past
experience that shows that not to be the case.  Here's an old change
for the Samsung ARM Chromebook:

https://chromium-review.googlesource.com/#/c/26607/

You probably can't dig into the bug link due to permissions, but I can
paste some of that in:

---

On Daisy / Snow we've got a NXP HDMI level converter on i2c2.  That
part has its own internal pullups.

Ideally it shouldn't hurt for us to also have the exynos's (weak) 400K
pullups on for the line too.  But according to our EE, we're just
barely within spec and so turning off the exynos pullups makes sense.

Specifically, the problem is the data line when the external HDMI
device is driving low.  The NXP chip doesn't manage to drive the data
line all the way low.  Measured on one board:
* If exynos has pullup: "low" is 560mV
* If exynos has no pull: "low" is 518mV
* If exynos has pulldown: "low" is 490mV

Low should really be 0, but exynos allows .3*VDD (or 540mV).


We should disable pullups in both the kernel and in U-Boot.  There's
even a chance that we'd want to enable pulldowns.


Note that to read EDID at all you need to also remove the strong
external pullups.

---

I did put your patch in my rk3288 board and it booted up OK, but I'm
not setup to test HDMI right now.

In v1 I think Addy said he was having trouble with HDMI and the
internal pulls on some board, so maybe he had a similar experience to
mine on snow.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] ARM: dts: rockchip: use internal pull-up resistors on I2C busses

[NEO-Technologies / Julien CHAUVEAU]

Hi Doug,

Le 29/10/2014 05:45, Doug Anderson a écrit :
> Julien,
>
> On Tue, Oct 28, 2014 at 3:36 AM, Julien CHAUVEAU
> <julien.chauveau@neo-technologies.fr> wrote:
>> According to the I2C bus specification, it is required to use pull-up resistors
>> on the clock and data lines. Probing the I2C busses with i2cdetect results in
>> bad results when no devices are connected and no external resistors are used.
>>
>> This patch configures the I2C busses to use the internal pull-up resistors
>> on the RK3066, RK3188 and RK3288 Rockchip processors. It should also be noted
>> that these default pull settings match the original configuration on these SoCs.
>>
>> Below is the results of using i2cdetect on a I2C bus with no devices connected
>> before (1) and after (2) applying this patch.
>>
>> (1) root@localhost:~# i2cdetect -y 3
>>       0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
>> 00:          03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
>> 10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f
>> 20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
>> 30: -- -- -- -- -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x81, state: 2
>> -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 2
>> -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 2
>> -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
>> -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
>> -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
>> ...
>>
>> (2) root@localhost:~# i2cdetect -y 3
>>       0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
>> 00:          -- -- -- -- -- -- -- -- -- -- -- -- --
>> 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>> 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>> 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>> 40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>> 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>> 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>> 70: -- -- -- -- -- -- -- --
>>
>> Signed-off-by: Julien CHAUVEAU <julien.chauveau@neo-technologies.fr>
>> ---
>> Changes since v1:
>> - fix the rk3066a pull settings (only available is pull_none/pull_default)
>> - remove the warnings in the results of i2cdetect
>>
>>   arch/arm/boot/dts/rk3066a.dtsi | 20 ++++++++++----------
>>   arch/arm/boot/dts/rk3188.dtsi  | 20 ++++++++++----------
>>   arch/arm/boot/dts/rk3288.dtsi  | 24 ++++++++++++------------
>>   3 files changed, 32 insertions(+), 32 deletions(-)
> I won't NAK this change myself, but I have to say I'm not a huge fan.
> On exynos boards the i2c has pulls by default, so there is precedent
> for what you're proposing at least.
>
> ...but I'll also say that most sane boards have external pulls on i2c
> and don't rely on the internal pulls.  If you've got a board where the
> internal pull works well enough (lucky you!) then I think you should
> override the pinctrl for just that board.

At the SoC level, it is IMHO better to enable the internal pull-up 
resistors.

Because the files rk3066a.dtsi, rk3188.dtsi and rk3288.dtsi are not 
board-specific, I think it is wrong to presume that correct pull-up 
resistors are applied to the I2C lines on the board.

Furthermore, as I explained to Addy on the linux-rockchip mailing list, 
it is harmless to enable the internal pull-up resistors on the SoC, but 
of course you need to keep the external pull-up resistors on the board 
(so, do not remove them!).

Moreover, if you absolutely want to disable the internal pull-up 
resistors for one or another board, it is IMHO better to do it at the 
board level, but not at the SoC level.

To each is own...

Julien/
//
/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] ARM: dts: rockchip: use internal pull-up resistors on I2C busses

[Heiko Stübner]

Hi Addy, Max, Wolfram,

after Doug's explanation of disfavour [0] and Julien's subsequent response I'm
not sure which direction to go. So if possible I'd like to collect some more
opinions of people knowing a lot more about i2c internals than myself :-) .


Thanks
Heiko


[0] http://lists.infradead.org/pipermail/linux-rockchip/2014-October/000934.html

Am Dienstag, 28. Oktober 2014, 11:36:36 schrieb Julien CHAUVEAU:
> According to the I2C bus specification, it is required to use pull-up
> resistors on the clock and data lines. Probing the I2C busses with
> i2cdetect results in bad results when no devices are connected and no
> external resistors are used.
> 
> This patch configures the I2C busses to use the internal pull-up resistors
> on the RK3066, RK3188 and RK3288 Rockchip processors. It should also be
> noted that these default pull settings match the original configuration on
> these SoCs.
> 
> Below is the results of using i2cdetect on a I2C bus with no devices
> connected before (1) and after (2) applying this patch.
> 
> (1) root@localhost:~# i2cdetect -y 3
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00:          03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
> 10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f
> 20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
> 30: -- -- -- -- -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x81, state: 2
> -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 2
> -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 2
> -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
> -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
> -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
> ...
> 
> (2) root@localhost:~# i2cdetect -y 3
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00:          -- -- -- -- -- -- -- -- -- -- -- -- --
> 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 70: -- -- -- -- -- -- -- --
> 
> Signed-off-by: Julien CHAUVEAU <julien.chauveau@neo-technologies.fr>
> ---
> Changes since v1:
> - fix the rk3066a pull settings (only available is pull_none/pull_default)
> - remove the warnings in the results of i2cdetect
> 
>  arch/arm/boot/dts/rk3066a.dtsi | 20 ++++++++++----------
>  arch/arm/boot/dts/rk3188.dtsi  | 20 ++++++++++----------
>  arch/arm/boot/dts/rk3288.dtsi  | 24 ++++++++++++------------
>  3 files changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/rk3066a.dtsi b/arch/arm/boot/dts/rk3066a.dtsi
> index ad9c2db..dbc1a0b 100644
> --- a/arch/arm/boot/dts/rk3066a.dtsi
> +++ b/arch/arm/boot/dts/rk3066a.dtsi
> @@ -202,36 +202,36 @@
> 
>  		i2c0 {
>  			i2c0_xfer: i2c0-xfer {
> -				rockchip,pins = <RK_GPIO2 28 RK_FUNC_1 &pcfg_pull_none>,
> -						<RK_GPIO2 29 RK_FUNC_1 &pcfg_pull_none>;
> +				rockchip,pins = <RK_GPIO2 28 RK_FUNC_1 &pcfg_pull_default>,
> +						<RK_GPIO2 29 RK_FUNC_1 &pcfg_pull_default>;
>  			};
>  		};
> 
>  		i2c1 {
>  			i2c1_xfer: i2c1-xfer {
> -				rockchip,pins = <RK_GPIO2 30 RK_FUNC_1 &pcfg_pull_none>,
> -						<RK_GPIO2 31 RK_FUNC_1 &pcfg_pull_none>;
> +				rockchip,pins = <RK_GPIO2 30 RK_FUNC_1 &pcfg_pull_default>,
> +						<RK_GPIO2 31 RK_FUNC_1 &pcfg_pull_default>;
>  			};
>  		};
> 
>  		i2c2 {
>  			i2c2_xfer: i2c2-xfer {
> -				rockchip,pins = <RK_GPIO3 0 RK_FUNC_1 &pcfg_pull_none>,
> -						<RK_GPIO3 1 RK_FUNC_1 &pcfg_pull_none>;
> +				rockchip,pins = <RK_GPIO3 0 RK_FUNC_1 &pcfg_pull_default>,
> +						<RK_GPIO3 1 RK_FUNC_1 &pcfg_pull_default>;
>  			};
>  		};
> 
>  		i2c3 {
>  			i2c3_xfer: i2c3-xfer {
> -				rockchip,pins = <RK_GPIO3 2 RK_FUNC_2 &pcfg_pull_none>,
> -						<RK_GPIO3 3 RK_FUNC_2 &pcfg_pull_none>;
> +				rockchip,pins = <RK_GPIO3 2 RK_FUNC_2 &pcfg_pull_default>,
> +						<RK_GPIO3 3 RK_FUNC_2 &pcfg_pull_default>;
>  			};
>  		};
> 
>  		i2c4 {
>  			i2c4_xfer: i2c4-xfer {
> -				rockchip,pins = <RK_GPIO3 4 RK_FUNC_1 &pcfg_pull_none>,
> -						<RK_GPIO3 5 RK_FUNC_1 &pcfg_pull_none>;
> +				rockchip,pins = <RK_GPIO3 4 RK_FUNC_1 &pcfg_pull_default>,
> +						<RK_GPIO3 5 RK_FUNC_1 &pcfg_pull_default>;
>  			};
>  		};
> 
> diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
> index ddaada7..ee2865f 100644
> --- a/arch/arm/boot/dts/rk3188.dtsi
> +++ b/arch/arm/boot/dts/rk3188.dtsi
> @@ -188,36 +188,36 @@
> 
>  		i2c0 {
>  			i2c0_xfer: i2c0-xfer {
> -				rockchip,pins = <RK_GPIO1 24 RK_FUNC_1 &pcfg_pull_none>,
> -						<RK_GPIO1 25 RK_FUNC_1 &pcfg_pull_none>;
> +				rockchip,pins = <RK_GPIO1 24 RK_FUNC_1 &pcfg_pull_up>,
> +						<RK_GPIO1 25 RK_FUNC_1 &pcfg_pull_up>;
>  			};
>  		};
> 
>  		i2c1 {
>  			i2c1_xfer: i2c1-xfer {
> -				rockchip,pins = <RK_GPIO1 26 RK_FUNC_1 &pcfg_pull_none>,
> -						<RK_GPIO1 27 RK_FUNC_1 &pcfg_pull_none>;
> +				rockchip,pins = <RK_GPIO1 26 RK_FUNC_1 &pcfg_pull_up>,
> +						<RK_GPIO1 27 RK_FUNC_1 &pcfg_pull_up>;
>  			};
>  		};
> 
>  		i2c2 {
>  			i2c2_xfer: i2c2-xfer {
> -				rockchip,pins = <RK_GPIO1 28 RK_FUNC_1 &pcfg_pull_none>,
> -						<RK_GPIO1 29 RK_FUNC_1 &pcfg_pull_none>;
> +				rockchip,pins = <RK_GPIO1 28 RK_FUNC_1 &pcfg_pull_up>,
> +						<RK_GPIO1 29 RK_FUNC_1 &pcfg_pull_up>;
>  			};
>  		};
> 
>  		i2c3 {
>  			i2c3_xfer: i2c3-xfer {
> -				rockchip,pins = <RK_GPIO3 14 RK_FUNC_2 &pcfg_pull_none>,
> -						<RK_GPIO3 15 RK_FUNC_2 &pcfg_pull_none>;
> +				rockchip,pins = <RK_GPIO3 14 RK_FUNC_2 &pcfg_pull_up>,
> +						<RK_GPIO3 15 RK_FUNC_2 &pcfg_pull_up>;
>  			};
>  		};
> 
>  		i2c4 {
>  			i2c4_xfer: i2c4-xfer {
> -				rockchip,pins = <RK_GPIO1 30 RK_FUNC_1 &pcfg_pull_none>,
> -						<RK_GPIO1 31 RK_FUNC_1 &pcfg_pull_none>;
> +				rockchip,pins = <RK_GPIO1 30 RK_FUNC_1 &pcfg_pull_up>,
> +						<RK_GPIO1 31 RK_FUNC_1 &pcfg_pull_up>;
>  			};
>  		};
> 
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index 874e66d..9a4b1f7 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -636,43 +636,43 @@
> 
>  		i2c0 {
>  			i2c0_xfer: i2c0-xfer {
> -				rockchip,pins = <0 15 RK_FUNC_1 &pcfg_pull_none>,
> -						<0 16 RK_FUNC_1 &pcfg_pull_none>;
> +				rockchip,pins = <0 15 RK_FUNC_1 &pcfg_pull_up>,
> +						<0 16 RK_FUNC_1 &pcfg_pull_up>;
>  			};
>  		};
> 
>  		i2c1 {
>  			i2c1_xfer: i2c1-xfer {
> -				rockchip,pins = <8 4 RK_FUNC_1 &pcfg_pull_none>,
> -						<8 5 RK_FUNC_1 &pcfg_pull_none>;
> +				rockchip,pins = <8 4 RK_FUNC_1 &pcfg_pull_up>,
> +						<8 5 RK_FUNC_1 &pcfg_pull_up>;
>  			};
>  		};
> 
>  		i2c2 {
>  			i2c2_xfer: i2c2-xfer {
> -				rockchip,pins = <6 9 RK_FUNC_1 &pcfg_pull_none>,
> -						<6 10 RK_FUNC_1 &pcfg_pull_none>;
> +				rockchip,pins = <6 9 RK_FUNC_1 &pcfg_pull_up>,
> +						<6 10 RK_FUNC_1 &pcfg_pull_up>;
>  			};
>  		};
> 
>  		i2c3 {
>  			i2c3_xfer: i2c3-xfer {
> -				rockchip,pins = <2 16 RK_FUNC_1 &pcfg_pull_none>,
> -						<2 17 RK_FUNC_1 &pcfg_pull_none>;
> +				rockchip,pins = <2 16 RK_FUNC_1 &pcfg_pull_up>,
> +						<2 17 RK_FUNC_1 &pcfg_pull_up>;
>  			};
>  		};
> 
>  		i2c4 {
>  			i2c4_xfer: i2c4-xfer {
> -				rockchip,pins = <7 17 RK_FUNC_1 &pcfg_pull_none>,
> -						<7 18 RK_FUNC_1 &pcfg_pull_none>;
> +				rockchip,pins = <7 17 RK_FUNC_1 &pcfg_pull_up>,
> +						<7 18 RK_FUNC_1 &pcfg_pull_up>;
>  			};
>  		};
> 
>  		i2c5 {
>  			i2c5_xfer: i2c5-xfer {
> -				rockchip,pins = <7 19 RK_FUNC_1 &pcfg_pull_none>,
> -						<7 20 RK_FUNC_1 &pcfg_pull_none>;
> +				rockchip,pins = <7 19 RK_FUNC_1 &pcfg_pull_up>,
> +						<7 20 RK_FUNC_1 &pcfg_pull_up>;
>  			};
>  		};

Re: [PATCH v2] ARM: dts: rockchip: use internal pull-up resistors on I2C busses

[Karl Palsson]

I'd be more inclined to have pulls disabled by default, it's more standard with what smaller
micros do, but I've no experience with these bigger cortex-a parts.  It's also the "least
surprise" path.  If you want to try and use the onboard pullups, you can specify that in your
board file, but for people deliberately selecting pullups for their timing and load expectations,
being required to take an extra step to turn off something seems unexpected.  

If you _want_ to be able to probe an i2c bus for devices added aftermarket, on a board that
didn't get i2c pull ups because no devices were planned, and you want to turn on the internal
pullups for that, I think that's something you need to do yourself, not making it a hard default
in the SoC dtsi file.

so, if it's off by default, you get this
                                         dtsi   dts
Board1, i2c periphs, designed pullups => off     -
board2, no peripsh, pulls in case     => off     -
board3, no periphs, forgot pulls, pray=> off    on

If you turn it on by default, sure, it causes no harm in most cases, but you're no longer getting
the values you expect, without having to turn off things that are not default anyway.

Sincerely,
Karl Palsson


On Wed, Oct 29, 2014 at 02:17:23PM +0100, Heiko Stübner wrote:
> Hi Addy, Max, Wolfram,
> 
> after Doug's explanation of disfavour [0] and Julien's subsequent response I'm
> not sure which direction to go. So if possible I'd like to collect some more
> opinions of people knowing a lot more about i2c internals than myself :-) .
> 
> 
> Thanks
> Heiko
> 
> 
> [0] http://lists.infradead.org/pipermail/linux-rockchip/2014-October/000934.html
> 
> Am Dienstag, 28. Oktober 2014, 11:36:36 schrieb Julien CHAUVEAU:
> > According to the I2C bus specification, it is required to use pull-up
> > resistors on the clock and data lines. Probing the I2C busses with
> > i2cdetect results in bad results when no devices are connected and no
> > external resistors are used.
> > 
> > This patch configures the I2C busses to use the internal pull-up resistors
> > on the RK3066, RK3188 and RK3288 Rockchip processors. It should also be
> > noted that these default pull settings match the original configuration on
> > these SoCs.
> > 
> > Below is the results of using i2cdetect on a I2C bus with no devices
> > connected before (1) and after (2) applying this patch.
> > 
> > (1) root@localhost:~# i2cdetect -y 3
> >      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> > 00:          03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
> > 10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f
> > 20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
> > 30: -- -- -- -- -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x81, state: 2
> > -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 2
> > -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 2
> > -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
> > -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
> > -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
> > ...
> > 
> > (2) root@localhost:~# i2cdetect -y 3
> >      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> > 00:          -- -- -- -- -- -- -- -- -- -- -- -- --
> > 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> > 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> > 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> > 40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> > 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> > 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> > 70: -- -- -- -- -- -- -- --
> > 
> > Signed-off-by: Julien CHAUVEAU <julien.chauveau@neo-technologies.fr>
> > ---
> > Changes since v1:
> > - fix the rk3066a pull settings (only available is pull_none/pull_default)
> > - remove the warnings in the results of i2cdetect
> > 
> >  arch/arm/boot/dts/rk3066a.dtsi | 20 ++++++++++----------
> >  arch/arm/boot/dts/rk3188.dtsi  | 20 ++++++++++----------
> >  arch/arm/boot/dts/rk3288.dtsi  | 24 ++++++++++++------------
> >  3 files changed, 32 insertions(+), 32 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/rk3066a.dtsi b/arch/arm/boot/dts/rk3066a.dtsi
> > index ad9c2db..dbc1a0b 100644
> > --- a/arch/arm/boot/dts/rk3066a.dtsi
> > +++ b/arch/arm/boot/dts/rk3066a.dtsi
> > @@ -202,36 +202,36 @@
> > 
> >  		i2c0 {
> >  			i2c0_xfer: i2c0-xfer {
> > -				rockchip,pins = <RK_GPIO2 28 RK_FUNC_1 &pcfg_pull_none>,
> > -						<RK_GPIO2 29 RK_FUNC_1 &pcfg_pull_none>;
> > +				rockchip,pins = <RK_GPIO2 28 RK_FUNC_1 &pcfg_pull_default>,
> > +						<RK_GPIO2 29 RK_FUNC_1 &pcfg_pull_default>;
> >  			};
> >  		};
> > 
> >  		i2c1 {
> >  			i2c1_xfer: i2c1-xfer {
> > -				rockchip,pins = <RK_GPIO2 30 RK_FUNC_1 &pcfg_pull_none>,
> > -						<RK_GPIO2 31 RK_FUNC_1 &pcfg_pull_none>;
> > +				rockchip,pins = <RK_GPIO2 30 RK_FUNC_1 &pcfg_pull_default>,
> > +						<RK_GPIO2 31 RK_FUNC_1 &pcfg_pull_default>;
> >  			};
> >  		};
> > 
> >  		i2c2 {
> >  			i2c2_xfer: i2c2-xfer {
> > -				rockchip,pins = <RK_GPIO3 0 RK_FUNC_1 &pcfg_pull_none>,
> > -						<RK_GPIO3 1 RK_FUNC_1 &pcfg_pull_none>;
> > +				rockchip,pins = <RK_GPIO3 0 RK_FUNC_1 &pcfg_pull_default>,
> > +						<RK_GPIO3 1 RK_FUNC_1 &pcfg_pull_default>;
> >  			};
> >  		};
> > 
> >  		i2c3 {
> >  			i2c3_xfer: i2c3-xfer {
> > -				rockchip,pins = <RK_GPIO3 2 RK_FUNC_2 &pcfg_pull_none>,
> > -						<RK_GPIO3 3 RK_FUNC_2 &pcfg_pull_none>;
> > +				rockchip,pins = <RK_GPIO3 2 RK_FUNC_2 &pcfg_pull_default>,
> > +						<RK_GPIO3 3 RK_FUNC_2 &pcfg_pull_default>;
> >  			};
> >  		};
> > 
> >  		i2c4 {
> >  			i2c4_xfer: i2c4-xfer {
> > -				rockchip,pins = <RK_GPIO3 4 RK_FUNC_1 &pcfg_pull_none>,
> > -						<RK_GPIO3 5 RK_FUNC_1 &pcfg_pull_none>;
> > +				rockchip,pins = <RK_GPIO3 4 RK_FUNC_1 &pcfg_pull_default>,
> > +						<RK_GPIO3 5 RK_FUNC_1 &pcfg_pull_default>;
> >  			};
> >  		};
> > 
> > diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
> > index ddaada7..ee2865f 100644
> > --- a/arch/arm/boot/dts/rk3188.dtsi
> > +++ b/arch/arm/boot/dts/rk3188.dtsi
> > @@ -188,36 +188,36 @@
> > 
> >  		i2c0 {
> >  			i2c0_xfer: i2c0-xfer {
> > -				rockchip,pins = <RK_GPIO1 24 RK_FUNC_1 &pcfg_pull_none>,
> > -						<RK_GPIO1 25 RK_FUNC_1 &pcfg_pull_none>;
> > +				rockchip,pins = <RK_GPIO1 24 RK_FUNC_1 &pcfg_pull_up>,
> > +						<RK_GPIO1 25 RK_FUNC_1 &pcfg_pull_up>;
> >  			};
> >  		};
> > 
> >  		i2c1 {
> >  			i2c1_xfer: i2c1-xfer {
> > -				rockchip,pins = <RK_GPIO1 26 RK_FUNC_1 &pcfg_pull_none>,
> > -						<RK_GPIO1 27 RK_FUNC_1 &pcfg_pull_none>;
> > +				rockchip,pins = <RK_GPIO1 26 RK_FUNC_1 &pcfg_pull_up>,
> > +						<RK_GPIO1 27 RK_FUNC_1 &pcfg_pull_up>;
> >  			};
> >  		};
> > 
> >  		i2c2 {
> >  			i2c2_xfer: i2c2-xfer {
> > -				rockchip,pins = <RK_GPIO1 28 RK_FUNC_1 &pcfg_pull_none>,
> > -						<RK_GPIO1 29 RK_FUNC_1 &pcfg_pull_none>;
> > +				rockchip,pins = <RK_GPIO1 28 RK_FUNC_1 &pcfg_pull_up>,
> > +						<RK_GPIO1 29 RK_FUNC_1 &pcfg_pull_up>;
> >  			};
> >  		};
> > 
> >  		i2c3 {
> >  			i2c3_xfer: i2c3-xfer {
> > -				rockchip,pins = <RK_GPIO3 14 RK_FUNC_2 &pcfg_pull_none>,
> > -						<RK_GPIO3 15 RK_FUNC_2 &pcfg_pull_none>;
> > +				rockchip,pins = <RK_GPIO3 14 RK_FUNC_2 &pcfg_pull_up>,
> > +						<RK_GPIO3 15 RK_FUNC_2 &pcfg_pull_up>;
> >  			};
> >  		};
> > 
> >  		i2c4 {
> >  			i2c4_xfer: i2c4-xfer {
> > -				rockchip,pins = <RK_GPIO1 30 RK_FUNC_1 &pcfg_pull_none>,
> > -						<RK_GPIO1 31 RK_FUNC_1 &pcfg_pull_none>;
> > +				rockchip,pins = <RK_GPIO1 30 RK_FUNC_1 &pcfg_pull_up>,
> > +						<RK_GPIO1 31 RK_FUNC_1 &pcfg_pull_up>;
> >  			};
> >  		};
> > 
> > diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> > index 874e66d..9a4b1f7 100644
> > --- a/arch/arm/boot/dts/rk3288.dtsi
> > +++ b/arch/arm/boot/dts/rk3288.dtsi
> > @@ -636,43 +636,43 @@
> > 
> >  		i2c0 {
> >  			i2c0_xfer: i2c0-xfer {
> > -				rockchip,pins = <0 15 RK_FUNC_1 &pcfg_pull_none>,
> > -						<0 16 RK_FUNC_1 &pcfg_pull_none>;
> > +				rockchip,pins = <0 15 RK_FUNC_1 &pcfg_pull_up>,
> > +						<0 16 RK_FUNC_1 &pcfg_pull_up>;
> >  			};
> >  		};
> > 
> >  		i2c1 {
> >  			i2c1_xfer: i2c1-xfer {
> > -				rockchip,pins = <8 4 RK_FUNC_1 &pcfg_pull_none>,
> > -						<8 5 RK_FUNC_1 &pcfg_pull_none>;
> > +				rockchip,pins = <8 4 RK_FUNC_1 &pcfg_pull_up>,
> > +						<8 5 RK_FUNC_1 &pcfg_pull_up>;
> >  			};
> >  		};
> > 
> >  		i2c2 {
> >  			i2c2_xfer: i2c2-xfer {
> > -				rockchip,pins = <6 9 RK_FUNC_1 &pcfg_pull_none>,
> > -						<6 10 RK_FUNC_1 &pcfg_pull_none>;
> > +				rockchip,pins = <6 9 RK_FUNC_1 &pcfg_pull_up>,
> > +						<6 10 RK_FUNC_1 &pcfg_pull_up>;
> >  			};
> >  		};
> > 
> >  		i2c3 {
> >  			i2c3_xfer: i2c3-xfer {
> > -				rockchip,pins = <2 16 RK_FUNC_1 &pcfg_pull_none>,
> > -						<2 17 RK_FUNC_1 &pcfg_pull_none>;
> > +				rockchip,pins = <2 16 RK_FUNC_1 &pcfg_pull_up>,
> > +						<2 17 RK_FUNC_1 &pcfg_pull_up>;
> >  			};
> >  		};
> > 
> >  		i2c4 {
> >  			i2c4_xfer: i2c4-xfer {
> > -				rockchip,pins = <7 17 RK_FUNC_1 &pcfg_pull_none>,
> > -						<7 18 RK_FUNC_1 &pcfg_pull_none>;
> > +				rockchip,pins = <7 17 RK_FUNC_1 &pcfg_pull_up>,
> > +						<7 18 RK_FUNC_1 &pcfg_pull_up>;
> >  			};
> >  		};
> > 
> >  		i2c5 {
> >  			i2c5_xfer: i2c5-xfer {
> > -				rockchip,pins = <7 19 RK_FUNC_1 &pcfg_pull_none>,
> > -						<7 20 RK_FUNC_1 &pcfg_pull_none>;
> > +				rockchip,pins = <7 19 RK_FUNC_1 &pcfg_pull_up>,
> > +						<7 20 RK_FUNC_1 &pcfg_pull_up>;
> >  			};
> >  		};
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2] ARM: dts: rockchip: use internal pull-up resistors on I2C busses

[Max Schwarz]

Hi,

I'll agree with Karl and Doug. If you (as a board vendor/maintainer/etc) want 
to use I2C, it's *your* responsibility to provide the pullup resistors by 
either including pullup resistors on the board or by enabling the internal 
ones.
Either way, you should think a moment about the consequences (frequency/trace 
length limitations), which is why I'm also against the pullup-by-default 
behavior.

Also, it's much harder to diagnose effects like Doug is describing (slightly 
out-of-spec due to internal + external pulls) than the effects you are seeing 
without any pullups. With your i2cdetect results my first thought would have 
been "are there pullups on the bus?".

Cheers,
  Max

Am Mittwoch, 29. Oktober 2014, 13:44:15 schrieb Karl Palsson:
> I'd be more inclined to have pulls disabled by default, it's more standard
> with what smaller micros do, but I've no experience with these bigger
> cortex-a parts.  It's also the "least surprise" path.  If you want to try
> and use the onboard pullups, you can specify that in your board file, but
> for people deliberately selecting pullups for their timing and load
> expectations, being required to take an extra step to turn off something
> seems unexpected.
> 
> If you _want_ to be able to probe an i2c bus for devices added aftermarket,
> on a board that didn't get i2c pull ups because no devices were planned,
> and you want to turn on the internal pullups for that, I think that's
> something you need to do yourself, not making it a hard default in the SoC
> dtsi file.
> 
> so, if it's off by default, you get this
>                                          dtsi   dts
> Board1, i2c periphs, designed pullups => off     -
> board2, no peripsh, pulls in case     => off     -
> board3, no periphs, forgot pulls, pray=> off    on
> 
> If you turn it on by default, sure, it causes no harm in most cases, but
> you're no longer getting the values you expect, without having to turn off
> things that are not default anyway.
> 
> Sincerely,
> Karl Palsson
> 
> On Wed, Oct 29, 2014 at 02:17:23PM +0100, Heiko Stübner wrote:
> > Hi Addy, Max, Wolfram,
> > 
> > after Doug's explanation of disfavour [0] and Julien's subsequent response
> > I'm not sure which direction to go. So if possible I'd like to collect
> > some more opinions of people knowing a lot more about i2c internals than
> > myself :-) .
> > 
> > 
> > Thanks
> > Heiko
> > 
> > 
> > [0]
> > http://lists.infradead.org/pipermail/linux-rockchip/2014-October/000934.h
> > tml> 
> > Am Dienstag, 28. Oktober 2014, 11:36:36 schrieb Julien CHAUVEAU:
> > > According to the I2C bus specification, it is required to use pull-up
> > > resistors on the clock and data lines. Probing the I2C busses with
> > > i2cdetect results in bad results when no devices are connected and no
> > > external resistors are used.
> > > 
> > > This patch configures the I2C busses to use the internal pull-up
> > > resistors
> > > on the RK3066, RK3188 and RK3288 Rockchip processors. It should also be
> > > noted that these default pull settings match the original configuration
> > > on
> > > these SoCs.
> > > 
> > > Below is the results of using i2cdetect on a I2C bus with no devices
> > > connected before (1) and after (2) applying this patch.
> > > 
> > > (1) root@localhost:~# i2cdetect -y 3
> > > 
> > >      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> > > 
> > > 00:          03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
> > > 10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f
> > > 20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
> > > 30: -- -- -- -- -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x81, state: 2
> > > -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 2
> > > -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 2
> > > -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
> > > -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
> > > -- rk3x-i2c 2005a000.i2c: timeout, ipd: 0x80, state: 3
> > > ...
> > > 
> > > (2) root@localhost:~# i2cdetect -y 3
> > > 
> > >      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> > > 
> > > 00:          -- -- -- -- -- -- -- -- -- -- -- -- --
> > > 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> > > 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> > > 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> > > 40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> > > 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> > > 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> > > 70: -- -- -- -- -- -- -- --
> > > 
> > > Signed-off-by: Julien CHAUVEAU <julien.chauveau@neo-technologies.fr>
> > > ---
> > > Changes since v1:
> > > - fix the rk3066a pull settings (only available is
> > > pull_none/pull_default)
> > > - remove the warnings in the results of i2cdetect
> > > 
> > >  arch/arm/boot/dts/rk3066a.dtsi | 20 ++++++++++----------
> > >  arch/arm/boot/dts/rk3188.dtsi  | 20 ++++++++++----------
> > >  arch/arm/boot/dts/rk3288.dtsi  | 24 ++++++++++++------------
> > >  3 files changed, 32 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/arch/arm/boot/dts/rk3066a.dtsi
> > > b/arch/arm/boot/dts/rk3066a.dtsi index ad9c2db..dbc1a0b 100644
> > > --- a/arch/arm/boot/dts/rk3066a.dtsi
> > > +++ b/arch/arm/boot/dts/rk3066a.dtsi
> > > @@ -202,36 +202,36 @@
> > > 
> > >  		i2c0 {
> > >  		
> > >  			i2c0_xfer: i2c0-xfer {
> > > 
> > > -				rockchip,pins = <RK_GPIO2 28 RK_FUNC_1 &pcfg_pull_none>,
> > > -						<RK_GPIO2 29 RK_FUNC_1 &pcfg_pull_none>;
> > > +				rockchip,pins = <RK_GPIO2 28 RK_FUNC_1 &pcfg_pull_default>,
> > > +						<RK_GPIO2 29 RK_FUNC_1 &pcfg_pull_default>;
> > > 
> > >  			};
> > >  		
> > >  		};
> > >  		
> > >  		i2c1 {
> > >  		
> > >  			i2c1_xfer: i2c1-xfer {
> > > 
> > > -				rockchip,pins = <RK_GPIO2 30 RK_FUNC_1 &pcfg_pull_none>,
> > > -						<RK_GPIO2 31 RK_FUNC_1 &pcfg_pull_none>;
> > > +				rockchip,pins = <RK_GPIO2 30 RK_FUNC_1 &pcfg_pull_default>,
> > > +						<RK_GPIO2 31 RK_FUNC_1 &pcfg_pull_default>;
> > > 
> > >  			};
> > >  		
> > >  		};
> > >  		
> > >  		i2c2 {
> > >  		
> > >  			i2c2_xfer: i2c2-xfer {
> > > 
> > > -				rockchip,pins = <RK_GPIO3 0 RK_FUNC_1 &pcfg_pull_none>,
> > > -						<RK_GPIO3 1 RK_FUNC_1 &pcfg_pull_none>;
> > > +				rockchip,pins = <RK_GPIO3 0 RK_FUNC_1 &pcfg_pull_default>,
> > > +						<RK_GPIO3 1 RK_FUNC_1 &pcfg_pull_default>;
> > > 
> > >  			};
> > >  		
> > >  		};
> > >  		
> > >  		i2c3 {
> > >  		
> > >  			i2c3_xfer: i2c3-xfer {
> > > 
> > > -				rockchip,pins = <RK_GPIO3 2 RK_FUNC_2 &pcfg_pull_none>,
> > > -						<RK_GPIO3 3 RK_FUNC_2 &pcfg_pull_none>;
> > > +				rockchip,pins = <RK_GPIO3 2 RK_FUNC_2 &pcfg_pull_default>,
> > > +						<RK_GPIO3 3 RK_FUNC_2 &pcfg_pull_default>;
> > > 
> > >  			};
> > >  		
> > >  		};
> > >  		
> > >  		i2c4 {
> > >  		
> > >  			i2c4_xfer: i2c4-xfer {
> > > 
> > > -				rockchip,pins = <RK_GPIO3 4 RK_FUNC_1 &pcfg_pull_none>,
> > > -						<RK_GPIO3 5 RK_FUNC_1 &pcfg_pull_none>;
> > > +				rockchip,pins = <RK_GPIO3 4 RK_FUNC_1 &pcfg_pull_default>,
> > > +						<RK_GPIO3 5 RK_FUNC_1 &pcfg_pull_default>;
> > > 
> > >  			};
> > >  		
> > >  		};
> > > 
> > > diff --git a/arch/arm/boot/dts/rk3188.dtsi
> > > b/arch/arm/boot/dts/rk3188.dtsi
> > > index ddaada7..ee2865f 100644
> > > --- a/arch/arm/boot/dts/rk3188.dtsi
> > > +++ b/arch/arm/boot/dts/rk3188.dtsi
> > > @@ -188,36 +188,36 @@
> > > 
> > >  		i2c0 {
> > >  		
> > >  			i2c0_xfer: i2c0-xfer {
> > > 
> > > -				rockchip,pins = <RK_GPIO1 24 RK_FUNC_1 &pcfg_pull_none>,
> > > -						<RK_GPIO1 25 RK_FUNC_1 &pcfg_pull_none>;
> > > +				rockchip,pins = <RK_GPIO1 24 RK_FUNC_1 &pcfg_pull_up>,
> > > +						<RK_GPIO1 25 RK_FUNC_1 &pcfg_pull_up>;
> > > 
> > >  			};
> > >  		
> > >  		};
> > >  		
> > >  		i2c1 {
> > >  		
> > >  			i2c1_xfer: i2c1-xfer {
> > > 
> > > -				rockchip,pins = <RK_GPIO1 26 RK_FUNC_1 &pcfg_pull_none>,
> > > -						<RK_GPIO1 27 RK_FUNC_1 &pcfg_pull_none>;
> > > +				rockchip,pins = <RK_GPIO1 26 RK_FUNC_1 &pcfg_pull_up>,
> > > +						<RK_GPIO1 27 RK_FUNC_1 &pcfg_pull_up>;
> > > 
> > >  			};
> > >  		
> > >  		};
> > >  		
> > >  		i2c2 {
> > >  		
> > >  			i2c2_xfer: i2c2-xfer {
> > > 
> > > -				rockchip,pins = <RK_GPIO1 28 RK_FUNC_1 &pcfg_pull_none>,
> > > -						<RK_GPIO1 29 RK_FUNC_1 &pcfg_pull_none>;
> > > +				rockchip,pins = <RK_GPIO1 28 RK_FUNC_1 &pcfg_pull_up>,
> > > +						<RK_GPIO1 29 RK_FUNC_1 &pcfg_pull_up>;
> > > 
> > >  			};
> > >  		
> > >  		};
> > >  		
> > >  		i2c3 {
> > >  		
> > >  			i2c3_xfer: i2c3-xfer {
> > > 
> > > -				rockchip,pins = <RK_GPIO3 14 RK_FUNC_2 &pcfg_pull_none>,
> > > -						<RK_GPIO3 15 RK_FUNC_2 &pcfg_pull_none>;
> > > +				rockchip,pins = <RK_GPIO3 14 RK_FUNC_2 &pcfg_pull_up>,
> > > +						<RK_GPIO3 15 RK_FUNC_2 &pcfg_pull_up>;
> > > 
> > >  			};
> > >  		
> > >  		};
> > >  		
> > >  		i2c4 {
> > >  		
> > >  			i2c4_xfer: i2c4-xfer {
> > > 
> > > -				rockchip,pins = <RK_GPIO1 30 RK_FUNC_1 &pcfg_pull_none>,
> > > -						<RK_GPIO1 31 RK_FUNC_1 &pcfg_pull_none>;
> > > +				rockchip,pins = <RK_GPIO1 30 RK_FUNC_1 &pcfg_pull_up>,
> > > +						<RK_GPIO1 31 RK_FUNC_1 &pcfg_pull_up>;
> > > 
> > >  			};
> > >  		
> > >  		};
> > > 
> > > diff --git a/arch/arm/boot/dts/rk3288.dtsi
> > > b/arch/arm/boot/dts/rk3288.dtsi
> > > index 874e66d..9a4b1f7 100644
> > > --- a/arch/arm/boot/dts/rk3288.dtsi
> > > +++ b/arch/arm/boot/dts/rk3288.dtsi
> > > @@ -636,43 +636,43 @@
> > > 
> > >  		i2c0 {
> > >  		
> > >  			i2c0_xfer: i2c0-xfer {
> > > 
> > > -				rockchip,pins = <0 15 RK_FUNC_1 &pcfg_pull_none>,
> > > -						<0 16 RK_FUNC_1 &pcfg_pull_none>;
> > > +				rockchip,pins = <0 15 RK_FUNC_1 &pcfg_pull_up>,
> > > +						<0 16 RK_FUNC_1 &pcfg_pull_up>;
> > > 
> > >  			};
> > >  		
> > >  		};
> > >  		
> > >  		i2c1 {
> > >  		
> > >  			i2c1_xfer: i2c1-xfer {
> > > 
> > > -				rockchip,pins = <8 4 RK_FUNC_1 &pcfg_pull_none>,
> > > -						<8 5 RK_FUNC_1 &pcfg_pull_none>;
> > > +				rockchip,pins = <8 4 RK_FUNC_1 &pcfg_pull_up>,
> > > +						<8 5 RK_FUNC_1 &pcfg_pull_up>;
> > > 
> > >  			};
> > >  		
> > >  		};
> > >  		
> > >  		i2c2 {
> > >  		
> > >  			i2c2_xfer: i2c2-xfer {
> > > 
> > > -				rockchip,pins = <6 9 RK_FUNC_1 &pcfg_pull_none>,
> > > -						<6 10 RK_FUNC_1 &pcfg_pull_none>;
> > > +				rockchip,pins = <6 9 RK_FUNC_1 &pcfg_pull_up>,
> > > +						<6 10 RK_FUNC_1 &pcfg_pull_up>;
> > > 
> > >  			};
> > >  		
> > >  		};
> > >  		
> > >  		i2c3 {
> > >  		
> > >  			i2c3_xfer: i2c3-xfer {
> > > 
> > > -				rockchip,pins = <2 16 RK_FUNC_1 &pcfg_pull_none>,
> > > -						<2 17 RK_FUNC_1 &pcfg_pull_none>;
> > > +				rockchip,pins = <2 16 RK_FUNC_1 &pcfg_pull_up>,
> > > +						<2 17 RK_FUNC_1 &pcfg_pull_up>;
> > > 
> > >  			};
> > >  		
> > >  		};
> > >  		
> > >  		i2c4 {
> > >  		
> > >  			i2c4_xfer: i2c4-xfer {
> > > 
> > > -				rockchip,pins = <7 17 RK_FUNC_1 &pcfg_pull_none>,
> > > -						<7 18 RK_FUNC_1 &pcfg_pull_none>;
> > > +				rockchip,pins = <7 17 RK_FUNC_1 &pcfg_pull_up>,
> > > +						<7 18 RK_FUNC_1 &pcfg_pull_up>;
> > > 
> > >  			};
> > >  		
> > >  		};
> > >  		
> > >  		i2c5 {
> > >  		
> > >  			i2c5_xfer: i2c5-xfer {
> > > 
> > > -				rockchip,pins = <7 19 RK_FUNC_1 &pcfg_pull_none>,
> > > -						<7 20 RK_FUNC_1 &pcfg_pull_none>;
> > > +				rockchip,pins = <7 19 RK_FUNC_1 &pcfg_pull_up>,
> > > +						<7 20 RK_FUNC_1 &pcfg_pull_up>;
> > > 
> > >  			};
> > >  		
> > >  		};
> > 
> > _______________________________________________
> > Linux-rockchip mailing list
> > Linux-rockchip@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2] ARM: dts: rockchip: use internal pull-up resistors on I2C busses

[NEO-Technologies / Julien CHAUVEAU]

Hi everyone,

Okay, I understand your opinion. So let's drop my patch in this case.

Thank you for your comments.

Julien


Le 29/10/2014 15:02, Max Schwarz a écrit :
> Hi,
>
> I'll agree with Karl and Doug. If you (as a board vendor/maintainer/etc) want
> to use I2C, it's *your* responsibility to provide the pullup resistors by
> either including pullup resistors on the board or by enabling the internal
> ones.
> Either way, you should think a moment about the consequences (frequency/trace
> length limitations), which is why I'm also against the pullup-by-default
> behavior.
>
> Also, it's much harder to diagnose effects like Doug is describing (slightly
> out-of-spec due to internal + external pulls) than the effects you are seeing
> without any pullups. With your i2cdetect results my first thought would have
> been "are there pullups on the bus?".
>
> Cheers,
>    Max
>
> Am Mittwoch, 29. Oktober 2014, 13:44:15 schrieb Karl Palsson:
>> I'd be more inclined to have pulls disabled by default, it's more standard
>> with what smaller micros do, but I've no experience with these bigger
>> cortex-a parts.  It's also the "least surprise" path.  If you want to try
>> and use the onboard pullups, you can specify that in your board file, but
>> for people deliberately selecting pullups for their timing and load
>> expectations, being required to take an extra step to turn off something
>> seems unexpected.
>>
>> If you _want_ to be able to probe an i2c bus for devices added aftermarket,
>> on a board that didn't get i2c pull ups because no devices were planned,
>> and you want to turn on the internal pullups for that, I think that's
>> something you need to do yourself, not making it a hard default in the SoC
>> dtsi file.
>>
>> so, if it's off by default, you get this
>>                                           dtsi   dts
>> Board1, i2c periphs, designed pullups => off     -
>> board2, no peripsh, pulls in case     => off     -
>> board3, no periphs, forgot pulls, pray=> off    on
>>
>> If you turn it on by default, sure, it causes no harm in most cases, but
>> you're no longer getting the values you expect, without having to turn off
>> things that are not default anyway.
>>
>> Sincerely,
>> Karl Palsson
>>
>> On Wed, Oct 29, 2014 at 02:17:23PM +0100, Heiko Stübner wrote:
>>> Hi Addy, Max, Wolfram,
>>>
>>> after Doug's explanation of disfavour [0] and Julien's subsequent response
>>> I'm not sure which direction to go. So if possible I'd like to collect
>>> some more opinions of people knowing a lot more about i2c internals than
>>> myself :-) .
>>>
>>>
>>> Thanks
>>> Heiko
>>>
>>>
>>> [0] http://lists.infradead.org/pipermail/linux-rockchip/2014-October/000934.html

Re: [PATCH v2] ARM: dts: rockchip: use internal pull-up resistors on I2C busses

[Karl Palsson]

However, and possibly out of line, but should we consider submitting a patch to remove the
pullups by default for exynos that Doug hinted at?  

Cheers,
Karl P


On Wed, Oct 29, 2014 at 03:34:27PM +0100, NEO-Technologies / Julien CHAUVEAU wrote:
> Hi everyone,
> 
> Okay, I understand your opinion. So let's drop my patch in this case.
> 
> Thank you for your comments.
> 
> Julien
> 
> 
> Le 29/10/2014 15:02, Max Schwarz a écrit :
> >Hi,
> >
> >I'll agree with Karl and Doug. If you (as a board vendor/maintainer/etc) want
> >to use I2C, it's *your* responsibility to provide the pullup resistors by
> >either including pullup resistors on the board or by enabling the internal
> >ones.
> >Either way, you should think a moment about the consequences (frequency/trace
> >length limitations), which is why I'm also against the pullup-by-default
> >behavior.
> >
> >Also, it's much harder to diagnose effects like Doug is describing (slightly
> >out-of-spec due to internal + external pulls) than the effects you are seeing
> >without any pullups. With your i2cdetect results my first thought would have
> >been "are there pullups on the bus?".
> >
> >Cheers,
> >   Max
> >
> >Am Mittwoch, 29. Oktober 2014, 13:44:15 schrieb Karl Palsson:
> >>I'd be more inclined to have pulls disabled by default, it's more standard
> >>with what smaller micros do, but I've no experience with these bigger
> >>cortex-a parts.  It's also the "least surprise" path.  If you want to try
> >>and use the onboard pullups, you can specify that in your board file, but
> >>for people deliberately selecting pullups for their timing and load
> >>expectations, being required to take an extra step to turn off something
> >>seems unexpected.
> >>
> >>If you _want_ to be able to probe an i2c bus for devices added aftermarket,
> >>on a board that didn't get i2c pull ups because no devices were planned,
> >>and you want to turn on the internal pullups for that, I think that's
> >>something you need to do yourself, not making it a hard default in the SoC
> >>dtsi file.
> >>
> >>so, if it's off by default, you get this
> >>                                          dtsi   dts
> >>Board1, i2c periphs, designed pullups => off     -
> >>board2, no peripsh, pulls in case     => off     -
> >>board3, no periphs, forgot pulls, pray=> off    on
> >>
> >>If you turn it on by default, sure, it causes no harm in most cases, but
> >>you're no longer getting the values you expect, without having to turn off
> >>things that are not default anyway.
> >>
> >>Sincerely,
> >>Karl Palsson
> >>
> >>On Wed, Oct 29, 2014 at 02:17:23PM +0100, Heiko Stübner wrote:
> >>>Hi Addy, Max, Wolfram,
> >>>
> >>>after Doug's explanation of disfavour [0] and Julien's subsequent response
> >>>I'm not sure which direction to go. So if possible I'd like to collect
> >>>some more opinions of people knowing a lot more about i2c internals than
> >>>myself :-) .
> >>>
> >>>
> >>>Thanks
> >>>Heiko
> >>>
> >>>
> >>>[0] http://lists.infradead.org/pipermail/linux-rockchip/2014-October/000934.html
>