Re: [PATCH v1 1/2] clk: intel: Add CGU clock driver for a new SoC

[Andy Shevchenko]

On Mon, Sep 02, 2019 at 03:43:13PM +0800, Tanwar, Rahul wrote:
> On 28/8/2019 11:09 PM, Andy Shevchenko wrote:
> > On Wed, Aug 28, 2019 at 03:00:17PM +0800, Rahul Tanwar wrote:

> > >   drivers/clk/intel/Kconfig       |  13 +
> > >   drivers/clk/intel/Makefile      |   4 +
> > Any plans what to do with existing x86 folder there?

> I checked the x86 folder. This driver's clock controller IP is totally
> different than other clock drivers inside x86. So having a common
> driver source is not a option. It is of course possible to move this
> driver inside x86 folder. Please let me know if you think moving
> this driver inside x86 folder makes more sense.

I'm talking about unambiguous folder where we keep Intel's drivers.
With your series it will be confusing x86 vs intel.

> > > +/*
> > > + * Calculate formula:
> > > + * rate = (prate * mult + (prate * frac) / frac_div) / div
> > > + */
> > > +static unsigned long
> > > +intel_pll_calc_rate(unsigned long prate, unsigned int mult,
> > > +		    unsigned int div, unsigned int frac, unsigned int frac_div)
> > > +{
> > > +	u64 crate, frate, rate64;
> > > +
> > > +	rate64 = prate;
> > > +	crate = rate64 * mult;
> > > +
> > > +	if (frac) {
> > This seems unnecessary.
> > I think you would like to check for frac_div instead?
> > Though I would rather to use frac = 0, frac_div = 1 and drop this conditional
> > completely.

> frac_div value is fixed to BIT(24) i.e. always a non zero value. mult & div
> are directly read from registers and by design the register values for
> mult & div is also always a non zero value. However, frac can logically
> be zero. So, I still find if (frac) condition most suitable here.

Then it's simple not needed.

> > > +		frate = rate64 * frac;
> > > +		do_div(frate, frac_div);
> > > +		crate += frate;
> > > +	}
> > > +	do_div(crate, div);
> > > +
> > > +	return (unsigned long)crate;

> > > +	hw = &pll->hw;
> > Seems redundant temporary variable.
> 
> Agree, will update in v2.

Though in another method you have similar pattern. So, perhaps you may leave it
for sake of consistency with patterns.

> > > +	pr_debug("Add clk: %s, id: %u\n", clk_hw_get_name(hw), id);
> > Is this useful?

> Yes, IMO, this proves very useful for system wide clock issues
> debugging during bootup.

You may use function tracer for that.

> > Does val == 0 follows the table, i.e. makes div == 1?
> 
> 0 val means output clock is ref clock i.e. div ==1. Agree that adding
> .val = 0, .div =1 entry will make it more clear & complete.
> 
> > > +	{ .val = 0, .div = 1 },
> > > +	{ .val = 1, .div = 2 },
> > > +	{ .val = 2, .div = 3 },

1

> > > +	{ .val = 3, .div = 4 },
> > > +	{ .val = 4, .div = 5 },
> > > +	{ .val = 5, .div = 6 },

1

> > > +	{ .val = 6, .div = 8 },
> > > +	{ .val = 7, .div = 10 },
> > > +	{ .val = 8, .div = 12 },

2

> > > +	{ .val = 9, .div = 16 },
> > > +	{ .val = 10, .div = 20 },
> > > +	{ .val = 11, .div = 24 },

4

> > > +	{ .val = 12, .div = 32 },
> > > +	{ .val = 13, .div = 40 },
> > > +	{ .val = 14, .div = 48 },

8

> > > +	{ .val = 15, .div = 64 },

16


So, now we see the pattern:

	div = val < 3 ? (val + 1) : (1 << ((val - 3) / 3));

So, can we eliminate table?

-- 
With Best Regards,
Andy Shevchenko