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