[PATCH V3 Resend] cpufreq: create cpufreq_generic_get() routine

[PATCH V3 Resend] cpufreq: create cpufreq_generic_get() routine

[Viresh Kumar]

cc'ng Russell/LAKML/Fengguang..

On 9 January 2014 14:08, Olof Johansson <olof@lixom.net> wrote:
> This patch breaks a bunch of ARM boards. In particular, the following
> defconfigs no longer build:

That's really bad, Rafael will scold me again :)

>           assabet_defconfig
>           badge4_defconfig
>           cerfcube_defconfig
>           collie_defconfig
>           h3600_defconfig
>           hackkit_defconfig
>           jornada720_defconfig
>           lart_defconfig
>           neponset_defconfig
>           pleb_defconfig
>           shannon_defconfig
>           simpad_defconfig
>
> Error is:
>
> drivers/built-in.o: In function `cpufreq_generic_get':
> drivers/cpufreq/cpufreq.c:189: undefined reference to `clk_get_rate'
>
> Seems like this needs to be guarded by HAVE_CLK?

Naah.. After some investigation I found this:

- We already have dummy implementations of clk routines in case
CONFIG_HAVE_CLK is not defined (I added them long back).

- There is one thing common among all above defconfigs, all
belong to SA1100 family :)

- And the problem is: SA1100 wanted to define its own clk routines
and selects CLKDEV_LOOKUP (which enables HAVE_CLK), but it
doesn't implement all clk routines. Which is *wrong*.

So, actually this patch brought an _existing_ bug in limelight. And
this should be fixed by adding dummy or meaningful implementation
of missing clk routines.

@Russell: If above looks correct then can you please communicate
what should we do here? I don't really know what exactly these
routines should have, simply a copy of dummy routines from clk.h
or some meaningful stuff. So, maybe you can write a patch, otherwise
let me know what to write and I will give it a try.

@Rafael: Please *don't* revert this patch, its not my fault this time :)

@Fengguang: Would it make sense to add build tests for all ARM
defconfigs in your build system? I thought its already there :)
That way I can always be sure that my stuff (would be helpful for
others as well though) isn't breaking build (Atleast) for any platform.

Thanks.

--
viresh

[PATCH V3 Resend] cpufreq: create cpufreq_generic_get() routine

[Amit Kucheria]

On Thu, Jan 9, 2014 at 2:56 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> cc'ng Russell/LAKML/Fengguang..
>
> On 9 January 2014 14:08, Olof Johansson <olof@lixom.net> wrote:
>> This patch breaks a bunch of ARM boards. In particular, the following
>> defconfigs no longer build:
>
> That's really bad, Rafael will scold me again :)
>
>>           assabet_defconfig
>>           badge4_defconfig
>>           cerfcube_defconfig
>>           collie_defconfig
>>           h3600_defconfig
>>           hackkit_defconfig
>>           jornada720_defconfig
>>           lart_defconfig
>>           neponset_defconfig
>>           pleb_defconfig
>>           shannon_defconfig
>>           simpad_defconfig
>>
>> Error is:
>>
>> drivers/built-in.o: In function `cpufreq_generic_get':
>> drivers/cpufreq/cpufreq.c:189: undefined reference to `clk_get_rate'
>>
>> Seems like this needs to be guarded by HAVE_CLK?
>
> Naah.. After some investigation I found this:
>
> - We already have dummy implementations of clk routines in case
> CONFIG_HAVE_CLK is not defined (I added them long back).
>
> - There is one thing common among all above defconfigs, all
> belong to SA1100 family :)
>
> - And the problem is: SA1100 wanted to define its own clk routines
> and selects CLKDEV_LOOKUP (which enables HAVE_CLK), but it
> doesn't implement all clk routines. Which is *wrong*.
>
> So, actually this patch brought an _existing_ bug in limelight. And
> this should be fixed by adding dummy or meaningful implementation
> of missing clk routines.
>
> @Russell: If above looks correct then can you please communicate
> what should we do here? I don't really know what exactly these
> routines should have, simply a copy of dummy routines from clk.h
> or some meaningful stuff. So, maybe you can write a patch, otherwise
> let me know what to write and I will give it a try.
>
> @Rafael: Please *don't* revert this patch, its not my fault this time :)
>
> @Fengguang: Would it make sense to add build tests for all ARM
> defconfigs in your build system? I thought its already there :)
> That way I can always be sure that my stuff (would be helpful for
> others as well though) isn't breaking build (Atleast) for any platform.

You can also talk to Kevin to give you "beta" access to our build
infrastructure for your trees. It is still in the process of being
scaled up but should atleast handle all ARM defconfigs but we don't
have anywhere near Fengguang's capacity.

Regards,
Amit

[PATCH V3 Resend] cpufreq: create cpufreq_generic_get() routine

[Russell King - ARM Linux]

On Thu, Jan 09, 2014 at 02:56:28PM +0530, Viresh Kumar wrote:
> Naah.. After some investigation I found this:
> 
> - We already have dummy implementations of clk routines in case
> CONFIG_HAVE_CLK is not defined (I added them long back).
> 
> - There is one thing common among all above defconfigs, all
> belong to SA1100 family :)
> 
> - And the problem is: SA1100 wanted to define its own clk routines
> and selects CLKDEV_LOOKUP (which enables HAVE_CLK), but it
> doesn't implement all clk routines. Which is *wrong*.

