Re: [PATCH v2 1/3] clk: bcm2835: Always round up clock divisor

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1: Type: text/plain, Size: 1874 bytes --]

Remi Pommarel <repk@triplefau.lt> writes:

> Make bcm2835_clock_choose_div always round up the chosen MASH divisor so that
> the resulting average rate will not be higher than the requested one.
>
> Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> ---
>  drivers/clk/bcm/clk-bcm2835.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> index 39bf582..1237716 100644
> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> @@ -1152,18 +1152,19 @@ static u32 bcm2835_clock_choose_div(struct clk_hw *hw,
>  {
>  	struct bcm2835_clock *clock = bcm2835_clock_from_hw(hw);
>  	const struct bcm2835_clock_data *data = clock->data;
> -	u32 unused_frac_mask = GENMASK(CM_DIV_FRAC_BITS - data->frac_bits, 0);
> +	u32 unused_frac_mask =
> +		GENMASK(CM_DIV_FRAC_BITS - data->frac_bits, 0) >> 1;
>  	u64 temp = (u64)parent_rate << CM_DIV_FRAC_BITS;
> +	u64 rem;
>  	u32 div;
>  
> -	do_div(temp, rate);
> +	rem = do_div(temp, rate);
>  	div = temp;
>  
> -	/* Round and mask off the unused bits */
> -	if (unused_frac_mask != 0) {
> -		div += unused_frac_mask >> 1;
> -		div &= ~unused_frac_mask;
> -	}
> +	/* Round up and mask off the unused bits */
> +	if ((div & unused_frac_mask) != 0 || rem != 0)
> +		div += unused_frac_mask + 1;
> +	div &= ~unused_frac_mask;

Suppose we've got 8 of our 12 frac bits populated.  You've added a ">>
1" to the unused_frac_mask, so it's only 0x7 instead of 0xf.  When you
say "round up", you add 0x8 (the high bit of the unused mask") then and
with ~0x7.  If you started with 0x1 in the low bits of div, you'd end up
with 0x8, so you've set an unused bit instead of actually rounding up.

Did my logic work, here?  I think you just want to drop the ">>1" in
unused_frac_mask.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]