[U-Boot] [PATCH 2/6] i.mx: Add the initial support for freescale i.MX6Q processor

[Stefano Babic <sbabic@denx.de>]

On 11/14/2011 10:42 AM, Jason Hui wrote:

>>
>> All get_usdhcX function are identical, except for two masks
>> (MXC_CCM_CSCDR1_USDHCx_PODF_MASK and MXC_CCM_CSCMR1_USDHCx_CLK_SEL).
>> Merge them in a get_usdhc_clk(usdhc_number)
> 
> Yes, I also notice this when writing the code. At that time, I just
> think to make clear about the code.
> Now, I think your comments is also reasonable. I will change it later.

ok


>>> +#if defined(CONFIG_FEC_MXC)
>>> +void imx_get_mac_from_fuse(unsigned char *mac)
>>> +{
>>> +     struct iim_regs *iim = (struct iim_regs *)IMX_IIM_BASE;
>>> +     struct fuse_bank *bank = &iim->bank[4];
>>> +     struct fuse_bank4_regs *fuse =
>>> +                     (struct fuse_bank4_regs *)bank->fuse_regs;
>>> +
>>> +     u32 mac_lo = readl(&fuse->mac_addr_low);
>>> +     u32 mac_hi = readl(&fuse->mac_addr_high);
>>> +
>>> +     *(u32 *)mac = mac_lo;
>>> +
>>> +     mac[4] = mac_hi & 0xff;
>>> +     mac[5] = (mac_hi >> 8) & 0xff;
>>> +
>>> +}
>>
>> Even if the implementation is slightly different, this function (except
>> for the bank number) does the same things for MX5 and MX6 - you can als
>> put it into imx-common.
> 
> Stefano, there is big difference for the memory layout. On i.mx5, the layout is:
> 
> 32-bit 32-bit 32-bit 32-bit 32-bit 32-bit
> mac0 mac1 mac2 mac3 mac4 mac5
> 
>  but on i.mx6:
> 
> 32bit                                 32bit [low 16bit]
> mac[0-3]                          mac[4-5]
> 
> It's due to i.mx6q use OCOTP fuse-controller other than the fuse-box
> on the i.mx5.
> 
> Thus, I think, I will keep the code as it is.

Ok - I was not aware about it.

>>
>> Checking this there something confusing in actual code. There is struct
>> mxc_ccm_reg in crm_regs.h and struct clkctl in imx-regs.h. The two
> 
> I did not searched the clkctl in the patch-set by using:
> grep -nR clkctl *
> 
>> structure are identical. I am asking myself why...it seems to me that
>> someting went wrong.
>> If there is no evident reason, we should even clean up this point.
>>
>> And it is better you use one of the already supplied names (mxc_ccm_reg
>> or clkctl), without adding a new one.
> 
> Yes, I'm using mxc_ccm_reg.

Ok - this structure is also used in most drivers - if I am not wrong
(and I will better check), the struct clkctl is used only to generate
the offsets in asm-offsets.h. If it is so, it could be drop in a future
clean up patch...

>> Maybe do we find a way to add a common include directory ? This file is
>> duplicated. We can use include/asm/arch/imx-common
> 
> I don't find one good way to add a common include directory. If I try
> to find one,
> I will put this head file to imx-common.

The easy way is to add include/asm/arch-imx-common, and then the header
are explicitely included as #include <asm/arch-imx-common/..>.
I have seen only another example in U-Boot for armv7:

arch/arm/cpu/armv7/highbank/timer.c:#include <asm/arch-armv7/systimer.h>

I am opened to other solutions, too, but I think that introducing a
common repository for IMX include files (as Linux with plat_imx does) is
the way to do.

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