Re: [PATCH 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

[Stephen Boyd <swboyd@chromium.org>]

Quoting Matthias Kaehlcke (2018-11-15 16:23:37)
> On Sun, Nov 11, 2018 at 06:12:29PM +0530, Taniya Das wrote:
> > On 11/4/2018 9:50 AM, Stephen Boyd wrote:
> > > Quoting Taniya Das (2018-11-02 20:06:00)
> > > > On 10/18/2018 5:02 AM, Stephen Boyd wrote:
> > > > > Quoting Taniya Das (2018-10-11 04:36:01)
> > > > > > --- a/drivers/cpufreq/Kconfig.arm
> > > > > > +++ b/drivers/cpufreq/Kconfig.arm
> > > > > > @@ -121,6 +121,17 @@ config ARM_QCOM_CPUFREQ_KRYO
> > > > > > 
> > > > > >             If in doubt, say N.
> > > > > > 
> > > > > > +config ARM_QCOM_CPUFREQ_HW
> > > > > > +       bool "QCOM CPUFreq HW driver"
> > > > > 
> > > > > Is there any reason this can't be a module?
> > > > > 
> > > > 
> > > > We do not have any use cases where we need to support it as module.
> > > 
> > > Ok, so it could easily be tristate then? Why not allow it?
> > > 
> > 
> > I have checked other vendors CPUfreq drivers and those too support only
> > "bool".
> 
> That's not entirely correct. Most drivers in Kconfig are 'tristate'
> and about 50% of those in KConfig.arm are. I'd say make it 'tristate'
> unless there are good reasons not to do so.

Yes, please make tristate.

> 
> > > > > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > > > > new file mode 100644
> > > > > > index 0000000..fe1c264
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > > > > @@ -0,0 +1,354 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > +/*
> > > > > > + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> > > > > > + */
> > > [...]
> > > > > > +
> > > > > > +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
> > > > > 
> > > > > Is this going to change in the future?
> > > > > 
> > > > 
> > > > Yes, they could change and that was the reason to introduce the offsets.
> > > > This was discussed earlier too with Sudeep and was to add them.
> > > > 
> > > > > > +       [REG_ENABLE]            = 0x0,
> > > 
> > > This is only used once? Maybe it could be removed.
> > > 
> > > > > > +       [REG_LUT_TABLE]         = 0x110,
> > > 
> > > And this is only used during probe to figure out the supported
> > > frequencies. So we definitely don't need to store around the registers
> > > after probe in an array of iomem pointers. The only one that we need
> > > after probe is the one below.
> > > 
> > > > > > +       [REG_PERF_STATE]        = 0x920,
> > > > > > +};
> > > > > > +
> > 
> > As these address offsets could change, so I am of the opinion to leave them
> > as it is.
> 
> As of now there is only one set of offsets. Let's just keep the code
> simple while this is the case and address different offsets when it is
> actually needed, as suggested by Stephen and Sudeep.

Yes, please simplify by getting rid of this and not storing anything in
the struct that's only used during probe.

> 
> > > 
> > > With fast switching we can avoid incurring any extra instructions, so
> > > please make another iomem pointer in the cpufreq_qcom struct just for
> > > writing the index or if possible, just pass the iomem pointer that
> > > points to the REG_PERF_STATE as the policy->driver_data variable here.
> > > Then we have the address in hand without any extra load. If my
> > > understanding is correct, we don't need to keep around anything besides
> > > this register address anyway so we should be able to just load it and
> > > write it immediately.
> > > 
> > 
> > The c->reg_bases[] is just an index to the updated bases addresses. I am not
> > clear as to why it would incur an extra instruction.
> > 
> > The below code would already take care of it.
> > 
> > +     for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
> > +             c->reg_bases[i] = base + offsets[i];
> > +
> 
> From a performance point of view using a direct iomem pointer
> seems like a micro-optimization that probably doesn't have a
> measurable impact. However I think the code shouldn't be more complex
> than necessary, and at this point the indirection isn't needed.
> 

Yes it's a micro-optimization for sure, in the task switching path so it
may actually be useful. Either way, I think we can greatly simplify by
just having the iomem pointer be the only pointer that is stored in the
policy driver_data.