ARM: shmobile: Lager: Correct I2C bus for VDD MPU regulator

PATCH In review Khiem Nguyen 5 participants 14 messages Jul 4, 2014 – Jul 13, 2014

Subsystems (2)

Device tree bindings SuperH

lore

Share & tools

lore

Khiem Nguyen

author

2014-07-04 00:19 UTC #0 lore raw

PATCH v2 1/1 ARM: shmobile: Lager: Correct I2C bus for VDD MPU regulator

I2C bus for VDD MPU regulator is IIC3, not I2C3.

Signed-off-by Khiem Nguyen <[email protected]>

---
v2:
- re-based on top of tag renesas-devel-v3.16-rc3-20140701.
- removed redundant spaces.

 arch/arm/boot/dts/r8a7790-lager.dts |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

arch/arm/boot/dts/r8a7790-lager.dts +5 -5

index 544202b..ffe0523 100644
@@ -215,9 +215,9 @@
 		renesas,function = "i2c2";
 	};
 
-	i2c3_pins: i2c3 {
-		renesas,groups = "i2c3";
-		renesas,function = "i2c3";
+	iic3_pins: iic3 {
+		renesas,groups = "iic3";
+		renesas,function = "iic3";
 	};
 
 	usb0_pins: usb0 {

@@ -368,9 +368,9 @@
 	pinctrl-names = "default";
 };
 
-&i2c3 {
+&iic3 {
 	pinctrl-names = "default";
-	pinctrl-0 = <&i2c3_pins>;
+	pinctrl-0 = <&iic3_pins>;
 	status = "okay";
 
 	vdd_dvfs: regulator@68 {

Signature · 1 line

1.7.9.5

Gaku Inami

Tested-by on v2 1/1

2014-07-04 05:40 UTC #1 lore raw

Quoted 30 lines · Khiem Nguyen

I2C bus for VDD MPU regulator is IIC3, not I2C3.
Signed-off-by Khiem Nguyen <[email protected]>
---
v2:
- re-based on top of tag renesas-devel-v3.16-rc3-20140701.
- removed redundant spaces.
 arch/arm/boot/dts/r8a7790-lager.dts |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
arch/arm/boot/dts/r8a7790-lager.dts +3 -3
index 544202b..ffe0523 100644
@@ -215,9 +215,9 @@
 		renesas,function = "i2c2";
 	};
 
-	i2c3_pins: i2c3 {
-		renesas,groups = "i2c3";
-		renesas,function = "i2c3";
+	iic3_pins: iic3 {
+		renesas,groups = "iic3";
+		renesas,function = "iic3";
 	};
 
 	usb0_pins: usb0 {
@@ -368,9 +368,9 @@
 	pinctrl-names = "default";

arch/arm/boot/dts/r8a7790-lager.dts line 376

Quoted 10 lines

arch/arm/boot/dts/r8a7790-lager.dts +2 -2

 };
 
-&i2c3 {
+&iic3 {
 	pinctrl-names = "default";
-	pinctrl-0 = <&i2c3_pins>;
+	pinctrl-0 = <&iic3_pins>;
 	status = "okay";
 
 	vdd_dvfs: regulator@68 {

Thank you for updating patch.
I tested your patch. Test reult is good.

[Test result] (in both SMP and non-SMP)
- kernel boot : OK
- regurator initialize : OK

  - cpufreq initialize   : OK
  - frequency scaling    : OK
  - changes governor     : OK

Tested-by Gaku Inami <[email protected]>

This is a bug fix of my cpufreq patch. Thank you for your fix.

Khiem Nguyen

author

2014-07-04 06:41 UTC #2 lore raw

In reply to Khiem Nguyen

Hi Inami-san,

Quoted 1 line · Gaku Inami

Quoted 1 line
I2C bus for VDD MPU regulator is IIC3, not I2C3.

[...]

Quoted 1 line

Quoted 1 line
Signed-off-by Khiem Nguyen <[email protected]>

[...]

Quoted 2 lines

Thank you for updating patch.
I tested your patch. Test reult is good.

Thanks for your testing.

[...]

Quoted 1 line

Tested-by Gaku Inami <[email protected]>

[...]

Quoted 1 line

This is a bug fix of my cpufreq patch. Thank you for your fix.

OK.
Let's wait for final comment from Magnus-san.

Best regards,
KHIEM Nguyen

Simon Horman

2014-07-06 14:37 UTC #3 lore raw

In reply to Khiem Nguyen

Quoted 9 lines · Khiem Nguyen

Hi Inami-san,
Quoted 1 line · Gaku Inami
Quoted 1 line
I2C bus for VDD MPU regulator is IIC3, not I2C3.
[...]
Quoted 1 line
Quoted 1 line
Signed-off-by Khiem Nguyen <[email protected]>
[...]
Quoted 2 lines
Thank you for updating patch.
I tested your patch. Test reult is good.

Quoted 8 lines

Thanks for your testing.
[...]
Quoted 1 line
Tested-by Gaku Inami <[email protected]>
[...]
Quoted 1 line
This is a bug fix of my cpufreq patch. Thank you for your fix.
OK.
Let's wait for final comment from Magnus-san.

Thanks, I have decided to queue this up.

Simon Horman

2014-07-06 14:58 UTC #4 lore raw

In reply to Khiem Nguyen

Quoted 12 lines · Simon Horman

Quoted 11 lines · Khiem Nguyen
Hi Inami-san,
Quoted 1 line · Gaku Inami
Quoted 1 line
I2C bus for VDD MPU regulator is IIC3, not I2C3.
[...]
Quoted 1 line
Quoted 1 line
Signed-off-by Khiem Nguyen <[email protected]>
[...]
Quoted 2 lines
Thank you for updating patch.
I tested your patch. Test reult is good.
Thanks for your testing.

Quoted 8 lines

Quoted 6 lines
[...]
Quoted 1 line
Tested-by Gaku Inami <[email protected]>
[...]
Quoted 1 line
This is a bug fix of my cpufreq patch. Thank you for your fix.
OK.
Let's wait for final comment from Magnus-san.
Thanks, I have decided to queue this up.

On second thoughts I will wait for Magnus.

In particular I think you still need to address his feedback:

Hi Khiem-san,

Thanks for your patch. Can you please extend the commit message to include the reason behind this patch?

From my side this looks like a software policy change. Unless I'm mistaken both I2C3 and IIC3 share the same pins on the SoC, and because of that it should be possible to access that particular I2C bus already without any modification.

So please explain why you want to change this.

Khiem Nguyen

author

2014-07-06 22:39 UTC #5 lore raw

In reply to Khiem Nguyen

Dear Simon-san, Magnus-san,
cc Inami-san,

Quoted 4 lines

Quoted 4 lines
Thanks for your patch. Can you please extend the commit message to include the reason behind this patch?
In particular I think you still need to address his feedback:

I'd like to explain it here. After all parties agree, I will send new version of my patch.

The reason of this patch is simply to align with information in datasheet of R-Car H2.
With this change, CPUFreq implementation for Lager board is aligned with implementation for Koelsch board.

Is it reasonable enough ?

Thanks.

Best regards,
KHIEM Nguyen

Quoted 38 lines · Simon Horman

Quoted 12 lines · Simon Horman
Quoted 11 lines · Khiem Nguyen
Hi Inami-san,
Quoted 1 line · Gaku Inami
Quoted 1 line
I2C bus for VDD MPU regulator is IIC3, not I2C3.
[...]
Quoted 1 line
Quoted 1 line
Signed-off-by Khiem Nguyen <[email protected]>
[...]
Quoted 2 lines
Thank you for updating patch.
I tested your patch. Test reult is good.
Thanks for your testing.
Quoted 8 lines
Quoted 6 lines
[...]
Quoted 1 line
Tested-by Gaku Inami <[email protected]>
[...]
Quoted 1 line
This is a bug fix of my cpufreq patch. Thank you for your fix.
OK.
Let's wait for final comment from Magnus-san.
Thanks, I have decided to queue this up.
On second thoughts I will wait for Magnus.
In particular I think you still need to address his feedback:
Hi Khiem-san,
Thanks for your patch. Can you please extend the commit message to include the reason behind this patch?
From my side this looks like a software policy change. Unless I'm mistaken both I2C3 and IIC3 share the same pins on the SoC, and because of that it should be possible to access that particular I2C bus already without any modification.
So please explain why you want to change this.

Magnus Damm

2014-07-10 08:23 UTC #6 lore raw

In reply to Khiem Nguyen

Hi Khiem-san, Wolfram, everyone,

[CC Wolfram]

Quoted 7 lines · Khiem Nguyen

I2C bus for VDD MPU regulator is IIC3, not I2C3.
Signed-off-by Khiem Nguyen <[email protected]>
---
v2:
- re-based on top of tag renesas-devel-v3.16-rc3-20140701.
- removed redundant spaces.

This patch is changing I2C master controller device for the I2C bus where the power chip is located. I've heard that all of our different I2C controllers come with a range of features and issues, so I think we should check with the I2C maintainer Wolfram Sang about this matter.

Wolfram, can you give us your opinion about this patch? Does it improve our I2C software support on Lager?

Khiem-san, in the future please CC Wolfram about issues related to I2C.

Thanks,

/ magnus

Khiem Nguyen

author

2014-07-10 08:43 UTC #7 lore raw

In reply to Khiem Nguyen

Quoted 14 lines · Magnus Damm

Hi Khiem-san, Wolfram, everyone,
[CC Wolfram]
Quoted 7 lines · Khiem Nguyen
I2C bus for VDD MPU regulator is IIC3, not I2C3.
Signed-off-by Khiem Nguyen <[email protected]>
---
v2:
- re-based on top of tag renesas-devel-v3.16-rc3-20140701.
- removed redundant spaces.

Quoted 8 lines

This patch is changing I2C master controller device for the I2C bus where the power chip is located. I've heard that all of our different I2C controllers come with a range of features and issues, so I think we should check with the I2C maintainer Wolfram Sang about this matter.
Wolfram, can you give us your opinion about this patch? Does it improve our I2C software support on Lager?

IICDVFS is used for controlling power chip, according to H2 datasheet. In H2/Lager board, name of IICDVFS is IIC3.
This patch is to correct I2C bus in H2/Lager board (revise to IIC3 from I2C3).

In M2/Koelsch board, name of IICDVFS is I2C6.

Quoted 2 lines

Khiem-san, in the future please CC Wolfram about issues related to I2C.

I got it. Thanks.

Quoted 3 lines

/ magnus

Signature · 2 lines

Best regards,
KHIEM Nguyen

Wolfram Sang

Reviewed-by on v2 1/1

2014-07-10 09:40 UTC #8 lore raw

In reply to Khiem Nguyen

Quoted 3 lines

Wolfram, can you give us your opinion about this patch? Does it improve our I2C software support on Lager?

Yes, it does. Khiem-san is right, IIC3 is dedicated to DVFS, so it should be activated. From that point of view:

Reviewed-by Wolfram Sang <[email protected]>

However, I sadly missed commit e489c2a9bc82713167d9f721ca764f4b0d37e543 ("ARM: shmobile: lager: enable i2c devices") which enables i2c cores for all busses. I do like the iic cores better because of SMBUS_QUICK support and DMA capability (although not yet supported). i2c cores have slave support as an advantage, yet they can be switched in when that is actually needed (still needs to be implemented, too). So, it might make sense to switch all busses to iic, I'd say.

All the best,

Wolfram

Magnus Damm

2014-07-10 09:46 UTC #9 lore raw

In reply to Khiem Nguyen

Hi Wolfram, Khiem-san, everyone,

Quoted 8 lines · Wolfram Sang

Quoted 3 lines
Wolfram, can you give us your opinion about this patch? Does it improve our I2C software support on Lager?
Yes, it does. Khiem-san is right, IIC3 is dedicated to DVFS, so it should be activated. From that point of view:
Reviewed-by Wolfram Sang <[email protected]>

Good - thanks for confirming!

Quoted 7 lines

However, I sadly missed commit e489c2a9bc82713167d9f721ca764f4b0d37e543 ("ARM: shmobile: lager: enable i2c devices") which enables i2c cores for all busses. I do like the iic cores better because of SMBUS_QUICK support and DMA capability (although not yet supported). i2c cores have slave support as an advantage, yet they can be switched in when that is actually needed (still needs to be implemented, too). So, it might make sense to switch all busses to iic, I'd say.

Yes, I agree about your observation. If you have time, can you cook up a patch to improve the situation?

Best,

/ magnus

Khiem Nguyen

author

2014-07-10 10:01 UTC #10 lore raw

In reply to Khiem Nguyen

Quoted 7 lines · Magnus Damm

Hi Wolfram, Khiem-san, everyone,
Quoted 4 lines · Wolfram Sang
Quoted 3 lines
Wolfram, can you give us your opinion about this patch? Does it improve our I2C software support on Lager?

Quoted 6 lines

Quoted 4 lines
Yes, it does. Khiem-san is right, IIC3 is dedicated to DVFS, so it should be activated. From that point of view:
Reviewed-by Wolfram Sang <[email protected]>
Good - thanks for confirming!

@Wolfram-san: Thanks for your review.

@Magnus-san, Simon-san: Then, is it OK to merge this patch ?

Quoted 16 lines

Quoted 8 lines
However, I sadly missed commit e489c2a9bc82713167d9f721ca764f4b0d37e543 ("ARM: shmobile: lager: enable i2c devices") which enables i2c cores for all busses. I do like the iic cores better because of SMBUS_QUICK support and DMA capability (although not yet supported). i2c cores have slave support as an advantage, yet they can be switched in when that is actually needed (still needs to be implemented, too). So, it might make sense to switch all busses to iic, I'd say.
Yes, I agree about your observation. If you have time, can you cook up a patch to improve the situation?
Best,
/ magnus

Signature · 2 lines

Best regards,
KHIEM Nguyen

Magnus Damm

2014-07-10 10:03 UTC #11 lore raw

In reply to Khiem Nguyen

Hi Khiem-san,

Quoted 11 lines · Khiem Nguyen

Quoted 10 lines · Magnus Damm
Hi Wolfram, Khiem-san, everyone,
Quoted 7 lines · Wolfram Sang
Quoted 3 lines
Wolfram, can you give us your opinion about this patch? Does it improve our I2C software support on Lager?
Yes, it does. Khiem-san is right, IIC3 is dedicated to DVFS, so it should be activated. From that point of view:

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
Reviewed-by Wolfram Sang <[email protected]>
Good - thanks for confirming!
@Wolfram-san: Thanks for your review.
@Magnus-san, Simon-san: Then, is it OK to merge this patch ?

Yes, from my side all is fine. I believe Simon will merge it when he has some time to spare.

Cheers,

/ magnus

Simon Horman

2014-07-11 08:45 UTC #12 lore raw

In reply to Khiem Nguyen

Quoted 17 lines · Magnus Damm

Hi Khiem-san,
Quoted 13 lines · Khiem Nguyen
Quoted 12 lines · Magnus Damm
Hi Wolfram, Khiem-san, everyone,
Quoted 8 lines · Wolfram Sang
Quoted 3 lines
Wolfram, can you give us your opinion about this patch? Does it improve our I2C software support on Lager?
Yes, it does. Khiem-san is right, IIC3 is dedicated to DVFS, so it should be activated. From that point of view:
Reviewed-by Wolfram Sang <[email protected]>

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Good - thanks for confirming!
@Wolfram-san: Thanks for your review.
@Magnus-san, Simon-san: Then, is it OK to merge this patch ?
Yes, from my side all is fine. I believe Simon will merge it when he has some time to spare.

Thanks, I have queued this up with Wolfram's Reviewed-by tag.

Khiem Nguyen

author

2014-07-13 23:19 UTC #13 lore raw

In reply to Khiem Nguyen

Dear Simon-san,

Quoted 11 lines · Simon Horman

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
Wolfram, can you give us your opinion about this patch? Does it improve our I2C software support on Lager?
Yes, it does. Khiem-san is right, IIC3 is dedicated to DVFS, so it should be activated. From that point of view:
Reviewed-by Wolfram Sang <[email protected]>
Thanks, I have queued this up with Wolfram's Reviewed-by tag.

Thanks. I got it.

Signature · 2 lines

Best regards,
KHIEM Nguyen