* simple framebuffer slower by factor of 20, on socfpga (arm) platform
From: Pavel Machek @ 2015-04-07 12:12 UTC (permalink / raw)
To: Marek Vasut, kernel list, dinh.linux
Cc: plagnioj, tomi.valkeinen, grant.likely, robh+dt, jg1.han,
robdclark, geert, linux-fbdev, devicetree
Hi!
I have an socfpga board, which uses has simple framebuffer implemented
in the FPGA. On 3.15, framebuffer is fast:
root@wagabuibui:~# time cat /dev/fb0 > /dev/null
real 0m 0.00s
user 0m 0.00s
sys 0m 0.00s
on 3.18, this takes 220msec. Similar slowdown exists for
writes. Simple framebuffer did not change at all between 3.15 and
3.18; resource flags of the framebuffer are still same (0x200).
If I enable caching on 3.18, it speeds up a bit, to 70msec or
so... Which means problem is not only in caching.
Any ideas?
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH] Staging: sm750fb: Remove extra parentheses around right bit shift operation
From: Dan Carpenter @ 2015-04-07 8:00 UTC (permalink / raw)
To: Amitoj Kaur Chawla
Cc: sudipm.mukherjee, teddy.wang, gregkh, linux-fbdev, devel,
linux-kernel
In-Reply-To: <20150405014147.GA9738@amitoj-Inspiron-3542>
Some people like to put parenthesis around stuff. It's harmless here.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] Staging: sm750fb: Remove extra parentheses around right bit shift operation
From: Sudip Mukherjee @ 2015-04-06 9:30 UTC (permalink / raw)
To: Amitoj Kaur Chawla; +Cc: teddy.wang, gregkh, linux-fbdev, devel, linux-kernel
In-Reply-To: <20150405014147.GA9738@amitoj-Inspiron-3542>
On Sun, Apr 05, 2015 at 07:11:47AM +0530, Amitoj Kaur Chawla wrote:
> Removes extra parentheses around bitwise right shift operation.
this is not applying. please work against staging-testing.
regards
sudip
>
^ permalink raw reply
* [PATCH] Staging: sm750fb: Remove extra parentheses around right bit shift operation
From: Amitoj Kaur Chawla @ 2015-04-05 1:53 UTC (permalink / raw)
To: sudipm.mukherjee, teddy.wang, gregkh, linux-fbdev, devel,
linux-kernel
Removes extra parentheses around bitwise right shift operation.
The issue was detected using the following coccinelle script.
@@
expression e, e1;
constant c;
@@
e -(e1
+e1
<<
-c);
+c;
@@
identifier i;
constant c;
type t;
expression e;
@@
t i -(e
+e
<<
-c);
+c;
@@
expression e, e1;
identifier f;
constant c;
@@
e1 = f(...,
-(e
+e
<<
-c)
+c
,...);
Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
---
drivers/staging/sm750fb/sm750.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 4074375..626b841 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -737,7 +737,7 @@ static int sm750fb_set_drv(struct lynxfb_par * par)
output->paths = sm750_crt;
crtc->channel = sm750_secondary;
/* not consider of padding stuffs for oScreen,need fix*/
- crtc->oScreen = (share->vidmem_size >> 1);
+ crtc->oScreen = share->vidmem_size >> 1;
crtc->vScreen = share->pvMem + crtc->oScreen;
}
break;
@@ -751,7 +751,7 @@ static int sm750fb_set_drv(struct lynxfb_par * par)
output->paths = sm750_crt;
crtc->channel = sm750_primary;
/* not consider of padding stuffs for oScreen,need fix*/
- crtc->oScreen = (share->vidmem_size >> 1);
+ crtc->oScreen = share->vidmem_size >> 1;
crtc->vScreen = share->pvMem + crtc->oScreen;
}
break;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] staging: sm7xxfb: Fix sparse warning
From: Giedrius Statkevičius @ 2015-04-04 19:31 UTC (permalink / raw)
To: Nickolaus Woodruff
Cc: sudipmmukherjee, teddywang, gregkh, devel, linux-fbdev,
linux-kernel
In-Reply-To: <20150404165120.GA3111@Caladan>
On Sat, 4 Apr 2015, Nickolaus Woodruff wrote:
> This patch fixes the following sparse warning in sm7xx.h:
>
> drivers/staging/sm7xxfb/sm7xx.h:122:17: warning: symbol 'vgamode'
> was not declared. Should it be static?
>
> Signed-off-by: Nickolaus Woodruff <nickolauswoodruff@gmail.com>
> ---
> drivers/staging/sm7xxfb/sm7xx.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/sm7xxfb/sm7xx.h b/drivers/staging/sm7xxfb/sm7xx.h
> index 7cc1896..c5d6253 100644
> --- a/drivers/staging/sm7xxfb/sm7xx.h
> +++ b/drivers/staging/sm7xxfb/sm7xx.h
> @@ -119,7 +119,7 @@ struct ModeInit {
> /**********************************************************************
> SM712 Mode table.
> **********************************************************************/
> -struct ModeInit vgamode[] = {
> +static struct ModeInit vgamode[] = {
Someone already fixed this:
https://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/sm7xxfb/sm7xx.h?h=staging-testing#n122
Please work against staging-testing branch of the staging tree.
Su pagarba / Regards,
Giedrius
^ permalink raw reply
* [PATCH] staging: sm7xxfb: Fix sparse warning
From: Nickolaus Woodruff @ 2015-04-04 19:22 UTC (permalink / raw)
To: sudipmmukherjee, teddywang, gregkh; +Cc: linux-fbdev, devel, linux-kernel
This patch fixes the following sparse warning in sm7xx.h:
drivers/staging/sm7xxfb/sm7xx.h:122:17: warning: symbol 'vgamode'
was not declared. Should it be static?
Signed-off-by: Nickolaus Woodruff <nickolauswoodruff@gmail.com>
---
drivers/staging/sm7xxfb/sm7xx.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/sm7xxfb/sm7xx.h b/drivers/staging/sm7xxfb/sm7xx.h
index 7cc1896..c5d6253 100644
--- a/drivers/staging/sm7xxfb/sm7xx.h
+++ b/drivers/staging/sm7xxfb/sm7xx.h
@@ -119,7 +119,7 @@ struct ModeInit {
/**********************************************************************
SM712 Mode table.
**********************************************************************/
-struct ModeInit vgamode[] = {
+static struct ModeInit vgamode[] = {
{
/* mode#0: 640 x 480 16Bpp 60Hz */
640, 480, 16, 60,
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] fbdev: sh_mobile_lcdc: Fix destruction of uninitialized mutex
From: Laurent Pinchart @ 2015-04-04 12:38 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1428065428-25473-1-git-send-email-geert+renesas@glider.be>
Hi Geert,
Thank you for the patch.
On Friday 03 April 2015 14:50:28 Geert Uytterhoeven wrote:
> If sh_mobile_lcdc_probe() fails after the allocation of driver-private
> data, but before the initialization of all channels, a warning will be
> printed due to the destruction of an uninitialized mutex:
>
> WARNING: CPU: 0 PID: 1 at kernel/locking/mutex-debug.c:116
> mutex_destroy+0x5c/0x7c() DEBUG_LOCKS_WARN_ON(mutex_is_locked(lock))
> ...
> Backtrace:
> ...
> [<c00425b4>] (mutex_destroy) from [<c01d5858>]
> (sh_mobile_lcdc_remove+0x1bc/0x230) r4:df6a4800 r3:00000000
> [<c01d569c>] (sh_mobile_lcdc_remove) from [<c01d6620>]
> (sh_mobile_lcdc_probe+0xd54/0xe28)
>
> Move the initialization of the mutexes from sh_mobile_lcdc_channel_init()
> to immediately after the allocation of driver-private data to fix this.
>
> Note that the interrupt number is moved to a new variable "irq", so we
> can reuse the existing variable "i" for iterating over the channels.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tomi, could you please pick this up ?
> ---
> drivers/video/fbdev/sh_mobile_lcdcfb.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c
> b/drivers/video/fbdev/sh_mobile_lcdcfb.c index
> d3013cd9f9769663..97141da154c663b8 100644
> --- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
> +++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
> @@ -2605,7 +2605,6 @@ sh_mobile_lcdc_channel_init(struct sh_mobile_lcdc_chan
> *ch) unsigned int max_size;
> unsigned int i;
>
> - mutex_init(&ch->open_lock);
> ch->notify = sh_mobile_lcdc_display_notify;
>
> /* Validate the format. */
> @@ -2704,7 +2703,7 @@ static int sh_mobile_lcdc_probe(struct platform_device
> *pdev) struct resource *res;
> int num_channels;
> int error;
> - int i;
> + int irq, i;
>
> if (!pdata) {
> dev_err(&pdev->dev, "no platform data defined\n");
> @@ -2712,8 +2711,8 @@ static int sh_mobile_lcdc_probe(struct platform_device
> *pdev) }
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - i = platform_get_irq(pdev, 0);
> - if (!res || i < 0) {
> + irq = platform_get_irq(pdev, 0);
> + if (!res || irq < 0) {
> dev_err(&pdev->dev, "cannot get platform resources\n");
> return -ENOENT;
> }
> @@ -2726,16 +2725,18 @@ static int sh_mobile_lcdc_probe(struct
> platform_device *pdev)
>
> priv->dev = &pdev->dev;
> priv->meram_dev = pdata->meram_dev;
> + for (i = 0; i < ARRAY_SIZE(priv->ch); i++)
> + mutex_init(&priv->ch[i].open_lock);
> platform_set_drvdata(pdev, priv);
>
> - error = request_irq(i, sh_mobile_lcdc_irq, 0,
> + error = request_irq(irq, sh_mobile_lcdc_irq, 0,
> dev_name(&pdev->dev), priv);
> if (error) {
> dev_err(&pdev->dev, "unable to request irq\n");
> goto err1;
> }
>
> - priv->irq = i;
> + priv->irq = irq;
> atomic_set(&priv->hw_usecnt, -1);
>
> for (i = 0, num_channels = 0; i < ARRAY_SIZE(pdata->ch); i++) {
--
Regards,
Laurent Pinchart
^ permalink raw reply
* [PATCH] fbdev: sh_mobile_lcdc: Fix destruction of uninitialized mutex
From: Geert Uytterhoeven @ 2015-04-03 12:50 UTC (permalink / raw)
To: linux-fbdev
If sh_mobile_lcdc_probe() fails after the allocation of driver-private
data, but before the initialization of all channels, a warning will be
printed due to the destruction of an uninitialized mutex:
WARNING: CPU: 0 PID: 1 at kernel/locking/mutex-debug.c:116 mutex_destroy+0x5c/0x7c()
DEBUG_LOCKS_WARN_ON(mutex_is_locked(lock))
...
Backtrace:
...
[<c00425b4>] (mutex_destroy) from [<c01d5858>] (sh_mobile_lcdc_remove+0x1bc/0x230)
r4:df6a4800 r3:00000000
[<c01d569c>] (sh_mobile_lcdc_remove) from [<c01d6620>] (sh_mobile_lcdc_probe+0xd54/0xe28)
Move the initialization of the mutexes from sh_mobile_lcdc_channel_init()
to immediately after the allocation of driver-private data to fix this.
Note that the interrupt number is moved to a new variable "irq", so we
can reuse the existing variable "i" for iterating over the channels.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
drivers/video/fbdev/sh_mobile_lcdcfb.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
index d3013cd9f9769663..97141da154c663b8 100644
--- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
+++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
@@ -2605,7 +2605,6 @@ sh_mobile_lcdc_channel_init(struct sh_mobile_lcdc_chan *ch)
unsigned int max_size;
unsigned int i;
- mutex_init(&ch->open_lock);
ch->notify = sh_mobile_lcdc_display_notify;
/* Validate the format. */
@@ -2704,7 +2703,7 @@ static int sh_mobile_lcdc_probe(struct platform_device *pdev)
struct resource *res;
int num_channels;
int error;
- int i;
+ int irq, i;
if (!pdata) {
dev_err(&pdev->dev, "no platform data defined\n");
@@ -2712,8 +2711,8 @@ static int sh_mobile_lcdc_probe(struct platform_device *pdev)
}
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- i = platform_get_irq(pdev, 0);
- if (!res || i < 0) {
+ irq = platform_get_irq(pdev, 0);
+ if (!res || irq < 0) {
dev_err(&pdev->dev, "cannot get platform resources\n");
return -ENOENT;
}
@@ -2726,16 +2725,18 @@ static int sh_mobile_lcdc_probe(struct platform_device *pdev)
priv->dev = &pdev->dev;
priv->meram_dev = pdata->meram_dev;
+ for (i = 0; i < ARRAY_SIZE(priv->ch); i++)
+ mutex_init(&priv->ch[i].open_lock);
platform_set_drvdata(pdev, priv);
- error = request_irq(i, sh_mobile_lcdc_irq, 0,
+ error = request_irq(irq, sh_mobile_lcdc_irq, 0,
dev_name(&pdev->dev), priv);
if (error) {
dev_err(&pdev->dev, "unable to request irq\n");
goto err1;
}
- priv->irq = i;
+ priv->irq = irq;
atomic_set(&priv->hw_usecnt, -1);
for (i = 0, num_channels = 0; i < ARRAY_SIZE(pdata->ch); i++) {
--
1.9.1
^ permalink raw reply related
* Re: Questions about simplefb and hand-over
From: Hans de Goede @ 2015-04-03 9:02 UTC (permalink / raw)
To: Johannes Pointner, linux-fbdev-u79uwXL29TY76Z2rM5mHXA, devicetree,
David Herrmann
In-Reply-To: <CAJKbBDCCU5zBAFLf_KH-nYLzJYbAU9S9z6Jfe8F3w5SY+-ZCuQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi,
On 03-04-15 10:57, Johannes Pointner wrote:
> Hello all,
>
> I'd like to use the simplefb in combination with tilcdc drm driver
> with a TI AM335x processor. I got the u-boot to simplefb part working,
> but I'm not sure how to do the hand-over part.
>
> Is it yet possible to do that? Does the tilcdc drm driver need
> additional functionality to support that? Or are there other things I
> have to take into account?
>
> Any suggestions would be highly appreciated!
There is a remove_conflicting_framebuffers call which is already used
on x86 to remove e.g. efifb when a kms driver loads, the plan for
simplefb / arm is to do the same. That is where we stand now, we've
a plan but you're the first person to actually try this.
Note that you may want to use simpledrm by David Herrmann (added to the
Cc) instead of simplefb as that will give you a kms driver right from
the start, simpledrm is not yet upstream but should go upstream soon.
Regards,
Hans
^ permalink raw reply
* Questions about simplefb and hand-over
From: Johannes Pointner @ 2015-04-03 8:57 UTC (permalink / raw)
To: linux-fbdev-u79uwXL29TY76Z2rM5mHXA, Hans de Goede, devicetree
Hello all,
I'd like to use the simplefb in combination with tilcdc drm driver
with a TI AM335x processor. I got the u-boot to simplefb part working,
but I'm not sure how to do the hand-over part.
Is it yet possible to do that? Does the tilcdc drm driver need
additional functionality to support that? Or are there other things I
have to take into account?
Any suggestions would be highly appreciated!
Thanks in Advance,
Hannes
^ permalink raw reply
* [PATCH v4] video: mxsfb: Make sure axi clock is enabled when accessing registers
From: Liu Ying @ 2015-04-03 4:51 UTC (permalink / raw)
To: linux-fbdev
Cc: Peter Chen, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Fabio Estevam, Greg Kroah-Hartman, linux-kernel, stable
The LCDIF engines embedded in i.MX6sl and i.MX6sx SoCs need the axi clock
as the engine's system clock. The clock should be enabled when accessing
LCDIF registers, otherwise the kernel would hang up. We should also keep
the clock enabled when the engine is being active to scan out frames from
memory. This patch makes sure the axi clock is enabled when accessing
registers so that the kernel hang up issue can be fixed.
Reported-by: Peter Chen <peter.chen@freescale.com>
Tested-by: Peter Chen <peter.chen@freescale.com>
Cc: <stable@vger.kernel.org> # 3.19+
Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
---
v3->v4:
* To address Tomi's comment, enable/disable the axi clock in
mxsfb_pan_display() directly instead of checking the host->enabled flag.
v2->v3:
* To address Tomi's comment, improve the commit message only.
v1->v2:
* Add 'Tested-by: Peter Chen <peter.chen@freescale.com>' tag.
* Add 'Cc: <stable@vger.kernel.org> # 3.19+' tag.
drivers/video/fbdev/mxsfb.c | 68 +++++++++++++++++++++++++++++++++++----------
1 file changed, 54 insertions(+), 14 deletions(-)
diff --git a/drivers/video/fbdev/mxsfb.c b/drivers/video/fbdev/mxsfb.c
index f8ac4a4..0f64165 100644
--- a/drivers/video/fbdev/mxsfb.c
+++ b/drivers/video/fbdev/mxsfb.c
@@ -316,6 +316,18 @@ static int mxsfb_check_var(struct fb_var_screeninfo *var,
return 0;
}
+static inline void mxsfb_enable_axi_clk(struct mxsfb_info *host)
+{
+ if (host->clk_axi)
+ clk_prepare_enable(host->clk_axi);
+}
+
+static inline void mxsfb_disable_axi_clk(struct mxsfb_info *host)
+{
+ if (host->clk_axi)
+ clk_disable_unprepare(host->clk_axi);
+}
+
static void mxsfb_enable_controller(struct fb_info *fb_info)
{
struct mxsfb_info *host = to_imxfb_host(fb_info);
@@ -333,14 +345,13 @@ static void mxsfb_enable_controller(struct fb_info *fb_info)
}
}
- if (host->clk_axi)
- clk_prepare_enable(host->clk_axi);
-
if (host->clk_disp_axi)
clk_prepare_enable(host->clk_disp_axi);
clk_prepare_enable(host->clk);
clk_set_rate(host->clk, PICOS2KHZ(fb_info->var.pixclock) * 1000U);
+ mxsfb_enable_axi_clk(host);
+
/* if it was disabled, re-enable the mode again */
writel(CTRL_DOTCLK_MODE, host->base + LCDC_CTRL + REG_SET);
@@ -380,11 +391,11 @@ static void mxsfb_disable_controller(struct fb_info *fb_info)
reg = readl(host->base + LCDC_VDCTRL4);
writel(reg & ~VDCTRL4_SYNC_SIGNALS_ON, host->base + LCDC_VDCTRL4);
+ mxsfb_disable_axi_clk(host);
+
clk_disable_unprepare(host->clk);
if (host->clk_disp_axi)
clk_disable_unprepare(host->clk_disp_axi);
- if (host->clk_axi)
- clk_disable_unprepare(host->clk_axi);
host->enabled = 0;
@@ -421,6 +432,8 @@ static int mxsfb_set_par(struct fb_info *fb_info)
mxsfb_disable_controller(fb_info);
}
+ mxsfb_enable_axi_clk(host);
+
/* clear the FIFOs */
writel(CTRL1_FIFO_CLEAR, host->base + LCDC_CTRL1 + REG_SET);
@@ -438,6 +451,7 @@ static int mxsfb_set_par(struct fb_info *fb_info)
ctrl |= CTRL_SET_WORD_LENGTH(3);
switch (host->ld_intf_width) {
case STMLCDIF_8BIT:
+ mxsfb_disable_axi_clk(host);
dev_err(&host->pdev->dev,
"Unsupported LCD bus width mapping\n");
return -EINVAL;
@@ -451,6 +465,7 @@ static int mxsfb_set_par(struct fb_info *fb_info)
writel(CTRL1_SET_BYTE_PACKAGING(0x7), host->base + LCDC_CTRL1);
break;
default:
+ mxsfb_disable_axi_clk(host);
dev_err(&host->pdev->dev, "Unhandled color depth of %u\n",
fb_info->var.bits_per_pixel);
return -EINVAL;
@@ -504,6 +519,8 @@ static int mxsfb_set_par(struct fb_info *fb_info)
fb_info->fix.line_length * fb_info->var.yoffset,
host->base + host->devdata->next_buf);
+ mxsfb_disable_axi_clk(host);
+
if (reenable)
mxsfb_enable_controller(fb_info);
@@ -582,10 +599,14 @@ static int mxsfb_pan_display(struct fb_var_screeninfo *var,
offset = fb_info->fix.line_length * var->yoffset;
+ mxsfb_enable_axi_clk(host);
+
/* update on next VSYNC */
writel(fb_info->fix.smem_start + offset,
host->base + host->devdata->next_buf);
+ mxsfb_disable_axi_clk(host);
+
return 0;
}
@@ -608,13 +629,17 @@ static int mxsfb_restore_mode(struct mxsfb_info *host,
unsigned line_count;
unsigned period;
unsigned long pa, fbsize;
- int bits_per_pixel, ofs;
+ int bits_per_pixel, ofs, ret = 0;
u32 transfer_count, vdctrl0, vdctrl2, vdctrl3, vdctrl4, ctrl;
+ mxsfb_enable_axi_clk(host);
+
/* Only restore the mode when the controller is running */
ctrl = readl(host->base + LCDC_CTRL);
- if (!(ctrl & CTRL_RUN))
- return -EINVAL;
+ if (!(ctrl & CTRL_RUN)) {
+ ret = -EINVAL;
+ goto err;
+ }
vdctrl0 = readl(host->base + LCDC_VDCTRL0);
vdctrl2 = readl(host->base + LCDC_VDCTRL2);
@@ -635,7 +660,8 @@ static int mxsfb_restore_mode(struct mxsfb_info *host,
break;
case 1:
default:
- return -EINVAL;
+ ret = -EINVAL;
+ goto err;
}
fb_info->var.bits_per_pixel = bits_per_pixel;
@@ -673,10 +699,14 @@ static int mxsfb_restore_mode(struct mxsfb_info *host,
pa = readl(host->base + host->devdata->cur_buf);
fbsize = fb_info->fix.line_length * vmode->yres;
- if (pa < fb_info->fix.smem_start)
- return -EINVAL;
- if (pa + fbsize > fb_info->fix.smem_start + fb_info->fix.smem_len)
- return -EINVAL;
+ if (pa < fb_info->fix.smem_start) {
+ ret = -EINVAL;
+ goto err;
+ }
+ if (pa + fbsize > fb_info->fix.smem_start + fb_info->fix.smem_len) {
+ ret = -EINVAL;
+ goto err;
+ }
ofs = pa - fb_info->fix.smem_start;
if (ofs) {
memmove(fb_info->screen_base, fb_info->screen_base + ofs, fbsize);
@@ -689,7 +719,11 @@ static int mxsfb_restore_mode(struct mxsfb_info *host,
clk_prepare_enable(host->clk);
host->enabled = 1;
- return 0;
+err:
+ if (ret)
+ mxsfb_disable_axi_clk(host);
+
+ return ret;
}
static int mxsfb_init_fbinfo_dt(struct mxsfb_info *host,
@@ -915,7 +949,9 @@ static int mxsfb_probe(struct platform_device *pdev)
}
if (!host->enabled) {
+ mxsfb_enable_axi_clk(host);
writel(0, host->base + LCDC_CTRL);
+ mxsfb_disable_axi_clk(host);
mxsfb_set_par(fb_info);
mxsfb_enable_controller(fb_info);
}
@@ -954,11 +990,15 @@ static void mxsfb_shutdown(struct platform_device *pdev)
struct fb_info *fb_info = platform_get_drvdata(pdev);
struct mxsfb_info *host = to_imxfb_host(fb_info);
+ mxsfb_enable_axi_clk(host);
+
/*
* Force stop the LCD controller as keeping it running during reboot
* might interfere with the BootROM's boot mode pads sampling.
*/
writel(CTRL_RUN, host->base + LCDC_CTRL + REG_CLR);
+
+ mxsfb_disable_axi_clk(host);
}
static struct platform_driver mxsfb_driver = {
--
2.1.0
^ permalink raw reply related
* Re: [PATCH v2] video: mxsfb: Make sure axi clock is enabled when accessing registers
From: Liu Ying @ 2015-04-03 3:39 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Liu Ying, linux-fbdev, Peter Chen,
Jean-Christophe Plagniol-Villard, Fabio Estevam,
Greg Kroah-Hartman, linux-kernel, stable
In-Reply-To: <550C03D3.7090504@ti.com>
2015-03-20 19:26 GMT+08:00 Tomi Valkeinen <tomi.valkeinen@ti.com>:
> On 11/03/15 05:03, Liu Ying wrote:
>
>>> Why do you check for host->enabled here, but not elsewhere?
>>
>> We need this check here to make sure the axi clock reference count is no greater
>> than 1. Looking at the context of mxsfb_set_par(), mxsfb_restore_mode() and
>
> Why is that? The clock framework handles ref counting for you. All the
> driver needs to take care of is to call as many times disable as it
> calls enable.
Okay. I'll remove the check and send another version.
Regards,
Liu Ying
>
> Tomi
>
>
^ permalink raw reply
* Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
From: Luis R. Rodriguez @ 2015-04-03 1:08 UTC (permalink / raw)
To: Toshi Kani
Cc: Luis R. Rodriguez, luto, mingo, tglx, hpa, jgross, JBeulich, bp,
suresh.b.siddha, venkatesh.pallipadi, airlied, linux-kernel,
linux-fbdev, x86, xen-devel, Ingo Molnar, Daniel Vetter,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Dave Hansen, Stefan Bader, konrad.wilk, ville.syrjala,
david.vrabel, bhelgaas, Roger Pau Monné, xen-devel
In-Reply-To: <1428018736.31093.102.camel@misato.fc.hp.com>
On Thu, Apr 02, 2015 at 05:52:16PM -0600, Toshi Kani wrote:
> On Thu, 2015-04-02 at 23:49 +0200, Luis R. Rodriguez wrote:
> > On Sat, Mar 28, 2015 at 12:56:30AM +0100, Luis R. Rodriguez wrote:
> > > On Fri, Mar 27, 2015 at 02:40:17PM -0600, Toshi Kani wrote:
> > > > On Fri, 2015-03-20 at 16:17 -0700, Luis R. Rodriguez wrote:
> > > > :
> > > > > @@ -734,6 +742,7 @@ void __init mtrr_bp_init(void)
> > > > > }
> > > > >
> > > > > if (mtrr_if) {
> > > > > + mtrr_enabled = true;
> > > > > set_num_var_ranges();
> > > > > init_table();
> > > > > if (use_intel()) {
> > > > get_mtrr_state();
> > > >
> > > > After setting mtrr_enabled to true, get_mtrr_state() reads
> > > > MSR_MTRRdefType and sets 'mtrr_state.enabled', which also indicates if
> > > > MTRRs are enabled or not on the system. So, potentially, we could have
> > > > a case that mtrr_enabled is set to true, but mtrr_state.enabled is set
> > > > to disabled when MTRRs are disabled by BIOS.
> > >
> > > Thanks for the review, in this case then we should update mtrr_enabled to false.
> > >
> > > > ps.
> > > > I recently cleaned up this part of the MTRR code in the patch below,
> > > > which is currently available in the -mm & -next trees.
> > > > https://lkml.org/lkml/2015/3/24/1063
> > >
> > > Great I will rebase and work with that and try to address this
> > > consideration you have raised.
> >
> > OK I'll mesh in this change as well in my next respin:
> >
> > diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> > index a83f27a..ecf7cb9 100644
> > --- a/arch/x86/kernel/cpu/mtrr/generic.c
> > +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> > @@ -438,7 +438,7 @@ static void __init print_mtrr_state(void)
> > }
> >
> > /* Grab all of the MTRR state for this CPU into *state */
> > -void __init get_mtrr_state(void)
> > +bool __init get_mtrr_state(void)
> > {
> > struct mtrr_var_range *vrs;
> > unsigned long flags;
> > @@ -482,6 +482,8 @@ void __init get_mtrr_state(void)
> >
> > post_set();
> > local_irq_restore(flags);
> > +
> > + return !!mtrr_state.enabled;
>
> This should be:
> return mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED;
>
> because the MTRR_STATE_MTRR_FIXED_ENABLED flag is ignored when the
> MTRR_STATE_MTRR_ENABLED flag is clear.
Thanks, I've used
return !!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED);
Amended.
Luis
^ permalink raw reply
* Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
From: Toshi Kani @ 2015-04-02 23:52 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Luis R. Rodriguez, luto, mingo, tglx, hpa, jgross, JBeulich, bp,
suresh.b.siddha, venkatesh.pallipadi, airlied, linux-kernel,
linux-fbdev, x86, xen-devel, Ingo Molnar, Daniel Vetter,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Dave Hansen, Stefan Bader, konrad.wilk, ville.syrjala,
david.vrabel, bhelgaas, Roger Pau Monné, xen-devel
In-Reply-To: <20150402214912.GZ5622@wotan.suse.de>
On Thu, 2015-04-02 at 23:49 +0200, Luis R. Rodriguez wrote:
> On Sat, Mar 28, 2015 at 12:56:30AM +0100, Luis R. Rodriguez wrote:
> > On Fri, Mar 27, 2015 at 02:40:17PM -0600, Toshi Kani wrote:
> > > On Fri, 2015-03-20 at 16:17 -0700, Luis R. Rodriguez wrote:
> > > :
> > > > @@ -734,6 +742,7 @@ void __init mtrr_bp_init(void)
> > > > }
> > > >
> > > > if (mtrr_if) {
> > > > + mtrr_enabled = true;
> > > > set_num_var_ranges();
> > > > init_table();
> > > > if (use_intel()) {
> > > get_mtrr_state();
> > >
> > > After setting mtrr_enabled to true, get_mtrr_state() reads
> > > MSR_MTRRdefType and sets 'mtrr_state.enabled', which also indicates if
> > > MTRRs are enabled or not on the system. So, potentially, we could have
> > > a case that mtrr_enabled is set to true, but mtrr_state.enabled is set
> > > to disabled when MTRRs are disabled by BIOS.
> >
> > Thanks for the review, in this case then we should update mtrr_enabled to false.
> >
> > > ps.
> > > I recently cleaned up this part of the MTRR code in the patch below,
> > > which is currently available in the -mm & -next trees.
> > > https://lkml.org/lkml/2015/3/24/1063
> >
> > Great I will rebase and work with that and try to address this
> > consideration you have raised.
>
> OK I'll mesh in this change as well in my next respin:
>
> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> index a83f27a..ecf7cb9 100644
> --- a/arch/x86/kernel/cpu/mtrr/generic.c
> +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> @@ -438,7 +438,7 @@ static void __init print_mtrr_state(void)
> }
>
> /* Grab all of the MTRR state for this CPU into *state */
> -void __init get_mtrr_state(void)
> +bool __init get_mtrr_state(void)
> {
> struct mtrr_var_range *vrs;
> unsigned long flags;
> @@ -482,6 +482,8 @@ void __init get_mtrr_state(void)
>
> post_set();
> local_irq_restore(flags);
> +
> + return !!mtrr_state.enabled;
This should be:
return mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED;
because the MTRR_STATE_MTRR_FIXED_ENABLED flag is ignored when the
MTRR_STATE_MTRR_ENABLED flag is clear.
Thanks,
-Toshi
^ permalink raw reply
* Re: [PATCH v1 06/47] mtrr: add __arch_phys_wc_add()
From: Luis R. Rodriguez @ 2015-04-02 22:54 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Andy Lutomirski, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
Juergen Gross, Jan Beulich, Borislav Petkov, Dave Airlie,
linux-kernel@vger.kernel.org, linux-fbdev, x86@kernel.org,
xen-devel@lists.xenproject.org, Ingo Molnar, Daniel Vetter,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen
In-Reply-To: <CAErSpo5mBtLuMcRJBYuxXqGBJRkqzQDQiimVgeLk2k+ztyCCCw@mail.gmail.com>
On Thu, Apr 2, 2015 at 3:35 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [-cc Venkatesh, Suresh]
>
> On Thu, Apr 2, 2015 at 3:55 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> On Thu, Apr 02, 2015 at 03:21:22PM -0500, Bjorn Helgaas wrote:
>>> On Fri, Mar 20, 2015 at 6:17 PM, Luis R. Rodriguez
>>> <mcgrof@do-not-panic.com> wrote:
>>> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
>>> > This is *only* a transitive API -- and as such no new
>>> > drivers are ever expected to use this.
>>>
>>> "transient"? Do you mean you intend to remove this API in the near future?
>>
>> That's correct, the problem is that in order to use PAT cleanly we'd need to
>> change these drivers ...
>
> I was just trying to ask whether you intended to write "transient"
> instead of "transitive." But I'm not doing a very good job :)
Yes, corrected, thanks.
Luis
^ permalink raw reply
* Re: [PATCH v1 06/47] mtrr: add __arch_phys_wc_add()
From: Bjorn Helgaas @ 2015-04-02 22:35 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Luis R. Rodriguez, Andy Lutomirski, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, Juergen Gross, Jan Beulich, Borislav Petkov,
Dave Airlie, linux-kernel@vger.kernel.org, linux-fbdev,
x86@kernel.org, xen-devel@lists.xenproject.org, Ingo Molnar,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
In-Reply-To: <20150402205520.GX5622@wotan.suse.de>
[-cc Venkatesh, Suresh]
On Thu, Apr 2, 2015 at 3:55 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Thu, Apr 02, 2015 at 03:21:22PM -0500, Bjorn Helgaas wrote:
>> On Fri, Mar 20, 2015 at 6:17 PM, Luis R. Rodriguez
>> <mcgrof@do-not-panic.com> wrote:
>> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
>> > This is *only* a transitive API -- and as such no new
>> > drivers are ever expected to use this.
>>
>> "transient"? Do you mean you intend to remove this API in the near future?
>
> That's correct, the problem is that in order to use PAT cleanly we'd need to
> change these drivers ...
I was just trying to ask whether you intended to write "transient"
instead of "transitive." But I'm not doing a very good job :)
Bjorn
^ permalink raw reply
* Re: [Xen-devel] [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
From: Luis R. Rodriguez @ 2015-04-02 22:12 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-fbdev, Daniel Vetter, Dave Hansen, Jan Beulich,
H. Peter Anvin, Ville Syrjälä, xen-devel, Suresh Siddha,
x86@kernel.org, Tomi Valkeinen, xen-devel@lists.xenproject.org,
Ingo Molnar, Borislav Petkov, Jean-Christophe Plagniol-Villard,
Antonino Daplas, Stefan Bader, Dave Airlie, Thomas Gleixner,
Ingo Molnar, Juergen Gross, Toshi Kani,
linux-kernel@vger.kernel.org, Andy Lutomirski, David Vrabel,
venkatesh.pallipadi, Roger Pau Monné
In-Reply-To: <CAErSpo5S7=CRyH0TKDfXzs-LWyP9kx_Xd=iFvEKyQZ5-humpxA@mail.gmail.com>
On Thu, Apr 2, 2015 at 3:09 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Apr 2, 2015 at 4:02 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>
>> ---
>> It is possible to enable CONFIG_MTRR and CONFIG_X86_PAT
>> and end up with a system with MTRR functionality disabled
>> PAT functionality enabled.
>
> This is missing a conjunction or something in "MTRR functionality
> disabled PAT functionality."
"and PAT functionality" -- fixed. Thanks.
Luis
^ permalink raw reply
* Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
From: Bjorn Helgaas @ 2015-04-02 22:09 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Konrad Rzeszutek Wilk, Andy Lutomirski, Ingo Molnar,
Thomas Gleixner, H. Peter Anvin, Juergen Gross, Jan Beulich,
Borislav Petkov, Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, linux-fbdev, x86@kernel.org,
xen-devel@lists.xenproject.org, Ingo Molnar, Daniel Vetter,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Dave Hansen, Stefan Bader, Ville Syrjälä, David Vrabel,
Toshi Kani, Roger Pau Monné, xen-devel
In-Reply-To: <20150402210253.GY5622@wotan.suse.de>
On Thu, Apr 2, 2015 at 4:02 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> ---
> It is possible to enable CONFIG_MTRR and CONFIG_X86_PAT
> and end up with a system with MTRR functionality disabled
> PAT functionality enabled.
This is missing a conjunction or something in "MTRR functionality
disabled PAT functionality."
Bjorn
^ permalink raw reply
* Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
From: Luis R. Rodriguez @ 2015-04-02 21:49 UTC (permalink / raw)
To: Toshi Kani
Cc: Luis R. Rodriguez, luto, mingo, tglx, hpa, jgross, JBeulich, bp,
suresh.b.siddha, venkatesh.pallipadi, airlied, linux-kernel,
linux-fbdev, x86, xen-devel, Ingo Molnar, Daniel Vetter,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Dave Hansen, Stefan Bader, konrad.wilk, ville.syrjala,
david.vrabel, bhelgaas, Roger Pau Monné, xen-devel
In-Reply-To: <20150327235630.GR5622@wotan.suse.de>
On Sat, Mar 28, 2015 at 12:56:30AM +0100, Luis R. Rodriguez wrote:
> On Fri, Mar 27, 2015 at 02:40:17PM -0600, Toshi Kani wrote:
> > On Fri, 2015-03-20 at 16:17 -0700, Luis R. Rodriguez wrote:
> > :
> > > @@ -734,6 +742,7 @@ void __init mtrr_bp_init(void)
> > > }
> > >
> > > if (mtrr_if) {
> > > + mtrr_enabled = true;
> > > set_num_var_ranges();
> > > init_table();
> > > if (use_intel()) {
> > get_mtrr_state();
> >
> > After setting mtrr_enabled to true, get_mtrr_state() reads
> > MSR_MTRRdefType and sets 'mtrr_state.enabled', which also indicates if
> > MTRRs are enabled or not on the system. So, potentially, we could have
> > a case that mtrr_enabled is set to true, but mtrr_state.enabled is set
> > to disabled when MTRRs are disabled by BIOS.
>
> Thanks for the review, in this case then we should update mtrr_enabled to false.
>
> > ps.
> > I recently cleaned up this part of the MTRR code in the patch below,
> > which is currently available in the -mm & -next trees.
> > https://lkml.org/lkml/2015/3/24/1063
>
> Great I will rebase and work with that and try to address this
> consideration you have raised.
OK I'll mesh in this change as well in my next respin:
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index a83f27a..ecf7cb9 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -438,7 +438,7 @@ static void __init print_mtrr_state(void)
}
/* Grab all of the MTRR state for this CPU into *state */
-void __init get_mtrr_state(void)
+bool __init get_mtrr_state(void)
{
struct mtrr_var_range *vrs;
unsigned long flags;
@@ -482,6 +482,8 @@ void __init get_mtrr_state(void)
post_set();
local_irq_restore(flags);
+
+ return !!mtrr_state.enabled;
}
/* Some BIOS's are messed up and don't set all MTRRs the same! */
diff --git a/arch/x86/kernel/cpu/mtrr/main.c b/arch/x86/kernel/cpu/mtrr/main.c
index ea5f363..f96195e 100644
--- a/arch/x86/kernel/cpu/mtrr/main.c
+++ b/arch/x86/kernel/cpu/mtrr/main.c
@@ -734,22 +742,25 @@ void __init mtrr_bp_init(void)
}
if (mtrr_if) {
+ mtrr_enabled = true;
set_num_var_ranges();
init_table();
if (use_intel()) {
- get_mtrr_state();
+ /* BIOS may override */
+ mtrr_enabled = get_mtrr_state();
if (mtrr_cleanup(phys_addr)) {
changed_by_mtrr_cleanup = 1;
@@ -745,11 +755,14 @@ void __init mtrr_bp_init(void)
}
}
}
+
+ if (!mtrr_enabled)
+ pr_info("mtrr: system does not support MTRR\n");
}
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.h b/arch/x86/kernel/cpu/mtrr/mtrr.h
index df5e41f..951884d 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.h
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.h
@@ -51,7 +51,7 @@ void set_mtrr_prepare_save(struct set_mtrr_context *ctxt);
void fill_mtrr_var_range(unsigned int index,
u32 base_lo, u32 base_hi, u32 mask_lo, u32 mask_hi);
-void get_mtrr_state(void);
+bool get_mtrr_state(void);
extern void set_mtrr_ops(const struct mtrr_ops *ops);
^ permalink raw reply related
* Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
From: Luis R. Rodriguez @ 2015-04-02 21:02 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Konrad Rzeszutek Wilk, Andy Lutomirski, Ingo Molnar,
Thomas Gleixner, H. Peter Anvin, Juergen Gross, Jan Beulich,
Borislav Petkov, Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, linux-fbdev, x86@kernel.org,
xen-devel@lists.xenproject.org, Ingo Molnar, Daniel Vetter,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Dave Hansen, Stefan Bader, Ville Syrjälä, David Vrabel,
Toshi Kani, Roger Pau Monné, xen-devel
In-Reply-To: <CAErSpo7MY7v2CWjVodpvRp+pB0cUgzN7g-RgBQhHVgA-8A5Lng@mail.gmail.com>
On Thu, Apr 02, 2015 at 03:28:51PM -0500, Bjorn Helgaas wrote:
> On Thu, Apr 2, 2015 at 3:20 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > On Thu, Apr 2, 2015 at 1:13 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >>
> >> On Thu, Mar 26, 2015 at 6:35 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> >>
> >> > I'll rephrase this to:
> >> >
> >> > ---
> >> > It is possible to enable CONFIG_MTRR and up with it
> >> > disabled at run time and yet CONFIG_X86_PAT continues
> >> > to kick through with all functionally enabled. This
> >> > can happen for instance on Xen where MTRR is not
> >> > supported but PAT is, this can happen now on Linux as
> >> > of commit 47591df50 by Juergen introduced as of v3.19.
> >>
> >> I still can't parse this. What does "up with it disabled at run time"
> >> mean?
> >
> > It means that technically even if your CPU/BIOS/system did support
> > MTRR if you use a kernel with MTRR support enabled you might end up
> > with a situation where under one situation MTRR might be enabled and
> > at another run time scenario with the same exact kernel and system you
> > will end up with MTRR disabled. Such is the case for example when
> > booting with Xen, which disables the CPU bits on the hypervisor code.
> > If you boot the same system without Xen you'll get MTRR.
>
> Your text is missing some words. You seem to be using "up" as a verb,
> but it's not a verb. Maybe you meant "end up"?
Indeed.
> Even then, it
> wouldn't make sense for CONFIG_MTRR to be "disabled at run time"
> because CONFIG_MTRR is a compile-time switch. The MTRR
> *functionality* could certainly be disabled at run-time, but not
> CONFIG_MTRR itself.
I'll clarify.
> >> And "... continues to kick through"? Probably some idiomatic
> >> usage I'm just too old to understand :)
> >
> > That means for example that in both the above circumstances even if
> > MTRR went disabled at run time with Xen, the kernel went through with
> > getting PAT enabled.
>
> "CONFIG_X86_PAT continues to kick through" doesn't seem a very precise
> way of describing this. But maybe it's enough for experts in this
> area (which I'm not).
I've rephrased this to:
---
It is possible to enable CONFIG_MTRR and CONFIG_X86_PAT
and end up with a system with MTRR functionality disabled
PAT functionality enabled. This can happen for instance
on Xen where MTRR is not supported but PAT is. This can
happen on Linux as of commit 47591df50 ("xen: Support Xen
pv-domains using PAT") by Juergen, introduced as of v3.19.
---
Luis
^ permalink raw reply
* Re: [PATCH v1 06/47] mtrr: add __arch_phys_wc_add()
From: Luis R. Rodriguez @ 2015-04-02 20:55 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Luis R. Rodriguez, Andy Lutomirski, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, jgross, Jan Beulich, Borislav Petkov,
Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, linux-fbdev, x86@kernel.org,
xen-devel@lists.xenproject.org, Ingo Molnar, Daniel Vetter,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen
In-Reply-To: <CAErSpo7wazwgkEHkLixA88Lcq1Te564Xdd6k=6292JdT3hu8pw@mail.gmail.com>
On Thu, Apr 02, 2015 at 03:21:22PM -0500, Bjorn Helgaas wrote:
> On Fri, Mar 20, 2015 at 6:17 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> >
> > Ideally on systems using PAT we can expect a swift
> > transition away from MTRR. There can be a few exceptions
> > to this, one is where device drivers are known to exist
> > on PATs with errata,
>
> This probably makes sense to someone steeped in MTRR and PAT, but not
> otherwise. "One exception is where drivers are known to exist on PATs
> with errata"? The drivers exist, independent of PAT/MTRR/errata. Do
> you mean there are drivers that can't be converted from MTRR to PAT
> because some PATs are broken?
Well there is that but it seems we have motivation to
address the PAT broken systems so this would be one of the
lower priority reasons to consider adding this API. The
more important reason is below.
> I don't really know anything about MTRR or PAT; I'm just trying to
> figure out how to parse this paragraph.
Sure.
> > another situation is observed on
> > old device drivers where devices had combined MMIO
> > register access with whatever area they typically
> > later wanted to end up using MTRR for on the same
> > PCI BAR. This situation can still be addressed by
> > splitting up ioremap'd PCI BAR into two ioremap'd
> > calls, one for MMIO registers, and another for whatever
> > is desirable for write-combining -- in order to
> > accomplish this though quite a bit of driver
> > restructuring is required.
> >
> > Device drivers which are known to require large
> > amount of re-work in order to split ioremap'd areas
> > can use __arch_phys_wc_add() to avoid regressions
> > when PAT is enabled.
> >
> > For a good example driver where things are neatly
> > split up on a PCI BAR refer the infiniband qib
> > driver. For a good example of a driver where good
> > amount of work is required refer to the infiniband
> > ipath driver.
> >
> > This is *only* a transitive API -- and as such no new
> > drivers are ever expected to use this.
>
> "transient"? Do you mean you intend to remove this API in the near future?
That's correct, the problem is that in order to use PAT cleanly we'd need to
change these drivers to not overlap ioremap'd areas otherwise things can get
quite complex, and changing the way we do the ioremap() calls on a driver might
require a bit of work. The atyfb driver changes I did are an example of the
types of changes that are expected. In the most complex worst cases there are
MTRR "hole" tricks used, and as can be observed with the atyfb driver changes
there are a series of things to consider when this is done specially in light
of eventually making strong UC the default instead of UC-.
I might be able to work around not adding this API by reviewing the users I had
in this series again and seeing if something similar to what I will do on atyfb
can be done in the meantime by using ioremap_uc(). Its not clear to me yet.
Luis
^ permalink raw reply
* Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
From: Bjorn Helgaas @ 2015-04-02 20:28 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Konrad Rzeszutek Wilk, Andy Lutomirski, Ingo Molnar,
Thomas Gleixner, H. Peter Anvin, Juergen Gross, Jan Beulich,
Borislav Petkov, Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, linux-fbdev, x86@kernel.org,
xen-devel@lists.xenproject.org, Ingo Molnar, Daniel Vetter,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Dave Hansen, Stefan Bader, Ville Syrjälä, David Vrabel,
Toshi Kani, Roger Pau Monné, xen-devel
In-Reply-To: <CAB=NE6Us2_sR95JQAKkbDTM9fqmh0N8rZgM3JXa=xSm4cO9DRw@mail.gmail.com>
On Thu, Apr 2, 2015 at 3:20 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Thu, Apr 2, 2015 at 1:13 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> On Thu, Mar 26, 2015 at 6:35 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>>
>> > I'll rephrase this to:
>> >
>> > ---
>> > It is possible to enable CONFIG_MTRR and up with it
>> > disabled at run time and yet CONFIG_X86_PAT continues
>> > to kick through with all functionally enabled. This
>> > can happen for instance on Xen where MTRR is not
>> > supported but PAT is, this can happen now on Linux as
>> > of commit 47591df50 by Juergen introduced as of v3.19.
>>
>> I still can't parse this. What does "up with it disabled at run time"
>> mean?
>
> It means that technically even if your CPU/BIOS/system did support
> MTRR if you use a kernel with MTRR support enabled you might end up
> with a situation where under one situation MTRR might be enabled and
> at another run time scenario with the same exact kernel and system you
> will end up with MTRR disabled. Such is the case for example when
> booting with Xen, which disables the CPU bits on the hypervisor code.
> If you boot the same system without Xen you'll get MTRR.
Your text is missing some words. You seem to be using "up" as a verb,
but it's not a verb. Maybe you meant "end up"? Even then, it
wouldn't make sense for CONFIG_MTRR to be "disabled at run time"
because CONFIG_MTRR is a compile-time switch. The MTRR
*functionality* could certainly be disabled at run-time, but not
CONFIG_MTRR itself.
>> And "... continues to kick through"? Probably some idiomatic
>> usage I'm just too old to understand :)
>
> That means for example that in both the above circumstances even if
> MTRR went disabled at run time with Xen, the kernel went through with
> getting PAT enabled.
"CONFIG_X86_PAT continues to kick through" doesn't seem a very precise
way of describing this. But maybe it's enough for experts in this
area (which I'm not).
Bjorn
^ permalink raw reply
* Re: [PATCH v1 06/47] mtrr: add __arch_phys_wc_add()
From: Bjorn Helgaas @ 2015-04-02 20:21 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Andy Lutomirski, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
jgross, Jan Beulich, Borislav Petkov, Suresh Siddha,
venkatesh.pallipadi, Dave Airlie, linux-kernel@vger.kernel.org,
linux-fbdev, x86@kernel.org, xen-devel@lists.xenproject.org,
Luis R. Rodriguez, Ingo Molnar, Daniel Vetter, Antonino Daplas,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen
In-Reply-To: <1426893517-2511-7-git-send-email-mcgrof@do-not-panic.com>
On Fri, Mar 20, 2015 at 6:17 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> Ideally on systems using PAT we can expect a swift
> transition away from MTRR. There can be a few exceptions
> to this, one is where device drivers are known to exist
> on PATs with errata,
This probably makes sense to someone steeped in MTRR and PAT, but not
otherwise. "One exception is where drivers are known to exist on PATs
with errata"? The drivers exist, independent of PAT/MTRR/errata. Do
you mean there are drivers that can't be converted from MTRR to PAT
because some PATs are broken?
I don't really know anything about MTRR or PAT; I'm just trying to
figure out how to parse this paragraph.
> another situation is observed on
> old device drivers where devices had combined MMIO
> register access with whatever area they typically
> later wanted to end up using MTRR for on the same
> PCI BAR. This situation can still be addressed by
> splitting up ioremap'd PCI BAR into two ioremap'd
> calls, one for MMIO registers, and another for whatever
> is desirable for write-combining -- in order to
> accomplish this though quite a bit of driver
> restructuring is required.
>
> Device drivers which are known to require large
> amount of re-work in order to split ioremap'd areas
> can use __arch_phys_wc_add() to avoid regressions
> when PAT is enabled.
>
> For a good example driver where things are neatly
> split up on a PCI BAR refer the infiniband qib
> driver. For a good example of a driver where good
> amount of work is required refer to the infiniband
> ipath driver.
>
> This is *only* a transitive API -- and as such no new
> drivers are ever expected to use this.
"transient"? Do you mean you intend to remove this API in the near future?
Bjorn
^ permalink raw reply
* Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
From: Luis R. Rodriguez @ 2015-04-02 20:20 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Konrad Rzeszutek Wilk, Andy Lutomirski, Ingo Molnar,
Thomas Gleixner, H. Peter Anvin, Juergen Gross, Jan Beulich,
Borislav Petkov, Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, linux-fbdev, x86@kernel.org,
xen-devel@lists.xenproject.org, Ingo Molnar, Daniel Vetter,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Dave Hansen, Stefan Bader, Ville Syrjälä, David Vrabel,
Toshi Kani, Roger Pau Monné, xen-devel
In-Reply-To: <CAErSpo5HJMr-HYKJRaPtb7=fb4BOq1b4EFs6S+YdG64_fGKc6Q@mail.gmail.com>
On Thu, Apr 2, 2015 at 1:13 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> On Thu, Mar 26, 2015 at 6:35 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>
> > I'll rephrase this to:
> >
> > ---
> > It is possible to enable CONFIG_MTRR and up with it
> > disabled at run time and yet CONFIG_X86_PAT continues
> > to kick through with all functionally enabled. This
> > can happen for instance on Xen where MTRR is not
> > supported but PAT is, this can happen now on Linux as
> > of commit 47591df50 by Juergen introduced as of v3.19.
>
> I still can't parse this. What does "up with it disabled at run time"
> mean?
It means that technically even if your CPU/BIOS/system did support
MTRR if you use a kernel with MTRR support enabled you might end up
with a situation where under one situation MTRR might be enabled and
at another run time scenario with the same exact kernel and system you
will end up with MTRR disabled. Such is the case for example when
booting with Xen, which disables the CPU bits on the hypervisor code.
If you boot the same system without Xen you'll get MTRR.
> And "... continues to kick through"? Probably some idiomatic
> usage I'm just too old to understand :)
That means for example that in both the above circumstances even if
MTRR went disabled at run time with Xen, the kernel went through with
getting PAT enabled.
> Please use the conventional citation format:
>
> 47591df50512 ("xen: Support Xen pv-domains using PAT")
>
> A one-character typo in a SHA1 makes it completely useless, so it's
> nice to have the summary line both for readability and a bit of
> redundancy.
Sure, fixed.
Luis
^ permalink raw reply
* Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
From: Bjorn Helgaas @ 2015-04-02 20:13 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Konrad Rzeszutek Wilk, Luis R. Rodriguez, Andy Lutomirski,
Ingo Molnar, Thomas Gleixner, H. Peter Anvin, jgross, Jan Beulich,
Borislav Petkov, Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, linux-fbdev, x86@kernel.org,
xen-devel@lists.xenproject.org, Ingo Molnar, Daniel Vetter,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Dave Hansen, Stefan Bader, Ville Syrjälä, David Vrabel,
Toshi Kani, Roger Pau Monné, xen-devel
In-Reply-To: <20150326233555.GE5622@wotan.suse.de>
On Thu, Mar 26, 2015 at 6:35 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> I'll rephrase this to:
>
> ---
> It is possible to enable CONFIG_MTRR and up with it
> disabled at run time and yet CONFIG_X86_PAT continues
> to kick through with all functionally enabled. This
> can happen for instance on Xen where MTRR is not
> supported but PAT is, this can happen now on Linux as
> of commit 47591df50 by Juergen introduced as of v3.19.
I still can't parse this. What does "up with it disabled at run time"
mean? And "... continues to kick through"? Probably some idiomatic
usage I'm just too old to understand :)
Please use the conventional citation format:
47591df50512 ("xen: Support Xen pv-domains using PAT")
A one-character typo in a SHA1 makes it completely useless, so it's
nice to have the summary line both for readability and a bit of
redundancy.
Bjorn
^ 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