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

[Chin Liang See]

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 <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.


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
>