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

[Heiko Stuebner]

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.

Heiko