Re: [PATCH v2 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[Viresh Kumar]

On 22-05-18, 16:13, Taniya Das wrote:
> On 5/22/2018 12:36 AM, skannan@xxxxxxxxxxxxxx wrote:
> > On 2018-05-21 02:01, Viresh Kumar wrote:
> > > On 19-05-18, 23:04, Taniya Das wrote:

> > > > +    /* Make sure the write goes through before proceeding */
> > > > +    mb();
> > > 
> > > Btw what happens right after this is done ? Are we guaranteed that the
> > > frequency is updated in the hardware after this ? What about enabling
> > > fast-switch for your platform ? Look at drivers/cpufreq/scmi-cpufreq.c
> > > to see how that is done.
> > 
> > Yeah, I don't think this is needed really.
> > 
> 
> Just want to make sure it doesn't really sit in the write buffer before
> return.

As per Saravana you will be dropping it now, right ?

> > > > +    index = readl_relaxed(c->perf_base);
> > > > +    index = min(index, LUT_MAX_ENTRIES - 1);
> > > 
> > > Will the hardware ever read a value over 39 here ?
> > 
> > The register could be initialized to whatever before the kernel is
> > brought up. Don't want to depend on it being correct to avoid out of
> > bounds access that could leak data.

Fair enough.

> > > > +static int qcom_read_lut(struct platform_device *pdev,
> > > > +             struct cpufreq_qcom *c)
> > > > +{
> > > > +    struct device *dev = &pdev->dev;
> > > > +    u32 data, src, lval, i, core_count, prev_cc;
> > > > +
> > > > +    c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> > > > +                sizeof(*c->table), GFP_KERNEL);
> > > > +    if (!c->table)
> > > > +        return -ENOMEM;
> > > > +
> > > > +    for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> > > > +        data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
> > > > +        src = ((data & GENMASK(31, 30)) >> 30);
> > > > +        lval = (data & GENMASK(7, 0));
> > > > +        core_count = CORE_COUNT_VAL(data);
> > > > +
> > > > +        if (!src)
> > > > +            c->table[i].frequency = INIT_RATE / 1000;
> > > > +        else
> > > > +            c->table[i].frequency = XO_RATE * lval / 1000;
> > > > +
> > > > +        c->table[i].driver_data = c->table[i].frequency;
> > > 
> > > Why do you need to use driver_data here? Why can't you simple use
> > > frequency field in the below conditional expressions ?
> > > 
> 
> The frequency field would be marked INVALID in case the core count does not
> match and the frequency data would be lost.
> 
> > > > +
> > > > +        dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> > > > +            i, c->table[i].frequency, core_count);
> > > > +
> > > > +        if (core_count != c->max_cores)
> > > > +            c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
> > > > +
> > > > +        /*
> > > > +         * Two of the same frequencies with the same core counts means
> > > > +         * end of table.
> > > > +         */
> > > > +        if (i > 0 && c->table[i - 1].driver_data ==
> > > > +            c->table[i].driver_data && prev_cc == core_count) {
> > > > +            struct cpufreq_frequency_table *prev = &c->table[i - 1];
> > > > +
> > > > +            if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
> > > 
> > > There can only be a single boost frequency at max ?
> > 
> > As of now, yes. If that changes, we'll change this code later.
> > 
> > > > +                prev->flags = CPUFREQ_BOOST_FREQ;
> > > > +                prev->frequency = prev->driver_data;
> > > 
> > > Okay you are using driver_data as a local variable to keep this value
> > > safe which you might have overwritten. Maybe use a simple variable
> > > prev_freq for this. It would be much more readable in that case and
> > > you wouldn't end up abusing the driver_data field.
> > > 
> 
> Please correct me, currently the driver_data is not used by cpufreq core and
> that was the reason to use it. In case you still think it is not a good way
> to handle it, I would try to handle it differently.

Yeah, the driver data will not be used by cpufreq core, but I think
the code would be far more readable if you use a local variable like
prev_freq instead.

> > > > +            }
> > > > +
> > > > +            break;
> > > > +        }
> > > > +        prev_cc = core_count;
> > > > +    }
> > > > +    c->table[i].frequency = CPUFREQ_TABLE_END;
> > > 
> > > Wouldn't you end up writing on c->table[40].frequency here if there
> > > are 40 frequency value present ?
> > 
> > Yeah, the loop condition needs to be fixed.
> > 
> 
> The table allocation is done for 'LUT_MAX_ENTRIES + 1'.
> Yes in case we have all [0-39] (i.e 40 entries) read from the hardware,
> would store the same and mark the 40th index as table end. Please correct if
> I missed something in your comment.

Ahh, you are right. I misread it.

-- 
viresh