[U-Boot] I2C on iMX25

["Benoît Thébaudeau" <benoit.thebaudeau@advansee.com>]

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