[U-Boot] I2C on iMX25

[U-Boot] I2C on iMX25

[Matthias Weißer]

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

[U-Boot] I2C on iMX25

[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.

> 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

-- 
=====================================================================
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
=====================================================================

[U-Boot] I2C on iMX25

[Matthias Weißer]

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

[U-Boot] I2C on iMX25

[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.

> > 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

[U-Boot] I2C on iMX25

[Matthias Weißer]

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

[U-Boot] I2C on iMX25

[Stefano Babic]

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

-- 
=====================================================================
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
=====================================================================

[U-Boot] I2C on iMX25

[Benoît Thébaudeau]

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