All of lore.kernel.org
 help / color / mirror / Atom feed
From: s.hauer@pengutronix.de (Sascha Hauer)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/2] Adding platform level cpuidle driver for i.MX SoCs.
Date: Mon, 12 Sep 2011 12:11:30 +0200	[thread overview]
Message-ID: <20110912101130.GQ31404@pengutronix.de> (raw)
In-Reply-To: <20110909143339.GD32138@S2100-06.ap.freescale.net>

On Fri, Sep 09, 2011 at 10:33:40PM +0800, Shawn Guo wrote:
> On Fri, Aug 26, 2011 at 09:27:04AM +0200, Sascha Hauer wrote:
> > On Thu, Aug 25, 2011 at 09:33:14PM -0500, Robert Lee wrote:
> > > Signed-off-by: Robert Lee <rob.lee@linaro.org>
> > > ---
> > > --- /dev/null
> > > +++ b/arch/arm/plat-mxc/cpuidle.c
> > > @@ -0,0 +1,112 @@
> > > +/*
> > > + * This file is licensed under the terms of the GNU General Public
> > > + * License version 2.  This program is licensed "as is" without any
> > > + * warranty of any kind, whether express or implied.
> > > + */
> > > +
> > > +#include <linux/io.h>
> > > +#include <linux/cpuidle.h>
> > > +#include <asm/proc-fns.h>
> > > +#include <mach/hardware.h>
> > > +#include <mach/system.h>
> > > +#include <mach/cpuidle.h>
> > > +
> > > +
> > > +#ifndef MXC_OVERRIDE_DEFAULT_CPUIDLE_PARAMS
> > 
> > Either there is a possibility to overwrite the cpuidle parameters
> > or there is none, but we don't need a define for this. I'm not
> > convinced we need this possibility at all though.
> > 
> > > +
> > > +#define MXC_X_MACRO(a, b, c) {c}
> > > +static struct imx_cpuidle_params default_cpuidle_params[] = \
> > > +	MXC_CPUIDLE_TABLE;
> > > +#undef MXC_X_MACRO
> > 
> > Hell! This is one of the worst unnecessary preprocessor abuses I've ever
> > seen. Do not show this in public again.
> > 
> > > +
> > > +static struct imx_cpuidle_params *imx_cpuidle_params = default_cpuidle_params;
> > > +
> > > +#else
> > > +
> > > +static struct imx_cpuidle_params *imx_cpuidle_params;
> > > +
> > > +#endif
> > > +
> > > +
> > > +
> > > +/* in case you want to override the mach level params at the board level */
> > > +void imx_cpuidle_board_params(struct imx_cpuidle_params *cpuidle_params)
> > > +{
> > > +	imx_cpuidle_params = cpuidle_params;
> > > +}
> > > +
> > > +static int imx_enter_idle(struct cpuidle_device *dev,
> > > +			       struct cpuidle_state *state)
> > > +{
> > > +	struct timeval before, after;
> > > +	int idle_time, i;
> > > +
> > > +	/* We only need to pass an index to the mach level so here we
> > > +	 * find the index of the name contained in the cpuidle_state
> > > +	 * to pass. */
> > > +	for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++)
> > 
> > A define for MXC_NUM_CPUIDLE_STATES looks wrong, it is not constant for
> > different SoCs.
> > 
> > > +		if (state == &dev->states[i])
> > > +			break;
> > > +
> > > +	local_irq_disable();
> > > +	local_fiq_disable();
> > > +
> > > +	do_gettimeofday(&before);
> > > +
> > > +	imx_cpu_do_idle(i);
> > 
> > We are currently working on running a single image on as many SoCs we
> > can. Take a step back and imagine what the linker says when it finds
> > 6 different functions named imx_cpu_do_idle()
> > 
> > > +
> > > +	do_gettimeofday(&after);
> > > +
> > > +	local_fiq_enable();
> > > +	local_irq_enable();
> > > +
> > > +	idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
> > > +			(after.tv_usec - before.tv_usec);
> > > +
> > > +	return idle_time;
> > > +}
> > > +
> > > +static struct cpuidle_driver imx_cpuidle_driver = {
> > > +	.name =         "imx_idle",
> > > +	.owner =        THIS_MODULE,
> > > +};
> > > +
> > > +static DEFINE_PER_CPU(struct cpuidle_device, imx_cpuidle_device);
> > > +
> > > +static int __init imx_cpuidle_init(void)
> > > +{
> > > +	struct cpuidle_device *device;
> > > +	int i;
> > > +
> > > +	#define MXC_X_MACRO(a, b, c) #a
> > > +	char *mxc_cpuidle_state_name[] = MXC_CPUIDLE_TABLE;
> > > +	#undef MXC_X_MACRO
> > > +
> > > +	#define MXC_X_MACRO(a, b, c) b
> > > +	char *mxc_cpuidle_state_desc[] = MXC_CPUIDLE_TABLE;
> > > +	#undef MXC_X_MACRO
> > > +
> > > +	if (imx_cpuidle_params == NULL)
> > > +		return -ENODEV;
> > > +
> > > +	cpuidle_register_driver(&imx_cpuidle_driver);
> > > +
> > > +	device = &per_cpu(imx_cpuidle_device, smp_processor_id());
> > > +	device->state_count = MXC_NUM_CPUIDLE_STATES;
> > > +
> > > +	for (i = 0; i < MXC_NUM_CPUIDLE_STATES && i < CPUIDLE_STATE_MAX; i++) {
> > > +		device->states[i].enter = imx_enter_idle;
> > > +		device->states[i].exit_latency = imx_cpuidle_params[i].latency;
> > > +		device->states[i].flags = CPUIDLE_FLAG_TIME_VALID;
> > > +		strcpy(device->states[i].name, mxc_cpuidle_state_name[i]);
> > > +		strcpy(device->states[i].desc, mxc_cpuidle_state_desc[i]);
> > > +	}
> > > +
> > > +	if (cpuidle_register_device(device)) {
> > > +		printk(KERN_ERR "imx_cpuidle_init: Failed registering\n");
> > > +		return -ENODEV;
> > > +	}
> > > +	return 0;
> > 
> > This should really be a platform driver.
> > 
> I do not understand the reason behind this comment.  The cpuidle is
> not really a platform device which typically has IO and IRQ such
> hardware resources requiring a platform driver to talk to.
> 
> Looking at the following cpuidle drivers, only davinci implemented it
> as a platform driver.
> 
>     arch/arm/mach-at91/cpuidle.c
>     arch/arm/mach-davinci/cpuidle.c
>     arch/arm/mach-exynos4/cpuidle.c
>     arch/arm/mach-kirkwood/cpuidle.c
>     arch/arm/mach-omap2/cpuidle34xx.c
>     arch/arm/mach-shmobile/cpuidle.c
> 
> Also, I'm seeing imx cpufreq sitting on the mainline is not a platform
> driver either.  I really did not think of any good reason for cpuidle
> to be a platform driver necessarily.

The reason may not be really of technical nature. A platform driver only
changes the thinking:

- A driver makes sure that the author knows that the hardware may or may
  not be present.
- A driver makes sure that it may run on different types of hardware if
  passed different platform specific data.
- A driver motivates the author to view the code as a single seperated
  topic and to put all related code into the driver.

In the 'real' device driver world all points above are clear to driver
authors, but when it comes to SoC internal stuff people tend to forget
this.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

  reply	other threads:[~2011-09-12 10:11 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-08-26  2:33 Robert Lee
2011-08-26  2:33 ` [PATCH 2/2] Adding support for the plat-mxc cpuidle driver to mach-mx5. Tested on i.MX51 Babbage board with ARM core running at 800Mhz. Idle OS ARM core power before patch = ~400mW. Idle OS ARM core power after patch = ~1.25mW Robert Lee
2011-08-26  8:19   ` Russell King - ARM Linux
2011-08-26  7:27 ` [PATCH 1/2] Adding platform level cpuidle driver for i.MX SoCs Sascha Hauer
2011-08-26 14:56   ` Rob Lee
2011-08-27 11:10     ` Sascha Hauer
2011-09-09 14:33   ` Shawn Guo
2011-09-12 10:11     ` Sascha Hauer [this message]
2011-09-12 12:10       ` Shawn Guo
2011-08-26  8:21 ` Russell King - ARM Linux

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110912101130.GQ31404@pengutronix.de \
    --to=s.hauer@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --subject='Re: [PATCH 1/2] Adding platform level cpuidle driver for i.MX SoCs.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.