No, it implements only what it's clk API implementation requires.

> @Russell: If above looks correct then can you please communicate
> what should we do here? I don't really know what exactly these
> routines should have, simply a copy of dummy routines from clk.h
> or some meaningful stuff. So, maybe you can write a patch, otherwise
> let me know what to write and I will give it a try.

I've no idea at the moment, and I don't have time to look at this.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH V3 Resend] cpufreq: create cpufreq_generic_get() routine

[Fengguang Wu]

Hi Viresh,

> @Fengguang: Would it make sense to add build tests for all ARM
> defconfigs in your build system? I thought its already there :)
> That way I can always be sure that my stuff (would be helpful for
> others as well though) isn't breaking build (Atleast) for any platform.

Sure. I just added build test workers for all defconfigs from all
archs. Note that due to the large number of configs, they'll have
longer delays, but should still finish in one day.

Thanks,
Fengguang

[PATCH V3 Resend] cpufreq: create cpufreq_generic_get() routine

[Rafael J. Wysocki]

On Thursday, January 09, 2014 02:56:28 PM Viresh Kumar wrote:
> cc'ng Russell/LAKML/Fengguang..
> 
> On 9 January 2014 14:08, Olof Johansson <olof@lixom.net> wrote:
> > This patch breaks a bunch of ARM boards. In particular, the following
> > defconfigs no longer build:
> 
> That's really bad, Rafael will scold me again :)
> 
> >           assabet_defconfig
> >           badge4_defconfig
> >           cerfcube_defconfig
> >           collie_defconfig
> >           h3600_defconfig
> >           hackkit_defconfig
> >           jornada720_defconfig
> >           lart_defconfig
> >           neponset_defconfig
> >           pleb_defconfig
> >           shannon_defconfig
> >           simpad_defconfig
> >
> > Error is:
> >
> > drivers/built-in.o: In function `cpufreq_generic_get':
> > drivers/cpufreq/cpufreq.c:189: undefined reference to `clk_get_rate'
> >
> > Seems like this needs to be guarded by HAVE_CLK?
> 
> Naah.. After some investigation I found this:
> 
> - We already have dummy implementations of clk routines in case
> CONFIG_HAVE_CLK is not defined (I added them long back).
> 
> - There is one thing common among all above defconfigs, all
> belong to SA1100 family :)
> 
> - And the problem is: SA1100 wanted to define its own clk routines
> and selects CLKDEV_LOOKUP (which enables HAVE_CLK), but it
> doesn't implement all clk routines. Which is *wrong*.
> 
> So, actually this patch brought an _existing_ bug in limelight. And
> this should be fixed by adding dummy or meaningful implementation
> of missing clk routines.
> 
> @Russell: If above looks correct then can you please communicate
> what should we do here? I don't really know what exactly these
> routines should have, simply a copy of dummy routines from clk.h
> or some meaningful stuff. So, maybe you can write a patch, otherwise
> let me know what to write and I will give it a try.
> 
> @Rafael: Please *don't* revert this patch, its not my fault this time :)

Well, if build is broken, it *always* is the fault of the commit that
introduced the breakage, even if that is a result of someone else doing
things incorrectly.  Why?  Because it potentially breaks bisection for
people and *that* is a big deal.

So yes, I'm going to revert it.  Please resubmit after you've addressed the
build breakage.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH V3 Resend] cpufreq: create cpufreq_generic_get() routine

[Viresh Kumar]

On 9 January 2014 17:16, Fengguang Wu <fengguang.wu@intel.com> wrote:
> Sure. I just added build test workers for all defconfigs from all
> archs. Note that due to the large number of configs, they'll have
> longer delays, but should still finish in one day.

Many thanks to you.. It would be very helpful :)

[PATCH V3 Resend] cpufreq: create cpufreq_generic_get() routine

[Viresh Kumar]

On 9 January 2014 17:03, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> No, it implements only what it's clk API implementation requires.

I understand that platform specific parts/drivers aren't using anything
outside of the clk API's currently implemented. But then there are
generic parts of kernel which are free to use this routine. They
may or may not call it on that particular platform, which is currently
happening for SA1100. And so I feel that we need to have dummy
APIs like clk.h in there, as they wouldn't harm at all or break any
existing stuff.

> I've no idea at the moment, and I don't have time to look at this.

Okay. I will go ahead with dummy implementations for now. See
if you can give that a short review.

Thanks.

[PATCH V3 Resend] cpufreq: create cpufreq_generic_get() routine

[Viresh Kumar]

On 9 January 2014 17:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Well, if build is broken, it *always* is the fault of the commit that
> introduced the breakage, even if that is a result of someone else doing
> things incorrectly.  Why?  Because it potentially breaks bisection for
> people and *that* is a big deal.

Agree :)

> So yes, I'm going to revert it.  Please resubmit after you've addressed the
> build breakage.

Will do that shortly.