Date:   Mon, 26 Nov 2018 18:55:37 -0800 (PST)
From:   Paul Walmsley <paul.walmsley@...ive.com>
To:     Stephen Boyd <sboyd@...nel.org>
cc:     Paul Walmsley <paul.walmsley@...ive.com>,
        devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
        Wesley Terpstra <wesley@...ive.com>,
        Palmer Dabbelt <palmer@...ive.com>,
        Michael Turquette <mturquette@...libre.com>,
        Megan Wachs <megan@...ive.com>, linux-kernel@...r.kernel.org,
        Paul Walmsley <paul@...an.com>
Subject: Re: [PATCH v2 1/3] clk: analogbits: add Wide-Range PLL library

On Wed, 21 Nov 2018, Stephen Boyd wrote:
> Quoting Paul Walmsley (2018-11-08 17:01:54)
> > On 10/25/18 12:47 AM, Stephen Boyd wrote:
> > > Quoting Paul Walmsley (2018-10-20 06:50:22)
> > >> diff --git a/drivers/clk/analogbits/wrpll-cln28hpc.c b/drivers/clk/analogbits/wrpll-cln28hpc.c
> > >> new file mode 100644
> > >> index 000000000000..ebdef859cbf5
> > >> --- /dev/null
> > >> +++ b/drivers/clk/analogbits/wrpll-cln28hpc.c

[...]

