[U-Boot] [PATCH 1/2][for v2008.10] Revert "85xx: Using proper I2C source clock divider for MPC8544"

[U-Boot] [PATCH 1/2][for v2008.10] Revert "85xx: Using proper I2C source clock divider for MPC8544"

[Kumar Gala]

This reverts commit dffd2446fb041f38ef034b0fcf41e51e5e489159.

The fix introduced by this patch is not correct.  The problem is
that the documentation is not correct for the MPC8544 with regards
to which bit in PORDEVSR2 is for the SEC_CFG.

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
 cpu/mpc85xx/speed.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cpu/mpc85xx/speed.c b/cpu/mpc85xx/speed.c
index 70dfad0..485ba20 100644
--- a/cpu/mpc85xx/speed.c
+++ b/cpu/mpc85xx/speed.c
@@ -102,9 +102,9 @@ int get_clocks (void)
 	 * PORDEVSR2_SEC_CFG bit is 0 on all 85xx boards that are not an 8544.
 	 */
 	if (gur->pordevsr2 & MPC85xx_PORDEVSR2_SEC_CFG)
-		gd->i2c1_clk = sys_info.freqSystemBus / 2;
-	else
 		gd->i2c1_clk = sys_info.freqSystemBus / 3;
+	else
+		gd->i2c1_clk = sys_info.freqSystemBus / 2;
 #else
 	/* Most 85xx SOCs use CCB/2, so this is the default behavior. */
 	gd->i2c1_clk = sys_info.freqSystemBus / 2;
-- 
1.5.5.1

[U-Boot] [PATCH 2/2][for v2008.10] 85xx: Using proper I2C source clock divider for MPC8544

[Kumar Gala]

The MPC8544 RM incorrect shows the SEC_CFG bit in PORDEVSR2 as being
bit 26, instead it should be bit 28.  This caused in incorrect
interpretation of the i2c_clk which is the same as the SEC clk on
MPC8544.  The SEC clk is controlled by cfg_sec_freq that is reported
in PORDEVSR2.

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
 include/asm-ppc/immap_85xx.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/asm-ppc/immap_85xx.h b/include/asm-ppc/immap_85xx.h
index 50c9dde..6c81c39 100644
--- a/include/asm-ppc/immap_85xx.h
+++ b/include/asm-ppc/immap_85xx.h
@@ -1579,7 +1579,7 @@ typedef struct ccsr_gur {
 #define MPC85xx_PORDEVSR_RIO_DEV_ID	0x00000007
 	uint	pordbgmsr;	/* 0xe0010 - POR debug mode status register */
 	uint	pordevsr2;	/* 0xe0014 - POR I/O device status regsiter 2 */
-#define MPC85xx_PORDEVSR2_SEC_CFG	0x00000020
+#define MPC85xx_PORDEVSR2_SEC_CFG	0x00000080
 	char	res1[8];
 	uint	gpporcr;	/* 0xe0020 - General-purpose POR configuration register */
 	char	res2[12];
-- 
1.5.5.1

[U-Boot] [PATCH 2/2][for v2008.10] 85xx: Using proper I2C source clock divider for MPC8544

[Wolfgang Denk]

Dear Kumar Gala,

In message <1224212330-14491-2-git-send-email-galak@kernel.crashing.org> you wrote:
> The MPC8544 RM incorrect shows the SEC_CFG bit in PORDEVSR2 as being
> bit 26, instead it should be bit 28.  This caused in incorrect
> interpretation of the i2c_clk which is the same as the SEC clk on
> MPC8544.  The SEC clk is controlled by cfg_sec_freq that is reported
> in PORDEVSR2.
> 
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> ---
>  include/asm-ppc/immap_85xx.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The explanation requiring the fewest assumptions is the  most  likely
to be correct.                                    -- William of Occam

[U-Boot] [PATCH 2/2][for v2008.10] 85xx: Using proper I2C source clock divider for MPC8544

[Wolfgang Grandegger]

Hi Kumar,

Kumar Gala wrote:
> The MPC8544 RM incorrect shows the SEC_CFG bit in PORDEVSR2 as being
> bit 26, instead it should be bit 28.  This caused in incorrect
> interpretation of the i2c_clk which is the same as the SEC clk on
> MPC8544.  The SEC clk is controlled by cfg_sec_freq that is reported
> in PORDEVSR2.
> 
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>

That makes sense now. I just got the confirmation from the customer,
that LWE_B[0] is indeed pulled down via FPGA:

  LWE_B[0]      Pulldown       SEC Frequency Ratio 2:1

Thanks for your investigations.

Wolfgang.

[U-Boot] [PATCH 2/2][for v2008.10] 85xx: Using proper I2C source clock divider for MPC8544

[Timur Tabi]

On Thu, Oct 16, 2008 at 9:58 PM, Kumar Gala <galak@kernel.crashing.org> wrote:

> -#define MPC85xx_PORDEVSR2_SEC_CFG      0x00000020
> +#define MPC85xx_PORDEVSR2_SEC_CFG      0x00000080

How about adding a comment that the RM is wrong?   Any time the code
disagrees with the RM, it *has* to be documented.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 2/2][for v2008.10] 85xx: Using proper I2C source clock divider for MPC8544

