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: off...@denx.de
=====================================================================
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
