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@altera.com Cc: Albert Aribaud albert.u.boot@aribaud.net Cc: Tom Rini trini@ti.com Cc: Wolfgang Denk wd@denx.de CC: Pavel Machek pavel@denx.de Cc: Dinh Nguyen dinguyen@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