[Kumar Gala]

On Oct 17, 2008, at 1:54 PM, Timur Tabi wrote:

> On Thu, Oct 16, 2008 at 9:58 PM, Kumar Gala  
> <galak@kernel.crashing.org> wrote:
>
>> -#define MPC85xx_PORDEVSR2_SEC_CFG      0x00000020
>> +#define MPC85xx_PORDEVSR2_SEC_CFG      0x00000080
>
> How about adding a comment that the RM is wrong?   Any time the code
> disagrees with the RM, it *has* to be documented.

Its documented in the commit.

- k

[U-Boot] [PATCH 2/2][for v2008.10] 85xx: Using proper I2C source clock divider for MPC8544

[Timur Tabi]

Kumar Gala wrote:

>> How about adding a comment that the RM is wrong?   Any time the code
>> disagrees with the RM, it *has* to be documented.
> 
> Its documented in the commit.

Sorry, but that's just not good enough for me.  I *hate* it when people say that
it is.

When I'm looking at some code, and I don't understand what it does, the last
thing I'm doing to do is scour through the git log trying to find some
explanation.

Would it really have been so hard to add this line?

/* The MPC8544 RM says it's bit 26, but it's really bit 28 */

Why don't we take your reasoning to the next level, and just say that code
comments are unnecessary because the git log contains everything we need to know?

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 2/2][for v2008.10] 85xx: Using proper I2C source clock divider for MPC8544

[Jerry Van Baren]

Timur Tabi wrote:
> Kumar Gala wrote:
> 
>>> How about adding a comment that the RM is wrong?   Any time the code
>>> disagrees with the RM, it *has* to be documented.
>> Its documented in the commit.
> 
> Sorry, but that's just not good enough for me.  I *hate* it when people say that
> it is.
> 
> When I'm looking at some code, and I don't understand what it does, the last
> thing I'm doing to do is scour through the git log trying to find some
> explanation.
> 
> Would it really have been so hard to add this line?
> 
> /* The MPC8544 RM says it's bit 26, but it's really bit 28 */

I have to agree with Timur, we *need* a comment in the source code for 
future reference.  The git log is good, but not sufficient.

Thanks,
gvb

[U-Boot] [PATCH 2/2][for v2008.10] 85xx: Using proper I2C source clock divider for MPC8544

[Wolfgang Denk]

Dear Jerry Van Baren,

In message <48FC815A.90501@ge.com> you wrote:
>
> > Would it really have been so hard to add this line?
> > 
> > /* The MPC8544 RM says it's bit 26, but it's really bit 28 */
> 
> I have to agree with Timur, we *need* a comment in the source code for 
> future reference.  The git log is good, but not sufficient.

Agreed, but in the first place we should figure out if it's  *really*
bit 28 (then the new code is wrong), or bit 24, then the doc is wrong
again.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
This is an unauthorized cybernetic announcement.

[U-Boot] [PATCH 2/2][for v2008.10] 85xx: Using proper I2C source clock divider for MPC8544

[Kumar Gala]

On Oct 20, 2008, at 2:22 PM, Wolfgang Denk wrote:

> Dear Jerry Van Baren,
>
> In message <48FC815A.90501@ge.com> you wrote:
>>
>>> Would it really have been so hard to add this line?
>>>
>>> /* The MPC8544 RM says it's bit 26, but it's really bit 28 */
>>
>> I have to agree with Timur, we *need* a comment in the source code  
>> for
>> future reference.  The git log is good, but not sufficient.
>
> Agreed, but in the first place we should figure out if it's  *really*
> bit 28 (then the new code is wrong), or bit 24, then the doc is wrong
> again.

Its bit 24, my commit message was incorrect.

- k

[U-Boot] [PATCH 1/2][for v2008.10] Revert "85xx: Using proper I2C source clock divider for MPC8544"

[Wolfgang Denk]

Dear Kumar Gala,

In message <1224212330-14491-1-git-send-email-galak@kernel.crashing.org> you wrote:
> This reverts commit dffd2446fb041f38ef034b0fcf41e51e5e489159.
> 
> The fix introduced by this patch is not correct.  The problem is
> that the documentation is not correct for the MPC8544 with regards
> to which bit in PORDEVSR2 is for the SEC_CFG.
> 
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> ---
>  cpu/mpc85xx/speed.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Honest error is to be pitied, not ridiculed.
                                       -- Philip Earl of Chesterfield