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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
