[U-Boot] [PATCH 3/7] clk: rockchip: add support for rk3328

[Simon Glass]

+Tom

Hi Kever,

On 22 February 2017 at 02:45, Kever Yang <kever.yang at rock-chips.com> wrote:
>
> Hi Simon,
>
>
> On 02/22/2017 02:06 AM, Simon Glass wrote:
>>
>> Hi Kever,
>>
>> On 17 February 2017 at 01:07, Kever Yang <kever.yang at rock-chips.com> wrote:
>>>
>>> Add rk3328 clock driver and cru structure definition.
>>>
>>> Signed-off-by: William Zhang <william.zhang at rock-chips.com>
>>> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
>>> ---
>>>
>>>   arch/arm/include/asm/arch-rockchip/cru_rk3328.h |  65 +++
>>>   drivers/clk/rockchip/Makefile                   |   1 +
>>>   drivers/clk/rockchip/clk_rk3328.c               | 607 ++++++++++++++++++++++++
>>>   3 files changed, 673 insertions(+)
>>>   create mode 100644 arch/arm/include/asm/arch-rockchip/cru_rk3328.h
>>>   create mode 100644 drivers/clk/rockchip/clk_rk3328.c
>>>

>> [..]
>>
>>> +
>>> +#define I2C_CLK_REG_MASK(bus) \
>>> +                       (CLK_I2C_DIV_CON_MASK << \
>>> +                       CLK_I2C ##bus## _DIV_CON_SHIFT | \
>>> +                       CLK_I2C_PLL_SEL_MASK << \
>>> +                       CLK_I2C ##bus## _PLL_SEL_SHIFT)
>>> +
>>> +#define (bus, clk_div) \
>>> +                        ((clk_div - 1) << \
>>> +                        CLK_I2C ##bus## _DIV_CON_SHIFT | \
>>> +                        CLK_I2C_PLL_SEL_GPLL << \
>>> +                        CLK_I2C ##bus## _PLL_SEL_SHIFT)
>>> +
>>> +#define I2C_CLK_DIV_VALUE(con, bus) \
>>> +                        (con >> CLK_I2C ##bus## _DIV_CON_SHIFT) & \
>>> +                        CLK_I2C_DIV_CON_MASK;
>>
>> Can we drop these three and instead write them out below?
>
>
> I don't know why you don't like this kind of MACRO, like the size_mb in sdram driver,
> we though this help people understand the C source and make the C source cold looks much clean,
> in some platform, maintainer may ask for this when there are multi controller and can reuse
> the same MACRO.
> Anyway, I will make this fallback to normal shift/mask style in next version.
>

Let me try to explain this.

In this code macros are created by pasting symbols together which
means (for example) that it is not possible to find the definition of
CLK_I2C 0_DIV_CON_SHIFT by searching the code. This can be very
confusing for people trying to figure out what is going on. It is bad
enough in a single file but gets worse when things migrate to header
files.

If you want to have a macro here, how about

#define CLK_I2C DIV_CON_SHIFT(bus)

instead? This can return the appropriate shift for the bus. The
'I2C_CLK_DIV_VALUE' macro is only used in rk3328_i2c_get_clk() and
ends up creating repetitive code (all the macro logic happens in each
case of the switch() , which the compiler hopefully can optimize, but
who knows? In any case conceptually I do not think it simplifies the
code. You end up unpicking the macros and printing out intermediate
values, etc. I have definitely done my share of that.

The rule of using shifts and masks in the code rather than hiding them
originally came from Wolfgang and I have got used to it. One thing I
have noticed is that with macros you can build up a tree of
interdependent macros such that it is very hard to figure out what is
actually going on, e.g. if you are looking for a bug. I am aware that
the code style in coreboot and UEFI are different, although I am not
an expert in either.

Does that make sense?

Regards,
Simon