Hi Heiko,
On 17 July 2016 at 09:33, Heiko Stübner <he...@sntech.de> wrote:
> Am Sonntag, 17. Juli 2016, 08:13:58 schrieb Simon Glass:
>> Hi Heiko,
>>
>> On 15 July 2016 at 16:17, Heiko Stuebner <he...@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 <he...@sntech.de>
>> > ---
>> >
>> > arch/arm/include/asm/arch-rockchip/cru_rk3188.h | 186 ++++++++++
>> > drivers/clk/Makefile | 1 +
>> > drivers/clk/clk_rk3188.c | 464
>> > ++++++++++++++++++++++++ 3 files changed, 651 insertions(+)
>> > create mode 100644 arch/arm/include/asm/arch-rockchip/cru_rk3188.h
>> > create mode 100644 drivers/clk/clk_rk3188.c
>>
>> Could you add a patch to move these files into a drivers/clk/rockchip
>> directory?
>
> ok
>
>> > new file mode 100644
>> > index 0000000..4c28393
>> > --- /dev/null
>> > +++ b/drivers/clk/clk_rk3188.c
>> > @@ -0,0 +1,465 @@
>> > +/*
>> > + * (C) Copyright 2015 Google, Inc
>> > + * (C) Copyright 2016 Heiko Stuebner <he...@sntech.de>
>> > + *
>> > + * SPDX-License-Identifier: GPL-2.0
>> > + */
>> > +
>> > +#include <common.h>
>> > +#include <clk-uclass.h>
>> > +#include <dm.h>
>> > +#include <errno.h>
>> > +#include <syscon.h>
>> > +#include <asm/io.h>
>> > +#include <asm/arch/clock.h>
>> > +#include <asm/arch/cru_rk3188.h>
>> > +#include <asm/arch/grf_rk3188.h>
>> > +#include <asm/arch/hardware.h>
>> > +#include <dt-bindings/clock/rk3188-cru.h>
>> > +#include <dm/device-internal.h>
>> > +#include <dm/lists.h>
>> > +#include <dm/uclass-internal.h>
>> > +
>> > +DECLARE_GLOBAL_DATA_PTR;
>> > +
>> > +struct rk3188_clk_priv {
>> > + struct rk3188_grf *grf;
>> > + struct rk3188_cru *cru;
>> > + ulong rate;
>> > + bool has_bwadj;
>> > +};
>> > +
>> > +struct pll_div {
>> > + u32 nr;
>> > + u32 nf;
>> > + u32 no;
>> > +};
>> > +
>> > +enum {
>> > + VCO_MAX_HZ = 2200U * 1000000,
>> > + VCO_MIN_HZ = 440 * 1000000,
>> > + OUTPUT_MAX_HZ = 2200U * 1000000,
>> > + OUTPUT_MIN_HZ = 30 * 1000000,
>> > + FREF_MAX_HZ = 2200U * 1000000,
>> > + FREF_MIN_HZ = 30 * 1000,
>> > +};
>> > +
>> > +enum {
>> > + /* PLL CON0 */
>> > + PLL_OD_MASK = 0x0f,
>> > +
>> > + /* PLL CON1 */
>> > + PLL_NF_MASK = 0x1fff,
>> > +
>> > + /* PLL CON2 */
>> > + PLL_BWADJ_MASK = 0x0fff,
>> > +
>> > + /* PLL CON3 */
>> > + PLL_RESET_SHIFT = 5,
>> > +
>> > + /* GRF_SOC_STATUS0 */
>> > + SOCSTS_DPLL_LOCK = 1 << 5,
>> > + SOCSTS_APLL_LOCK = 1 << 6,
>> > + SOCSTS_CPLL_LOCK = 1 << 7,
>> > + SOCSTS_GPLL_LOCK = 1 << 8,
>> > +};
>> > +
>> > +#define RATE_TO_DIV(input_rate, output_rate) \
>> > + ((input_rate) / (output_rate) - 1);
>> > +
>> > +#define DIV_TO_RATE(input_rate, div) ((input_rate) / ((div) + 1))
>> > +
>> > +#define PLL_DIVISORS(hz, _nr, _no) {\
>> > + .nr = _nr, .nf = (u32)((u64)hz * _nr * _no / OSC_HZ), .no = _no};\
>> > + _Static_assert(((u64)hz * _nr * _no / OSC_HZ) * OSC_HZ /\
>> > + (_nr * _no) == hz, #hz "Hz cannot be hit with PLL
>> > "\
>> > + "divisors on line " __stringify(__LINE__));
>> > +
>> > +/* Keep divisors as low as possible to reduce jitter and power usage */
>> > +static const struct pll_div apll_init_cfg = PLL_DIVISORS(APLL_HZ, 1, 1);
>> > +static const struct pll_div gpll_init_cfg = PLL_DIVISORS(GPLL_HZ, 2, 2);
>> > +static const struct pll_div cpll_init_cfg = PLL_DIVISORS(CPLL_HZ, 1, 2);
>> > +
>> > +void *rockchip_get_cru(void)
>> > +{
>> > + struct rk3188_clk_priv *priv;
>> > + struct udevice *dev;
>> > + int ret;
>> > +
>> > + ret = uclass_get_device_by_name(UCLASS_CLK,
>> > + "clock-controller@20000000",
>> > &dev);
>>
>> This seems odd. Could you use uclass_get_device(UCLASS_CLK, 0, .&dev) ?
>
> Index 0 actually gets me the 24MHz oscillator fixed clock :-), which is why I
> switched to the by-name variant to not depend on some dts or uclass ordering.
> I'm wondering how that works on the other socs.
I suspect this might have become broken by Stephen's clock changes. I
had a bit of a look at this but have not resolved it yet. For example
on firefly, HDMI does not work now.
There is this:
enum rk_clk_id {
CLK_OSC,
CLK_ARM,
CLK_DDR,
CLK_CODEC,
CLK_GENERAL,
CLK_NEW,
CLK_COUNT,
};
and it used to check against the platform data (see rkclk_get_clk() in
v2016.05). I'll have a think about it.
>
>>
>> > + if (ret)
>> > + return ERR_PTR(ret);
>> > +
>> > + priv = dev_get_priv(dev);
>> > +
>> > + return priv->cru;
>> > +}
>> > +
>> > +static int rkclk_set_pll(struct rk3188_cru *cru, enum rk_clk_id clk_id,
>> > + const struct pll_div *div, bool has_bwadj)
>> > +{
>> > + int pll_id = rk_pll_id(clk_id);
>> > + struct rk3188_pll *pll = &cru->pll[pll_id];
>> > + /* All PLLs have same VCO and output frequency range restrictions.
>> > */ + uint vco_hz = OSC_HZ / 1000 * div->nf / div->nr * 1000;
>> > + uint output_hz = vco_hz / div->no;
>> > +
>> > + debug("PLL at %x: nf=%d, nr=%d, no=%d, vco=%u Hz, output=%u Hz\n",
>> > + (uint)pll, div->nf, div->nr, div->no, vco_hz, output_hz);
>> > + assert(vco_hz >= VCO_MIN_HZ && vco_hz <= VCO_MAX_HZ &&
>> > + output_hz >= OUTPUT_MIN_HZ && output_hz <= OUTPUT_MAX_HZ &&
>> > + (div->no == 1 || !(div->no % 2)));
>> > +
>> > + /* enter reset */
>> > + rk_setreg(&pll->con3, 1 << PLL_RESET_SHIFT);
>> > +
>> > + rk_clrsetreg(&pll->con0,
>> > + CLKR_MASK << CLKR_SHIFT | PLL_OD_MASK,
>> > + ((div->nr - 1) << CLKR_SHIFT) | (div->no - 1));
>> > + rk_clrsetreg(&pll->con1, CLKF_MASK, div->nf - 1);
>> > +
>> > + if (has_bwadj)
>> > + rk_clrsetreg(&pll->con2, PLL_BWADJ_MASK, (div->nf >> 1) -
>> > 1); +
>> > + udelay(10);
>> > +
>> > + /* return from reset */
>> > + rk_clrreg(&pll->con3, 1 << PLL_RESET_SHIFT);
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static inline unsigned int log2(unsigned int value)
>>
>> Hmm this should go in a common file. Perhaps bitfield.h or common.h?
>
> it looks like uboot is already carrying ilog2() in include/linux/log2.h ?
Yes.
>
> [...]
>
>> > +static int rk3188_clk_probe(struct udevice *dev)
>> > +{
>> > + struct rk3188_clk_priv *priv = dev_get_priv(dev);
>> > +
>> > + priv->cru = (struct rk3188_cru *)dev_get_addr(dev);
>> > + priv->grf = syscon_get_first_range(ROCKCHIP_SYSCON_GRF);
>> > + priv->has_bwadj = of_device_is_compatible(dev,
>> > "rockchip,rk3188a-cru") + ? 1 : 0;
>>
>> You should add a .data member to your udevice_id array below using a
>> two-member enum, and check dev_get_driver_data() here.
>
> ah, nice to know. I was wondering if and how uboot was handling .data stuff
> but my short skimming through the sources didn't reveal that. Will change.
I'm AFK for a few hours now.
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