> > >> +/**
> > >> + * __wrpll_update_parent_rate() - update PLL data when parent rate changes
> > >> + * @c: ptr to a struct analogbits_wrpll_cfg record to write PLL data to
> > >> + * @parent_rate: PLL input refclk rate (pre-R-divider)
> > >> + *
> > >> + * Pre-compute some data used by the PLL configuration algorithm when
> > >> + * the PLL's reference clock rate changes.  The intention is to avoid
> > >> + * computation when the parent rate remains constant - expected to be
> > >> + * the common case.
> > >> + *
> > >> + * Returns: 0 upon success or -1 if the reference clock rate is out of range.
> > >> + */
> > >> +static int __wrpll_update_parent_rate(struct analogbits_wrpll_cfg *c,
> > >> +                                     unsigned long parent_rate)
> > >> +{
> > >> +       u8 max_r_for_parent;

[...]

> > >> +
> > >> +       if (parent_rate > MAX_INPUT_FREQ || parent_rate < MIN_POST_DIVR_FREQ)
> > >> +               return -1;

[...]

> > 
> > > Is this case even possible?
> > 
> > If analogbits_wrpll_configure_for_rate() is called with a parent_rate
> > outside of the PLL's specification range, it seems reasonable to fail --
> > or do you see the situation differently?  These parent rate restrictions
> > are defined in the PLL datasheet.
> 
> I've been pushing driver writers to use clk_hw_set_rate_range() so that
> the core framework will clamp the frequency to what is supported. Can
> you somehow use that for these clks?

Although the code in this patch is related to a hardware clock source (the 
WRPLL), this code isn't dependent on the Linux clock framework.  It's just 
a library that implements algorithms that are specific to this PLL.  

Patch 3 ("clk: sifive: add a driver for the SiFive FU540 PRCI IP block") 
adds drivers/clk/sifive/fu540-prci.c, which integrates the library with 
the CCF.

We could also make the code in this patch dependent on the Linux clock 
framework.  My thought was that CCF dependencies weren't really needed in 
this file, and that the algorithms would be easier to read and analyze 
without them.

So if it's all right with you, I'd propose that we keep clock 
framework-dependent functions localized into 
drivers/clk/sifive/fu540-prci.c.

Let me know what you'd like.

> > >  It would be nicer
> > > if this function just returned void.
> > 
> > 
> >  Is it that you'd prefer the rate check to be moved into
> > analogbits_wrpll_configure_for_rate()?  Or that you'd prefer that I not
> > validate the input parent rate at all?
> 
> I lost the context! But I think I'm just saying that if the clamping is
> done in the core framework with limits then this function can just
> return void because there aren't errors to be had.

OK.  The outcome here seems connected with the discussion outcome on 
the above comment, since this code is written to avoid adding clock 
framework dependencies.  So will wait for your thoughts on that before 
taking action here.

> > >> +/*
> > >> + * Public functions
> > >> + */
> > >> +
> > >> +/**
> > >> + * analogbits_wrpll_configure() - compute PLL configuration for a target rate
> > >> + * @c: ptr to a struct analogbits_wrpll_cfg record to write into
> > >> + * @target_rate: target PLL output clock rate (post-Q-divider)
> > >> + * @parent_rate: PLL input refclk rate (pre-R-divider)
> > >> + *
> > >> + * Given a pointer to a PLL context @c, a desired PLL target output
> > >> + * rate @target_rate, and a reference clock input rate @parent_rate,
> > >> + * compute the appropriate PLL signal configuration values.  PLL
> > >> + * reprogramming is not glitchless, so the caller should switch any
> > >> + * downstream logic to a different clock source or clock-gate it
> > >> + * before presenting these values to the PLL configuration signals.
> > >> + *
> > >> + * The caller must pass this function a pre-initialized struct
> > >> + * analogbits_wrpll_cfg record: either initialized to zero (with the
> > >> + * exception of the .name and .flags fields) or read from the PLL.
> > >> + *
> > >> + * Context: Any context.  Caller must protect the memory pointed to by @c
> > >> + *          from simultaneous access or modification.
> > >> + *
> > >> + * Return: 0 upon success; anything else upon failure.
> > >> + */
> > >> +int analogbits_wrpll_configure_for_rate(struct analogbits_wrpll_cfg *c,
> > >> +                                       u32 target_rate,
> > >> +                                       unsigned long parent_rate)
> > >> +{
> > >> +       unsigned long ratio;
> > >> +       u64 target_vco_rate, delta, best_delta, f_pre_div, vco, vco_pre;
> > >> +       u32 best_f, f, post_divr_freq, fbcfg;
> > >> +       u8 fbdiv, divq, best_r, r;
> > >> +
> > >> +       if (!c)
> > >> +               return -1;
> > > -EINVAL? Of course, it's probably better to just not care and blow up if
> > > the user of the API passes in NULL.
> > 
> > 
> > Agreed for statically-scoped functions it's most likely overkill.  But
> > for globally-scoped functions like this, it seemed better to catch the
> > errors rather than to blow up. The underlying approach was intended to
> > be standard precondition-based (paranoia-based?) development.  Seems
> > like I often screw up the calling side during development, so figured
> > this approach would save human time.  The performance impact seems
> > minimal.  That said, if it's your strong preference or requirement for
> > these to be dropped, I will of course do it - let me know.
> 
> I would still prefer to remove it. It's a semi-global API that's really
> only used and exported by clk providers. We should expect an oops and
> big callstack/crash if some driver author is using the API improperly.
> Hopefully those developers are testing their code, and they see that
> it's experiencing problems here. So the calling side screwing up during
> development will very quickly realize they're doing something wrong vs.
> noticing later that some clk is not there.

OK - dropped the null pointer check.

> > >> +               return -1;
> > >> +       }
> > >> +
> > >> +       fbcfg = WRPLL_FLAGS_INT_FEEDBACK_MASK | WRPLL_FLAGS_EXT_FEEDBACK_MASK;
> > >> +       if ((c->flags & fbcfg) == fbcfg) {
> > >> +               WARN(1, "%s called with invalid PLL config", __func__);
> > >> +               return -1;
> > >> +       }
> > >> +
> > >> +       if (c->flags == WRPLL_FLAGS_EXT_FEEDBACK_MASK) {
> > >> +               WARN(1, "%s: external feedback mode not currently supported",
> > >> +                    __func__);
> > >> +               return -1;
> > >> +       }
> > > Like a bunch of this stuff, why are we checking things that the caller
> > > should know to do itself? I'd prefer we keep things simple and assume
> > > the caller knows what they're doing.
> > 
> > My thinking was that it's easy to screw up if someone is unfamiliar with
> > the library.  Since the preconditions are computationally simple to
> > verify, the runtime cost is essentially zero.  Put differently, they are
> > intended to trade off a small amount of CPU time to save a larger amount
> > of human time.
> 
> Can we move the checks to build time with some macro that defines the
> configuration structure and has BUILD_BUG_ON() checks? 

In the short term, I agree that it's unlikely that PLLs will be 
instantiated with this library using anything other than static C 
structure records.  In the future, these PLLs may be instantiated 
completely dynamically from data not available at build time, at which 
point the static checks won't catch problems there.

> Sure it doesn't have much runtime overhead, but it has a codesize 
> overhead and mental logic overhead for something that would be better 
> served with some static check or documentation/comments and code review. 
> Put another way, it's not like this configuration is coming from random 
> user input and so we need to validate the input to make sure it's sane. 
> We can validate the input when merging the code or when building the 
> code and avoid this all.

Seems like we have similar goals in mind; just different ways of viewing 
the problem.  I personally rest easier knowing that the preconditions are 
always satisfied before entering the function.  That way there's no need 
to worry about whether a given library user (which may not be the Linux 
clock framework) really is careful about what's being passed in.  Another 
nice side effect about verifying parameter domains at runtime is that it 
can reduce the amount of anguish involved in debugging runtime memory 
corruption bugs, since the preconditions can catch corruption closer to 
when it actually happens.

In any case - I will post a revision with them implemented statically, as 
you request.

> > >> +}
> > >> diff --git a/include/linux/clk/analogbits-wrpll-cln28hpc.h b/include/linux/clk/analogbits-wrpll-cln28hpc.h
> > >> new file mode 100644
> > >> index 000000000000..cc4268f16067
> > >> --- /dev/null
> > >> +++ b/include/linux/clk/analogbits-wrpll-cln28hpc.h
> > >> +
> > >> +/**
> > >> + * struct analogbits_wrpll_cfg - WRPLL configuration values
> > >> + * @divr: reference divider value (6 bits), as presented to the PLL signals.
> > >> + * @divf: feedback divider value (9 bits), as presented to the PLL signals.
> > >> + * @divq: output divider value (3 bits), as presented to the PLL signals.
> > >> + * @flags: PLL configuration flags.  See above for more information.
> > >> + * @range: PLL loop filter range.  See below for more information.
> > >> + * @_output_rate_cache: cached output rates, swept across DIVQ.
> 
> Drop the full-stops on here too please.

Done, thanks.

> > > We don't typically care about this in the kernel, just mark it
> > > static if it's private and non-static if it's public for functions.
> > 
> > Used to do it this way a long ago, perhaps a bit less incompetently -
> > here's an example:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-omap2/omap_hwmod.c?h=v4.1#n383
> > 
> > I personally prefer the explicit indication that the identifier was
> > scoped file-local in the calling functions.  But am fine changing it per
> > your comments, and have done so.
> 
> Ok. I haven't really seen the underscores, except for in the cases when
> some framework API needs to be split into some backend multi-plexing
> function to handle different options while keeping the original function
> signature around. Put another way, it's not a kernel idiom for driver
> code, but I'm not too worried about it, so go with your own style if it
> makes you happy.

They've been dropped already from this patch based on our earlier 
discussion, so will just keep them dropped.

> > > And for struct members we have
> > > kernel doc 'private' tag that can tell users to not do something. Or
> > > worst case, an opaque pointer is stored and only defined in the C file.
> > 
> > 
> > In any case, I've changed the identifier names to align to your comments.
> 
> Ok. Please also use the private tag to indicate that these are internal
> things that shouldn't be used.

Done.

> > >> + */
> > >> +struct analogbits_wrpll_cfg {
> > >> +       u8 divr;
> > >> +       u8 divq;
> > >> +       u8 range;
> > >> +       u8 flags;
> > >> +       u16 divf;
> > >> +       u32 _output_rate_cache[DIVQ_VALUES];
> > >> +       unsigned long _parent_rate;
> > >> +       u8 _max_r;
> > >> +       u8 _init_r;
> > >> +};
> > >> +
> > >> +/*
> > >> + * Function prototypes
> > >> + */
> > > Please don't have these types of comments. They don't help.
> > 
> > Dropped this one.  Do you want me to nuke the similar comments in
> > wrpll-cln28hpc.c, separating the private function section from the
> > public functions, for example?
> 
> Yes.

Done.

Thanks for the review,

- Paul

Powered by blists - more mailing lists