[U-Boot] [PATCH v3 01/19] arm: socfpga: Restructure clock manager driver

[Ley Foon Tan]

On Fri, Mar 31, 2017 at 4:53 AM, Dinh Nguyen <dinh.linux at gmail.com> wrote:
> On Thu, Mar 30, 2017 at 8:08 AM, Ley Foon Tan <ley.foon.tan at intel.com> wrote:
>> Restructure clock manager driver in the preparation to support A10.
>> Move the Gen5 specific code to _gen5 files. No functional change.
>>
>> Change all uint32_t to u32 and change to use macro BIT(n) for bit shift.
>>
>> Signed-off-by: Ley Foon Tan <ley.foon.tan at intel.com>
>> ---
>>  arch/arm/mach-socfpga/Makefile                     |   3 +-
>>  arch/arm/mach-socfpga/clock_manager.c              | 515 +--------------------
>>  .../{clock_manager.c => clock_manager_gen5.c}      | 104 ++---
>>  arch/arm/mach-socfpga/include/mach/clock_manager.h | 316 +------------
>>  .../mach/{clock_manager.h => clock_manager_gen5.h} | 151 +++---
>>  5 files changed, 125 insertions(+), 964 deletions(-)
>>  copy arch/arm/mach-socfpga/{clock_manager.c => clock_manager_gen5.c} (88%)
>>  copy arch/arm/mach-socfpga/include/mach/{clock_manager.h => clock_manager_gen5.h} (80%)
>>
>> diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile
>> index 809cd47..b76de4c 100644
>
> [snip]
>
>>
>>  /* function to write a clock register that has phase information */
>> -static void cm_write_with_phase(uint32_t value,
>> -                               uint32_t reg_address, uint32_t mask)
>> +static int cm_write_with_phase(u32 value, u32 reg_address, u32 mask)
>
> You changed the return type of this function here...
Will update commit message for more detail.
>
>>  {
>> +       int ret;
>> +
>>         /* poll until phase is zero */
>> -       while (readl(reg_address) & mask)
>> -               ;
>> +       ret = wait_for_bit(__func__, (const u32 *)reg_address, mask,
>> +                          false, 20000, false);
>> +       if (ret)
>> +               return ret;
>>
>>         writel(value, reg_address);
>>
>> -       while (readl(reg_address) & mask)
>> -               ;
>> +       return wait_for_bit(__func__, (const u32 *)reg_address, mask,
>> +                           false, 20000, false);
>>  }
>
> because now, you got a 20 seconds timeout and a return value if there
> is a timeout,
>
>>
>>  /*
>> @@ -233,11 +215,6 @@ void cm_basic_init(const struct cm_config * const cfg)
>>
>>         writel(cfg->gpiodiv, &clock_manager_base->per_pll.gpiodiv);
>>
>> -#define LOCKED_MASK \
>> -       (CLKMGR_INTER_SDRPLLLOCKED_MASK  | \
>> -       CLKMGR_INTER_PERPLLLOCKED_MASK  | \
>> -       CLKMGR_INTER_MAINPLLLOCKED_MASK)
>> -
>>         cm_wait_for_lock(LOCKED_MASK);
>>
>>         /* write the sdram clock counters before toggling outreset all */
>> @@ -257,13 +234,13 @@ void cm_basic_init(const struct cm_config * const cfg)
>>          * after locking, but before taking out of bypass
>>          * assert/deassert outresetall
>>          */
>> -       uint32_t mainvco = readl(&clock_manager_base->main_pll.vco);
>> +       u32 mainvco = readl(&clock_manager_base->main_pll.vco);
>>
>>         /* assert main outresetall */
>>         writel(mainvco | CLKMGR_MAINPLLGRP_VCO_OUTRESETALL_MASK,
>>                &clock_manager_base->main_pll.vco);
>>
>> -       uint32_t periphvco = readl(&clock_manager_base->per_pll.vco);
>> +       u32 periphvco = readl(&clock_manager_base->per_pll.vco);
>>
>>         /* assert pheriph outresetall */
>>         writel(periphvco | CLKMGR_PERPLLGRP_VCO_OUTRESETALL_MASK,
>> @@ -291,20 +268,20 @@ void cm_basic_init(const struct cm_config * const cfg)
>>          * are aligned nicely; so we can change any phase.
>>          */
>>         cm_write_with_phase(cfg->ddrdqsclk,
>> -                           (uint32_t)&clock_manager_base->sdr_pll.ddrdqsclk,
>> +                           (u32)&clock_manager_base->sdr_pll.ddrdqsclk,
>>                             CLKMGR_SDRPLLGRP_DDRDQSCLK_PHASE_MASK);
>>
>>         /* SDRAM DDR2XDQSCLK */
>>         cm_write_with_phase(cfg->ddr2xdqsclk,
>> -                           (uint32_t)&clock_manager_base->sdr_pll.ddr2xdqsclk,
>> +                           (u32)&clock_manager_base->sdr_pll.ddr2xdqsclk,
>>                             CLKMGR_SDRPLLGRP_DDR2XDQSCLK_PHASE_MASK);
>>
>>         cm_write_with_phase(cfg->ddrdqclk,
>> -                           (uint32_t)&clock_manager_base->sdr_pll.ddrdqclk,
>> +                           (u32)&clock_manager_base->sdr_pll.ddrdqclk,
>>                             CLKMGR_SDRPLLGRP_DDRDQCLK_PHASE_MASK);
>>
>>         cm_write_with_phase(cfg->s2fuser2clk,
>> -                           (uint32_t)&clock_manager_base->sdr_pll.s2fuser2clk,
>> +                           (u32)&clock_manager_base->sdr_pll.s2fuser2clk,
>>                             CLKMGR_SDRPLLGRP_S2FUSER2CLK_PHASE_MASK);
>>
>
> but you're not checking the return value at all?
Will check the return value from cm_write_with_phase() and cm_basic_init().
Thanks.

Regards
Ley Foon