Re: [PATCH] clk: Hi6220: enable stub clock driver for ARCH_HISI

[Daniel Thompson]

On 08/08/16 15:42, Amit Kucheria wrote:
> On Mon, Aug 8, 2016 at 12:12 PM, Leo Yan <leo.yan@xxxxxxxxxx> wrote:
> > Hi Amit,
> >
> > On Mon, Aug 08, 2016 at 11:23:31AM +0530, Amit Kucheria wrote:
> > > On Mon, Aug 8, 2016 at 9:07 AM, Leo Yan <leo.yan@xxxxxxxxxx> wrote:
> > > > In current kernel config 'CONFIG_STUB_CLK_HI6220' is disabled by
> > > > default, as result stub clock driver has not been registered and
> > > > CPUFreq driver cannot work.
> > >
> > > I have a related patch that has been pending for a while:
> > > https://lkml.org/lkml/2016/6/20/375 but it was tied only to the
> > > thermal driver.
> >
> > I also have concern this patch may duplicate with yours.
> >
> > > Is the stub mandatory for the architecture? Will other SoCs in the
> > > family will use the same stub?
> >
> > I don't think stub driver is mandartory for archtitecture and it's
> > only used by Hi6220 on Hikey. AFAIK, currently stub driver is only used
> > by CPU frequency change.
> >
> > The logic is:
> >   Thermal cooling device driver
> >       `-> CPUFreq DT driver
> >               `-> Stub clock driver
> >
> > ARM is working on Hikey for EAS profiling, so usually the use case is
> > to enable CPUFreq driver and stub clock driver. Sometimes it only
> > need enable this driver without thermal driver; so this is why we
> > cannot rely on thermal driver to enable stub clock driver.
> >
> > I'm open-minded if you have better idea to enable it.
> >
> > > > This patch is to enable stub clock driver in config for ARCH_HISI.
> > > >
> > > > Reported-by: Dietmar Eggemann <dietmar.eggemann@xxxxxxx>
> > > > Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> > > > ---
> > > >  drivers/clk/hisilicon/Kconfig | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
> > > > index 3f537a0..9e0a95e 100644
> > > > --- a/drivers/clk/hisilicon/Kconfig
> > > > +++ b/drivers/clk/hisilicon/Kconfig
> > > > @@ -23,5 +23,6 @@ config RESET_HISI
> > > >  config STUB_CLK_HI6220
> > > >         bool "Hi6220 Stub Clock Driver"
> > > >         depends on COMMON_CLK_HI6220 && MAILBOX
> > > > +       default ARCH_HISI
>
> Instead of forcing this up on the entire arch, why not restrict it to
> just the cpufreq driver?  ARM_HISI_ACPU_CPUFREQ?

I'm struggling to understand the concern here. default doesn't force 
this option on anything, it just sets the default.

Personally I might prefer 'default y' because it is simpler and involves 
less thinking. However the other drivers in this directory use 'default 
ARCH_HISI' (i.e. they do not default on for a COMPILE_TEST) so copying 
their approach is quite reasonable.


Daniel.