Re: [PATCH 1/4] Smedia Glamo 3362 core/resource driver

From: Samuel Ortiz
Date: Wed Aug 18 2010 - 09:39:20 EST

Hi Thibaut,

On Wed, Jul 07, 2010 at 03:12:03PM +0200, Thibaut Girka wrote:
> +#define GLAMO_IRQ_HOSTBUS 0
> +#define GLAMO_IRQ_JPEG 1
> +#define GLAMO_IRQ_MPEG 2
> +#define GLAMO_IRQ_MPROC1 3
> +#define GLAMO_IRQ_MPROC0 4
> +#define GLAMO_IRQ_CMDQUEUE 5
> +#define GLAMO_IRQ_2D 6
> +#define GLAMO_IRQ_MMC 7
> +#define GLAMO_IRQ_RISC 8
This is conflicting with enum glamo_irq, please fix your namespaces.


> +
> +/*
> + * Glamo internal settings
> + *
> + * We run the memory interface from the faster PLLB on 2.6.28 kernels and
> + * above. Couple of GTA02 users report trouble with memory bus when they
> + * upgraded from 2.6.24. So this parameter allows reversion to 2.6.24
> + * scheme if their Glamo chip needs it.
So you're saying that a couple users reported a bug on a specific kernel
revision, with the same HW, but all other users haven't ?
Couldnt that be related to a specific HW revision ?
Also, this code should be scheduled for 2.6.37, and we're not going to
carry code to allow it to run on 2.6.28.


> +static const struct reg_range reg_range[] = {
> + { 0x0000, 0x76, "General", 1 },
> + { 0x0200, 0x18, "Host Bus", 1 },
> + { 0x0300, 0x38, "Memory", 1 },
> +/* { 0x0400, 0x100, "Sensor", 0 }, */
> +/* { 0x0500, 0x300, "ISP", 0 }, */
> +/* { 0x0800, 0x400, "JPEG", 0 }, */
> +/* { 0x0c00, 0xcc, "MPEG", 0 }, */
> + { 0x1100, 0xb2, "LCD 1", 0 },
> + { 0x1200, 0x64, "LCD 2", 0 },
> + { 0x1400, 0x42, "MMC", 0 },
> +/* { 0x1500, 0x080, "MPU 0", 0 },
> + { 0x1580, 0x080, "MPU 1", 0 },
> + { 0x1600, 0x080, "Cmd Queue", 0 },
> + { 0x1680, 0x080, "RISC CPU", 0 },*/
> + { 0x1700, 0x400, "2D Unit", 0 },
> +/* { 0x1b00, 0x900, "3D Unit", 0 }, */
> +};
Please remove the commented code from this array.


