[U-Boot] [PATCH v2] socfpga: Adding Clock Manager driver

[Pavel Machek]

Hi!

> Clock Manager driver will be called to reconfigure all the
> clocks setting based on user input. The input are passed to
> Preloader through handoff files
> 
> Signed-off-by: Chin Liang See <clsee at altera.com>
> Cc: Albert Aribaud <albert.u.boot at aribaud.net>
> Cc: Tom Rini <trini at ti.com>
> Cc: Wolfgang Denk <wd at denx.de>
> CC: Pavel Machek <pavel at denx.de>
> Cc: Dinh Nguyen <dinguyen at altera.com>
> ---
> Changes for v2
> - merge the handoff file and driver into single patch

> +#include <common.h>
> +#include <asm/io.h>
> +#include <asm/arch/clock_manager.h>
> +
> +static const struct socfpga_clock_manager *clock_manager_base =
> +		(void *)SOCFPGA_CLKMGR_ADDRESS;
> +
> +#define CLKMGR_BYPASS_ENUM_ENABLE	1
> +#define CLKMGR_BYPASS_ENUM_DISABLE	0
> +#define CLKMGR_STAT_BUSY_ENUM_IDLE	0x0
> +#define CLKMGR_STAT_BUSY_ENUM_BUSY	0x1
> +#define CLKMGR_BYPASS_PERPLLSRC_ENUM_SELECT_EOSC1	0x0
> +#define CLKMGR_BYPASS_PERPLLSRC_ENUM_SELECT_INPUT_MUX	0x1
> +#define CLKMGR_BYPASS_SDRPLLSRC_ENUM_SELECT_EOSC1	0x0
> +#define CLKMGR_BYPASS_SDRPLLSRC_ENUM_SELECT_INPUT_MUX	0x1

This is not too consistent. I guess dropping the 0x here would
help. And maybe CLKMGR_STAT_IDLE and CLKMGR_STAT_BUSY would be better
define names? Dropping _ENUM would help, too.

> +static inline void cm_wait_for_lock(uint32_t mask)
> +{
> +	register uint32_t inter_val;
> +	do {
> +		inter_val = readl(&clock_manager_base->inter) & mask;
> +	} while (inter_val != mask);
> +}
> +
> +/* function to poll in the fsm busy bit */
> +static inline void cm_wait4fsm(void)
> +{
> +	register uint32_t inter_val;
> +	do {
> +		inter_val = readl(&clock_manager_base->stat) & CLKMGR_STAT_BUSY_ENUM_BUSY;
> +	} while (inter_val);
> +}

wait4fsm vs. wait_for_lock. Pick one style...

And actually ... maybe 

    while (readl(&clock_manager_base->stat) & CLKMGR_STAT_BUSY_ENUM_BUSY)
	;

is easier to read? No need for variable...

> +/* function to write a clock register that has phase information */
> +static inline void cm_write_with_phase(uint32_t value,
> +	uint32_t reg_address, uint32_t mask)
> +{
> +	/* poll until phase is zero */
> +	do {} while (readl(reg_address) & mask);
> +
> +	writel(value, reg_address);
> +
> +	do {} while (readl(reg_address) & mask);
> +}

drop do {} .

> +/*
> + * Setup clocks while making no assumptions of the
> + * previous state of the clocks.

...no assumptions about...?

> + * Start by being paranoid and gate all sw managed clocks
> + *
> + * Put all plls in bypass
> + *
> + * Put all plls VCO registers back to reset value (bgpwr dwn).
> + *
> + * Put peripheral and main pll src to reset value to avoid glitch.
> + *
> + * Delay 5 us.
> + *
> + * Deassert bg pwr dn and set numerator and denominator
> + *
> + * Start 7 us timer.
> + *
> + * set internal dividers
> + *
> + * Wait for 7 us timer.
> + *
> + * Enable plls
> + *
> + * Set external dividers while plls are locking
> + *
> + * Wait for pll lock
> + *
> + * Assert/deassert outreset all.
> + *
> + * Take all pll's out of bypass
> + *
> + * Clear safe mode
> + *
> + * set source main and peripheral clocks
> + *
> + * Ungate clocks
> + */

Drop empty lines, add "." to end sentences, and spell out "bg pwr dn"?

> +void cm_basic_init(const cm_config_t *cfg)
> +{

Split to smaller functions? Then you will not need the summary
comment...

> +	/* 7 us must have elapsed before we can enable the VCO */
> +	for ( ; get_timer(start) < timeout ; )
> +		;

while() ?

> --- a/arch/arm/cpu/armv7/socfpga/spl.c
> +++ b/arch/arm/cpu/armv7/socfpga/spl.c
> @@ -28,10 +28,100 @@ u32 spl_boot_device(void)
>  void spl_board_init(void)
>  {
>  #ifndef CONFIG_SOCFPGA_VIRTUAL_TARGET
> +
> +	cm_config_t cm_default_cfg = {
> +		/* main group */
> +		MAIN_VCO_BASE,

This will generate pretty bad code, no? Should it be static?

> +		CLKMGR_MAINPLLGRP_MPUCLK_CNT_SET(
> +			CONFIG_HPS_MAINPLLGRP_MPUCLK_CNT),
> +		CLKMGR_MAINPLLGRP_MAINCLK_CNT_SET(
> +			CONFIG_HPS_MAINPLLGRP_MAINCLK_CNT),
> +		CLKMGR_MAINPLLGRP_DBGATCLK_CNT_SET(
> +			CONFIG_HPS_MAINPLLGRP_DBGATCLK_CNT),
> +		CLKMGR_MAINPLLGRP_MAINQSPICLK_CNT_SET(
> +			CONFIG_HPS_MAINPLLGRP_MAINQSPICLK_CNT),
> +		CLKMGR_PERPLLGRP_PERNANDSDMMCCLK_CNT_SET(
> +			CONFIG_HPS_MAINPLLGRP_MAINNANDSDMMCCLK_CNT),

It would be good to somehow shorted identifiers so this fits to one
line.

> +typedef struct {
...
> +	uint32_t sdram_vco_base;
> +	uint32_t ddrdqsclk;
> +	uint32_t ddr2xdqsclk;
> +	uint32_t ddrdqclk;
> +	uint32_t s2fuser2clk;
> +} cm_config_t;

typedefs for structs are usually not welcome...

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html