[U-Boot] [PATCH] mx1: add mx1/l support for mxc_i2c

[U-Boot] [PATCH] mx1: add mx1/l support for mxc_i2c

[Eric Jarrige]

Signed-off-by: Eric Jarrige <eric.jarrige@armadeus.org>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Heiko Schocher <hs@denx.de>
---
 drivers/i2c/mxc_i2c.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
index 89d1973..597e9ee 100644
--- a/drivers/i2c/mxc_i2c.c
+++ b/drivers/i2c/mxc_i2c.c
@@ -27,7 +27,9 @@
 
 #if defined(CONFIG_HARD_I2C)
 
+#ifndef CONFIG_IMX
 #include <asm/arch/clock.h>
+#endif
 #include <asm/arch/imx-regs.h>
 
 #define IADR	0x00
@@ -63,6 +65,8 @@
 #define I2C_BASE        I2C2_BASE_ADDR
 #elif defined(CONFIG_SYS_I2C_MX35_PORT1)
 #define I2C_BASE	I2C_BASE_ADDR
+#elif defined(CONFIG_SYS_I2C_MX1_PORT1)
+#define I2C_BASE	IMX_I2C_BASE
 #else
 #error "define CONFIG_SYS_I2C_MX<Processor>_PORTx to use the mx I2C driver"
 #endif
@@ -94,6 +98,8 @@ void i2c_init(int speed, int unused)
 	/* start the required I2C clock */
 	writel(readl(&sc_regs->cgr0) | (3 << I2C_CLK_OFFSET),
 		&sc_regs->cgr0);
+#elif defined(CONFIG_IMX)
+	freq = get_HCLK();
 #else
 	freq = mxc_get_clock(MXC_IPG_PERCLK);
 #endif

[U-Boot] [PATCH] mx1: add mx1/l support for mxc_i2c

[Heiko Schocher]

Hello Stefano,

Eric Jarrige wrote:
> Signed-off-by: Eric Jarrige <eric.jarrige@armadeus.org>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Heiko Schocher <hs@denx.de>
> ---
>  drivers/i2c/mxc_i2c.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)

from my side of view, this patch is Ok, could you Ack it, or
do you want to pick it up, so I give my:

Acked-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] mx1: add mx1/l support for mxc_i2c

[Marek Vasut]

On Monday, August 22, 2011 10:56:43 PM Eric Jarrige wrote:
> Signed-off-by: Eric Jarrige <eric.jarrige@armadeus.org>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Heiko Schocher <hs@denx.de>
> ---
>  drivers/i2c/mxc_i2c.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
> index 89d1973..597e9ee 100644
> --- a/drivers/i2c/mxc_i2c.c
> +++ b/drivers/i2c/mxc_i2c.c
> @@ -27,7 +27,9 @@
> 
>  #if defined(CONFIG_HARD_I2C)
> 
> +#ifndef CONFIG_IMX
>  #include <asm/arch/clock.h>
> +#endif
>  #include <asm/arch/imx-regs.h>
> 
>  #define IADR	0x00
> @@ -63,6 +65,8 @@
>  #define I2C_BASE        I2C2_BASE_ADDR
>  #elif defined(CONFIG_SYS_I2C_MX35_PORT1)
>  #define I2C_BASE	I2C_BASE_ADDR
> +#elif defined(CONFIG_SYS_I2C_MX1_PORT1)
> +#define I2C_BASE	IMX_I2C_BASE
>  #else
>  #error "define CONFIG_SYS_I2C_MX<Processor>_PORTx to use the mx I2C
> driver" #endif
> @@ -94,6 +98,8 @@ void i2c_init(int speed, int unused)
>  	/* start the required I2C clock */
>  	writel(readl(&sc_regs->cgr0) | (3 << I2C_CLK_OFFSET),
>  		&sc_regs->cgr0);
> +#elif defined(CONFIG_IMX)
> +	freq = get_HCLK();
>  #else
>  	freq = mxc_get_clock(MXC_IPG_PERCLK);
>  #endif

Please, no more ifdefs -- can't you actually modify your header files or add 
some which would define mxc_get_clock() ? That'd make this way much more clean.

Cheers
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] mx1: add mx1/l support for mxc_i2c

[Stefano Babic]

