| Message ID | 50602BBD.5030605@denx.de |
|---|---|
| State | Changes Requested |
| Headers | show |
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 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
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
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();