Skip to contentSkip to conversation

Minor display fixes

PATCH In review Daniel Stone 3 participants 10 messages Aug 27, 2008 – Sep 13, 2008

Subsystems (5)
Arm (32-bit) Arm sub-architectures Framebuffer layer OMAP2+ support +1 more subsystems
lore
Share & tools
lore

Daniel Stone

author
#0 lore raw

cover Minor display fixes

Hi,
The following two patches fix two minor display issues.

One is that sgx_fck was useless, and didn't work (at least on ES2.0 and above; haven't checked the TRM for pre-ES2.0).

The other is that the dispc IRQ handling was rather odd and inflexible, so due to the needs of ... an external consumer ... I made it a bit more sensible.

Tested on 3430.

Cheers,
Daniel

author
#1 lore raw

PATCH 1/2 ARM: OMAP2: Fix definition of SGX clock register bits

The GFX/SGX functional and interface clocks have different masks, for some unknown reason, so split EN_SGX_SHIFT into one each for fclk and iclk.

Correct according to the TRM and the far more important 'does this actually work at all?' metric.

Signed-off-by Daniel Stone <[email protected]>
---
 arch/arm/mach-omap2/clock34xx.h       |    4 ++--
 arch/arm/mach-omap2/cm-regbits-34xx.h |    8 +++++---
 2 files changed, 7 insertions(+), 5 deletions(-)