On 09/19/2011 08:57 AM, Marek Vasut wrote:
> On Monday, August 22, 2011 10:56:43 PM Eric Jarrige wrote:
>> Signed-off-by: Eric Jarrige <eric.jarrige@armadeus.org>
>> Cc: Stefano Babic <sbabic@denx.de>
>> Cc: Heiko Schocher <hs@denx.de>
>> @@ -94,6 +98,8 @@ void i2c_init(int speed, int unused)
>>  	/* start the required I2C clock */
>>  	writel(readl(&sc_regs->cgr0) | (3 << I2C_CLK_OFFSET),
>>  		&sc_regs->cgr0);
>> +#elif defined(CONFIG_IMX)
>> +	freq = get_HCLK();
>>  #else
>>  	freq = mxc_get_clock(MXC_IPG_PERCLK);
>>  #endif
> 
> Please, no more ifdefs -- can't you actually modify your header files or add 
> some which would define mxc_get_clock() ? That'd make this way much more clean.

In principl, I agree completely with Marek, and we have tried to get rid
as much of possible of #ifdef in drivers. I know we have already
discussed about the opportunity to fix the whole IMX stuff, and I agreed
with you that this processor is obsolete and make no sense to invest a
lot of time for it. However, what about to define mxc_get_clock() as
simple macro ? Maybe in imx-regs.h, as exceptiion for this processor
(normally the imx-regs.h contains only defines and structures for the
internal registers) ? Something like:

	#define mxc_get_clock(a)	(get_HCLK())

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH] mx1: add mx1/l support for mxc_i2c

[Eric Jarrige]

Hi Stefano,

On 19 sept. 2011, at 09:26, Stefano Babic wrote:

> On 09/19/2011 08:57 AM, Marek Vasut wrote:
>> On Monday, August 22, 2011 10:56:43 PM Eric Jarrige wrote:
>>> Signed-off-by: Eric Jarrige <eric.jarrige@armadeus.org>
>>> Cc: Stefano Babic <sbabic@denx.de>
>>> Cc: Heiko Schocher <hs@denx.de>
>>> @@ -94,6 +98,8 @@ void i2c_init(int speed, int unused)
>>> 	/* start the required I2C clock */
>>> 	writel(readl(&sc_regs->cgr0) | (3 << I2C_CLK_OFFSET),
>>> 		&sc_regs->cgr0);
>>> +#elif defined(CONFIG_IMX)
>>> +	freq = get_HCLK();
>>> #else
>>> 	freq = mxc_get_clock(MXC_IPG_PERCLK);
>>> #endif
>> 
>> Please, no more ifdefs -- can't you actually modify your header files or add 
>> some which would define mxc_get_clock() ? That'd make this way much more clean.
> 
> In principl, I agree completely with Marek, and we have tried to get rid
> as much of possible of #ifdef in drivers. I know we have already
> discussed about the opportunity to fix the whole IMX stuff, and I agreed
> with you that this processor is obsolete and make no sense to invest a
> lot of time for it. However, what about to define mxc_get_clock() as
> simple macro ? Maybe in imx-regs.h, as exceptiion for this processor
> (normally the imx-regs.h contains only defines and structures for the
> internal registers) ? Something like:
> 
> 	#define mxc_get_clock(a)	(get_HCLK())

I agree with you to get rid of all these #ifdef and my current patch does not help at all.
I can update the imx-regs.h with the change you propose but that won't help for the
future iMX CPU.
IMHO the problem comes from all the hard coded constants in the driver such as
mxc_get_clock(MXC_IPG_PERCLK)  and others I2C_BASE...
Every chipset use a different root clock for its i2c controller.

Why not changing the line
	freq = mxc_get_clock(MXC_IPG_PERCLK);

by something more generic such as  a macro:

	freq = MXC_GET_I2C_CLOCK(); /* or GET_MXC_I2C_CLOCK() */


that can be defined in clock.h with the correct definition for each chipset?
Using a macro would clarify that is chipset dependent as each CPU use
its own clock.h

For the mx1, I can add a clock.h with the correct implementation of the
macro:
#define MXC_GET_I2C_CLOCK() get_HCLK()
or any other solution that avoid any new ifdef.

Does it makes sense?

With all these constants moved to header files it would be easier and faster to support
new CPU as the final hardware abstraction layer moved to header file specific for each
CPU.

Best regards,
Eric

[U-Boot] [PATCH] mx1: add mx1/l support for mxc_i2c

[stefano babic]

Am 19/09/2011 22:34, schrieb Eric Jarrige:
> Hi Stefano,
>
> On 19 sept. 2011, at 09:26, Stefano Babic wrote:
>
Hi Eric,