> +static void glamo_irq_demux_handler(unsigned int irq, struct irq_desc *desc)
> +{
> + struct glamo_core *glamo = get_irq_desc_data(desc);
> + desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
> +
> + if (unlikely(desc->status & IRQ_INPROGRESS)) {
> + desc->status |= (IRQ_PENDING | IRQ_MASKED);
> + desc->chip->mask(irq);
> + desc->chip->ack(irq);
> + return;
> + }
> + kstat_incr_irqs_this_cpu(irq, desc);
> +
> + desc->chip->ack(irq);
> + desc->status |= IRQ_INPROGRESS;
> +
> + do {
> + uint16_t irqstatus;
> + int i;
> +
> + if (unlikely((desc->status &
> + (IRQ_PENDING | IRQ_MASKED | IRQ_DISABLED)) ==
> + (IRQ_PENDING | IRQ_MASKED))) {
> + /* dealing with pending IRQ, unmasking */
> + desc->chip->unmask(irq);
> + desc->status &= ~IRQ_MASKED;
> + }
> +
> + desc->status &= ~IRQ_PENDING;
> +
> + /* read IRQ status register */
> + irqstatus = __reg_read(glamo, GLAMO_REG_IRQ_STATUS);
> + for (i = 0; i < 9; ++i) {
> + if (irqstatus & BIT(i))
> + generic_handle_irq(glamo->irq_base + i);
> + }
> +
> + } while ((desc->status & (IRQ_PENDING | IRQ_DISABLED)) == IRQ_PENDING);
So here you're busy looping in an interrupt handler, and that's not
recommended.


> +#ifdef CONFIG_DEBUG_FS
> +static ssize_t debugfs_regs_write(struct file *file,
> + const char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + struct glamo_core *glamo = ((struct seq_file *)file->private_data)->private;
> + char buf[14];
> + unsigned int reg;
> + unsigned int val;
> + int buf_size;
> +
> + buf_size = min(count, sizeof(buf) - 1);
> + if (copy_from_user(buf, user_buf, buf_size))
> + return -EFAULT;
> + if (sscanf(buf, "%x %x", &reg, &val) != 2)
> + return -EFAULT;
> +
> + dev_info(&glamo->pdev->dev, "reg %#02x <-- %#04x\n", reg, val);
> +
> + glamo_reg_write(glamo, reg, val);
> +
> + return count;
> +}
> +
> +static int glamo_show_regs(struct seq_file *s, void *pos)
> +{
> + struct glamo_core *glamo = s->private;
> + int i, n;
> + const struct reg_range *rr = reg_range;
> +
> + spin_lock(&glamo->lock);
> + for (i = 0; i < ARRAY_SIZE(reg_range); ++i, ++rr) {
> + if (!rr->dump)
> + continue;
> + seq_printf(s, "\n%s\n", rr->name);
> + for (n = rr->start; n < rr->start + rr->count; n += 2) {
> + if ((n & 15) == 0)
> + seq_printf(s, "\n%04X: ", n);
> + seq_printf(s, "%04x ", __reg_read(glamo, n));
> + }
> + seq_printf(s, "\n");
> + }
> + spin_unlock(&glamo->lock);
> +
> + return 0;
> +}
> +
> +static int debugfs_open_file(struct inode *inode, struct file *file)
> +{
> + return single_open(file, glamo_show_regs, inode->i_private);
> +}
> +
> +static const struct file_operations debugfs_regs_ops = {
> + .open = debugfs_open_file,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .write = debugfs_regs_write,
> + .release = single_release,
> +};
> +
> +struct glamo_engine_reg_set {
> + uint16_t reg;
> + uint16_t mask_suspended;
> + uint16_t mask_enabled;
> +};
I think you want this definition outside of the DEBUGFS if.else.endif
scope. Have you tried building this driver without DEBUGFS enabled ?


> +static const struct glamo_engine_reg_set glamo_lcd_regs[] = {
> + { GLAMO_REG_CLOCK_LCD,
> + GLAMO_CLOCK_LCD_EN_M5CLK |
> + GLAMO_CLOCK_LCD_DG_M5CLK |
> + GLAMO_CLOCK_LCD_EN_DMCLK,
> +
> + GLAMO_CLOCK_LCD_EN_DHCLK |
> + GLAMO_CLOCK_LCD_EN_DCLK
> + },
> + { GLAMO_REG_CLOCK_GEN5_1,
> + GLAMO_CLOCK_GEN51_EN_DIV_DMCLK,
> +
> + GLAMO_CLOCK_GEN51_EN_DIV_DHCLK |
> + GLAMO_CLOCK_GEN51_EN_DIV_DCLK
> + }
> +};
Identation could be clearer here:
static const struct glamo_engine_reg_set glamo_lcd_regs[] = {
{
GLAMO_REG_CLOCK_LCD,

GLAMO_CLOCK_LCD_EN_M5CLK |
GLAMO_CLOCK_LCD_DG_M5CLK |
GLAMO_CLOCK_LCD_EN_DMCLK,

GLAMO_CLOCK_LCD_EN_DHCLK |
GLAMO_CLOCK_LCD_EN_DCLK
},
[...]



> +/***********************************************************************
> + * script support
> + ***********************************************************************/
> +
> +#define GLAMO_SCRIPT_END 0xffff
> +#define GLAMO_SCRIPT_WAIT 0xfffe
> +#define GLAMO_SCRIPT_LOCK_PLL 0xfffd
> +
> +/*
> + * couple of people reported artefacts with 2.6.28 changes, this
> + * allows reversion to 2.6.24 settings
> +*/
Same remark as with the module option: This is going to be a 2.6.37
driver, and I'd like you to check if you're still seeing those issues
before carrying these kind of hacks.


> +static const struct glamo_script glamo_init_script[] = {
> + { GLAMO_REG_CLOCK_HOST, 0x1000 },
> + { GLAMO_SCRIPT_WAIT, 2 },
> + { GLAMO_REG_CLOCK_MEMORY, 0x1000 },
> + { GLAMO_REG_CLOCK_MEMORY, 0x2000 },
> + { GLAMO_REG_CLOCK_LCD, 0x1000 },
> + { GLAMO_REG_CLOCK_MMC, 0x1000 },
> + { GLAMO_REG_CLOCK_ISP, 0x1000 },
> + { GLAMO_REG_CLOCK_ISP, 0x3000 },
> + { GLAMO_REG_CLOCK_JPEG, 0x1000 },
> + { GLAMO_REG_CLOCK_3D, 0x1000 },
> + { GLAMO_REG_CLOCK_3D, 0x3000 },
> + { GLAMO_REG_CLOCK_2D, 0x1000 },
> + { GLAMO_REG_CLOCK_2D, 0x3000 },
> + { GLAMO_REG_CLOCK_RISC1, 0x1000 },
> + { GLAMO_REG_CLOCK_MPEG, 0x1000 },
> + { GLAMO_REG_CLOCK_MPEG, 0x3000 },
> + { GLAMO_REG_CLOCK_MPROC, 0x1000 /*0x100f*/ },
> + { GLAMO_SCRIPT_WAIT, 2 },
> + { GLAMO_REG_CLOCK_HOST, 0x0000 },
> + { GLAMO_REG_CLOCK_MEMORY, 0x0000 },
> + { GLAMO_REG_CLOCK_LCD, 0x0000 },
> + { GLAMO_REG_CLOCK_MMC, 0x0000 },
> + { GLAMO_REG_PLL_GEN1, 0x05db }, /* 48MHz */
> + { GLAMO_REG_PLL_GEN3, 0x0aba }, /* 90MHz */
> + { GLAMO_SCRIPT_LOCK_PLL, 0 },
> + /*
> + * b9 of this register MUST be zero to get any interrupts on INT#
> + * the other set bits enable all the engine interrupt sources
> + */
> + { GLAMO_REG_IRQ_ENABLE, 0x0100 },
> + { GLAMO_REG_CLOCK_GEN6, 0x2000 },
> + { GLAMO_REG_CLOCK_GEN7, 0x0101 },
> + { GLAMO_REG_CLOCK_GEN8, 0x0100 },
> + { GLAMO_REG_CLOCK_HOST, 0x000d },
> + /*
> + * b7..b4 = 0 = no wait states on read or write
> + * b0 = 1 select PLL2 for Host interface, b1 = enable it
> + */
> + { GLAMO_REG_HOSTBUS(1), 0x0e03 /* this is replaced by script parser */ },
> + { GLAMO_REG_HOSTBUS(2), 0x07ff }, /* TODO: Disable all */
> + { GLAMO_REG_HOSTBUS(10), 0x0000 },
> + { GLAMO_REG_HOSTBUS(11), 0x4000 },
> + { GLAMO_REG_HOSTBUS(12), 0xf00e },
> +
> + /* S-Media recommended "set tiling mode to 512 mode for memory access
> + * more efficiency when 640x480" */
> + { GLAMO_REG_MEM_TYPE, 0x0c74 }, /* 8MB, 16 word pg wr+rd */
> + { GLAMO_REG_MEM_GEN, 0xafaf }, /* 63 grants min + max */
> +
> + { GLAMO_REG_MEM_TIMING1, 0x0108 },
> + { GLAMO_REG_MEM_TIMING2, 0x0010 }, /* Taa = 3 MCLK */
> + { GLAMO_REG_MEM_TIMING3, 0x0000 },
> + { GLAMO_REG_MEM_TIMING4, 0x0000 }, /* CE1# delay fall/rise */
> + { GLAMO_REG_MEM_TIMING5, 0x0000 }, /* UB# LB# */
> + { GLAMO_REG_MEM_TIMING6, 0x0000 }, /* OE# */
> + { GLAMO_REG_MEM_TIMING7, 0x0000 }, /* WE# */
> + { GLAMO_REG_MEM_TIMING8, 0x1002 }, /* MCLK delay, was 0x1000 */
> + { GLAMO_REG_MEM_TIMING9, 0x6006 },
> + { GLAMO_REG_MEM_TIMING10, 0x00ff },
> + { GLAMO_REG_MEM_TIMING11, 0x0001 },
> + { GLAMO_REG_MEM_POWER1, 0x0020 },
> + { GLAMO_REG_MEM_POWER2, 0x0000 },
> + { GLAMO_REG_MEM_DRAM1, 0x0000 },
> + { GLAMO_SCRIPT_WAIT, 1 },
> + { GLAMO_REG_MEM_DRAM1, 0xc100 },
> + { GLAMO_SCRIPT_WAIT, 1 },
> + { GLAMO_REG_MEM_DRAM1, 0xe100 },
> + { GLAMO_REG_MEM_DRAM2, 0x01d6 },
> + { GLAMO_REG_CLOCK_MEMORY, 0x000b },
> +};
Shouldnt those values be set by your platform bootloader ? They're obviously
not, but I find it weird.


> +#ifdef CONFIG_PM
> +#if 0
> +static struct glamo_script glamo_resume_script[] = {
> +
> + { GLAMO_REG_PLL_GEN1, 0x05db }, /* 48MHz */
> + { GLAMO_REG_PLL_GEN3, 0x0aba }, /* 90MHz */
> + { GLAMO_REG_DFT_GEN6, 1 },
> + { 0xfffe, 100 },
> + { 0xfffd, 0 },
> + { 0x200, 0x0e03 },
> +
> + /*
> + * b9 of this register MUST be zero to get any interrupts on INT#
> + * the other set bits enable all the engine interrupt sources
> + */
> + { GLAMO_REG_IRQ_ENABLE, 0x01ff },
> + { GLAMO_REG_CLOCK_HOST, 0x0018 },
> + { GLAMO_REG_CLOCK_GEN5_1, 0x18b1 },
> +
> + { GLAMO_REG_MEM_DRAM1, 0x0000 },
> + { 0xfffe, 1 },
> + { GLAMO_REG_MEM_DRAM1, 0xc100 },
> + { 0xfffe, 1 },
> + { GLAMO_REG_MEM_DRAM1, 0xe100 },
> + { GLAMO_REG_MEM_DRAM2, 0x01d6 },
> + { GLAMO_REG_CLOCK_MEMORY, 0x000b },
> +};
> +#endif
Please remove all #if 0 code from this driver.

Cheers,
Samuel.

--
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/