Re: [TEGRA194_CPUFREQ Patch v2 2/3] cpufreq: Add Tegra194 cpufreq driver

[sumitg]

On 05/04/20 7:35 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 04.04.2020 22:29, Sumit Gupta ÐÐÑÐÑ:
> ...
> > +static void tegra_read_counters(struct work_struct *work)
> > +{
> > +     struct read_counters_work *read_counters_work;
> > +     struct tegra_cpu_ctr *c;
> > +     u64 val;
> > +
> > +     /*
> > +      * ref_clk_counter(32 bit counter) runs on constant clk,
> > +      * pll_p(408MHz).
> > +      * It will take = 2 ^ 32 / 408 MHz to overflow ref clk counter
> > +      *              = 10526880 usec = 10.527 sec to overflow
> > +      *
> > +      * Like wise core_clk_counter(32 bit counter) runs on core clock.
> > +      * It's synchronized to crab_clk (cpu_crab_clk) which runs at
> > +      * freq of cluster. Assuming max cluster clock ~2000MHz,
> > +      * It will take = 2 ^ 32 / 2000 MHz to overflow core clk counter
> > +      *              = ~2.147 sec to overflow
> > +      */
> > +     read_counters_work = container_of(work, struct read_counters_work,
> > +                                       work);
> > +     c = &read_counters_work->c;
> > +
> > +     val = read_freq_feedback();
> > +     c->last_refclk_cnt = lower_32_bits(val);
> > +     c->last_coreclk_cnt = upper_32_bits(val);
> > +     udelay(c->delay);
> > +     val = read_freq_feedback();
> > +     c->refclk_cnt = lower_32_bits(val);
> > +     c->coreclk_cnt = upper_32_bits(val);
> > +}
> > +
> > +/*
> > + * Return instantaneous cpu speed
> > + * Instantaneous freq is calculated as -
> > + * -Takes sample on every query of getting the freq.
> > + *   - Read core and ref clock counters;
> > + *   - Delay for X us
> > + *   - Read above cycle counters again
> > + *   - Calculates freq by subtracting current and previous counters
> > + *     divided by the delay time or eqv. of ref_clk_counter in delta time
> > + *   - Return Kcycles/second, freq in KHz
> > + *
> > + *   delta time period = x sec
> > + *                     = delta ref_clk_counter / (408 * 10^6) sec
> > + *   freq in Hz = cycles/sec
> > + *              = (delta cycles / x sec
> > + *              = (delta cycles * 408 * 10^6) / delta ref_clk_counter
> > + *   in KHz     = (delta cycles * 408 * 10^3) / delta ref_clk_counter
> > + *
> > + * @cpu - logical cpu whose freq to be updated
>
>
> > + * Returns freq in KHz on success, 0 if cpu is offline
>
> I don't see any checks in the code about whether CPU is offline.
>
> Googling for "queue_work_on offline cpu" suggests that this function
> should hang.
Tried this and didn't get crash or hang.
show_cpuinfo_cur_freq() returns "<unknown>" value on reading frequency 
from a cluster having both it's cores offline.
If only one cpu is offline, then frequency of other online cpu which is 
managing the policy of that cluster is returned.
Have still added below check as guard in get|set() freq calls.
	if (!cpu_online(cpu))
		return -EINVAL;
Thankyou for the input.

> > + */
> > +static unsigned int tegra194_get_speed_common(u32 cpu, u32 delay)
> > +{
> > +     struct read_counters_work read_counters_work;
> > +     struct tegra_cpu_ctr c;
> > +     u32 delta_refcnt;
> > +     u32 delta_ccnt;
> > +     u32 rate_mhz;
> > +
> > +     read_counters_work.c.cpu = cpu;
> > +     read_counters_work.c.delay = delay;
> > +     INIT_WORK_ONSTACK(&read_counters_work.work, tegra_read_counters);
> > +     queue_work_on(cpu, read_counters_wq, &read_counters_work.work);
> > +     flush_work(&read_counters_work.work);
> > +     c = read_counters_work.c;
> > +
> > +     if (c.coreclk_cnt < c.last_coreclk_cnt)
> > +             delta_ccnt = c.coreclk_cnt + (MAX_CNT - c.last_coreclk_cnt);
> > +     else
> > +             delta_ccnt = c.coreclk_cnt - c.last_coreclk_cnt;
> > +     if (!delta_ccnt)
> > +             return 0;
> > +
> > +     /* ref clock is 32 bits */
> > +     if (c.refclk_cnt < c.last_refclk_cnt)
> > +             delta_refcnt = c.refclk_cnt + (MAX_CNT - c.last_refclk_cnt);
> > +     else
> > +             delta_refcnt = c.refclk_cnt - c.last_refclk_cnt;
> > +     if (!delta_refcnt) {
> > +             pr_debug("cpufreq: %d is idle, delta_refcnt: 0\n", cpu);
> > +             return 0;
> > +     }
> > +     rate_mhz = ((unsigned long)(delta_ccnt * REF_CLK_MHZ)) / delta_refcnt;
> > +
> > +     return (rate_mhz * KHZ); /* in KHz */
> > +}