I2C on iMX25

U-Boot Matthias Weißer 3 participants 7 messages Sep 24, 2012 Arm (32-bit)

Share & tools

Matthias Weißer

author

2012-09-24 09:32 UTC #0 lore raw

Hi Stefano

I am currently in the process of updating my zmx25 board support for a new hardware revision where I need I2C access. I2C on imx25 currently fails to build:

mxc_i2c.c: In function 'i2c_imx_get_clk':
mxc_i2c.c:101:31: error: 'MXC_IPG_PERCLK' undeclared (first use in this function)

I can easily fix this by replacing MXC_IPG_PERCLK with MXC_I2C_CLK. But MXC_I2C_CLK is only defined for imx25. So, this change will break all other imx chips. I can now add MXC_IPG_PERCLK to arch-mx25/clock.h and adopt generic.c accordingly but I don't think that this is the right way to go as the i2c clock can be different from perclk. Doing this #define MXC_IPG_PERCLK MXC_I2C_CLK
in my config file is even more ugly.

Do you have any advice for me how to solve that?

Regards
Matthias

Stefano Babic

2012-09-24 09:45 UTC #1 lore raw

Quoted 2 lines · Matthias Wei?er

Hi Stefano

Hi Matthias,

Quoted 7 lines

I am currently in the process of updating my zmx25 board support for a new hardware revision where I need I2C access. I2C on imx25 currently fails to build:
mxc_i2c.c: In function 'i2c_imx_get_clk':
mxc_i2c.c:101:31: error: 'MXC_IPG_PERCLK' undeclared (first use in this function)

Ok, I see.

Quoted 4 lines

I can easily fix this by replacing MXC_IPG_PERCLK with MXC_I2C_CLK. But MXC_I2C_CLK is only defined for imx25. So, this change will break all other imx chips.

But this seems the right solution. The mxc_get_clk() gets as parameter an enum representing a peripheral or a special clock name, valid for a SOC. The driver should use the peripheral name.

Quoted 5 lines

I can now add MXC_IPG_PERCLK to arch-mx25/clock.h and adopt generic.c accordingly but I don't think that this is the right way to go as the i2c clock can be different from perclk. Doing this #define MXC_IPG_PERCLK MXC_I2C_CLK
in my config file is even more ugly.

This is wrong.

Really I think the right way is to add MXC_I2C_CLK to the other SOCs, adding the case in their specific mxc_get_clock() implementation, for example for mx6 something like this:

arch/arm/cpu/armv7/mx5/clock.c +1 -0

index c67c3cf..8fa737a 100644
@@ -482,6 +482,7 @@ unsigned int mxc_get_clock(enum mxc_clock clk)
        case MXC_IPG_CLK:
                return get_ipg_clk();
        case MXC_IPG_PERCLK:
+       case MXC_I2C_CLK:
                return get_ipg_per_clk();
        case MXC_UART_CLK:
                return get_uart_clk();

and updating the mxc_i2c driver to follow the same rule.

Best regards,
Stefano

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

Matthias Weißer

author

2012-09-24 11:04 UTC #2 lore raw

Hi Stefano

Am 24.09.2012 11:45, schrieb Stefano Babic:

Quoted 17 lines · Matthias Wei?er

Quoted 2 lines
Hi Stefano
Hi Matthias,
Quoted 7 lines
I am currently in the process of updating my zmx25 board support for a new hardware revision where I need I2C access. I2C on imx25 currently fails to build:
mxc_i2c.c: In function 'i2c_imx_get_clk':
mxc_i2c.c:101:31: error: 'MXC_IPG_PERCLK' undeclared (first use in this function)
Ok, I see.
Quoted 1 line

Quoted 7 lines

Quoted 3 lines
I can easily fix this by replacing MXC_IPG_PERCLK with MXC_I2C_CLK. But MXC_I2C_CLK is only defined for imx25. So, this change will break all other imx chips.
But this seems the right solution. The mxc_get_clk() gets as parameter an enum representing a peripheral or a special clock name, valid for a SOC. The driver should use the peripheral name.

ACK

Quoted 1 line

and updating the mxc_i2c driver to follow the same rule.

I can create such a patch but I am not able to runtime test it on any other system then imx25. Will do so.

Regards
Matthias

Benoît Thébaudeau

2012-09-24 11:05 UTC #3 lore raw

In reply to Stefano Babic

Hi Stefano, Matthias,

Quoted 12 lines · Stefano Babic

Quoted 2 lines · Matthias Wei?er
Hi Stefano
Hi Matthias,
Quoted 6 lines
I am currently in the process of updating my zmx25 board support for a new
hardware revision where I need I2C access. I2C on imx25 currently fails
to build:

Quoted 6 lines

