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: Sat, 27 Aug 2011 13:10:16 +0200	[thread overview]
Message-ID: <20110827111016.GI31404@pengutronix.de> (raw)
In-Reply-To: <CAMXH7KH=1D7UhHoEdOcgZ9kXvneWrk04dTRuU-d2X-mXCHqUJQ@mail.gmail.com>

On Fri, Aug 26, 2011 at 09:56:31AM -0500, Rob Lee wrote:
> Sascha and all,
> 
> Just FYI, this is my first submission to the community so I'm sure that I
> have much to learn about community style beyond what is given in
> the CodingStyle and Submit* documents.  Please give
> "community newbie" level details in your feedback.
> 
> My comments are below.
> 
> On 26 August 2011 02:27, Sascha Hauer <s.hauer@pengutronix.de> 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.
> >
> 
> This was simply to avoid the unnecessary memory usage by creating
> the default values if someone decided to override the default cpuidle
> parameters for their build.

Is a board known that wants to override this? If not, just drop it
and we'll wait until someone has valid reasons to do so. I think
this is a SoC only feature and the board has nothing to change here.

> 
> > > +
> > > +#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.
> >
> 
> Based on your response, it appears that standard C X-macros are not Linux
> kernel community friendly.

I never heard of standard C X-macros. You have an array of structs here,
there are no macros required to handle them.

> 
> > > +
> > > +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++)

btw please use cpuidle_set_statedata() / cpuidle_get_statedata instead
of looping around the states until you found the right one.

> >
> > A define for MXC_NUM_CPUIDLE_STATES looks wrong, it is not constant for
> > different SoCs.
> >
> 
> The CPUIDLE states will be constant among a family of SoCs such as mach-mx5, but
> the way I've written the driver, I've assumed a mach level defines a
> family which now
> that I think about it, obviously isn't the case for mach-imx.
> 
> If you have time, please give your thoughts on the organization of the mach
> directories with regards to mach-imx and mach-mx5 keeping in mind that
> i.MX6 will be coming soon.  This will help me in trying to make this driver
> more acceptable and I can pass this info on to others to discuss and learn
> from as well.

Generally:

- Put everything you need into a single driver (a platform driver)
- Do not call any functions imx_ or mx5_ functions outside your driver
- It seems that the cpuidle driver handles the same resources as
  platform_suspend_ops. I don't know enough of powermanagement to tell
  how both relate to each other, but as both use the same resources,
  both should be handled in the same place. That could mean that there
  are changes necessary to the current mx5 suspend ops.

When this is done, we can still decide whether we need a completely new
driver on i.MX6 or whether we can reuse the old one.

BTW. you don't need to think about i.MX6, you can also think about
i.MX3, i.MX2,..

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-08-27 11:10 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 [this message]
2011-09-09 14:33   ` Shawn Guo
2011-09-12 10:11     ` Sascha Hauer
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=20110827111016.GI31404@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.