[U-Boot] I2C on iMX25
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
On 24/09/2012 11:32, Matthias Weißer wrote:
Hi Stefano
Hi Matthias,
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.
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.
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:
diff --git a/arch/arm/cpu/armv7/mx5/clock.c b/arch/arm/cpu/armv7/mx5/clock.c index c67c3cf..8fa737a 100644 --- a/arch/arm/cpu/armv7/mx5/clock.c +++ b/arch/arm/cpu/armv7/mx5/clock.c @@ -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
Hi Stefano
Am 24.09.2012 11:45, schrieb Stefano Babic:
On 24/09/2012 11:32, Matthias Weißer wrote:
Hi Stefano
Hi Matthias,
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.
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
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
Hi Stefano, Matthias,
On Monday, September 24, 2012 11:45:33 AM, Stefano Babic wrote:
On 24/09/2012 11:32, Matthias Weißer wrote:
Hi Stefano
Hi Matthias,
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.
I had the same issue a while ago. I have a fix for that. I will try to post it tonight.
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.
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.
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:
diff --git a/arch/arm/cpu/armv7/mx5/clock.c b/arch/arm/cpu/armv7/mx5/clock.c index c67c3cf..8fa737a 100644 --- a/arch/arm/cpu/armv7/mx5/clock.c +++ b/arch/arm/cpu/armv7/mx5/clock.c @@ -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.
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
Am 24.09.2012 13:05, schrieb Benoît Thébaudeau:
Hi Stefano, Matthias,
On Monday, September 24, 2012 11:45:33 AM, Stefano Babic wrote:
On 24/09/2012 11:32, Matthias Weißer wrote:
Hi Stefano
Hi Matthias,
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.
I had the same issue a while ago. I have a fix for that. I will try to post it tonight.
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.
What is the advantage of such a function over i2c_imx_get_clk(MXC_I2C_CLK)?
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:
diff --git a/arch/arm/cpu/armv7/mx5/clock.c b/arch/arm/cpu/armv7/mx5/clock.c index c67c3cf..8fa737a 100644 --- a/arch/arm/cpu/armv7/mx5/clock.c +++ b/arch/arm/cpu/armv7/mx5/clock.c @@ -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.
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
On Monday, September 24, 2012 1:34:57 PM, Matthias Weißer wrote:
Am 24.09.2012 13:05, schrieb Benoît Thébaudeau:
Hi Stefano, Matthias,
On Monday, September 24, 2012 11:45:33 AM, Stefano Babic wrote:
On 24/09/2012 11:32, Matthias Weißer wrote:
Hi Stefano
Hi Matthias,
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.
I had the same issue a while ago. I have a fix for that. I will try to post it tonight.
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.
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.
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:
diff --git a/arch/arm/cpu/armv7/mx5/clock.c b/arch/arm/cpu/armv7/mx5/clock.c index c67c3cf..8fa737a 100644 --- a/arch/arm/cpu/armv7/mx5/clock.c +++ b/arch/arm/cpu/armv7/mx5/clock.c @@ -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.
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.
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
On 24/09/2012 13:05, Benoît Thébaudeau wrote:
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.
diff --git a/arch/arm/cpu/armv7/mx5/clock.c b/arch/arm/cpu/armv7/mx5/clock.c index c67c3cf..8fa737a 100644 --- a/arch/arm/cpu/armv7/mx5/clock.c +++ b/arch/arm/cpu/armv7/mx5/clock.c @@ -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.
That can be a good solution. What do you think about my imx_get_i2cclk()?
My preference goes to not add it.
Regards, Stefano
participants (3)
-
Benoît Thébaudeau -
Matthias Weißer -
Stefano Babic