Hi Michael and Stephen
Any chance to take a look at this patch? :)
On 2018/4/8 15:28, Shawn Lin wrote:
> CLK_DIVIDER_EVEN is used for clock divders that should only
> use even number in the div field.
>
> Two clock divder should consider to use this flag
> (1) The divder is physically only support even div number to
> generate 50% duty cycle clock rate.
> (2) The divder's clock consumer request it to use even div number
> to generate the most closest requested rate for whatever reason.
>
> In some platforms, for instance Rockchip, the eMMC/SDIO/SDMMC should
> request divder to use even number when working at a high throughput
> speed mode reliably. However, that wasn't guaranteed by clock framework.
> So the previous tricky is to carefully assign magic clock rate to their
> parents as well as consumer's clock rate when requesting. That works
> bad in practice if folks change the parents clock rate or the clock
> hierarchy randomly. That also work bad if the consumer's clock rate
> came from the DT, which is changed so fraquent for different boards.
>
> To make it's less prone to make mistake and to make it really respect
> the fact that the divder should use even number to the div field, we
> need the clock framework's help. Now we have CLK_DIVIDER_POWER_OF_TWO,
> which could guarantee the div field is even number, however, obviously
> it skips the even numbers which isn't the power of 2, but maybe which is
> the best div to generate closest clock rate for consumer.
>
> Look at the examples here when doing some random test on Rockchip's
> platforms, by changing the requested clock rate from DT, namely
> assigning different max-frquency for MMC node,
>
> when the mmc host driver requests 80MHz with CLK_DIVIDER_POWER_OF_TWO
> flag for the clock divder, it shows the final clock rate is 61.44Mhz
>
> pll_vpll0 1 1 983039999 0 0
> vpll0 4 4 983039999 0 0
> clk_emmc_div 1 1 122880000 0 0
> clk_emmc 1 1 122880000 0 0
> emmc_sample 0 0 61440000 0 155
> emmc_drv 0 0 61440000 0 180
>
> With this patch and add CLK_DIVIDER_EVEN flag for clk_emmc_div, we get
> the final clock rate, 67.7376MHz.
>
> pll_vpll1 1 1 812851199 0 0
> vpll1 2 2 812851199 0 0
> clk_emmc_div 1 1 135475200 0 0
> clk_emmc 1 1 135475200 0 0
> emmc_sample 0 0 67737600 0 113
> emmc_drv 0 0 67737600 0 180
>
> Apprently 67737600 is better than 61440000 when requesting 80MHz.
> Of course, we could have more case that worsen the gap between
> the desired rate and the actual rate if using CLK_DIVIDER_POWER_OF_TWO.
>
> Alternatively, clk_div_table could be resorted to handle different kinds
> of div limilation, and it seems to be applied to irregular div calculation
> in practice by current clock provider drivers, but even number for div
> field is a rather common, regular and symmetrical requirement. So it is
> worth to introduces CLK_DIVIDER_EVEN flag for clock framework to make
> the best in this process.
>
> Signed-off-by: Shawn Lin <shawn.lin at rock-chips.com>
>
> ---
>
> Changes in v3:
> - Fix a wrong if condition check
>
> Changes in v2:
> - Avoid to use div 1 if (CLK_DIVIDER_EVEN | CLK_DIVIDER_POWER_OF_TWO)
> suggested by Heiko. And seems actually no other flags should be bothered
> by this newly added CLK_DIVIDER_EVEN.
>
> drivers/clk/clk-divider.c | 26 ++++++++++++++++++++++++--
> include/linux/clk-provider.h | 3 +++
> 2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index b6234a5..8f305f1 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -159,8 +159,17 @@ static bool _is_valid_table_div(const struct clk_div_table *table,
> static bool _is_valid_div(const struct clk_div_table *table, unsigned int div,
> unsigned long flags)
> {
> - if (flags & CLK_DIVIDER_POWER_OF_TWO)
> - return is_power_of_2(div);
> + bool is_valid;
> +
> + if (flags & CLK_DIVIDER_POWER_OF_TWO) {
> + is_valid = is_power_of_2(div);
> + if (flags & CLK_DIVIDER_EVEN)
> + return is_valid && (div != 1);
> + return is_valid;
> + }
> +
> + if (flags & CLK_DIVIDER_EVEN)
> + return !(div % 2);
> if (table)
> return _is_valid_table_div(table, div);
> return true;
> @@ -208,6 +217,12 @@ static int _div_round_up(const struct clk_div_table *table,
> {
> int div = DIV_ROUND_UP_ULL((u64)parent_rate, rate);
>
> + /*
> + * Check even before power of two to avoid div 1 if combination
> + * happens, which applies to all the following similarities.
> + */
> + if (flags & CLK_DIVIDER_EVEN)
> + div = !(div % 2) ? div : (div + 1);
> if (flags & CLK_DIVIDER_POWER_OF_TWO)
> div = __roundup_pow_of_two(div);
> if (table)
> @@ -226,6 +241,11 @@ static int _div_round_closest(const struct clk_div_table *table,
> up = DIV_ROUND_UP_ULL((u64)parent_rate, rate);
> down = parent_rate / rate;
>
> + if (flags & CLK_DIVIDER_EVEN) {
> + up = !(up % 2) ? up : (up + 1);
> + down = !(down % 2) ? down : (down - 1);
> + }
> +
> if (flags & CLK_DIVIDER_POWER_OF_TWO) {
> up = __roundup_pow_of_two(up);
> down = __rounddown_pow_of_two(down);
> @@ -264,6 +284,8 @@ static int _next_div(const struct clk_div_table *table, int div,
> {
> div++;
>
> + if (flags & CLK_DIVIDER_EVEN)
> + div = !(div % 2) ? div : (div + 1);
> if (flags & CLK_DIVIDER_POWER_OF_TWO)
> return __roundup_pow_of_two(div);
> if (table)
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 210a890..7c59611 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -388,6 +388,8 @@ struct clk_div_table {
> * CLK_DIVIDER_MAX_AT_ZERO - For dividers which are like CLK_DIVIDER_ONE_BASED
> * except when the value read from the register is zero, the divisor is
> * 2^width of the field.
> + * CLK_DIVIDER_EVEN - For the dividers which could only use even number in the
> + * div field.
> */
> struct clk_divider {
> struct clk_hw hw;
> @@ -409,6 +411,7 @@ struct clk_divider {
> #define CLK_DIVIDER_ROUND_CLOSEST BIT(4)
> #define CLK_DIVIDER_READ_ONLY BIT(5)
> #define CLK_DIVIDER_MAX_AT_ZERO BIT(6)
> +#define CLK_DIVIDER_EVEN BIT(7)
>
> extern const struct clk_ops clk_divider_ops;
> extern const struct clk_ops clk_divider_ro_ops;
>