Quoted 4 lines
mxc_i2c.c: In function 'i2c_imx_get_clk':
mxc_i2c.c:101:31: error: 'MXC_IPG_PERCLK' undeclared (first use in this
function)
Ok, I see.

I had the same issue a while ago. I have a fix for that. I will try to post it tonight.

Quoted 11 lines

Quoted 5 lines
I can easily fix this by replacing MXC_IPG_PERCLK with MXC_I2C_CLK. But
MXC_I2C_CLK is only defined for imx25. So, this change will break all other
imx chips.
But this seems the right solution. The mxc_get_clk() gets as parameter
an enum representing a peripheral or a special clock name, valid for a
SOC. The driver should use the peripheral name.

Yes and no. The best would be to add a clock abstraction function imx_get_i2cclk(), like what exists for UART. This is what I did.

Quoted 8 lines

Quoted 6 lines
I can now add MXC_IPG_PERCLK to arch-mx25/clock.h and adopt generic.c accordingly but I don't think that this is the right way to go
as the i2c clock can be different from perclk. Doing this #define MXC_IPG_PERCLK MXC_I2C_CLK
in my config file is even more ugly.
This is wrong.

I agree. MXC_IPG_PERCLK means something else, and the I?C clock is not that clock on i.MX25.

Quoted 19 lines

Really I think the right way is to add MXC_I2C_CLK to the other SOCs, adding the case in their specific mxc_get_clock() implementation, for example for mx6 something like this:

arch/arm/cpu/armv7/mx5/clock.c +1 -0

index c67c3cf..8fa737a 100644
@@ -482,6 +482,7 @@ unsigned int mxc_get_clock(enum mxc_clock clk)
        case MXC_IPG_CLK:
                return get_ipg_clk();
        case MXC_IPG_PERCLK:
+       case MXC_I2C_CLK:
                return get_ipg_per_clk();
        case MXC_UART_CLK:
                return get_uart_clk();

Quoted 1 line

and updating the mxc_i2c driver to follow the same rule.

That can be a good solution. What do you think about my imx_get_i2cclk()?

Also, note that there are some broken clocks for i.MX25. I?C is one of them. It should be:

	case MXC_I2C_CLK:
		return imx_get_perclk(I2C_PER_CLK);

Best regards,
Beno?t

Matthias Weißer

author

2012-09-24 11:34 UTC #4 lore raw

Am 24.09.2012 13:05, schrieb Beno?t Th?baudeau:

Quoted 31 lines

Hi Stefano, Matthias,
Quoted 12 lines · Stefano Babic
Quoted 2 lines · Matthias Wei?er
Hi Stefano
Hi Matthias,
Quoted 6 lines
I am currently in the process of updating my zmx25 board support for a new
hardware revision where I need I2C access. I2C on imx25 currently fails
to build:
Quoted 6 lines
Quoted 4 lines
mxc_i2c.c: In function 'i2c_imx_get_clk':
mxc_i2c.c:101:31: error: 'MXC_IPG_PERCLK' undeclared (first use in this
function)
Ok, I see.
I had the same issue a while ago. I have a fix for that. I will try to post it tonight.
Quoted 6 lines
Quoted 5 lines
I can easily fix this by replacing MXC_IPG_PERCLK with MXC_I2C_CLK. But
MXC_I2C_CLK is only defined for imx25. So, this change will break all other
imx chips.

Quoted 8 lines

Quoted 5 lines
But this seems the right solution. The mxc_get_clk() gets as parameter
an enum representing a peripheral or a special clock name, valid for a
SOC. The driver should use the peripheral name.
Yes and no. The best would be to add a clock abstraction function imx_get_i2cclk(), like what exists for UART. This is what I did.

What is the advantage of such a function over i2c_imx_get_clk(MXC_I2C_CLK)?

Quoted 19 lines

Really I think the right way is to add MXC_I2C_CLK to the other SOCs, adding the case in their specific mxc_get_clock() implementation, for example for mx6 something like this:

arch/arm/cpu/armv7/mx5/clock.c +1 -0

index c67c3cf..8fa737a 100644
@@ -482,6 +482,7 @@ unsigned int mxc_get_clock(enum mxc_clock clk)
        case MXC_IPG_CLK:
                return get_ipg_clk();
        case MXC_IPG_PERCLK:
+       case MXC_I2C_CLK:
                return get_ipg_per_clk();
        case MXC_UART_CLK:
                return get_uart_clk();

Quoted 8 lines

Quoted 1 line
and updating the mxc_i2c driver to follow the same rule.
That can be a good solution. What do you think about my imx_get_i2cclk()?
Also, note that there are some broken clocks for i.MX25. I?C is one of them. It should be:
	case MXC_I2C_CLK:
		return imx_get_perclk(I2C_PER_CLK);

Why that? My understanding is that imx_get_perclk picks the right clock as long as the 16 first entries of enum mxc_clock ar in the rigth order.

Regards
Matthias

