Date:   Tue, 30 Apr 2019 13:22:43 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Paul Walmsley <paul.walmsley@...ive.com>
Cc:     Paul Walmsley <paul.walmsley@...ive.com>,
        devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
        Paul Walmsley <paul@...an.com>,
        Wesley Terpstra <wesley@...ive.com>,
        Palmer Dabbelt <palmer@...ive.com>,
        Michael Turquette <mturquette@...libre.com>,
        Megan Wachs <megan@...ive.com>
Subject: Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library

Quoting Paul Walmsley (2019-04-29 18:14:14)
> On Mon, 29 Apr 2019, Stephen Boyd wrote:
> > 
> > Nitpick: This might be easier to read with a switch statement:
> > 
> >       switch (post_divr_freq) {
> >       case 0 ... 11000000:
> >               return 1;
> >       case 11000001 ... 18000000:
> >               return 2;
> >       case 18000001 ... 30000000:
> >               return 3;
> >       case 30000001 ... 50000000:
> >               return 4;
> >       case 50000000 ... 80000000:
> >               return 5;
> >       case 80000001 ... 130000000:
> >               return 6;
> >       }
> > 
> >       return 7;
> 
> To be equivalent to the original code, we'd need to write:
> 
>        switch (post_divr_freq) {
>        case 0 ... 10999999:
>                return 1;
>        case 11000000 ... 17999999:
>                return 2;
> (etc.)
> 
> In any case, it's been changed to use the gcc case range operator.

Ah right, thanks!

> > > +{
> > > +       return (c->flags & WRPLL_FLAGS_INT_FEEDBACK_MASK) ? 2 : 1;
> > > +}
> > > +
> > > +/**
> > > + * __wrpll_calc_divq() - determine DIVQ based on target PLL output clock rate
> > > + * @target_rate: target PLL output clock rate
> > > + * @vco_rate: pointer to a u64 to store the computed VCO rate into
> > > + *
> > > + * Determine a reasonable value for the PLL Q post-divider, based on the
> > > + * target output rate @target_rate for the PLL.  Along with returning the
> > > + * computed Q divider value as the return value, this function stores the
> > > + * desired target VCO rate into the variable pointed to by @vco_rate.
> > > + *
> > > + * Context: Any context.  Caller must protect the memory pointed to by
> > > + *          @vco_rate from simultaneous access or modification.
> > > + *
> > > + * Return: a positive integer DIVQ value to be programmed into the hardware
> > > + *         upon success, or 0 upon error (since 0 is an invalid DIVQ value)
> > 
> > Why are we doing that? Can't we return a normal error code and test for
> > it being negative and then consider the number if its greater than 0 to
> > be valid?
> 
> One motivation here is that this function returns a divisor value.  So a 
> zero represents a divide by zero, which is intrinsically an error for a 
> function that returns a divisor.  The other motivation is that the current 
> return value directly maps to what the hardware expects to see.
>  
> Let me know if you want me to change this anyway.

Ok, sounds fine.

>   
> > > + */
> > > +static u8 __wrpll_calc_divq(u32 target_rate, u64 *vco_rate)
> > 
> > Why does target_rate need to be u32? 
> 
> I don't think there's any specific requirement for it to be a u32.

Ok.

> 
> > Can it be unsigned long?
> 
> There are two basic principles motivating this:
> 
> 1. Use the shortest type that fits what will be contained in the variable.
>    This increases the chance that static analysis will catch any 
>    inadvertent overflows (for example, via gcc -Woverflow).
> 
> 2. Use fixed-width types for hardware-constrained values that are 
>    unrelated to the CPU's native word length.  This is a general design 
>    practice, both to avoid confusion as to whether the variable's range 
>    does in fact depend on the compiler's implementation, and to avoid API 
>    problems if the width does change.  Although this last case doesn't 
>    apply here, the general application of this practice avoids problems 
>    like the longstanding API problem we've had with clk_set_rate(), which 
>    can't take a 64 bit clock rate argument if the kernel is built with a 
>    compiler that uses 32 bit longs.
> 

Sure, makes sense.

> 
> > > +
> > > +       if (parent_rate > MAX_INPUT_FREQ || parent_rate < MIN_POST_DIVR_FREQ)
> > > +               return -1;
> > > +
> > > +       c->parent_rate = parent_rate;
> > > +       max_r_for_parent = div_u64(parent_rate, MIN_POST_DIVR_FREQ);
> > > +       c->max_r = min_t(u8, MAX_DIVR_DIVISOR, max_r_for_parent);
> > 
> > Then this min_t can be min() which is simpler to reason about.
> 
> To me they have the same meaning - min_t doesn't seem too obscure:
> 
> $ fgrep -Ir min_t\( linux/ | wc -l
> 3320
> $ 
> 
> and using it means we don't have to use a type that's needlessly large for 
> the range of values that the variable will contain.  However, if getting 
> rid of min_t() is more important to you than that principle, it can 
> of course be changed.  Do you feel strongly about it?

It's not about obscurity. min_t() is an indicator that we have to coerce
type for comparison with casting. It's nice to avoid casts if possible
and use native types for the comparison.  assignment. It's good that you
aren't using min() with a cast though, so this look fine to me and I'm
not going to stay worried about this.

> > > + * mutually exclusive.  If both bits are set, or both are zero, the struct
> > > + * analogbits_wrpll_cfg record is uninitialized or corrupt.
> > > + */
> > > +#define WRPLL_FLAGS_BYPASS_SHIFT               0
> > > +#define WRPLL_FLAGS_BYPASS_MASK                BIT(WRPLL_FLAGS_BYPASS_SHIFT)
> > > +#define WRPLL_FLAGS_RESET_SHIFT                1
> > > +#define WRPLL_FLAGS_RESET_MASK         BIT(WRPLL_FLAGS_RESET_SHIFT)
> > > +#define WRPLL_FLAGS_INT_FEEDBACK_SHIFT 2
> > > +#define WRPLL_FLAGS_INT_FEEDBACK_MASK  BIT(WRPLL_FLAGS_INT_FEEDBACK_SHIFT)
> > > +#define WRPLL_FLAGS_EXT_FEEDBACK_SHIFT 3
> > > +#define WRPLL_FLAGS_EXT_FEEDBACK_MASK  BIT(WRPLL_FLAGS_EXT_FEEDBACK_SHIFT)
> > 
> > Maybe you can use FIELD_GET/FIELD_SET?
> 
> To me BIT() is clearer and more concise.  However, please let me know if 
> the use of the FIELD_*() macros is important to you, and I will change it.

I'm not strong on it so up to you.

> > > + */
> > > +struct analogbits_wrpll_cfg {
> > > +       u8 divr;
> > > +       u8 divq;
> > > +       u8 range;
> > > +       u8 flags;
> > > +       u16 divf;
> > > +/* private: */
> > > +       u32 output_rate_cache[DIVQ_VALUES];
> > > +       unsigned long parent_rate;
> > > +       u8 max_r;
> > > +       u8 init_r;
> > > +};
> > > +
> > > +int analogbits_wrpll_configure_for_rate(struct analogbits_wrpll_cfg *c,
> > > +                                       u32 target_rate,
> > > +                                       unsigned long parent_rate);
> > > +
> > > +unsigned int analogbits_wrpll_calc_max_lock_us(struct analogbits_wrpll_cfg *c);
> > > +
> > > +unsigned long analogbits_wrpll_calc_output_rate(struct analogbits_wrpll_cfg *c,
> > > +                                               unsigned long parent_rate);
> > 
> > I wonder if it may be better to remove analogbits_ from all these
> > exported functions. I suspect that it wouldn't conflict if it was
> > prefixed with wrpll_ and it's shorter this way. Up to you.
> 
> I don't have a strong preference either way.  I've changed it to remove 
> the analogbits prefix as you request.
> 

Alright sounds good!

Powered by blists - more mailing lists