arch/arm/mach-omap2/clock34xx.h +2 -2
index d7d6ffd..6518b1e 100644
@@ -1323,7 +1323,7 @@ static struct clk sgx_fck = {
 	.name		= "sgx_fck",
 	.init		= &omap2_init_clksel_parent,
 	.enable_reg	= _OMAP34XX_CM_REGADDR(OMAP3430ES2_SGX_MOD, CM_FCLKEN),
-	.enable_bit	= OMAP3430ES2_EN_SGX_SHIFT,
+	.enable_bit	= OMAP3430ES2_EN_SGX_FSHIFT,
 	.clksel_reg	= _OMAP34XX_CM_REGADDR(OMAP3430ES2_SGX_MOD, CM_CLKSEL),
 	.clksel_mask	= OMAP3430ES2_CLKSEL_SGX_MASK,
 	.clksel		= sgx_clksel,

@@ -1337,7 +1337,7 @@ static struct clk sgx_ick = {
 	.parent		= &l3_ick,
 	.init		= &omap2_init_clk_clkdm,
 	.enable_reg	= _OMAP34XX_CM_REGADDR(OMAP3430ES2_SGX_MOD, CM_ICLKEN),
-	.enable_bit	= OMAP3430ES2_EN_SGX_SHIFT,
+	.enable_bit	= OMAP3430ES2_EN_SGX_ISHIFT,
 	.flags		= CLOCK_IN_OMAP3430ES2,
 	.clkdm		= { .name = "sgx_clkdm" },
 	.recalc		= &followparent_recalc,
arch/arm/mach-omap2/cm-regbits-34xx.h +5 -3
index ffb695b..06a78a7 100644
@@ -339,9 +339,11 @@
 #define OMAP3430ES1_CLKACTIVITY_GFX_SHIFT		0
 #define OMAP3430ES1_CLKACTIVITY_GFX_MASK		(1 << 0)
 
-/* CM_FCLKEN_SGX */
-#define OMAP3430ES2_EN_SGX_SHIFT			1
-#define OMAP3430ES2_EN_SGX_MASK				(1 << 1)
+/* CM_FCLKEN_SGX/CM_ICLKEN_SGX */
+#define OMAP3430ES2_EN_SGX_FSHIFT			1
+#define OMAP3430ES2_EN_SGX_FMASK			(1 << 1)
+#define OMAP3430ES2_EN_SGX_ISHIFT			0
+#define OMAP3430ES2_EN_SGX_IMASK			(1 << 0)
 
 /* CM_CLKSEL_SGX */
 #define OMAP3430ES2_CLKSEL_SGX_SHIFT			0
Signature · 1 line
1.5.6.3
author
#2 lore raw

PATCH 2/2 FB: OMAP: DISPC: Allow multiple external IRQ handlers

Previously, the only external (to dispc.c) IRQ handler was RFBI's frame done handler. dispc's IRQ framework was very dumb: you could only have one handler, and the semantics of {request,free}_irq were odd, to say the least.

The new framework allows multiple consumers to register arbitrary IRQ masks.

Signed-off-by Daniel Stone <[email protected]>
---
 drivers/video/omap/dispc.c |   94 ++++++++++++++++++++++++++-----------------
 drivers/video/omap/dispc.h |    7 ++-
 drivers/video/omap/rfbi.c  |    6 ++-
 3 files changed, 65 insertions(+), 42 deletions(-)
drivers/video/omap/dispc.c +57 -37
index df50a2f..2a9d8aa 100644
@@ -160,6 +160,8 @@ struct resmap {
 	unsigned long	*map;
 };
 
+#define MAX_IRQ_HANDLERS            4
+
 static struct {
 	u32		base;
 

@@ -172,9 +174,11 @@ static struct {
 
 	int		ext_mode;
 
-	unsigned long	enabled_irqs;
-	void		(*irq_callback)(void *);
-	void		*irq_callback_data;
+	struct {
+		u32	irq_mask;
+		void	(*callback)(void *);
+		void	*data;
+	} irq_handlers[MAX_IRQ_HANDLERS];
 	struct completion	frame_done;
 
 	int		fir_hinc[OMAPFB_PLANE_NUM];

@@ -814,56 +818,70 @@ static void set_lcd_timings(void)
 	panel->pixel_clock = fck / lck_div / pck_div / 1000;
 }
 
-int omap_dispc_request_irq(void (*callback)(void *data), void *data)
+static void recalc_irq_mask(void)
 {
-	int r = 0;
+	int i;
+	unsigned long irq_mask = DISPC_IRQ_MASK_ERROR;
 
-	BUG_ON(callback == NULL);
+	for (i = 0; i < MAX_IRQ_HANDLERS; i++) {
+		if (!dispc.irq_handlers[i].callback)
+			continue;
 
-	if (dispc.irq_callback)
-		r = -EBUSY;
-	else {
-		dispc.irq_callback = callback;
-		dispc.irq_callback_data = data;
+		irq_mask |= dispc.irq_handlers[i].irq_mask;
 	}
 
-	return r;
-}
-EXPORT_SYMBOL(omap_dispc_request_irq);
-
-void omap_dispc_enable_irqs(int irq_mask)
-{
 	enable_lcd_clocks(1);
-	dispc.enabled_irqs = irq_mask;
-	irq_mask |= DISPC_IRQ_MASK_ERROR;
 	MOD_REG_FLD(DISPC_IRQENABLE, 0x7fff, irq_mask);
 	enable_lcd_clocks(0);
 }
-EXPORT_SYMBOL(omap_dispc_enable_irqs);
 
-void omap_dispc_disable_irqs(int irq_mask)
+int omap_dispc_request_irq(unsigned long irq_mask, void (*callback)(void *data),
+			   void *data)
 {
-	enable_lcd_clocks(1);
-	dispc.enabled_irqs &= ~irq_mask;
-	irq_mask &= ~DISPC_IRQ_MASK_ERROR;
-	MOD_REG_FLD(DISPC_IRQENABLE, 0x7fff, irq_mask);
-	enable_lcd_clocks(0);
+	int i;
+
+	BUG_ON(callback == NULL);
+
+	for (i = 0; i < MAX_IRQ_HANDLERS; i++) {
+		if (dispc.irq_handlers[i].callback)
+			continue;
+
+		dispc.irq_handlers[i].irq_mask = irq_mask;
+		dispc.irq_handlers[i].callback = callback;
+		dispc.irq_handlers[i].data = data;
+		recalc_irq_mask();
+
+		return 0;
+	}
+
+	return -EBUSY;
 }
-EXPORT_SYMBOL(omap_dispc_disable_irqs);
+EXPORT_SYMBOL(omap_dispc_request_irq);
 
-void omap_dispc_free_irq(void)
+void omap_dispc_free_irq(unsigned long irq_mask, void (*callback)(void *data),
+			 void *data)
 {
-	enable_lcd_clocks(1);
-	omap_dispc_disable_irqs(DISPC_IRQ_MASK_ALL);
-	dispc.irq_callback = NULL;
-	dispc.irq_callback_data = NULL;
-	enable_lcd_clocks(0);
+	int i;
+
+	for (i = 0; i < MAX_IRQ_HANDLERS; i++) {
+		if (dispc.irq_handlers[i].callback == callback &&
+		    dispc.irq_handlers[i].data == data) {
+			dispc.irq_handlers[i].irq_mask = 0;
+			dispc.irq_handlers[i].callback = NULL;
+			dispc.irq_handlers[i].data = NULL;
+			recalc_irq_mask();
+			return;
+		}
+	}
+
+	BUG();
 }
 EXPORT_SYMBOL(omap_dispc_free_irq);
 
 static irqreturn_t omap_dispc_irq_handler(int irq, void *dev)
 {
 	u32 stat = dispc_read_reg(DISPC_IRQSTATUS);
+	int i = 0;
 
 	if (stat & DISPC_IRQ_FRAMEMASK)
 		complete(&dispc.frame_done);

@@ -875,8 +893,11 @@ static irqreturn_t omap_dispc_irq_handler(int irq, void *dev)
 		}
 	}
 
-	if ((stat & dispc.enabled_irqs) && dispc.irq_callback)
-		dispc.irq_callback(dispc.irq_callback_data);
+	for (i = 0; i < MAX_IRQ_HANDLERS; i++) {
+		if (unlikely(dispc.irq_handlers[i].callback &&
+			     (stat & dispc.irq_handlers[i].irq_mask)))
+		dispc.irq_handlers[i].callback(dispc.irq_handlers[i].data);
+	}
 
 	dispc_write_reg(DISPC_IRQSTATUS, stat);
 

@@ -1471,8 +1492,7 @@ static int omap_dispc_init(struct omapfb_device *fbdev, int ext_mode,
 	l = dispc_read_reg(DISPC_IRQSTATUS);
 	dispc_write_reg(DISPC_IRQSTATUS, l);
 
-	/* Enable those that we handle always */
-	omap_dispc_enable_irqs(DISPC_IRQ_FRAMEMASK);
+	recalc_irq_mask();
 
 	if ((r = request_irq(INT_24XX_DSS_IRQ, omap_dispc_irq_handler,
 			   0, MODULE_NAME, fbdev)) < 0) {
drivers/video/omap/dispc.h +4 -3
index eb1512b..f7746f4 100644
@@ -37,7 +37,8 @@ extern void omap_dispc_set_lcd_size(int width, int height);
 extern void omap_dispc_enable_lcd_out(int enable);
 extern void omap_dispc_enable_digit_out(int enable);
 
-extern int  omap_dispc_request_irq(void (*callback)(void *data), void *data);
-extern void omap_dispc_free_irq(void);
-
+extern int  omap_dispc_request_irq(unsigned long irq_mask,
+                                   void (*callback)(void *data), void *data);
+extern void omap_dispc_free_irq(unsigned long irq_mask,
+                                void (*callback)(void *data), void *data);
 #endif
drivers/video/omap/rfbi.c +4 -2
index 4f1f4be..7bc7780 100644
@@ -57,6 +57,7 @@
 
 #define DISPC_BASE		0x48050400
 #define DISPC_CONTROL		0x0040
+#define DISPC_IRQ_FRAMEMASK     0x0001
 
 static struct {
 	u32		base;

@@ -547,7 +548,8 @@ static int rfbi_init(struct omapfb_device *fbdev)
 	l = (0x01 << 2);
 	rfbi_write_reg(RFBI_CONTROL, l);
 
-	if ((r = omap_dispc_request_irq(rfbi_dma_callback, NULL)) < 0) {
+	if ((r = omap_dispc_request_irq(DISPC_IRQ_FRAMEMASK, rfbi_dma_callback,
+                                        NULL)) < 0) {
 		dev_err(fbdev->dev, "can't get DISPC irq\n");
 		rfbi_enable_clocks(0);
 		return r;

@@ -564,7 +566,7 @@ static int rfbi_init(struct omapfb_device *fbdev)
 
 static void rfbi_cleanup(void)
 {
-	omap_dispc_free_irq();
+	omap_dispc_free_irq(DISPC_IRQ_FRAMEMASK, rfbi_dma_callback, NULL);
 	rfbi_put_clocks();
 }
Signature · 1 line
1.5.6.3
#3 lore raw

Re: [PATCH 1/2] ARM: OMAP2: Fix definition of SGX clock register bits

In reply to Daniel Stone

Hello Daniel,

Quoted 3 lines · Daniel Stone

The GFX/SGX functional and interface clocks have different masks, for some unknown reason, so split EN_SGX_SHIFT into one each for fclk and iclk.

thanks for the fix, this looks mostly good - one comment:

Quoted 6 lines
arch/arm/mach-omap2/cm-regbits-34xx.h +0 -0
index ffb695b..06a78a7 100644
@@ -339,9 +339,11 @@
 #define OMAP3430ES1_CLKACTIVITY_GFX_SHIFT		0

arch/arm/mach-omap2/cm-regbits-34xx.h line 346

Quoted 10 lines
arch/arm/mach-omap2/cm-regbits-34xx.h +5 -3
 #define OMAP3430ES1_CLKACTIVITY_GFX_MASK		(1 << 0)
 
-/* CM_FCLKEN_SGX */
-#define OMAP3430ES2_EN_SGX_SHIFT			1
-#define OMAP3430ES2_EN_SGX_MASK				(1 << 1)
+/* CM_FCLKEN_SGX/CM_ICLKEN_SGX */
+#define OMAP3430ES2_EN_SGX_FSHIFT			1
+#define OMAP3430ES2_EN_SGX_FMASK			(1 << 1)
+#define OMAP3430ES2_EN_SGX_ISHIFT			0
+#define OMAP3430ES2_EN_SGX_IMASK			(1 << 0)

To keep these consistent with prior practice in prm-regbits-34xx.h for name collisions, could you change the above to look something like:

/* CM_FCLKEN_SGX */
#define OMAP3430ES2_CM_FCLKEN_SGX_EN_SGX_SHIFT 1
#define OMAP3430ES2_CM_FCLKEN_SGX_EN_SGX_MASK (1 << 1)

/* CM_ICLKEN_SGX */
#define OMAP3430ES2_CM_ICLKEN_SGX_EN_SGX_SHIFT 0
#define OMAP3430ES2_CM_ICLKEN_SGX_EN_SGX_MASK (1 << 0)

Ugly, but unambiguous.

- Paul

author
#4 lore raw

Re: [PATCH 1/2] ARM: OMAP2: Fix definition of SGX clock register bits

Quoted 1 line · ext Paul Walmsley

Hello Daniel,

Hi,

Quoted 31 lines · Daniel Stone
Quoted 3 lines

The GFX/SGX functional and interface clocks have different masks, for some unknown reason, so split EN_SGX_SHIFT into one each for fclk and iclk.

thanks for the fix, this looks mostly good - one comment:

Quoted 6 lines
arch/arm/mach-omap2/cm-regbits-34xx.h +0 -0
index ffb695b..06a78a7 100644
@@ -339,9 +339,11 @@
 #define OMAP3430ES1_CLKACTIVITY_GFX_SHIFT		0

arch/arm/mach-omap2/cm-regbits-34xx.h line 346

Quoted 10 lines
arch/arm/mach-omap2/cm-regbits-34xx.h +5 -3
 #define OMAP3430ES1_CLKACTIVITY_GFX_MASK		(1 << 0)
 
-/* CM_FCLKEN_SGX */
-#define OMAP3430ES2_EN_SGX_SHIFT			1
-#define OMAP3430ES2_EN_SGX_MASK				(1 << 1)
+/* CM_FCLKEN_SGX/CM_ICLKEN_SGX */
+#define OMAP3430ES2_EN_SGX_FSHIFT			1
+#define OMAP3430ES2_EN_SGX_FMASK			(1 << 1)
+#define OMAP3430ES2_EN_SGX_ISHIFT			0
+#define OMAP3430ES2_EN_SGX_IMASK			(1 << 0)

To keep these consistent with prior practice in prm-regbits-34xx.h for name collisions, could you change the above to look something like:

/* CM_FCLKEN_SGX */
#define OMAP3430ES2_CM_FCLKEN_SGX_EN_SGX_SHIFT 1
#define OMAP3430ES2_CM_FCLKEN_SGX_EN_SGX_MASK (1 << 1)

Quoted 5 lines

/* CM_ICLKEN_SGX */
#define OMAP3430ES2_CM_ICLKEN_SGX_EN_SGX_SHIFT 0
#define OMAP3430ES2_CM_ICLKEN_SGX_EN_SGX_MASK (1 << 0)

Ugly, but unambiguous.

Yep, looks fine to me: just wasn't really sure what to put. Should I resend, or?

Cheers,
Daniel

#5 lore raw

Re: [PATCH 1/2] ARM: OMAP2: Fix definition of SGX clock register bits

Hello Daniel,

Quoted 2 lines · Daniel Stone

Yep, looks fine to me: just wasn't really sure what to put. Should I resend, or?

yes, please resend - that will make it convenient for Tony to apply the patches.

regards,

- Paul

author
#6 lore raw

Re: [PATCH 1/2] ARM: OMAP2: Fix definition of SGX clock register bits

Quoted 9 lines · ext Paul Walmsley

Hello Daniel,

Quoted 2 lines · Daniel Stone

Yep, looks fine to me: just wasn't really sure what to put. Should I resend, or?

yes, please resend - that will make it convenient for Tony to apply the patches.

Hi,
I'll do that. Thanks a lot for the review.

Cheers,
Daniel

maintainer
#7 lore raw

Re: [PATCH 1/2] ARM: OMAP2: Fix definition of SGX clock register bits

Quoted 5 lines · Daniel Stone
Quoted 4 lines · ext Paul Walmsley

Hello Daniel,

On Wed, 27 Aug 2008, Daniel Stone wrote:

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

Yep, looks fine to me: just wasn't really sure what to put. Should I resend, or?

yes, please resend - that will make it convenient for Tony to apply the patches.

Hi,
I'll do that. Thanks a lot for the review.

ping, any news on the updated patch?

Tony

author
#8 lore raw

Re: [PATCH 1/2] ARM: OMAP2: Fix definition of SGX clock register bits

Quoted 9 lines · ext Tony Lindgren
Quoted 8 lines · Daniel Stone
Quoted 7 lines · ext Paul Walmsley

Hello Daniel,

Quoted 2 lines · Daniel Stone

Yep, looks fine to me: just wasn't really sure what to put. Should I resend, or?

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

yes, please resend - that will make it convenient for Tony to apply the patches.

Hi,
I'll do that. Thanks a lot for the review.

ping, any news on the updated patch?

Sorry, my bad. Inlined below.

Cheers,
Daniel

From d8f04561d17adc8c52f8c2d6091bc9897904feb7 Mon Sep 17 00:00:00 2001
From: Daniel Stone <[email protected]>
Date: Wed, 27 Aug 2008 04:31:59 +0300
Subject: [PATCH] ARM: OMAP2: Fix definition of SGX clock register bits

The GFX/SGX functional and interface clocks have different masks, for some unknown reason, so split EN_SGX_SHIFT into one each for fclk and iclk.

Correct according to the TRM and the far more important 'does this actually work at all?' metric.

Signed-off-by Daniel Stone <[email protected]>
---
 arch/arm/mach-omap2/clock34xx.h       |    4 ++--
 arch/arm/mach-omap2/cm-regbits-34xx.h |    8 ++++++--
 2 files changed, 8 insertions(+), 4 deletions(-)
arch/arm/mach-omap2/clock34xx.h +2 -2
index d7d6ffd..ce96270 100644
@@ -1323,7 +1323,7 @@ static struct clk sgx_fck = {
 	.name		= "sgx_fck",
 	.init		= &omap2_init_clksel_parent,
 	.enable_reg	= _OMAP34XX_CM_REGADDR(OMAP3430ES2_SGX_MOD, CM_FCLKEN),
-	.enable_bit	= OMAP3430ES2_EN_SGX_SHIFT,
+	.enable_bit	= OMAP3430ES2_CM_FCLKEN_SGX_EN_SGX_SHIFT,
 	.clksel_reg	= _OMAP34XX_CM_REGADDR(OMAP3430ES2_SGX_MOD, CM_CLKSEL),
 	.clksel_mask	= OMAP3430ES2_CLKSEL_SGX_MASK,
 	.clksel		= sgx_clksel,

@@ -1337,7 +1337,7 @@ static struct clk sgx_ick = {
 	.parent		= &l3_ick,
 	.init		= &omap2_init_clk_clkdm,
 	.enable_reg	= _OMAP34XX_CM_REGADDR(OMAP3430ES2_SGX_MOD, CM_ICLKEN),
-	.enable_bit	= OMAP3430ES2_EN_SGX_SHIFT,
+	.enable_bit	= OMAP3430ES2_CM_ICLKEN_SGX_EN_SGX_SHIFT,
 	.flags		= CLOCK_IN_OMAP3430ES2,
 	.clkdm		= { .name = "sgx_clkdm" },
 	.recalc		= &followparent_recalc,
arch/arm/mach-omap2/cm-regbits-34xx.h +6 -2
index ffb695b..fe8e9ef 100644
@@ -340,8 +340,12 @@
 #define OMAP3430ES1_CLKACTIVITY_GFX_MASK		(1 << 0)
 
 /* CM_FCLKEN_SGX */
-#define OMAP3430ES2_EN_SGX_SHIFT			1
-#define OMAP3430ES2_EN_SGX_MASK				(1 << 1)
+#define OMAP3430ES2_CM_FCLKEN_SGX_EN_SGX_SHIFT		1
+#define OMAP3430ES2_CM_FCLKEN_SGX_EN_SGX_MASK		(1 << 1)
+
+/* CM_ICLKEN_SGX */
+#define OMAP3430ES2_CM_ICLKEN_SGX_EN_SGX_SHIFT		0
+#define OMAP3430ES2_CM_ICLKEN_SGX_EN_SGX_MASK		(1 << 0)
 
 /* CM_CLKSEL_SGX */
 #define OMAP3430ES2_CLKSEL_SGX_SHIFT			0
Signature · 1 line
1.5.6.3
maintainer
#9 lore raw

Re: [PATCH 1/2] ARM: OMAP2: Fix definition of SGX clock register bits

Quoted 13 lines · Daniel Stone
Quoted 12 lines · ext Tony Lindgren
Quoted 11 lines · Daniel Stone
Quoted 9 lines · ext Paul Walmsley

Hello Daniel,

Quoted 2 lines · Daniel Stone

Yep, looks fine to me: just wasn't really sure what to put. Should I resend, or?

yes, please resend - that will make it convenient for Tony to apply the patches.

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

Hi,
I'll do that. Thanks a lot for the review.

ping, any news on the updated patch?

Sorry, my bad. Inlined below.

Thanks, that's pushed now.

Looks like your second patch needs updating though, it does not apply. Can you please refresh that one too?

Thanks

Tony