[U-Boot] [PATCH V2 2/2] imx: mx6 add i2c4 clock support for i.MX6SX

[Peng Fan]

Hi Stefano,

On Mon, Jun 29, 2015 at 09:13:01AM +0200, Stefano Babic wrote:
>Hi Peng,
>
>On 29/06/2015 03:33, Peng Fan wrote:
>> Hi Stefano,
>> 
>> On Sun, Jun 28, 2015 at 10:52:50AM +0200, Stefano Babic wrote:
>>> Hi Peng,
>>>
>>> On 25/06/2015 03:33, Peng Fan wrote:
>>>> Add I2C4 clock support for i.MX6SX. Since we use runtime check,
>>>> but not macro, we need to remove `#ifdef ..` in crm_regs.h, or
>>>> we will get compliation failure for other platforms.
>>>
>>>               ^----- compliation ?
>> I mean gcc will complains about failing to compile the code.
>
>Right. Fix the commit message.
>
>
>>> Agree to group together macros that belong only to a SOC variant, else
>>> it is difficult to find them. But what has
>>> MXC_CCM_CCGR6_VDOAXICLK_OFFSET to do with addings I2C4 clock to i.MX6SX
>>> ? It should be posted in a separate patch.
>> 
>> I just move this piece of code from the back to the front, see the end
>> of the patch.
>> I removed the #ifdef in header file, because it is really not a good idea
>> to see so many #ifdefs in header file. The bad point here is some CCM macros
>> are exposed to other platforms, so we should take care to use the CCM macros.
>
>That is correct. We have a lot of macro across all i.MXes, not only
>i.MX6es. This allow to have consistent code, and we must take care of
>not breaking other SOCs.
>
>> 
>> Take the following as example:
>> In crm_regs.h
>> #ifdef CONFIG_MX6SX
>> #define MXC_CCM_CCGR6_PWM7_MASK 	(3 << 30)
>> #else
>> #define MXC_CCM_CCGR6_VDOAXICLK_MASK	(3 << 12)
>> #endif
>> 
>> u32 test()
>> {
>> 	if (is_cpu_type(MXC_CPU_MX6SX))
>> 		return MXC_CCM_CCGR6_PWM7_MASK;		
>> 	else
>> 		return MXC_CCM_CCGR6_VDOAXICLK_OFFSET;
>> }
>
>Here there is a bad use of these kind of macro. To use a define, we
>should have the same name and not different names as in this case.
>
>> 
>> Here gcc can not successfully compile the code if we want to build for i.mx6sx,
>> becuase MXC_CCM_CCGR6_VDOAXICLK_OFFSET is not defined for i.mx6sx.
>
>Right,  but this is bad code and must be fixed.
>
>> 
>> So:
>> #define MXC_CCM_CCGR6_PWM7_MASK     (3 << 30)
>> /* This CCM Definition does not exist on i.MX6SX */
>> #define MXC_CCM_CCGR6_VDOAXICLK_MASK    (3 << 12)
>
>Better should be to add also a common name for both, that is then used
>in the function.

To those which have same offset and same bit width, we can have a common
name to cover for different SOCs. To those same offset, but different bit
width, we need the ifdefs.  Also some CCM definitions are not used by
U-Boot, maybe we can removed them, such as GPU and etc. Anyway, better not
to do big change to avoid breaking something, so when adding new definitions,
avoid adding ifdefs and doing small code refactor.

>
>Best regards,
>Stefano Babic
>
>-- 
>=====================================================================
>DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>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
>=====================================================================

Regards,
Peng
--