>> On 09/19/2011 08:57 AM, Marek Vasut wrote:
>>> On Monday, August 22, 2011 10:56:43 PM Eric Jarrige wrote:
>>>> Signed-off-by: Eric Jarrige <eric.jarrige@armadeus.org>
>>>> Cc: Stefano Babic <sbabic@denx.de>
>>>> Cc: Heiko Schocher <hs@denx.de>
>>>> @@ -94,6 +98,8 @@ void i2c_init(int speed, int unused)
>>>> 	/* start the required I2C clock */
>>>> 	writel(readl(&sc_regs->cgr0) | (3 << I2C_CLK_OFFSET),
>>>> 		&sc_regs->cgr0);
>>>> +#elif defined(CONFIG_IMX)
>>>> +	freq = get_HCLK();
>>>> #else
>>>> 	freq = mxc_get_clock(MXC_IPG_PERCLK);
>>>> #endif
>>> Please, no more ifdefs -- can't you actually modify your header files or add 
>>> some which would define mxc_get_clock() ? That'd make this way much more clean.
>> In principl, I agree completely with Marek, and we have tried to get rid
>> as much of possible of #ifdef in drivers. I know we have already
>> discussed about the opportunity to fix the whole IMX stuff, and I agreed
>> with you that this processor is obsolete and make no sense to invest a
>> lot of time for it. However, what about to define mxc_get_clock() as
>> simple macro ? Maybe in imx-regs.h, as exceptiion for this processor
>> (normally the imx-regs.h contains only defines and structures for the
>> internal registers) ? Something like:
>>
>> 	#define mxc_get_clock(a)	(get_HCLK())
> I agree with you to get rid of all these #ifdef and my current patch does not help at all.
> I can update the imx-regs.h with the change you propose but that won't help for the
> future iMX CPU.
> IMHO the problem comes from all the hard coded constants in the driver such as
> mxc_get_clock(MXC_IPG_PERCLK)
...I see the clock name is not so generic, and it is not related to the
interface..

>   and others I2C_BASE...
> Every chipset use a different root clock for its i2c controller.
>
> Why not changing the line
> 	freq = mxc_get_clock(MXC_IPG_PERCLK);
>
> by something more generic such as  a macro:
>
> 	freq = MXC_GET_I2C_CLOCK(); /* or GET_MXC_I2C_CLOCK() */

We have already this mechanism - this is the goal of the mxc_get_clock()
function: a common way to get clocks for all i.MX processors.
We have for example MXC_CSPI_CLK (SPI), MXC_UART_CLK, and so on. The
line should become:

freq = mxc_get_clock(MXC_I2C_CLK)

and then each processor can return its own value. This already works for the other interfaces (FEC, UART, SPI,..)



>
> that can be defined in clock.h with the correct definition for each chipset?
We need only to extend the mxc_clock enumeration type. The
implementation of mxc_get_clock() is SOC specific.
> Using a macro would clarify that is chipset dependent as each CPU use
> its own clock.h
>
> For the mx1, I can add a clock.h with the correct implementation of the
> macro:
> #define MXC_GET_I2C_CLOCK() get_HCLK()
> or any other solution that avoid any new ifdef.
>
> Does it makes sense?

Yes, it makes sense to change, check my proposal 

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-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH] mx1: add mx1/l support for mxc_i2c

[Eric Jarrige]

Hi Stafano,

On 19 sept. 2011, at 22:59, stefano babic wrote:
> Am 19/09/2011 22:34, schrieb Eric Jarrige:
>> On 19 sept. 2011, at 09:26, Stefano Babic wrote:
>>> On 09/19/2011 08:57 AM, Marek Vasut wrote:
>>>> On Monday, August 22, 2011 10:56:43 PM Eric Jarrige wrote:
>>> 	#define mxc_get_clock(a)	(get_HCLK())
>> I agree with you to get rid of all these #ifdef and my current patch does not help at all.
>> I can update the imx-regs.h with the change you propose but that won't help for the
>> future iMX CPU.
>> IMHO the problem comes from all the hard coded constants in the driver such as
>> mxc_get_clock(MXC_IPG_PERCLK)
> ...I see the clock name is not so generic, and it is not related to the
> interface..

Right, IMHO that's the point.

> 
> We have already this mechanism - this is the goal of the mxc_get_clock()
> function: a common way to get clocks for all i.MX processors.
> We have for example MXC_CSPI_CLK (SPI), MXC_UART_CLK, and so on. The
> line should become:
> 
> freq = mxc_get_clock(MXC_I2C_CLK)
> 
> and then each processor can return its own value. This already works for the other interfaces (FEC, UART, SPI,..)
> 
>> 
>> that can be defined in clock.h with the correct definition for each chipset?
> We need only to extend the mxc_clock enumeration type. The
> implementation of mxc_get_clock() is SOC specific.
>> Using a macro would clarify that is chipset dependent as each CPU use
>> its own clock.h
>> 
>> For the mx1, I can add a clock.h with the correct implementation of the
>> macro:
>> #define MXC_GET_I2C_CLOCK() get_HCLK()
>> or any other solution that avoid any new ifdef.
>> 
>> Does it makes sense?
> 
> Yes, it makes sense to change, check my proposal 

