[U-Boot] [PATCH 1/7] rockchip: rk3368: Add clok drvier

[Simon Glass]

Hi Andy,

On 4 May 2017 at 02:48, Andy Yan <andy.yan at rock-chips.com> wrote:
> Hi Simon:
>
>
> On 2017年04月29日 08:26, Simon Glass wrote:
>>
>> Hi Andy,
>>
>> On 20 April 2017 at 20:31, Andy Yan <andy.yan at rock-chips.com> wrote:
>>>
>>> Add driver to setup the various PLLs and peripheral
>>> clocks on the RK3368.
>>
>> Subject: clock
>
>
>     Sorry, it's my fault.
>
>>
>>> Signed-off-by: Andy Yan <andy.yan at rock-chips.com>
>>> ---
>>>
>>>   arch/arm/include/asm/arch-rockchip/cru_rk3368.h | 110 +++++++++
>>>   drivers/clk/rockchip/Makefile                   |   1 +
>>>   drivers/clk/rockchip/clk_rk3368.c               | 296
>>> ++++++++++++++++++++++++
>>>   3 files changed, 407 insertions(+)
>>>   create mode 100644 arch/arm/include/asm/arch-rockchip/cru_rk3368.h
>>>   create mode 100644 drivers/clk/rockchip/clk_rk3368.c
>>>
>>> diff --git a/arch/arm/include/asm/arch-rockchip/cru_rk3368.h
>>> b/arch/arm/include/asm/arch-rockchip/cru_rk3368.h
>>> new file mode 100644
>>> index 0000000..122c8be
>>> --- /dev/null
>>> +++ b/arch/arm/include/asm/arch-rockchip/cru_rk3368.h
>>> @@ -0,0 +1,110 @@
>>> +/*
>>> + * (C) Copyright 2016 Rockchip Electronics Co., Ltd
>>> + * Author: Andy Yan <andy.yan at rock-chips.com>
>>> + * SPDX-License-Identifier:     GPL-2.0+
>>> + */
>>> +#ifndef _ASM_ARCH_CRU_RK3368_H
>>> +#define _ASM_ARCH_CRU_RK3368_H
>>> +
>>> +#include <common.h>
>>> +
>>> +#define CRU_BASE       0xff760000
>>
>> This should come from device tree.
>
>
>     This will be removed in next version.
>
>>
>>> +
>>> +/* RK3368 clock numbers */
>>> +enum rk3368_pll_id {
>>> +       APLLB,
>>> +       APLLL,
>>> +       DPLL,
>>> +       CPLL,
>>> +       GPLL,
>>> +       NPLL,
>>> +       PLL_COUNT,
>>> +};
>>> +
>>> +struct rk3368_cru {
>>> +       struct rk3368_pll {
>>> +               unsigned int con0;
>>> +               unsigned int con1;
>>> +               unsigned int con2;
>>> +               unsigned int con3;
>>> +       } pll[6];
>>> +       unsigned int reserved[0x28];
>>> +       unsigned int clksel_con[56];
>>> +       unsigned int reserved1[8];
>>> +       unsigned int clkgate_con[25];
>>> +       unsigned int reserved2[7];
>>> +       unsigned int glb_srst_fst_val;
>>> +       unsigned int glb_srst_snd_val;
>>> +       unsigned int reserved3[0x1e];
>>> +       unsigned int softrst_con[15];
>>> +       unsigned int reserved4[0x11];
>>> +       unsigned int misc_con;
>>> +       unsigned int glb_cnt_th;
>>> +       unsigned int glb_rst_con;
>>> +       unsigned int glb_rst_st;
>>> +       unsigned int reserved5[0x1c];
>>> +       unsigned int sdmmc_con[2];
>>> +       unsigned int sdio0_con[2];
>>> +       unsigned int sdio1_con[2];
>>> +       unsigned int emmc_con[2];
>>> +};
>>> +check_member(rk3368_cru, emmc_con[1], 0x41c);
>>> +
>>> +struct rk3368_clk_priv {
>>> +       struct rk3368_cru *cru;
>>> +       ulong rate;
>>> +       bool has_bwadj;
>>> +};
>>> +
>>> +enum {
>>> +       /*PLL CON0*/
>>> +       PLL_NR_SHIFT            = 8,
>>> +       PLL_NR_MASK             = GENMASK(13, 8),
>>
>> Can you do GENMASK(13, 8) << PLL_NR_SHIFT
>>
>> for these? That means that the mask is shifted.
>
>
>     GENMASK(13, 8)= 0x3f00, GENMASK(13, 8) << PLL_NR_SHIFT will be 0x3f0000,
> this is not we wanted value.

Ah yes. Well you could do:

       PLL_NR_MASK             = GENMASK(13, PLL_NR_SHIFT),

but that doesn't make a lot of sense since the 13 should really be 5 +
PLL_NR_SHIFT.

So let's leave it as you have it. The two lines are right next to each
other so it should be obvious they are related.

Regards,
Simon