Benoît Thébaudeau

2012-09-24 12:35 UTC #5 lore raw

Quoted 44 lines · Matthias Wei?er

Am 24.09.2012 13:05, schrieb Beno?t Th?baudeau:
Quoted 43 lines
Hi Stefano, Matthias,
Quoted 13 lines · Stefano Babic
Quoted 2 lines · Matthias Wei?er
Hi Stefano
Hi Matthias,
Quoted 7 lines
I am currently in the process of updating my zmx25 board support for a new
hardware revision where I need I2C access. I2C on imx25 currently
fails
to build:
Quoted 7 lines
Quoted 5 lines
mxc_i2c.c: In function 'i2c_imx_get_clk':
mxc_i2c.c:101:31: error: 'MXC_IPG_PERCLK' undeclared (first use in
this
function)
Ok, I see.
I had the same issue a while ago. I have a fix for that. I will try to post it
tonight.
Quoted 14 lines
Quoted 7 lines
I can easily fix this by replacing MXC_IPG_PERCLK with MXC_I2C_CLK.
But
MXC_I2C_CLK is only defined for imx25. So, this change will break
all other
imx chips.
But this seems the right solution. The mxc_get_clk() gets as parameter
an enum representing a peripheral or a special clock name, valid for
a
SOC. The driver should use the peripheral name.

Quoted 5 lines

Quoted 2 lines
Yes and no. The best would be to add a clock abstraction function imx_get_i2cclk(), like what exists for UART. This is what I did.
What is the advantage of such a function over
i2c_imx_get_clk(MXC_I2C_CLK)?

Not to introduce a clock ID that does not match register controls, but this is really a nit. The MXC_I2C_CLK solution is less noisy than adding a new function, so let's stick to it, all the more Stefano prefers it. I will update my local patch with that before posting it.

Quoted 32 lines

Quoted 26 lines
Quoted 21 lines
Really I think the right way is to add MXC_I2C_CLK to the other SOCs,
adding the case in their specific mxc_get_clock() implementation, for
example for mx6 something like this:
arch/arm/cpu/armv7/mx5/clock.c +1 -0
index c67c3cf..8fa737a 100644
@@ -482,6 +482,7 @@ unsigned int mxc_get_clock(enum mxc_clock clk)
        case MXC_IPG_CLK:
                return get_ipg_clk();
        case MXC_IPG_PERCLK:
+       case MXC_I2C_CLK:
                return get_ipg_per_clk();
        case MXC_UART_CLK:
                return get_uart_clk();
Quoted 1 line
and updating the mxc_i2c driver to follow the same rule.
That can be a good solution. What do you think about my imx_get_i2cclk()?
Quoted 5 lines
Also, note that there are some broken clocks for i.MX25. I?C is one of them. It
should be:
	case MXC_I2C_CLK:
		return imx_get_perclk(I2C_PER_CLK);

Quoted 4 lines

Why that? My understanding is that imx_get_perclk picks the right clock
as long as the 16 first entries of enum mxc_clock ar in the rigth order.

You're right. I looked too quickly at my local changes when I said that this clock was broken. It works. What I did locally is split the per clocks away from enum mxc_clock to be cleaner than having a mix of all types of clocks, but this is actually only cosmetic, and I'm not sure I will keep this change.

Best regards,
Beno?t

Stefano Babic

2012-09-24 11:54 UTC #6 lore raw

In reply to Benoît Thébaudeau

Quoted 8 lines · Beno?t Th?baudeau

Quoted 5 lines
But this seems the right solution. The mxc_get_clk() gets as parameter
an enum representing a peripheral or a special clock name, valid for a
SOC. The driver should use the peripheral name.
Yes and no. The best would be to add a clock abstraction function imx_get_i2cclk(), like what exists for UART. This is what I did.

However, this duplicates the interface because we have a mxc_get_clk() and a function names for each peripheral. We have then a plethora of new functions, one for each peripheral, and all SOCs must implement them to be consistent.

I prefer to have only one function, available for all SOCs. Everybody who starts with a new iMX SOC then knows that he must implement mxc_get_clk(), and that is all.

Quoted 15 lines

arch/arm/cpu/armv7/mx5/clock.c +1 -0

index c67c3cf..8fa737a 100644
@@ -482,6 +482,7 @@ unsigned int mxc_get_clock(enum mxc_clock clk)
        case MXC_IPG_CLK:
                return get_ipg_clk();
        case MXC_IPG_PERCLK:
+       case MXC_I2C_CLK:
                return get_ipg_per_clk();
        case MXC_UART_CLK:
                return get_uart_clk();

Quoted 4 lines

Quoted 1 line
and updating the mxc_i2c driver to follow the same rule.
That can be a good solution. What do you think about my imx_get_i2cclk()?

My preference goes to not add it.

Regards,
Stefano

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================