Re: [PATCH] Palmchip BK3710 IDE driver

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

Anton Salnikov wrote:

> This is Palmchip BK3710 IDE controller support for kernel version 2.6.24-rc8.
> The IDE controller logic supports PIO, multiword DMA and ultra-DMA modes.
> Supports interface to compact Flash (CF) configured in True-IDE mode.

> Signed-off-by: Anton Salnikov <asalnikov@ru.mvista.com>
> ---

> Index: 2.6.24-rc8.ide/drivers/ide/arm/palm_bk3710.c
> ===================================================================
> --- /dev/null
> +++ 2.6.24-rc8.ide/drivers/ide/arm/palm_bk3710.c
> @@ -0,0 +1,428 @@
> +static inline u8 hwif_read8(u32 base, u32 reg)
> +{
> +	return readb(base + reg);
> +}
> +
> +static inline void hwif_write8(u32 base, u8 val, u32 reg)
> +{
> +	writeb(val, base + reg);
> +}
> +
> +static inline u16 hwif_read16(u32 base, u32 reg)
> +{
> +	return readw(base + reg);
> +}
> +
> +static inline void hwif_write16(u32 base, u16 val, u32 reg)
> +{
> +	writew(val, base + reg);
> +}
> +
> +static inline u32 hwif_read32(u32 base, u32 reg)
> +{
> +	return readl(base + reg);
> +}
> +
> +static inline void hwif_write32(u32 base, u32 val, u32 reg)
> +{
> +	writel(val, base + reg);
> +}

    Hm, I don't see why you need those accessors but well...

> +static void palm_bk3710_setudmamode(u32 base, unsigned int dev,
> +				    unsigned int mode)
> +{
> +	u8 tenv, trp, t0;
> +	u32 val32;
> +	u16 val16;
> +
> +	/* DMA Data Setup */
> +	t0 = (palm_bk3710_udmatimings[mode].cycletime + ide_palm_clk - 1)
> +			/ ide_palm_clk - 1;
> +	tenv = (20 + ide_palm_clk - 1) / ide_palm_clk - 1;
> +	trp = (palm_bk3710_udmatimings[mode].rptime + ide_palm_clk - 1)
> +			/ ide_palm_clk - 1;
> +
> +	/* udmatim Register */
> +	val16 = hwif_read16(base, BK3710_UDMATIM) & (dev ? 0xFF0F : 0xFFF0);
> +	val16 |= (mode << ((!dev) ? 0 : 4));

    Why not (dev ? 4 : 0) like the rest?

> +	hwif_write16(base, val16, BK3710_UDMATIM);
> +
> +	/* udmastb Ultra DMA Access Strobe Width */
> +	val32 = hwif_read32(base, BK3710_UDMASTB) & (0xFF << (dev ? 0 : 8));
> +	val32 |= (t0 << (dev ? 8 : 0));
> +	hwif_write32(base, val32, BK3710_UDMASTB);
> +
> +	/* udmatrp Ultra DMA Ready to Pause Time */
> +	val32 = hwif_read32(base, BK3710_UDMATRP) & (0xFF << (dev ? 0 : 8));
> +	val32 |= (trp << (dev ? 8 : 0));
> +	hwif_write32(base, val32, BK3710_UDMATRP);
> +
> +	/* udmaenv Ultra DMA envelop Time */
> +	val32 = hwif_read32(base, BK3710_UDMAENV) & (0xFF << (dev ? 0 : 8));
> +	val32 |= (tenv << (dev ? 8 : 0));
> +	hwif_write32(base, val32, BK3710_UDMAENV);
> +
> +	/* Enable UDMA for Device */
> +	val16 = hwif_read16(base, BK3710_UDMACTL) | (1 << dev);
> +	hwif_write16(base, val16, BK3710_UDMACTL);
> +}
> +
> +static void palm_bk3710_setdmamode(u32 base, unsigned int dev,
> +				   unsigned short min_cycle,
> +				   unsigned int mode)
> +{
> +	u8 td, tkw, t0;
> +	u32 val32;
> +	u16 val16;
> +
> +	unsigned cycletime = max_t(int, ide_timing_find_mode(mode)->cycle,
> +				   min_cycle);

    Hm, why integer comparison and then unsigned maximum?

> +
> +	/* DMA Data Setup */
> +	t0 = (cycletime + ide_palm_clk - 1) / ide_palm_clk;
> +	td = (ide_timing_find_mode(mode)->active + ide_palm_clk - 1)
> +			/ ide_palm_clk;

    Couldn't you call ide_timing_find_mode() only once?

[...]

> +static int __devinit palm_bk3710_probe(struct platform_device *pdev)
> +{
> +	hw_regs_t ide_ctlr_info;
> +	int index = 0;
> +	int pribase;
> +	struct clk *clkp;
> +	struct resource *mem, *irq;
> +	ide_hwif_t *hwif;
> +	u32 base;
> +	int ret;
> +
> +	clkp = clk_get(NULL, "IDECLK");
> +	if (IS_ERR(clkp))
> +		return -ENODEV;
> +
> +	ideclkp = clkp;
> +	clk_enable(ideclkp);
> +	ide_palm_clk = clk_get_rate(ideclkp)/100000;
> +	ide_palm_clk = (10000/ide_palm_clk) + 1;
> +	/* Register the IDE interface with Linux ATA Interface */
> +	memset(&ide_ctlr_info, 0, sizeof(ide_ctlr_info));
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (mem == NULL) {
> +		printk(KERN_INFO "failed to get memory region resource\n");

    This deserves KERN_ERR, not KERN_INFO.

> +		return -ENODEV;
> +	}
> +	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (irq == NULL) {
> +		printk(KERN_INFO "failed to get IRQ resource\n");

    This too...

> +		return -ENODEV;
> +	}
> +
> +	base = mem->start;
> +
> +	/* Configure the Palm Chip controller */
> +	palm_bk3710_chipinit(base);
> +
> +	pribase = mem->start + IDE_PALM_ATA_PRI_REG_OFFSET;
> +	for (index = 0; index < IDE_NR_PORTS - 2; index++)
> +		ide_ctlr_info.io_ports[index] = pribase + index;
> +	ide_ctlr_info.io_ports[IDE_CONTROL_OFFSET] = mem->start +
> +			IDE_PALM_ATA_PRI_CTL_OFFSET;
> +	ide_ctlr_info.irq = irq->start;
> +	ide_ctlr_info.chipset = ide_palm3710;
> +
> +	if (!request_mem_region(mem->start, 8, "palm_bk3710")) {
> +		printk(KERN_ERR "Error, memory region in use.\n");
> +		return -EBUSY;
> +	}

    Well, you either have to also register sub-resources for the IDE 
command/control register blocks since with hwif->mmiops == 1 the IDE core 
won't do this for you, or don't register any sub-resources at all, being 
content with what platform_device_add() did for us. BTW, don't forget extend 
the platform resource to account for IDE command/control register blocks.

> +	if (ide_register_hw(&ide_ctlr_info, NULL, 0, &hwif) < 0) {
> +		printk(KERN_WARNING "Palm Chip BK3710 IDE Register Fail\n");
> +		return -ENODEV;
> +	}
> +
> +	hwif->set_pio_mode = &palm_bk3710_set_pio_mode;
> +	hwif->set_dma_mode = &palm_bk3710_set_dma_mode;
> +	hwif->mmio = 1;

    Well, dealing with a memory-mapped IDE device, the driver lack 
default_hwif_mmiops() call...

MBR, Sergei