lkml.org 
[lkml]   [2019]   [Sep]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v1 1/2] clk: intel: Add CGU clock driver for a new SoC
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


\
 
 \ /
  Last update: 2019-09-02 14:21    [W:0.091 / U:30.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site