Hi Pavel, > > 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 <cl...@altera.com> >> Cc: Albert Aribaud <albert.u.b...@aribaud.net> >> Cc: Tom Rini <tr...@ti.com> >> Cc: Wolfgang Denk <w...@denx.de> >> CC: Pavel Machek <pa...@denx.de> >> Cc: Dinh Nguyen <dingu...@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.
Fixed > >> +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... Fixed by using _for_ > > And actually ... maybe > > while (readl(&clock_manager_base->stat) & CLKMGR_STAT_BUSY_ENUM_BUSY) > ; > > is easier to read? No need for variable... Yup, fixed > >> +/* 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 {} . > Fixed >> +/* >> + * 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"? Fixed > >> +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() ? Fixed > >> --- 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? Its already a local variable. > >> + 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. Yup, I tried but the variable contain few info there such as PLL type, which portion of PLL and functionality. > >> +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... Actually the struct only used in clock configuration. By doing this, we won't need to have the malloc. Thanks Chin Liang > > Thanks, > Pavel > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) > http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot