* Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is
From: Linus Walleij @ 2011-12-14 15:50 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Ulf Hansson, Guennadi Liakhovetski, linux-mmc@vger.kernel.org,
linux-pm@vger.kernel.org, Chris Ball, linux-sh@vger.kernel.org
In-Reply-To: <201112141128.56029.rjw@sisk.pl>
On Wed, Dec 14, 2011 at 11:28 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, December 14, 2011, Linus Walleij wrote:
>> But this delta is dependent on a lot of stuff that only the platform
>> knows, like nominal CPU frequency, bus speed etc, so certainly the
>> platform must be able to modify that number.
>
> You seem to be confusing things. The exact meaning of this number is:
> "I may want to use the device 100 us from now (but not earlier), so please
> make it possible to do that". [It roughly means "don't put the device into
> a low-power state that takes more than 100 us to resume from", but it's a bit
> more complicated than that.] It doesn't mean "don't suspend the device for
> the next 100 us".
Yes you're right, I get it now!
Thanks for the explanation Rafael.
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Thanks,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v4 REPOST 5/5] imx6q: Remove unconditional dependency on
From: Dave Martin @ 2011-12-14 15:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20111214140500.GA1700@richard-laptop>
On Wed, Dec 14, 2011 at 10:05:04PM +0800, Richard Zhao wrote:
> On Wed, Dec 14, 2011 at 09:26:24PM +0800, Shawn Guo wrote:
> > Hi Dave,
> >
> > Sorry for that I did not look into previous post to point it out.
> >
> > On Wed, Dec 14, 2011 at 11:39:41AM +0000, Dave Martin wrote:
> > > The i.MX6 Quad SoC will work without the l2x0 L2 cache controller
> > > support built into the kernel, so this patch removes the dependency
> > > on CACHE_L2X0 and selects MIGHT_HAVE_CACHE_L2X0 instead.
> > >
> > > This makes the l2x0 support optional, so that it can be turned off
> > > when desired for debugging purposes etc.
> > >
> > > Thanks to Shawn Guo for this suggestion. [1]
> > >
> > > Signed-off-by: Dave Martin <dave.martin@linaro.org>
> > >
> > > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074602.html
> > > ---
> > > arch/arm/mach-imx/Kconfig | 2 +-
> > > 1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> > > index 29a3d61..1fb93f2 100644
> > > --- a/arch/arm/mach-imx/Kconfig
> > > +++ b/arch/arm/mach-imx/Kconfig
> > > @@ -609,13 +609,13 @@ comment "i.MX6 family:"
> > > config SOC_IMX6Q
> > > bool "i.MX6 Quad support"
> > > select ARM_GIC
> > > - select CACHE_L2X0
> > > select CPU_V7
> > > select HAVE_ARM_SCU
> > > select HAVE_IMX_GPC
> > > select HAVE_IMX_MMDC
> > > select HAVE_IMX_SRC
> > > select HAVE_SMP
> > > + select MIGHT_HAVE_CACHE_L2X0
> >
> > The option SOC_IMX6Q is only available when ARCH_IMX_V6_V7 is selected.
> > Since MIGHT_HAVE_CACHE_L2X0 has been selected by ARCH_IMX_V6_V7 in
> > patch #1, this line seems redundant here.
> Would it be better keep this one and remove patch #1 one? imx5 doesn't have
> l2x0.
Do you mean to remove MIGHT_HAVE_CACHE_L2X0 from ARCH_IMX_V6_V7, and select
it only from SOC_IMX6Q?
Cheers
---Dave
^ permalink raw reply
* Re: [PATCH v4 REPOST 5/5] imx6q: Remove unconditional dependency on
From: Richard Zhao @ 2011-12-14 14:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20111214132622.GA1520@S2100-06.ap.freescale.net>
On Wed, Dec 14, 2011 at 09:26:24PM +0800, Shawn Guo wrote:
> Hi Dave,
>
> Sorry for that I did not look into previous post to point it out.
>
> On Wed, Dec 14, 2011 at 11:39:41AM +0000, Dave Martin wrote:
> > The i.MX6 Quad SoC will work without the l2x0 L2 cache controller
> > support built into the kernel, so this patch removes the dependency
> > on CACHE_L2X0 and selects MIGHT_HAVE_CACHE_L2X0 instead.
> >
> > This makes the l2x0 support optional, so that it can be turned off
> > when desired for debugging purposes etc.
> >
> > Thanks to Shawn Guo for this suggestion. [1]
> >
> > Signed-off-by: Dave Martin <dave.martin@linaro.org>
> >
> > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074602.html
> > ---
> > arch/arm/mach-imx/Kconfig | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> > index 29a3d61..1fb93f2 100644
> > --- a/arch/arm/mach-imx/Kconfig
> > +++ b/arch/arm/mach-imx/Kconfig
> > @@ -609,13 +609,13 @@ comment "i.MX6 family:"
> > config SOC_IMX6Q
> > bool "i.MX6 Quad support"
> > select ARM_GIC
> > - select CACHE_L2X0
> > select CPU_V7
> > select HAVE_ARM_SCU
> > select HAVE_IMX_GPC
> > select HAVE_IMX_MMDC
> > select HAVE_IMX_SRC
> > select HAVE_SMP
> > + select MIGHT_HAVE_CACHE_L2X0
>
> The option SOC_IMX6Q is only available when ARCH_IMX_V6_V7 is selected.
> Since MIGHT_HAVE_CACHE_L2X0 has been selected by ARCH_IMX_V6_V7 in
> patch #1, this line seems redundant here.
Would it be better keep this one and remove patch #1 one? imx5 doesn't have
l2x0.
Thanks
Richard
>
> Regards,
> Shawn
>
> > select USE_OF
> >
> > help
> > --
> > 1.7.4.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 REPOST 4/5] highbank: Unconditionally require l2x0 L2
From: Dave Martin @ 2011-12-14 13:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4EE8A69C.40304@gmail.com>
On Wed, Dec 14, 2011 at 07:37:32AM -0600, Rob Herring wrote:
>
> On 12/14/2011 05:39 AM, Dave Martin wrote:
> > If running in the Normal World on a TrustZone-enabled SoC, Linux
> > does not have complete control over the L2 cache controller
> > configuration. The kernel cannot work reliably on such platforms
> > without the l2x0 cache support code built in.
> >
> > This patch unconditionally enables l2x0 support for the Highbank
> > SoC.
> >
> > Thanks to Rob Herring for this suggestion. [1]
> >
> > Signed-off-by: Dave Martin <dave.martin@linaro.org>
> >
> > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074495.html
>
> Doesn't this need to be above the SOB? Otherwise:
You may be right ... certainly I see no reason _not_ to change it.
So I'll change it.
>
> Acked-by: Rob Herring <rob.herring@calxeda.com>
Thanks
---Dave
^ permalink raw reply
* Re: [PATCH v4 REPOST 4/5] highbank: Unconditionally require l2x0
From: Rob Herring @ 2011-12-14 13:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1323862781-3465-5-git-send-email-dave.martin@linaro.org>
On 12/14/2011 05:39 AM, Dave Martin wrote:
> If running in the Normal World on a TrustZone-enabled SoC, Linux
> does not have complete control over the L2 cache controller
> configuration. The kernel cannot work reliably on such platforms
> without the l2x0 cache support code built in.
>
> This patch unconditionally enables l2x0 support for the Highbank
> SoC.
>
> Thanks to Rob Herring for this suggestion. [1]
>
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074495.html
Doesn't this need to be above the SOB? Otherwise:
Acked-by: Rob Herring <rob.herring@calxeda.com>
> ---
> arch/arm/Kconfig | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index d33eb39..744296d 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -340,12 +340,12 @@ config ARCH_HIGHBANK
> select ARM_AMBA
> select ARM_GIC
> select ARM_TIMER_SP804
> + select CACHE_L2X0
> select CLKDEV_LOOKUP
> select CPU_V7
> select GENERIC_CLOCKEVENTS
> select HAVE_ARM_SCU
> select HAVE_SMP
> - select MIGHT_HAVE_CACHE_L2X0
> select USE_OF
> help
> Support for the Calxeda Highbank SoC based boards.
^ permalink raw reply
* Re: [PATCH v4 REPOST 5/5] imx6q: Remove unconditional dependency on
From: Shawn Guo @ 2011-12-14 13:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1323862781-3465-6-git-send-email-dave.martin@linaro.org>
Hi Dave,
Sorry for that I did not look into previous post to point it out.
On Wed, Dec 14, 2011 at 11:39:41AM +0000, Dave Martin wrote:
> The i.MX6 Quad SoC will work without the l2x0 L2 cache controller
> support built into the kernel, so this patch removes the dependency
> on CACHE_L2X0 and selects MIGHT_HAVE_CACHE_L2X0 instead.
>
> This makes the l2x0 support optional, so that it can be turned off
> when desired for debugging purposes etc.
>
> Thanks to Shawn Guo for this suggestion. [1]
>
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074602.html
> ---
> arch/arm/mach-imx/Kconfig | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> index 29a3d61..1fb93f2 100644
> --- a/arch/arm/mach-imx/Kconfig
> +++ b/arch/arm/mach-imx/Kconfig
> @@ -609,13 +609,13 @@ comment "i.MX6 family:"
> config SOC_IMX6Q
> bool "i.MX6 Quad support"
> select ARM_GIC
> - select CACHE_L2X0
> select CPU_V7
> select HAVE_ARM_SCU
> select HAVE_IMX_GPC
> select HAVE_IMX_MMDC
> select HAVE_IMX_SRC
> select HAVE_SMP
> + select MIGHT_HAVE_CACHE_L2X0
The option SOC_IMX6Q is only available when ARCH_IMX_V6_V7 is selected.
Since MIGHT_HAVE_CACHE_L2X0 has been selected by ARCH_IMX_V6_V7 in
patch #1, this line seems redundant here.
Regards,
Shawn
> select USE_OF
>
> help
> --
> 1.7.4.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply
* Re: [PATCH 02/57] fbdev: sh_mobile_lcdc: Mark init-only symbols with __devinit(const)
From: Laurent Pinchart @ 2011-12-14 12:30 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-3-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Guennadi,
On Wednesday 14 December 2011 12:07:26 Guennadi Liakhovetski wrote:
> On Wed, 14 Dec 2011, Laurent Pinchart wrote:
> > On Tuesday 13 December 2011 23:23:32 Guennadi Liakhovetski wrote:
> > > On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > > > default_720p and sh_mobile_lcdc_check_interface are used at device
> > > > initialization time only. Mark them as __devinitconst and __devinit
> > > > respectively.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >
> > > > drivers/video/sh_mobile_lcdcfb.c | 5 +++--
> > > > 1 files changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/video/sh_mobile_lcdcfb.c
> > > > b/drivers/video/sh_mobile_lcdcfb.c index 8b18360..a6bf4fb 100644
> > > > --- a/drivers/video/sh_mobile_lcdcfb.c
> > > > +++ b/drivers/video/sh_mobile_lcdcfb.c
> > > > @@ -1459,7 +1459,7 @@ static int sh_mobile_lcdc_notify(struct
> > > > notifier_block *nb,
> > > >
> > > > * Probe/remove and driver init/exit
> > > > */
> > > >
> > > > -static const struct fb_videomode default_720p = {
> > > > +static const struct fb_videomode default_720p __devinitconst = {
> > > >
> > > > .name = "HDMI 720p",
> > > > .xres = 1280,
> > > > .yres = 720,
> > > >
> > > > @@ -1528,7 +1528,8 @@ static int sh_mobile_lcdc_remove(struct
> > > > platform_device *pdev)
> > > >
> > > > return 0;
> > > >
> > > > }
> > > >
> > > > -static int sh_mobile_lcdc_check_interface(struct sh_mobile_lcdc_chan
> > > > *ch) +static int __devinit
> > > > +sh_mobile_lcdc_check_interface(struct sh_mobile_lcdc_chan *ch)
> > >
> > > Personally, I don't like this type of line splitting very much, I
> > > prefer the grep output to show the function type and all attributes,
> > > but this, certainly, would not be the reason to change this specific
> > > hunk:-) But this might be: the whole file so far has all function
> > > definitions at least up to and including the first parameter on one
> > > line, so, I find, this change would introduce an inconsistency in the
> > > file style. The line would __only__ be 83 characters long if you put
> > > it all on one line. I think, it would look better then:-)
> >
> > It's a matter of limits, as usual... I find 80 to be a nice limit,
> > although in some cases I'm fine with exceeding it (one example is a long
> > list of #defines with a small comment describing each macro, as in the
> > list of FOURCCs in linux/videodev2.h for instance). For code I try to
> > respect the 80 characters limit. Another reason is that it produces
> > checkpatch.pl warnings, which force me to manually look at all the
> > warnings to check which ones are acceptable.
> >
> > Regarding consistency, other patches in this series use the same style,
> > so that function won't feel alone anymore ;-)
> >
> > I can change this (and other instances of the same coding style) if you
> > insist. BTW, a possible improvement would be to shorten the
> > sh_mobile_lcdc_ prefix. I find it too long, and I was thinking about
> > reducing it to shm_lcdc_ (although shm reffers to shared memory, so that
> > might not be the best idea).
>
> How about sh_lcdc?
Sounds good to me. Does anyone have an objection ?
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v4 REPOST 1/5] ARM: l2x0/pl310: Refactor Kconfig to be
From: Anton Vorontsov @ 2011-12-14 12:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1323862781-3465-2-git-send-email-dave.martin@linaro.org>
On Wed, Dec 14, 2011 at 11:39:37AM +0000, Dave Martin wrote:
> Making CACHE_L2X0 depend on (huge list of MACH_ and ARCH_ configs)
> is bothersome to maintain and likely to lead to merge conflicts.
>
> This patch moves the knowledge of which platforms have a L2x0 or
> PL310 cache controller to the individual machines. To enable this,
> a new MIGHT_HAVE_CACHE_L2X0 config option is introduced to allow
> machines to indicate that they may have such a cache controller
> independently of each other.
>
> Boards/SoCs which cannot reliably operate without the L2 cache
> controller support will need to select CACHE_L2X0 directly from
> their own Kconfigs instead. This applies to some TrustZone-enabled
> boards where Linux runs in the Normal World, for example.
>
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
For CNS3xxx bits:
Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
Thanks!
^ permalink raw reply
* [PATCH v4 REPOST 5/5] imx6q: Remove unconditional dependency on l2x0 L2 cache support
From: Dave Martin @ 2011-12-14 11:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1323862781-3465-1-git-send-email-dave.martin@linaro.org>
The i.MX6 Quad SoC will work without the l2x0 L2 cache controller
support built into the kernel, so this patch removes the dependency
on CACHE_L2X0 and selects MIGHT_HAVE_CACHE_L2X0 instead.
This makes the l2x0 support optional, so that it can be turned off
when desired for debugging purposes etc.
Thanks to Shawn Guo for this suggestion. [1]
Signed-off-by: Dave Martin <dave.martin@linaro.org>
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074602.html
---
arch/arm/mach-imx/Kconfig | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index 29a3d61..1fb93f2 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -609,13 +609,13 @@ comment "i.MX6 family:"
config SOC_IMX6Q
bool "i.MX6 Quad support"
select ARM_GIC
- select CACHE_L2X0
select CPU_V7
select HAVE_ARM_SCU
select HAVE_IMX_GPC
select HAVE_IMX_MMDC
select HAVE_IMX_SRC
select HAVE_SMP
+ select MIGHT_HAVE_CACHE_L2X0
select USE_OF
help
--
1.7.4.1
^ permalink raw reply related
* [PATCH v4 REPOST 4/5] highbank: Unconditionally require l2x0 L2 cache controller support
From: Dave Martin @ 2011-12-14 11:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1323862781-3465-1-git-send-email-dave.martin@linaro.org>
If running in the Normal World on a TrustZone-enabled SoC, Linux
does not have complete control over the L2 cache controller
configuration. The kernel cannot work reliably on such platforms
without the l2x0 cache support code built in.
This patch unconditionally enables l2x0 support for the Highbank
SoC.
Thanks to Rob Herring for this suggestion. [1]
Signed-off-by: Dave Martin <dave.martin@linaro.org>
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074495.html
---
arch/arm/Kconfig | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index d33eb39..744296d 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -340,12 +340,12 @@ config ARCH_HIGHBANK
select ARM_AMBA
select ARM_GIC
select ARM_TIMER_SP804
+ select CACHE_L2X0
select CLKDEV_LOOKUP
select CPU_V7
select GENERIC_CLOCKEVENTS
select HAVE_ARM_SCU
select HAVE_SMP
- select MIGHT_HAVE_CACHE_L2X0
select USE_OF
help
Support for the Calxeda Highbank SoC based boards.
--
1.7.4.1
^ permalink raw reply related
* [PATCH v4 REPOST 3/5] omap4: Unconditionally require l2x0 L2 cache controller support
From: Dave Martin @ 2011-12-14 11:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1323862781-3465-1-git-send-email-dave.martin@linaro.org>
If running in the Normal World on a TrustZone-enabled SoC, Linux
does not have complete control over the L2 cache controller
configuration. The kernel cannot work reliably on such platforms
without the l2x0 cache support code built in.
This patch unconditionally enables l2x0 support for the OMAP4 SoCs.
Thanks to Rob Herring for this suggestion. [1]
Signed-off-by: Dave Martin <dave.martin@linaro.org>
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074495.html
---
arch/arm/mach-omap2/Kconfig | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index bb1b670..94e568a 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -41,11 +41,11 @@ config ARCH_OMAP4
bool "TI OMAP4"
default y
depends on ARCH_OMAP2PLUS
+ select CACHE_L2X0
select CPU_V7
select ARM_GIC
select HAVE_SMP
select LOCAL_TIMERS if SMP
- select MIGHT_HAVE_CACHE_L2X0
select PL310_ERRATA_588369
select PL310_ERRATA_727915
select ARM_ERRATA_720789
--
1.7.4.1
^ permalink raw reply related
* [PATCH v4 REPOST 2/5] ARM: SMP: Refactor Kconfig to be more maintainable
From: Dave Martin @ 2011-12-14 11:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1323862781-3465-1-git-send-email-dave.martin@linaro.org>
Making SMP depend on (huge list of MACH_ and ARCH_ configs) is
bothersome to maintain and likely to lead to merge conflicts.
This patch moves the knowledge of which platforms are SMP-capable
to the individual machines. To enable this, a new HAVE_SMP config
option is introduced to allow machines to indicate that they can
run in a SMP configuration.
Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
arch/arm/Kconfig | 15 +++++++++++----
arch/arm/mach-exynos/Kconfig | 1 +
arch/arm/mach-imx/Kconfig | 1 +
arch/arm/mach-msm/Kconfig | 1 +
arch/arm/mach-omap2/Kconfig | 1 +
arch/arm/mach-realview/Kconfig | 4 ++++
arch/arm/mach-vexpress/Kconfig | 1 +
7 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 16a4b9e..d33eb39 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -344,6 +344,7 @@ config ARCH_HIGHBANK
select CPU_V7
select GENERIC_CLOCKEVENTS
select HAVE_ARM_SCU
+ select HAVE_SMP
select MIGHT_HAVE_CACHE_L2X0
select USE_OF
help
@@ -636,6 +637,7 @@ config ARCH_TEGRA
select GENERIC_GPIO
select HAVE_CLK
select HAVE_SCHED_CLOCK
+ select HAVE_SMP
select MIGHT_HAVE_CACHE_L2X0
select ARCH_HAS_CPUFREQ
help
@@ -706,6 +708,7 @@ config ARCH_SHMOBILE
select HAVE_CLK
select CLKDEV_LOOKUP
select HAVE_MACH_CLKDEV
+ select HAVE_SMP
select GENERIC_CLOCKEVENTS
select MIGHT_HAVE_CACHE_L2X0
select NO_IOPORT
@@ -909,6 +912,7 @@ config ARCH_U8500
select CLKDEV_LOOKUP
select ARCH_REQUIRE_GPIOLIB
select ARCH_HAS_CPUFREQ
+ select HAVE_SMP
select MIGHT_HAVE_CACHE_L2X0
help
Support for ST-Ericsson's Ux500 architecture
@@ -1430,14 +1434,17 @@ menu "Kernel Features"
source "kernel/time/Kconfig"
+config HAVE_SMP
+ bool
+ help
+ This option should be selected by machines which have an SMP-
+ capable CPU.
+
config SMP
bool "Symmetric Multi-Processing"
depends on CPU_V6K || CPU_V7
depends on GENERIC_CLOCKEVENTS
- depends on REALVIEW_EB_ARM11MP || REALVIEW_EB_A9MP || \
- MACH_REALVIEW_PB11MP || MACH_REALVIEW_PBX || ARCH_OMAP4 || \
- ARCH_EXYNOS4 || ARCH_TEGRA || ARCH_U8500 || ARCH_VEXPRESS_CA9X4 || \
- ARCH_MSM_SCORPIONMP || ARCH_SHMOBILE || ARCH_HIGHBANK || SOC_IMX6Q
+ depends on HAVE_SMP
depends on MMU
select USE_GENERIC_SMP_HELPERS
select HAVE_ARM_SCU if !ARCH_MSM_SCORPIONMP
diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
index 7f2347b..e1efbca 100644
--- a/arch/arm/mach-exynos/Kconfig
+++ b/arch/arm/mach-exynos/Kconfig
@@ -17,6 +17,7 @@ choice
config ARCH_EXYNOS4
bool "SAMSUNG EXYNOS4"
+ select HAVE_SMP
select MIGHT_HAVE_CACHE_L2X0
help
Samsung EXYNOS4 SoCs based systems
diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index 5f7f9c2..29a3d61 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -615,6 +615,7 @@ config SOC_IMX6Q
select HAVE_IMX_GPC
select HAVE_IMX_MMDC
select HAVE_IMX_SRC
+ select HAVE_SMP
select USE_OF
help
diff --git a/arch/arm/mach-msm/Kconfig b/arch/arm/mach-msm/Kconfig
index ebde97f..e6beaff 100644
--- a/arch/arm/mach-msm/Kconfig
+++ b/arch/arm/mach-msm/Kconfig
@@ -67,6 +67,7 @@ config MSM_SOC_REV_A
bool
config ARCH_MSM_SCORPIONMP
bool
+ select HAVE_SMP
config ARCH_MSM_ARM11
bool
diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index c841578..bb1b670 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -43,6 +43,7 @@ config ARCH_OMAP4
depends on ARCH_OMAP2PLUS
select CPU_V7
select ARM_GIC
+ select HAVE_SMP
select LOCAL_TIMERS if SMP
select MIGHT_HAVE_CACHE_L2X0
select PL310_ERRATA_588369
diff --git a/arch/arm/mach-realview/Kconfig b/arch/arm/mach-realview/Kconfig
index 3dd620f..c593be4 100644
--- a/arch/arm/mach-realview/Kconfig
+++ b/arch/arm/mach-realview/Kconfig
@@ -12,6 +12,7 @@ config REALVIEW_EB_A9MP
bool "Support Multicore Cortex-A9 Tile"
depends on MACH_REALVIEW_EB
select CPU_V7
+ select HAVE_SMP
select MIGHT_HAVE_CACHE_L2X0
help
Enable support for the Cortex-A9MPCore tile fitted to the
@@ -22,6 +23,7 @@ config REALVIEW_EB_ARM11MP
depends on MACH_REALVIEW_EB
select CPU_V6K
select ARCH_HAS_BARRIERS if SMP
+ select HAVE_SMP
select MIGHT_HAVE_CACHE_L2X0
help
Enable support for the ARM11MPCore tile fitted to the Realview(R)
@@ -41,6 +43,7 @@ config MACH_REALVIEW_PB11MP
select CPU_V6K
select ARM_GIC
select HAVE_PATA_PLATFORM
+ select HAVE_SMP
select MIGHT_HAVE_CACHE_L2X0
select ARCH_HAS_BARRIERS if SMP
help
@@ -82,6 +85,7 @@ config MACH_REALVIEW_PBX
bool "Support RealView(R) Platform Baseboard Explore"
select ARM_GIC
select HAVE_PATA_PLATFORM
+ select HAVE_SMP
select MIGHT_HAVE_CACHE_L2X0
select ARCH_SPARSEMEM_ENABLE if CPU_V7 && !REALVIEW_HIGH_PHYS_OFFSET
select ZONE_DMA if SPARSEMEM
diff --git a/arch/arm/mach-vexpress/Kconfig b/arch/arm/mach-vexpress/Kconfig
index a8aefc8..9b3d0fb 100644
--- a/arch/arm/mach-vexpress/Kconfig
+++ b/arch/arm/mach-vexpress/Kconfig
@@ -8,6 +8,7 @@ config ARCH_VEXPRESS_CA9X4
select ARM_ERRATA_720789
select ARM_ERRATA_751472
select ARM_ERRATA_753970
+ select HAVE_SMP
select MIGHT_HAVE_CACHE_L2X0
endmenu
--
1.7.4.1
^ permalink raw reply related
* [PATCH v4 REPOST 1/5] ARM: l2x0/pl310: Refactor Kconfig to be more maintainable
From: Dave Martin @ 2011-12-14 11:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1323862781-3465-1-git-send-email-dave.martin@linaro.org>
Making CACHE_L2X0 depend on (huge list of MACH_ and ARCH_ configs)
is bothersome to maintain and likely to lead to merge conflicts.
This patch moves the knowledge of which platforms have a L2x0 or
PL310 cache controller to the individual machines. To enable this,
a new MIGHT_HAVE_CACHE_L2X0 config option is introduced to allow
machines to indicate that they may have such a cache controller
independently of each other.
Boards/SoCs which cannot reliably operate without the L2 cache
controller support will need to select CACHE_L2X0 directly from
their own Kconfigs instead. This applies to some TrustZone-enabled
boards where Linux runs in the Normal World, for example.
Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
arch/arm/Kconfig | 8 ++++++++
arch/arm/mach-exynos/Kconfig | 1 +
arch/arm/mach-omap2/Kconfig | 1 +
arch/arm/mach-realview/Kconfig | 5 +++++
arch/arm/mach-vexpress/Kconfig | 1 +
arch/arm/mm/Kconfig | 15 ++++++++-------
arch/arm/plat-mxc/Kconfig | 1 +
7 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 44789ef..16a4b9e 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -344,6 +344,7 @@ config ARCH_HIGHBANK
select CPU_V7
select GENERIC_CLOCKEVENTS
select HAVE_ARM_SCU
+ select MIGHT_HAVE_CACHE_L2X0
select USE_OF
help
Support for the Calxeda Highbank SoC based boards.
@@ -361,6 +362,7 @@ config ARCH_CNS3XXX
select CPU_V6K
select GENERIC_CLOCKEVENTS
select ARM_GIC
+ select MIGHT_HAVE_CACHE_L2X0
select MIGHT_HAVE_PCI
select PCI_DOMAINS if PCI
help
@@ -381,6 +383,7 @@ config ARCH_PRIMA2
select GENERIC_CLOCKEVENTS
select CLKDEV_LOOKUP
select GENERIC_IRQ_CHIP
+ select MIGHT_HAVE_CACHE_L2X0
select USE_OF
select ZONE_DMA
help
@@ -633,6 +636,7 @@ config ARCH_TEGRA
select GENERIC_GPIO
select HAVE_CLK
select HAVE_SCHED_CLOCK
+ select MIGHT_HAVE_CACHE_L2X0
select ARCH_HAS_CPUFREQ
help
This enables support for NVIDIA Tegra based systems (Tegra APX,
@@ -703,6 +707,7 @@ config ARCH_SHMOBILE
select CLKDEV_LOOKUP
select HAVE_MACH_CLKDEV
select GENERIC_CLOCKEVENTS
+ select MIGHT_HAVE_CACHE_L2X0
select NO_IOPORT
select SPARSE_IRQ
select MULTI_IRQ_HANDLER
@@ -904,6 +909,7 @@ config ARCH_U8500
select CLKDEV_LOOKUP
select ARCH_REQUIRE_GPIOLIB
select ARCH_HAS_CPUFREQ
+ select MIGHT_HAVE_CACHE_L2X0
help
Support for ST-Ericsson's Ux500 architecture
@@ -914,6 +920,7 @@ config ARCH_NOMADIK
select CPU_ARM926T
select CLKDEV_LOOKUP
select GENERIC_CLOCKEVENTS
+ select MIGHT_HAVE_CACHE_L2X0
select ARCH_REQUIRE_GPIOLIB
help
Support for the Nomadik platform by ST-Ericsson
@@ -973,6 +980,7 @@ config ARCH_ZYNQ
select ARM_GIC
select ARM_AMBA
select ICST
+ select MIGHT_HAVE_CACHE_L2X0
select USE_OF
help
Support for Xilinx Zynq ARM Cortex A9 Platform
diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
index 724ec0f..7f2347b 100644
--- a/arch/arm/mach-exynos/Kconfig
+++ b/arch/arm/mach-exynos/Kconfig
@@ -17,6 +17,7 @@ choice
config ARCH_EXYNOS4
bool "SAMSUNG EXYNOS4"
+ select MIGHT_HAVE_CACHE_L2X0
help
Samsung EXYNOS4 SoCs based systems
diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index 5034147..c841578 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -44,6 +44,7 @@ config ARCH_OMAP4
select CPU_V7
select ARM_GIC
select LOCAL_TIMERS if SMP
+ select MIGHT_HAVE_CACHE_L2X0
select PL310_ERRATA_588369
select PL310_ERRATA_727915
select ARM_ERRATA_720789
diff --git a/arch/arm/mach-realview/Kconfig b/arch/arm/mach-realview/Kconfig
index dba6d0c..3dd620f 100644
--- a/arch/arm/mach-realview/Kconfig
+++ b/arch/arm/mach-realview/Kconfig
@@ -12,6 +12,7 @@ config REALVIEW_EB_A9MP
bool "Support Multicore Cortex-A9 Tile"
depends on MACH_REALVIEW_EB
select CPU_V7
+ select MIGHT_HAVE_CACHE_L2X0
help
Enable support for the Cortex-A9MPCore tile fitted to the
Realview(R) Emulation Baseboard platform.
@@ -21,6 +22,7 @@ config REALVIEW_EB_ARM11MP
depends on MACH_REALVIEW_EB
select CPU_V6K
select ARCH_HAS_BARRIERS if SMP
+ select MIGHT_HAVE_CACHE_L2X0
help
Enable support for the ARM11MPCore tile fitted to the Realview(R)
Emulation Baseboard platform.
@@ -39,6 +41,7 @@ config MACH_REALVIEW_PB11MP
select CPU_V6K
select ARM_GIC
select HAVE_PATA_PLATFORM
+ select MIGHT_HAVE_CACHE_L2X0
select ARCH_HAS_BARRIERS if SMP
help
Include support for the ARM(R) RealView(R) Platform Baseboard for
@@ -51,6 +54,7 @@ config MACH_REALVIEW_PB1176
select CPU_V6
select ARM_GIC
select HAVE_TCM
+ select MIGHT_HAVE_CACHE_L2X0
help
Include support for the ARM(R) RealView(R) Platform Baseboard for
ARM1176JZF-S.
@@ -78,6 +82,7 @@ config MACH_REALVIEW_PBX
bool "Support RealView(R) Platform Baseboard Explore"
select ARM_GIC
select HAVE_PATA_PLATFORM
+ select MIGHT_HAVE_CACHE_L2X0
select ARCH_SPARSEMEM_ENABLE if CPU_V7 && !REALVIEW_HIGH_PHYS_OFFSET
select ZONE_DMA if SPARSEMEM
help
diff --git a/arch/arm/mach-vexpress/Kconfig b/arch/arm/mach-vexpress/Kconfig
index 9311484..a8aefc8 100644
--- a/arch/arm/mach-vexpress/Kconfig
+++ b/arch/arm/mach-vexpress/Kconfig
@@ -8,5 +8,6 @@ config ARCH_VEXPRESS_CA9X4
select ARM_ERRATA_720789
select ARM_ERRATA_751472
select ARM_ERRATA_753970
+ select MIGHT_HAVE_CACHE_L2X0
endmenu
diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
index 67f75a0..d92aa3b 100644
--- a/arch/arm/mm/Kconfig
+++ b/arch/arm/mm/Kconfig
@@ -816,14 +816,15 @@ config CACHE_FEROCEON_L2_WRITETHROUGH
Say Y here to use the Feroceon L2 cache in writethrough mode.
Unless you specifically require this, say N for writeback mode.
+config MIGHT_HAVE_CACHE_L2X0
+ bool
+ help
+ This option should be selected by machines which have a L2x0
+ or PL310 cache controller.
+
config CACHE_L2X0
- bool "Enable the L2x0 outer cache controller"
- depends on REALVIEW_EB_ARM11MP || MACH_REALVIEW_PB11MP || MACH_REALVIEW_PB1176 || \
- REALVIEW_EB_A9MP || ARCH_IMX_V6_V7 || MACH_REALVIEW_PBX || \
- ARCH_NOMADIK || ARCH_OMAP4 || ARCH_EXYNOS4 || ARCH_TEGRA || \
- ARCH_U8500 || ARCH_VEXPRESS_CA9X4 || ARCH_SHMOBILE || \
- ARCH_PRIMA2 || ARCH_ZYNQ || ARCH_CNS3XXX || ARCH_HIGHBANK
- default y
+ bool "Enable the L2x0 outer cache controller" if MIGHT_HAVE_CACHE_L2X0
+ default MIGHT_HAVE_CACHE_L2X0
select OUTER_CACHE
select OUTER_CACHE_SYNC
help
diff --git a/arch/arm/plat-mxc/Kconfig b/arch/arm/plat-mxc/Kconfig
index b3a1f2b..b30708e 100644
--- a/arch/arm/plat-mxc/Kconfig
+++ b/arch/arm/plat-mxc/Kconfig
@@ -20,6 +20,7 @@ config ARCH_IMX_V6_V7
bool "i.MX3, i.MX6"
select AUTO_ZRELADDR if !ZBOOT_ROM
select ARM_PATCH_PHYS_VIRT
+ select MIGHT_HAVE_CACHE_L2X0
help
This enables support for systems based on the Freescale i.MX3 and i.MX6
family.
--
1.7.4.1
^ permalink raw reply related
* [PATCH v4 REPOST 0/5] Refactor common Kconfigs for easier maintenance
From: Dave Martin @ 2011-12-14 11:39 UTC (permalink / raw)
To: linux-arm-kernel
Common Kconfig options which depend on a long list of board- and
SoC- specific Kconfigs can be cumbersome to maintain, leading to
annoying merge conflicts (although rather trivial ones).
This series factors out the dependencies of CACHE_L2X0 and SMP so
that the knowledge about when these can be enabled is moved to the
relevant board/SoC Kconfig files instead. New
MIGHT_HAVE_CACHE_L2X0 and HAVE_SMP options are defined to mediate
the dependencies.
This series has been substantially reworked compared with the
previous post, and is now in two parts:
* The first two patches simply refactor the way the Kconfig
options for CACHE_L2X0 and SMP are implemented, without
making any other changes.
* The final three patches apply functional changes suggested by
the contributors to this series, to make the config
dependencies more correct for some specific boards.
Thanks to Rob Herring, Shawn Guo and Russell King for their
contributions to this series. Thanks also to David Brown for
pointing out the lack of a comprehensive CC list.
I have briefly build-tested on some of the affected boards, but any
further reviews or Tested-Bys would be much appreciated.
Dave Martin (5):
ARM: l2x0/pl310: Refactor Kconfig to be more maintainable
ARM: SMP: Refactor Kconfig to be more maintainable
omap4: Unconditionally require l2x0 L2 cache controller support
highbank: Unconditionally require l2x0 L2 cache controller support
imx6q: Remove unconditional dependency on l2x0 L2 cache support
arch/arm/Kconfig | 23 +++++++++++++++++++----
arch/arm/mach-exynos/Kconfig | 2 ++
arch/arm/mach-imx/Kconfig | 3 ++-
arch/arm/mach-msm/Kconfig | 1 +
arch/arm/mach-omap2/Kconfig | 2 ++
arch/arm/mach-realview/Kconfig | 9 +++++++++
arch/arm/mach-vexpress/Kconfig | 2 ++
arch/arm/mm/Kconfig | 15 ++++++++-------
arch/arm/plat-mxc/Kconfig | 1 +
9 files changed, 46 insertions(+), 12 deletions(-)
--
1.7.4.1
^ permalink raw reply
* Re: [PATCH 33/57] fbdev: sh_mobile_lcdc: Add sh_mobile_format_info() function
From: Laurent Pinchart @ 2011-12-14 11:30 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-34-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Damian,
On Wednesday 14 December 2011 04:04:52 Damian Hobson-Garcia wrote:
> On 2011/12/13 23:02, Laurent Pinchart wrote:
> > --- a/drivers/video/sh_mobile_lcdcfb.c
> > +++ b/drivers/video/sh_mobile_lcdcfb.c
> > @@ -447,6 +447,75 @@ static int sh_mobile_lcdc_display_notify(struct
> > sh_mobile_lcdc_chan *ch,
> >
> > * Format helpers
> > */
> >
> > +struct sh_mobile_lcdc_format_info {
> > + u32 fourcc;
> > + unsigned int bpp;
> > + bool yuv;
> > + u32 lddfr;
> > +};
> > +
> > +static const struct sh_mobile_lcdc_format_info sh_mobile_format_infos[]
> > = { + {
> > + .fourcc = V4L2_PIX_FMT_RGB565,
> > + .bpp = 12,
>
> I think that this should be 16 instead of 12.
Oops, you're right. Thanks. I'll fix that.
> > @@ -665,37 +726,20 @@ static void __sh_mobile_lcdc_start(struct
> > sh_mobile_lcdc_priv *priv)
> >
> > /* Setup geometry, format, frame buffer memory and operation mode. */
> > for (k = 0; k < ARRAY_SIZE(priv->ch); k++) {
> >
> > + const struct sh_mobile_lcdc_format_info *format;
> > + u32 fourcc;
> > +
> >
> > ch = &priv->ch[k];
> > if (!ch->enabled)
> >
> > continue;
> >
> > sh_mobile_lcdc_geometry(ch);
> >
> > - switch (sh_mobile_format_fourcc(&ch->info->var)) {
> > - case V4L2_PIX_FMT_RGB565:
> > - tmp = LDDFR_PKF_RGB16;
> > - break;
> > - case V4L2_PIX_FMT_BGR24:
> > - tmp = LDDFR_PKF_RGB24;
> > - break;
> > - case V4L2_PIX_FMT_BGR32:
> > - tmp = LDDFR_PKF_ARGB32;
> > - break;
> > - case V4L2_PIX_FMT_NV12:
> > - case V4L2_PIX_FMT_NV21:
> > - tmp = LDDFR_CC | LDDFR_YF_420;
> > - break;
> > - case V4L2_PIX_FMT_NV16:
> > - case V4L2_PIX_FMT_NV61:
> > - tmp = LDDFR_CC | LDDFR_YF_422;
> > - break;
> > - case V4L2_PIX_FMT_NV24:
> > - case V4L2_PIX_FMT_NV42:
> > - tmp = LDDFR_CC | LDDFR_YF_444;
> > - break;
> > - }
> > + fourcc = sh_mobile_format_fourcc(&ch->info->var);
> > + format = sh_mobile_format_info(fourcc);
> > + tmp = format->lddfr;
>
> Do you need to check if format is NULL here?
The fourcc is validated in both sh_mobile_lcdc_channel_init() (for the value
passed to the driver through platform data) and sh_mobile_check_var() (for the
value requested by userspace). format should thus never be NULL here.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is
From: Ulf Hansson @ 2011-12-14 11:12 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Guennadi Liakhovetski, linux-mmc@vger.kernel.org,
linux-pm@vger.kernel.org, Chris Ball, linux-sh@vger.kernel.org
In-Reply-To: <201112141115.00945.rjw@sisk.pl>
>> You have a point. But I am not convinced. :-)
>>
>> Some host drivers already make use of autosuspend. I think this is most
>> straightforward solution to this problem right now.
>
> The problem is not about _when_ to suspend (which autosuspend is about),
> but _what_ _state_ to go when suspended. That's quite a different issue.
I was kind of taking a more simple approach, I were considering
runtime_suspend as _one_ state. Right now there is no host driver having
different levels of runtime_suspend state, if I am correct. But ofcourse
that could be the future.
Moreover, I definitely do not think that a fixed timeout of 100us is
applicable for all use cases, this must at least be configurable.
>
>> However, we could also do pm_runtime_get_sync of the host device in
>> claim host and vice verse in release host, thus preventing the host
>> driver from triggering runtime_suspend|resume for every request. Though,
>> I am not 100% sure this is really what you want either.
>
> No, I don't want that. I want the device to be suspended when possible,
> but I don't want that to cause the system to go into an overly deep power
> state as a result.
Before just skipping my proposal, I think you should know some more
background to why I suggested this:
1.
mmc_claim_host is using mmc_host_enable, which kind of mean the same
thing for a host driver as doing get_sync. Vice verse for mmc_release_host.
2.
When executing mmc/sd commands/requests the host must always be claimed
(and thus the host is always enabled). But more important some mmc/sd
commands must be executed as a sequence, without the host being disabled
in between the commands (since a disable might mean that the clock to
card gets disabled). To solve this, the mmc_claim_host is used, to make
sure the host is enabled during the complete command sequence.
I happily continue this discussion, to see if someone more can break the
idea about having get_sync in mmc_host_enable. :-)
>
>> Using PM QoS as you propose, might prevent some hosts from doing
>> runtime_suspend|resume completely and thus those might not fulfill power
>> consumption requirements instead.
>
> I'm not sure what scenario you have in mind. Care to elaborate?
Well, suppose a the host drivers start considering the PM QoS
requirement, but never can fulfill the requirement of 100us for it's
runtime_suspend callback function...
>
>> I do not think we can take this decision at this level. Is performance more
>> important than power save, that is kind of the question.
>
> Yes, it is. Also, the number used here is somewhat arbitrary.
For you maybe power management is less important, but I doubt everybody
else agree to that. It is a more complex balance I believe.
>
> However, since no one except for SH7372 is now using device PM QoS, no one
> else will be affected by this change at the moment.
True, but that is not a good reason for adding more stuff to the mmc core.
>
> Thanks,
> Rafael
>
By the way, I like to discuss with you so please take no offense from my
comments. :-)
BR
Ulf Hansson
^ permalink raw reply
* Re: [PATCH 11/57] fbdev: sh_mobile_lcdc: Handle HDMI/MIPI transmitter device directly
From: Laurent Pinchart @ 2011-12-14 11:10 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-12-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Guennadi,
On Wednesday 14 December 2011 01:03:06 Guennadi Liakhovetski wrote:
> On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > Pass a pointer to the transmitter device through platform data, retrieve
> > the corresponding sh_mobile_lcdc_entity structure in the probe method
> > and call the transmitter display_on/off methods directly.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >
> > drivers/video/sh_mobile_lcdcfb.c | 33
> > ++++++++++++++++++++++++++++----- drivers/video/sh_mobile_lcdcfb.h |
> > 2 ++
> > include/video/sh_mobile_lcdc.h | 2 ++
> > 3 files changed, 32 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/video/sh_mobile_lcdcfb.c
> > b/drivers/video/sh_mobile_lcdcfb.c index afa1fac..f1bbae6 100644
> > --- a/drivers/video/sh_mobile_lcdcfb.c
> > +++ b/drivers/video/sh_mobile_lcdcfb.c
> > @@ -338,6 +338,13 @@ static void sh_mobile_lcdc_deferred_io_touch(struct
> > fb_info *info)
> >
> > static void sh_mobile_lcdc_display_on(struct sh_mobile_lcdc_chan *ch)
> > {
> >
> > struct sh_mobile_lcdc_board_cfg *board_cfg = &ch->cfg.board_cfg;
> >
> > + int ret;
> > +
> > + if (ch->tx_dev) {
> > + ret = ch->tx_dev->ops->display_on(ch->tx_dev, ch->info);
>
> ->ops or ->display_o{n,ff}() cannot be NULL?
They must be provided, at least with the transmitters we currently have. This
code will likely get reworked to support things like HDMI-on-DSI, so I don't
think adding a pre
> also
>
> + int ret = ch->tx_dev->ops->display_on(ch->tx_dev, ch->info);
>
> would suffice;-)
Or even if (ch->tx_dev->ops->display_on(ch->tx_dev, ch->info) < 0)
I'll fix that.
> > + if (ret < 0)
> > + return;
> > + }
> >
> > /* HDMI must be enabled before LCDC configuration */
> > if (board_cfg->display_on && try_module_get(board_cfg->owner)) {
> >
> > @@ -354,6 +361,9 @@ static void sh_mobile_lcdc_display_off(struct
> > sh_mobile_lcdc_chan *ch)
> >
> > board_cfg->display_off(board_cfg->board_data);
> > module_put(board_cfg->owner);
> >
> > }
> >
> > +
> > + if (ch->tx_dev)
> > + ch->tx_dev->ops->display_off(ch->tx_dev);
> >
> > }
> >
> > /*
> > -----------------------------------------------------------------------
> > ------
> >
> > @@ -1490,18 +1500,21 @@ static int sh_mobile_lcdc_remove(struct
> > platform_device *pdev)
> >
> > sh_mobile_lcdc_stop(priv);
> >
> > for (i = 0; i < ARRAY_SIZE(priv->ch); i++) {
> >
> > - info = priv->ch[i].info;
> > + struct sh_mobile_lcdc_chan *ch = &priv->ch[i];
> >
> > + info = ch->info;
> >
> > if (!info || !info->device)
> >
> > continue;
> >
> > - if (priv->ch[i].sglist)
> > - vfree(priv->ch[i].sglist);
> > + if (ch->tx_dev)
> > + module_put(ch->cfg.tx_dev->dev.driver->owner);
>
> This is now the same ->owner, as the one taken in your
> sh_mobile_lcdc_display_o{n,ff}() functions? IIUC, you're now adding this
> new ->owner field to later (17/57) remove the original one?
It's the same owner, yes. The idea is to get a reference to the transmitter
device module at probe() time and release it at remove() time instead of doing
so in display_on() and display_off(), and to remove the owner field hack from
platform data.
> > +
> > + if (ch->sglist)
> > + vfree(ch->sglist);
> >
> > if (info->screen_base)
> >
> > dma_free_coherent(&pdev->dev, info->fix.smem_len,
> >
> > - info->screen_base,
> > - priv->ch[i].dma_handle);
> > + info->screen_base, ch->dma_handle);
> >
> > fb_dealloc_cmap(&info->cmap);
> > framebuffer_release(info);
> >
> > }
> >
> > @@ -1596,6 +1609,16 @@ sh_mobile_lcdc_channel_init(struct
> > sh_mobile_lcdc_priv *priv,
> >
> > info->pseudo_palette = &ch->pseudo_palette;
> > info->flags = FBINFO_FLAG_DEFAULT;
> >
> > + if (cfg->tx_dev) {
> > + if (!cfg->tx_dev->dev.driver ||
> > + !try_module_get(cfg->tx_dev->dev.driver->owner)) {
> > + dev_warn(priv->dev, "unable to get transmitter "
> > + "device\n");
>
> Grrr... Pleeeease, don't split strings. If you really have to program on
> vt220;-) at least do
>
> + dev_warn(priv->dev,
> + "unable to get transmitter device\n");
Sorry about that. Will fix.
> > + return -EINVAL;
> > + }
> > + ch->tx_dev = platform_get_drvdata(cfg->tx_dev);
> > + }
> > +
> >
> > /* Iterate through the modes to validate them and find the highest
> >
> > * resolution.
> > */
> >
> > diff --git a/drivers/video/sh_mobile_lcdcfb.h
> > b/drivers/video/sh_mobile_lcdcfb.h index d79e5aa..9601b92 100644
> > --- a/drivers/video/sh_mobile_lcdcfb.h
> > +++ b/drivers/video/sh_mobile_lcdcfb.h
> > @@ -41,6 +41,8 @@ struct sh_mobile_lcdc_entity {
> >
> > */
> >
> > struct sh_mobile_lcdc_chan {
> >
> > struct sh_mobile_lcdc_priv *lcdc;
> >
> > + struct sh_mobile_lcdc_entity *tx_dev;
> > +
> >
> > unsigned long *reg_offs;
> > unsigned long ldmt1r_value;
> > unsigned long enabled; /* ME and SE in LDCNT2R */
> >
> > diff --git a/include/video/sh_mobile_lcdc.h
> > b/include/video/sh_mobile_lcdc.h index fe30b75..0ec59e1 100644
> > --- a/include/video/sh_mobile_lcdc.h
> > +++ b/include/video/sh_mobile_lcdc.h
> > @@ -186,6 +186,8 @@ struct sh_mobile_lcdc_chan_cfg {
> >
> > struct sh_mobile_lcdc_bl_info bl_info;
> > struct sh_mobile_lcdc_sys_bus_cfg sys_bus_cfg; /* only for SYSn I/F */
> > struct sh_mobile_meram_cfg *meram_cfg;
> >
> > +
> > + struct platform_device *tx_dev; /* HDMI/MIPI transmitter device */
>
> "MIPI" is too generic, IMHO
Will HDMI/DSI do ?
> Hm, could we, maybe, have different names for sh_mobile_lcdc_chan::tx_dev
> and sh_mobile_lcdc_chan_cfg::tx_dev?;-)
Sure. Do you have any suggestion ? :-)
> > };
> >
> > struct sh_mobile_lcdc_info {
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 02/57] fbdev: sh_mobile_lcdc: Mark init-only symbols with
From: Guennadi Liakhovetski @ 2011-12-14 11:07 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-3-git-send-email-laurent.pinchart@ideasonboard.com>
On Wed, 14 Dec 2011, Laurent Pinchart wrote:
> Hi Guennadi,
>
> On Tuesday 13 December 2011 23:23:32 Guennadi Liakhovetski wrote:
> > On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > > default_720p and sh_mobile_lcdc_check_interface are used at device
> > > initialization time only. Mark them as __devinitconst and __devinit
> > > respectively.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >
> > > drivers/video/sh_mobile_lcdcfb.c | 5 +++--
> > > 1 files changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/video/sh_mobile_lcdcfb.c
> > > b/drivers/video/sh_mobile_lcdcfb.c index 8b18360..a6bf4fb 100644
> > > --- a/drivers/video/sh_mobile_lcdcfb.c
> > > +++ b/drivers/video/sh_mobile_lcdcfb.c
> > > @@ -1459,7 +1459,7 @@ static int sh_mobile_lcdc_notify(struct
> > > notifier_block *nb,
> > >
> > > * Probe/remove and driver init/exit
> > > */
> > >
> > > -static const struct fb_videomode default_720p = {
> > > +static const struct fb_videomode default_720p __devinitconst = {
> > >
> > > .name = "HDMI 720p",
> > > .xres = 1280,
> > > .yres = 720,
> > >
> > > @@ -1528,7 +1528,8 @@ static int sh_mobile_lcdc_remove(struct
> > > platform_device *pdev)
> > >
> > > return 0;
> > >
> > > }
> > >
> > > -static int sh_mobile_lcdc_check_interface(struct sh_mobile_lcdc_chan
> > > *ch) +static int __devinit
> > > +sh_mobile_lcdc_check_interface(struct sh_mobile_lcdc_chan *ch)
> >
> > Personally, I don't like this type of line splitting very much, I prefer
> > the grep output to show the function type and all attributes, but this,
> > certainly, would not be the reason to change this specific hunk:-) But
> > this might be: the whole file so far has all function definitions at least
> > up to and including the first parameter on one line, so, I find, this
> > change would introduce an inconsistency in the file style. The line would
> > __only__ be 83 characters long if you put it all on one line. I think, it
> > would look better then:-)
>
> It's a matter of limits, as usual... I find 80 to be a nice limit, although in
> some cases I'm fine with exceeding it (one example is a long list of #defines
> with a small comment describing each macro, as in the list of FOURCCs in
> linux/videodev2.h for instance). For code I try to respect the 80 characters
> limit. Another reason is that it produces checkpatch.pl warnings, which force
> me to manually look at all the warnings to check which ones are acceptable.
>
> Regarding consistency, other patches in this series use the same style, so
> that function won't feel alone anymore ;-)
>
> I can change this (and other instances of the same coding style) if you
> insist. BTW, a possible improvement would be to shorten the sh_mobile_lcdc_
> prefix. I find it too long, and I was thinking about reducing it to shm_lcdc_
> (although shm reffers to shared memory, so that might not be the best idea).
How about sh_lcdc?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 06/57] fbdev: sh_mobile_hdmi: Don't access LCDC channel in notifier callback
From: Laurent Pinchart @ 2011-12-14 10:47 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-7-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Guennadi,
On Wednesday 14 December 2011 00:02:11 Guennadi Liakhovetski wrote:
> On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > Instead of relying on info->par being a pointer to an LCDC channel, cast
> > the notifier block pointer to an sh_hdmi pointer.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >
> > drivers/video/sh_mobile_hdmi.c | 6 +++---
> > 1 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/video/sh_mobile_hdmi.c
> > b/drivers/video/sh_mobile_hdmi.c index 647ba98..b1e90f5 100644
> > --- a/drivers/video/sh_mobile_hdmi.c
> > +++ b/drivers/video/sh_mobile_hdmi.c
> > @@ -225,6 +225,8 @@ struct sh_hdmi {
> >
> > struct notifier_block notifier;
> >
> > };
> >
> > +#define notifier_to_hdmi(n) container_of(n, struct sh_hdmi, notifier)
> > +
> >
> > static void hdmi_write(struct sh_hdmi *hdmi, u8 data, u8 reg)
> > {
> >
> > iowrite8(data, hdmi->base + reg);
> >
> > @@ -1204,9 +1206,7 @@ static int sh_hdmi_notify(struct notifier_block
> > *nb,
> >
> > {
> >
> > struct fb_event *event = data;
> > struct fb_info *info = event->info;
> >
> > - struct sh_mobile_lcdc_chan *ch = info->par;
> > - struct sh_mobile_lcdc_board_cfg *board_cfg = &ch->cfg.board_cfg;
> > - struct sh_hdmi *hdmi = board_cfg->board_data;
> > + struct sh_hdmi *hdmi = notifier_to_hdmi(nb);
> >
> > if (!hdmi || nb != &hdmi->notifier || hdmi->info != info)
>
> Then you also can drop the first two of the three checks above. If I'm not
> mistaken, in a HDMI / LCD set up, if this notifier is called for the LCD
> panel, hdmi will wtill point to the HDMI-related sh_hdmi object, whose
> ->info will then mismatch the info pointer, derived from the notifier
> "data" pointer.
That's correct. The first two checks don't hurt, but will always be false.
I'll remove them.
> > return NOTIFY_DONE;
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 06/57] fbdev: sh_mobile_hdmi: Don't access LCDC channel in notifier callback
From: Laurent Pinchart @ 2011-12-14 10:41 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-7-git-send-email-laurent.pinchart@ideasonboard.com>
On Wednesday 14 December 2011 00:02:11 Guennadi Liakhovetski wrote:
> On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > Instead of relying on info->par being a pointer to an LCDC channel, cast
> > the notifier block pointer to an sh_hdmi pointer.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >
> > drivers/video/sh_mobile_hdmi.c | 6 +++---
> > 1 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/video/sh_mobile_hdmi.c
> > b/drivers/video/sh_mobile_hdmi.c index 647ba98..b1e90f5 100644
> > --- a/drivers/video/sh_mobile_hdmi.c
> > +++ b/drivers/video/sh_mobile_hdmi.c
> > @@ -225,6 +225,8 @@ struct sh_hdmi {
> >
> > struct notifier_block notifier;
> >
> > };
> >
> > +#define notifier_to_hdmi(n) container_of(n, struct sh_hdmi, notifier)
> > +
> >
> > static void hdmi_write(struct sh_hdmi *hdmi, u8 data, u8 reg)
> > {
> >
> > iowrite8(data, hdmi->base + reg);
> >
> > @@ -1204,9 +1206,7 @@ static int sh_hdmi_notify(struct notifier_block
> > *nb,
> >
> > {
> >
> > struct fb_event *event = data;
> > struct fb_info *info = event->info;
> >
> > - struct sh_mobile_lcdc_chan *ch = info->par;
> > - struct sh_mobile_lcdc_board_cfg *board_cfg = &ch->cfg.board_cfg;
> > - struct sh_hdmi *hdmi = board_cfg->board_data;
> > + struct sh_hdmi *hdmi = notifier_to_hdmi(nb);
> >
> > if (!hdmi || nb != &hdmi->notifier || hdmi->info != info)
>
> Then you also can drop the first two of the three checks above. If I'm not
> mistaken, in a HDMI / LCD set up, if this notifier is called for the LCD
> panel, hdmi will wtill point to the HDMI-related sh_hdmi object, whose
> ->info will then mismatch the info pointer, derived from the notifier
> "data" pointer.
>
> > return NOTIFY_DONE;
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 02/57] fbdev: sh_mobile_lcdc: Mark init-only symbols with __devinit(const)
From: Laurent Pinchart @ 2011-12-14 10:40 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-3-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Guennadi,
On Tuesday 13 December 2011 23:23:32 Guennadi Liakhovetski wrote:
> On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > default_720p and sh_mobile_lcdc_check_interface are used at device
> > initialization time only. Mark them as __devinitconst and __devinit
> > respectively.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >
> > drivers/video/sh_mobile_lcdcfb.c | 5 +++--
> > 1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/video/sh_mobile_lcdcfb.c
> > b/drivers/video/sh_mobile_lcdcfb.c index 8b18360..a6bf4fb 100644
> > --- a/drivers/video/sh_mobile_lcdcfb.c
> > +++ b/drivers/video/sh_mobile_lcdcfb.c
> > @@ -1459,7 +1459,7 @@ static int sh_mobile_lcdc_notify(struct
> > notifier_block *nb,
> >
> > * Probe/remove and driver init/exit
> > */
> >
> > -static const struct fb_videomode default_720p = {
> > +static const struct fb_videomode default_720p __devinitconst = {
> >
> > .name = "HDMI 720p",
> > .xres = 1280,
> > .yres = 720,
> >
> > @@ -1528,7 +1528,8 @@ static int sh_mobile_lcdc_remove(struct
> > platform_device *pdev)
> >
> > return 0;
> >
> > }
> >
> > -static int sh_mobile_lcdc_check_interface(struct sh_mobile_lcdc_chan
> > *ch) +static int __devinit
> > +sh_mobile_lcdc_check_interface(struct sh_mobile_lcdc_chan *ch)
>
> Personally, I don't like this type of line splitting very much, I prefer
> the grep output to show the function type and all attributes, but this,
> certainly, would not be the reason to change this specific hunk:-) But
> this might be: the whole file so far has all function definitions at least
> up to and including the first parameter on one line, so, I find, this
> change would introduce an inconsistency in the file style. The line would
> __only__ be 83 characters long if you put it all on one line. I think, it
> would look better then:-)
It's a matter of limits, as usual... I find 80 to be a nice limit, although in
some cases I'm fine with exceeding it (one example is a long list of #defines
with a small comment describing each macro, as in the list of FOURCCs in
linux/videodev2.h for instance). For code I try to respect the 80 characters
limit. Another reason is that it produces checkpatch.pl warnings, which force
me to manually look at all the warnings to check which ones are acceptable.
Regarding consistency, other patches in this series use the same style, so
that function won't feel alone anymore ;-)
I can change this (and other instances of the same coding style) if you
insist. BTW, a possible improvement would be to shorten the sh_mobile_lcdc_
prefix. I find it too long, and I was thinking about reducing it to shm_lcdc_
(although shm reffers to shared memory, so that might not be the best idea).
> > {
> >
> > int interface_type = ch->cfg.interface_type;
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed
From: Rafael J. Wysocki @ 2011-12-14 10:28 UTC (permalink / raw)
To: Linus Walleij
Cc: Ulf Hansson, Guennadi Liakhovetski, linux-mmc@vger.kernel.org,
linux-pm@vger.kernel.org, Chris Ball, linux-sh@vger.kernel.org
In-Reply-To: <CACRpkdZnmCUfk0h+wK=KUvrv5oCudEAdEc_TY6=yZg_3uTqkgw@mail.gmail.com>
On Wednesday, December 14, 2011, Linus Walleij wrote:
> On Wed, Dec 14, 2011 at 10:00 AM, Ulf Hansson
> <ulf.hansson@stericsson.com> wrote:
> > Guennadi Liakhovetski wrote:
>
> > Using PM QoS as you propose, might prevent some hosts from doing
> > runtime_suspend|resume completely and thus those might not fulfill power
> > consumption requirements instead. I do not think we can take this decision
> > at this level. Is performance more important than power save, that is kind
> > of the question.
>
> I agree with this point. The problematic part of the patch (IMHO) is this:
>
> >> + This constraint prevents runtime-suspending the
> >> + device, if the expected wakeup latency is larger than 100us.
> (...)
> >> + int ret = dev_pm_qos_add_request(host->parent,
> >> + &host->pm_qos, 100);
>
> So we hardcode 100us (is that really 100us by the way? I cannot
> follow this code path but usually these figures are in ms, but what
> do I know) as the in-between back-to-back transfers.
They are in microseconds.
> But this delta is dependent on a lot of stuff that only the platform
> knows, like nominal CPU frequency, bus speed etc, so certainly the
> platform must be able to modify that number.
You seem to be confusing things. The exact meaning of this number is:
"I may want to use the device 100 us from now (but not earlier), so please
make it possible to do that". [It roughly means "don't put the device into
a low-power state that takes more than 100 us to resume from", but it's a bit
more complicated than that.] It doesn't mean "don't suspend the device for
the next 100 us".
> At the very least, please make this stuff optional using Kconfig
> so it can be shut off, because I fear it will screw up our PM usecases.
I'm not sure how that's possible, at least until your platform starts
to use device PM QoS.
> Ulfs patch to the mmci driver actually use 50ms for back-to-back
> intergap between any two hardware-affecting calls into the driver.
Which is kind of independent.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is first claimed
From: Rafael J. Wysocki @ 2011-12-14 10:15 UTC (permalink / raw)
To: Ulf Hansson
Cc: Guennadi Liakhovetski, linux-mmc@vger.kernel.org,
linux-pm@vger.kernel.org, Chris Ball, linux-sh@vger.kernel.org
In-Reply-To: <4EE865CA.8000407@stericsson.com>
On Wednesday, December 14, 2011, Ulf Hansson wrote:
> Guennadi Liakhovetski wrote:
> > Hi Ulf
> >
> > On Tue, 13 Dec 2011, Ulf Hansson wrote:
> >
> >> Guennadi Liakhovetski wrote:
> >>> Some MMC hosts implement a fine-grained runtime PM, whereby they
> >>> runtime-suspend and -resume the host interface on each transfer. This can
> >>> negatively affect performance, if the user was trying to transfer data
> >>> blocks back-to-back. This patch adds a PM QoS constraint to avoid such a
> >>> throughput reduction. This constraint prevents runtime-suspending the
> >>> device, if the expected wakeup latency is larger than 100us.
> >>>
> >>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >> I think host drivers can use autosuspend with some ms delay for this instead.
> >> This will mean that requests coming in bursts will not be affected (well only
> >> the first request in the burst will suffer from the runtime resume latency).
> >
> > I think, Rafael is the best person to explain, why exactly this is not
> > desired. In short, this is the wrong location to make such decisions and
> > to define these criteria. The only thing, that the driver may be aware of
> > is how quickly it wants to be able to wake up, if it got suspended. And
> > it's already the PM subsystem, that has to decide, whether it can satisfy
> > this requirement or not. Rafael will correct me, if my explanation is
> > wrong.
>
> You have a point. But I am not convinced. :-)
>
> Some host drivers already make use of autosuspend. I think this is most
> straightforward solution to this problem right now.
The problem is not about _when_ to suspend (which autosuspend is about),
but _what_ _state_ to go when suspended. That's quite a different issue.
> However, we could also do pm_runtime_get_sync of the host device in
> claim host and vice verse in release host, thus preventing the host
> driver from triggering runtime_suspend|resume for every request. Though,
> I am not 100% sure this is really what you want either.
No, I don't want that. I want the device to be suspended when possible,
but I don't want that to cause the system to go into an overly deep power
state as a result.
> Using PM QoS as you propose, might prevent some hosts from doing
> runtime_suspend|resume completely and thus those might not fulfill power
> consumption requirements instead.
I'm not sure what scenario you have in mind. Care to elaborate?
> I do not think we can take this decision at this level. Is performance more
> important than power save, that is kind of the question.
Yes, it is. Also, the number used here is somewhat arbitrary.
However, since no one except for SH7372 is now using device PM QoS, no one
else will be affected by this change at the moment.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is
From: Linus Walleij @ 2011-12-14 9:27 UTC (permalink / raw)
To: Ulf Hansson, Guennadi Liakhovetski
Cc: linux-mmc@vger.kernel.org, linux-pm@vger.kernel.org, Chris Ball,
linux-sh@vger.kernel.org, Rafael J. Wysocki
In-Reply-To: <4EE865CA.8000407@stericsson.com>
On Wed, Dec 14, 2011 at 10:00 AM, Ulf Hansson
<ulf.hansson@stericsson.com> wrote:
> Guennadi Liakhovetski wrote:
> Using PM QoS as you propose, might prevent some hosts from doing
> runtime_suspend|resume completely and thus those might not fulfill power
> consumption requirements instead. I do not think we can take this decision
> at this level. Is performance more important than power save, that is kind
> of the question.
I agree with this point. The problematic part of the patch (IMHO) is this:
>> + This constraint prevents runtime-suspending the
>> + device, if the expected wakeup latency is larger than 100us.
(...)
>> + int ret = dev_pm_qos_add_request(host->parent,
>> + &host->pm_qos, 100);
So we hardcode 100us (is that really 100us by the way? I cannot
follow this code path but usually these figures are in ms, but what
do I know) as the in-between back-to-back transfers.
But this delta is dependent on a lot of stuff that only the platform
knows, like nominal CPU frequency, bus speed etc, so certainly the
platform must be able to modify that number.
At the very least, please make this stuff optional using Kconfig
so it can be shut off, because I fear it will screw up our PM usecases.
Ulfs patch to the mmci driver actually use 50ms for back-to-back
intergap between any two hardware-affecting calls into the driver.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH/RFC] mmc: add a device PM QoS constraint when a host is
From: Ulf Hansson @ 2011-12-14 9:00 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-mmc@vger.kernel.org, linux-pm@vger.kernel.org, Chris Ball,
linux-sh@vger.kernel.org, Rafael J. Wysocki
In-Reply-To: <Pine.LNX.4.64.1112131658330.20293@axis700.grange>
Guennadi Liakhovetski wrote:
> Hi Ulf
>
> On Tue, 13 Dec 2011, Ulf Hansson wrote:
>
>> Guennadi Liakhovetski wrote:
>>> Some MMC hosts implement a fine-grained runtime PM, whereby they
>>> runtime-suspend and -resume the host interface on each transfer. This can
>>> negatively affect performance, if the user was trying to transfer data
>>> blocks back-to-back. This patch adds a PM QoS constraint to avoid such a
>>> throughput reduction. This constraint prevents runtime-suspending the
>>> device, if the expected wakeup latency is larger than 100us.
>>>
>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> I think host drivers can use autosuspend with some ms delay for this instead.
>> This will mean that requests coming in bursts will not be affected (well only
>> the first request in the burst will suffer from the runtime resume latency).
>
> I think, Rafael is the best person to explain, why exactly this is not
> desired. In short, this is the wrong location to make such decisions and
> to define these criteria. The only thing, that the driver may be aware of
> is how quickly it wants to be able to wake up, if it got suspended. And
> it's already the PM subsystem, that has to decide, whether it can satisfy
> this requirement or not. Rafael will correct me, if my explanation is
> wrong.
You have a point. But I am not convinced. :-)
Some host drivers already make use of autosuspend. I think this is most
straightforward solution to this problem right now.
However, we could also do pm_runtime_get_sync of the host device in
claim host and vice verse in release host, thus preventing the host
driver from triggering runtime_suspend|resume for every request. Though,
I am not 100% sure this is really what you want either.
Using PM QoS as you propose, might prevent some hosts from doing
runtime_suspend|resume completely and thus those might not fulfill power
consumption requirements instead. I do not think we can take this
decision at this level. Is performance more important than power save,
that is kind of the question.
>
>> I believe that runtime resume callback should ofcourse be optimized so they
>> are executed as fast as possible. But moreover, if they take more 100us, is
>> that really a reason for not executing them at all?
>
> I think it is a reason not to execute them during an intensive IO, yes. I
> cannot imagine a case, where if you have multiple IO requests waiting in
> the queue to your medium, you would want to switch off and immediately on
> again. Well, of course, such situations might exist, but then you just
> have to define and use a different governor on your system. This is also
> the flexibility, that this API is giving you.
I totally agree.
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
>
Br
Ulf Hansson
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox