[U-Boot] [PATCH v3 10/16] rockchip: rk3188: Add clock driver

[Simon Glass]

On 21 February 2017 at 11:07, Simon Glass <sjg at chromium.org> wrote:
> Hi Heiko,
>
> On 18 February 2017 at 07:19, Heiko Stuebner <heiko at sntech.de> wrote:
>> Hi Simon,
>>
>> Am Montag, 6. Februar 2017, 07:35:12 CET schrieb Simon Glass:
>>> On 3 February 2017 at 08:09, Heiko Stuebner <heiko at sntech.de> wrote:
>>> > Add a driver for setting up and modifying the various PLLs and peripheral
>>> > clocks on the RK3188.
>>> >
>>> > Signed-off-by: Heiko Stuebner <heiko at sntech.de>
>>> > ---
>>> >
>>> >  arch/arm/include/asm/arch-rockchip/cru_rk3188.h | 191 +++++++++
>>> >  drivers/clk/rockchip/Makefile                   |   1 +
>>> >  drivers/clk/rockchip/clk_rk3188.c               | 523
>>> >  ++++++++++++++++++++++++ 3 files changed, 715 insertions(+)
>>> >  create mode 100644 arch/arm/include/asm/arch-rockchip/cru_rk3188.h
>>> >  create mode 100644 drivers/clk/rockchip/clk_rk3188.c
>>>
>>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>>
>>> With one comment below.
>>>
>>> > diff --git a/arch/arm/include/asm/arch-rockchip/cru_rk3188.h
>>> > b/arch/arm/include/asm/arch-rockchip/cru_rk3188.h new file mode 100644
>>> > index 0000000000..74f0fedcc6
>>> > --- /dev/null
>>> > +++ b/arch/arm/include/asm/arch-rockchip/cru_rk3188.h
>>> > @@ -0,0 +1,191 @@
>>> > +/*
>>> > + * (C) Copyright 2016 Heiko Stuebner <heiko at sntech.de>
>>> > + *
>>> > + * SPDX-License-Identifier:     GPL-2.0+
>>> > + */
>>> > +#ifndef _ASM_ARCH_CRU_RK3188_H
>>> > +#define _ASM_ARCH_CRU_RK3188_H
>>> > +
>>> > +#define OSC_HZ         (24 * 1000 * 1000)
>>> > +
>>> > +#define APLL_HZ                (1608 * 1000000)
>>> > +#define GPLL_HZ                (594 * 1000000)
>>> > +#define CPLL_HZ                (384 * 1000000)
>>> > +
>>> > +/* The SRAM is clocked off aclk_cpu, so we want to max it out for boot
>>> > speed */ +#define CPU_ACLK_HZ    297000000
>>> > +#define CPU_HCLK_HZ    148500000
>>> > +#define CPU_PCLK_HZ    74250000
>>> > +#define CPU_H2P_HZ     74250000
>>> > +
>>> > +#define PERI_ACLK_HZ   148500000
>>> > +#define PERI_HCLK_HZ   148500000
>>> > +#define PERI_PCLK_HZ   74250000
>>> > +
>>> > +/* Private data for the clock driver - used by rockchip_get_cru() */
>>> > +struct rk3188_clk_priv {
>>> > +       struct rk3188_grf *grf;
>>> > +       struct rk3188_cru *cru;
>>> > +       ulong rate;
>>> > +       bool has_bwadj;
>>> > +};
>>> > +
>>> > +struct rk3188_cru {
>>> > +       struct rk3188_pll {
>>> > +               u32 con0;
>>> > +               u32 con1;
>>> > +               u32 con2;
>>> > +               u32 con3;
>>> > +       } pll[4];
>>> > +       u32 cru_mode_con;
>>> > +       u32 cru_clksel_con[35];
>>> > +       u32 cru_clkgate_con[10];
>>> > +       u32 reserved1[2];
>>> > +       u32 cru_glb_srst_fst_value;
>>> > +       u32 cru_glb_srst_snd_value;
>>> > +       u32 reserved2[2];
>>> > +       u32 cru_softrst_con[9];
>>> > +       u32 cru_misc_con;
>>> > +       u32 reserved3[2];
>>> > +       u32 cru_glb_cnt_th;
>>> > +};
>>> > +check_member(rk3188_cru, cru_glb_cnt_th, 0x0140);
>>> > +
>>> > +/* CRU_CLKSEL0_CON */
>>> > +enum {
>>> > +       /* a9_core_div: core = core_src / (a9_core_div + 1) */
>>> > +       A9_CORE_DIV_SHIFT       = 9,
>>> > +       A9_CORE_DIV_MASK        = 0x1f,
>>>
>>> Can you define
>>>
>>> A9_CORE_DIV_MASK = 0x1f << A9_CORE_DIV_SHIFT
>>>
>>> and similarly for other masks. I got this wrong with rk3288, and I
>>> think shifting the mask makes the code easier in places.
>>
>> I'd disagree here. We're using this scheme everywhere on Rockchip platforms.
>> For example please look at all the pinmux defines
>>         GPIO3C1_SHIFT           = 2,
>>         GPIO3C1_MASK            = 3,
>>         GPIO3C1_GPIO            = 0,
>>         GPIO3C1_SDMMC1_DATA0,
>>         GPIO3C1_RMII_TXD1,
>>         GPIO3C1_RESERVED,
>>
>> and numerous other places and I'd think mixing paradigms in one soc and
>> between all Rockchip socs would be somewhat unwise.
>
> The new ones are going to use this approach and I will get around to
> converting them at some point. I do think I did it wrong in the first
> place.
>
> Anyway let's not hold this up for this as we have the same issue with rk3399.
>
> Regards,
> Simon

Applied to u-boot-rockchip, thanks!