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 _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