Your proposal looks good and should be easy to implement for all new iMX CPU.

What is the main added values of the unique method "mxc_get_clock" compared
to the other model? 
On PowerPC the frequencies are saved in the gd-> structure whereas the iMX legacy
principle used one accessor method for each clock but that lacks of a common
naming convention.
I can imagine that using the gd-> structure optimize the memory footprint on PowerPC.

What is your strategy regarding the legacy SOCs? 
Do you have a refactoring plan to update the other CPU to the new interface
mxc_get_clock() ? 

A wrapping "#define mxc_get_clock(a)	(get_HCLK())" in clock.h should do the job for
the mx1 but is it a common rule or a kind of exception for the mx1?

Sorry still few questions regarding some remaining open points?
What are your plan/ideas to remove the list of #ifdef that provides the i2C_BASE
and CLOCK_OFFSET for each SOC?

Same question for the hard coded table of clock dividers  "u16 div[]" as this table may
change from CPU to CPU ?

Thanks and best regards,
Eric

 

[U-Boot] [PATCH] mx1: add mx1/l support for mxc_i2c

[Stefano Babic]

On 09/20/2011 10:54 PM, Eric Jarrige wrote:

> Your proposal looks good and should be easy to implement for all new iMX CPU.
> 
> What is the main added values of the unique method "mxc_get_clock" compared
> to the other model? 
> On PowerPC the frequencies are saved in the gd-> structure

Well, both methods are valid. I presume that on the ARM architecture
(gd_t is the same for all ARM processors) this brings to a larger
structure, even if we can compile only the parts  related to the chosen
processor, and could be difficult to maintain. We have to fill all
fields at the startup, and this is the same as calling mxc_get_clock()
with all possible values. With the accessor, the clock is getted only
when it is strictly required.

> whereas the iMX legacy
> principle used one accessor method for each clock but that lacks of a common
> naming convention.

This is not exactly true. There is a common naming convention regarding
the interfaces: if you need the USB clock, you call the accessor with
MXC_USB_CLK, even if the real clock for a specific processor has another
name (PERCLK or whatever). Additionally, there could be clocks related
to a single processor, that can be retrieved with the same mechanism,
and they can have a specific name. And PERCLK is really a common name
for MX3/MX5 processors, but using it directly in the I2C driver instead
of a I2C-related name is not genaral and wrong.

> I can imagine that using the gd-> structure optimize the memory footprint on PowerPC.

Yes if the clock must not be calculated. I mean, it is different if the
clock should be retrieved by PLL values that can be different for each
board - in this case, we need always the code to compute the desired
clock. See examples in the MX3/MX5 cpu files.

> What is your strategy regarding the legacy SOCs? 

I have no strategy...if you mean the old MC9328 and generally what we
see under arch/arm/cpu/arm920t/imx. As we discussed before, these
processors are obsolete and makes no real sense to invest a lot of time
for them. At the moment, there is only one board (the mx1ads, and one
was removed) using this processor in u-boot. Your board will be the
second one. Outside your post, there was no patches for this processor
in the last years.

> Do you have a refactoring plan to update the other CPU to the new interface
> mxc_get_clock() ?

New CPUs ? New i.MX SOC should have this interface - see recent patches
for the MX28.

> 
> A wrapping "#define mxc_get_clock(a)	(get_HCLK())" in clock.h should do the job for
> the mx1 but is it a common rule or a kind of exception for the mx1?

This is an exception. As you can see in code, the other processors
provide the common accessor.
This is only because the mx1 is quite old and there is not much
development around it. The macro guarantees to have the same API

But if there will be a need to extend this mechanism for this driver,
also the mx1 should be adapted.

> 
> Sorry still few questions regarding some remaining open points?
> What are your plan/ideas to remove the list of #ifdef that provides the i2C_BASE
> and CLOCK_OFFSET for each SOC?

At the moment the driver supports only one interface, decided at compile
time. There are case where multiple interface are required and then this
issue must be also fixed.

> 
> Same question for the hard coded table of clock dividers  "u16 div[]" as this table may
> change from CPU to CPU ?

There are pending patches by Marek regarding this driver - some tests
are still required, then they will go to mainline.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================