Re: [PATCH] Palmchip BK3710 IDE driver

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

Hi,

On Friday 25 January 2008, Anton Salnikov wrote:
> This is Palmchip BK3710 IDE controller support for kernel version 2.6.24.
> The IDE controller logic supports PIO, multiword DMA and ultra-DMA modes.
> Supports interface to compact Flash (CF) configured in True-IDE mode.

Unfortunately some changes merged after 2.6.24 broke the build:

drivers/ide/arm/palm_bk3710.c: In function ‘palm_bk3710_set_dma_mode’:
drivers/ide/arm/palm_bk3710.c:243: error: ‘struct hwif_s’ has no member named ‘dma_master’
drivers/ide/arm/palm_bk3710.c: In function ‘palm_bk3710_set_pio_mode’:
drivers/ide/arm/palm_bk3710.c:259: error: ‘struct hwif_s’ has no member named ‘dma_master’

[ ->dma_master is gone, ->dma_base should be used instead ]

drivers/ide/arm/palm_bk3710.c: In function ‘palm_bk3710_probe’:
drivers/ide/arm/palm_bk3710.c:389: error: too many arguments to function ‘ide_register_hw’

[ 'initializing' argument is gone, just removing it is OK ]

drivers/ide/arm/palm_bk3710.c:404: error: too many arguments to function ‘ide_setup_dma’

[ 'num_ports' argument is gone, just removing it is OK ]


Please re-sync the patch with the current Linus' git tree.


There are also few less critical issues left...

> Signed-off-by: Anton Salnikov <asalnikov@ru.mvista.com>
> ---
> 
>  drivers/ide/Kconfig           |    8 
>  drivers/ide/arm/Makefile      |    1 
>  drivers/ide/arm/palm_bk3710.c |  424 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/ide/ide-proc.c        |    1 
>  include/linux/ide.h           |    2 
>  5 files changed, 435 insertions(+), 1 deletion(-)
> 
> Index: 2.6.24.ide/drivers/ide/Kconfig
> ===================================================================
> --- 2.6.24.ide.orig/drivers/ide/Kconfig
> +++ 2.6.24.ide/drivers/ide/Kconfig
> @@ -1008,6 +1008,14 @@ config BLK_DEV_Q40IDE
>  	  normally be on; disable it only if you are running a custom hard
>  	  drive subsystem through an expansion card.
>  
> +config BLK_DEV_PALMCHIP_BK3710
> +	bool "Palmchip bk3710 IDE controller support"

'tristate'

> +	depends on ARCH_DAVINCI
> +	select BLK_DEV_IDEDMA_PCI
> +	help
> +	  Say Y here if you want to support the onchip IDE controller on the
> +	  TI DaVinci SoC
> +
>  config BLK_DEV_MPC8xx_IDE
>  	bool "MPC8xx IDE support"
>  	depends on 8xx && (LWMON || IVMS8 || IVML24 || TQM8xxL) && IDE=y && 
> BLK_DEV_IDE=y && !PPC_MERGE
> Index: 2.6.24.ide/drivers/ide/arm/Makefile
> ===================================================================
> --- 2.6.24.ide.orig/drivers/ide/arm/Makefile
> +++ 2.6.24.ide/drivers/ide/arm/Makefile

[...]

> +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);
> +}

drivers/ide/arm/palm_bk3710.c: In function ‘hwif_read8’:
drivers/ide/arm/palm_bk3710.c:93: warning: passing argument 1 of ‘readb’ makes pointer from integer without a cast
drivers/ide/arm/palm_bk3710.c: In function ‘hwif_write8’:
drivers/ide/arm/palm_bk3710.c:98: warning: passing argument 2 of ‘writeb’ makes pointer from integer without a cast
drivers/ide/arm/palm_bk3710.c: In function ‘hwif_read16’:
drivers/ide/arm/palm_bk3710.c:103: warning: passing argument 1 of ‘readw’ makes pointer from integer without a cast
drivers/ide/arm/palm_bk3710.c: In function ‘hwif_write16’:
drivers/ide/arm/palm_bk3710.c:108: warning: passing argument 2 of ‘writew’ makes pointer from integer without a cast
drivers/ide/arm/palm_bk3710.c: In function ‘hwif_read32’:
drivers/ide/arm/palm_bk3710.c:113: warning: passing argument 1 of ‘readl’ makes pointer from integer without a cast
drivers/ide/arm/palm_bk3710.c: In function ‘hwif_write32’:
drivers/ide/arm/palm_bk3710.c:118: warning: passing argument 2 of ‘writel’ makes pointer from integer without a cast

'base' should be of 'void __iomem *' type for read*/write* ops

Besides: because the driver supports multiple controllers now these wrappers
doesn't provide any value and just obfuscate the code (as noticed by Alan).

Please remove them.

> +static void palm_bk3710_setudmamode(u32 base, unsigned int dev,

'base' should be of 'void __iomem *' type for read*/*write ops

[ casting from u32 can be done in the caller ]

> +				    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 ? 4 : 0));
> +	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,

ditto

> +				   unsigned short min_cycle,
> +				   unsigned int mode)
> +{
> +	u8 td, tkw, t0;
> +	u32 val32;
> +	u16 val16;
> +	struct ide_timing *t;
> +	int cycletime;
> +
> +	t = ide_timing_find_mode(mode);
> +	cycletime = max_t(int, t->cycle, min_cycle);
> +
> +	/* DMA Data Setup */
> +	t0 = (cycletime + ide_palm_clk - 1) / ide_palm_clk;
> +	td = (t->active + ide_palm_clk - 1) / ide_palm_clk;
> +	tkw = t0 - td - 1;
> +	td -= 1;
> +
> +	val32 = hwif_read32(base, BK3710_DMASTB) & (0xFF << (dev ? 0 : 8));
> +	val32 |= (td << (dev ? 8 : 0));
> +	hwif_write32(base, val32, BK3710_DMASTB);
> +
> +	val32 = hwif_read32(base, BK3710_DMARCVR) & (0xFF << (dev ? 0 : 8));
> +	val32 |= (tkw << (dev ? 8 : 0));
> +	hwif_write32(base, val32, BK3710_DMARCVR);
> +
> +	/* Disable UDMA for Device */
> +	val16 = hwif_read16(base, BK3710_UDMACTL) & ~(1 << dev);
> +	hwif_write16(base, val16, BK3710_UDMACTL);
> +}
> +
> +static void palm_bk3710_setpiomode(u32 base, ide_drive_t *mate,

ditto

> +				   unsigned int dev, unsigned int cycletime,
> +				   unsigned int mode)
> +{
> +	u8 t2, t2i, t0;
> +	u32 val32;
> +	struct ide_timing *t;
> +
> +	/* PIO Data Setup */
> +	t0 = (cycletime + ide_palm_clk - 1) / ide_palm_clk;
> +	t2 = (ide_timing_find_mode(XFER_PIO_0 + mode)->active +
> +	      ide_palm_clk - 1)	/ ide_palm_clk;
> +
> +	t2i = t0 - t2 - 1;
> +	t2 -= 1;
> +
> +	val32 = hwif_read32(base, BK3710_DATSTB) & (0xFF << (dev ? 0 : 8));
> +	val32 |= (t2 << (dev ? 8 : 0));
> +	hwif_write32(base, val32, BK3710_DATSTB);
> +
> +	val32 = hwif_read32(base, BK3710_DATRCVR) & (0xFF << (dev ? 0 : 8));
> +	val32 |= (t2i << (dev ? 8 : 0));
> +	hwif_write32(base, val32, BK3710_DATRCVR);
> +
> +	if (mate && mate->present) {
> +		u8 mode2 = ide_get_best_pio_mode(mate, 255, 4);
> +
> +		if (mode2 < mode)
> +			mode = mode2;
> +	}
> +
> +	/* TASKFILE Setup */
> +	t = ide_timing_find_mode(XFER_PIO_0 + mode);
> +	t0 = (t->cyc8b + ide_palm_clk - 1) / ide_palm_clk;
> +	t2 = (t->act8b + ide_palm_clk - 1) / ide_palm_clk;
> +
> +	t2i = t0 - t2 - 1;
> +	t2 -= 1;
> +
> +	val32 = hwif_read32(base, BK3710_REGSTB) & (0xFF << (dev ? 0 : 8));
> +	val32 |= (t2 << (dev ? 8 : 0));
> +	hwif_write32(base, val32, BK3710_REGSTB);
> +
> +	val32 = hwif_read32(base, BK3710_REGRCVR) & (0xFF << (dev ? 0 : 8));
> +	val32 |= (t2i << (dev ? 8 : 0));
> +	hwif_write32(base, val32, BK3710_REGRCVR);
> +}
> +
> +static void palm_bk3710_set_dma_mode(ide_drive_t *drive, u8 xferspeed)
> +{
> +	int is_slave = drive->dn & 1;
> +	u32 base = drive->hwif->dma_master;
> +
> +	if (xferspeed >= XFER_UDMA_0) {
> +		palm_bk3710_setudmamode(base, is_slave,
> +					xferspeed - XFER_UDMA_0);
> +	} else {
> +		palm_bk3710_setdmamode(base, is_slave, drive->id->eide_dma_min,
> +				       xferspeed);
> +	}
> +}
> +
> +static void palm_bk3710_set_pio_mode(ide_drive_t *drive, u8 pio)
> +{
> +	unsigned int cycle_time;
> +	int is_slave = drive->dn & 1;
> +	ide_drive_t *mate;
> +	u32 base = drive->hwif->dma_master;
> +
> +	/*
> +	 * Obtain the drive PIO data for tuning the Palm Chip registers
> +	 */
> +	cycle_time = ide_pio_cycle_time(drive, pio);
> +	mate = ide_get_paired_drive(drive);
> +	palm_bk3710_setpiomode(base, mate, is_slave, cycle_time, pio);
> +}
> +
> +static void __devinit palm_bk3710_chipinit(u32 base)

'u32' -> 'void __iomem *'

> +{
> +	/*
> +	 * enable the reset_en of ATA controller so that when ata signals
> +	 * are brought out, by writing into device config. at that
> +	 * time por_n signal should not be 'Z' and have a stable value.
> +	 */
> +	hwif_write32(base, 0x0300, BK3710_MISCCTL);
> +
> +	/* wait for some time and deassert the reset of ATA Device. */
> +	mdelay(100);
> +
> +	/* Deassert the Reset */
> +	hwif_write32(base, 0x0200, BK3710_MISCCTL);
> +
> +	/*
> +	 * Program the IDETIMP Register Value based on the following assumptions
> +	 *
> +	 * (ATA_IDETIMP_IDEEN		, ENABLE ) |
> +	 * (ATA_IDETIMP_SLVTIMEN	, DISABLE) |
> +	 * (ATA_IDETIMP_RDYSMPL		, 70NS)    |
> +	 * (ATA_IDETIMP_RDYRCVRY	, 50NS)    |
> +	 * (ATA_IDETIMP_DMAFTIM1	, PIOCOMP) |
> +	 * (ATA_IDETIMP_PREPOST1	, DISABLE) |
> +	 * (ATA_IDETIMP_RDYSEN1		, DISABLE) |
> +	 * (ATA_IDETIMP_PIOFTIM1	, DISABLE) |
> +	 * (ATA_IDETIMP_DMAFTIM0	, PIOCOMP) |
> +	 * (ATA_IDETIMP_PREPOST0	, DISABLE) |
> +	 * (ATA_IDETIMP_RDYSEN0		, DISABLE) |
> +	 * (ATA_IDETIMP_PIOFTIM0	, DISABLE)
> +	 */
> +	hwif_write16(base, 0xB388, BK3710_IDETIMP);
> +
> +	/*
> +	 * Configure  SIDETIM  Register
> +	 * (ATA_SIDETIM_RDYSMPS1	,120NS ) |
> +	 * (ATA_SIDETIM_RDYRCYS1	,120NS )
> +	 */
> +	hwif_write8(base, 0, BK3710_SIDETIM);
> +
> +	/*
> +	 * UDMACTL Ultra-ATA DMA Control
> +	 * (ATA_UDMACTL_UDMAP1	, 0 ) |
> +	 * (ATA_UDMACTL_UDMAP0	, 0 )
> +	 *
> +	 */
> +	hwif_write16(base, 0, BK3710_UDMACTL);
> +
> +	/*
> +	 * MISCCTL Miscellaneous Conrol Register
> +	 * (ATA_MISCCTL_RSTMODEP	, 1) |
> +	 * (ATA_MISCCTL_RESETP		, 0) |
> +	 * (ATA_MISCCTL_TIMORIDE	, 1)
> +	 */
> +	hwif_write32(base, 0x201, BK3710_MISCCTL);
> +
> +	/*
> +	 * IORDYTMP IORDY Timer for Primary Register
> +	 * (ATA_IORDYTMP_IORDYTMP     , 0xffff  )
> +	 */
> +	hwif_write32(base, 0xFFFF, BK3710_IORDYTMP);
> +
> +	/*
> +	 * Configure BMISP Register
> +	 * (ATA_BMISP_DMAEN1	, DISABLE )	|
> +	 * (ATA_BMISP_DMAEN0	, DISABLE )	|
> +	 * (ATA_BMISP_IORDYINT	, CLEAR)	|
> +	 * (ATA_BMISP_INTRSTAT	, CLEAR)	|
> +	 * (ATA_BMISP_DMAERROR	, CLEAR)
> +	 */
> +	hwif_write16(base, 0, BK3710_BMISP);
> +
> +	palm_bk3710_setpiomode(base, NULL, 0, 600, 0);
> +	palm_bk3710_setpiomode(base, NULL, 1, 600, 0);
> +}
> +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;

ditto

> +	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_ERR "failed to get memory region resource\n");
> +		return -ENODEV;
> +	}
> +	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (irq == NULL) {
> +		printk(KERN_ERR "failed to get IRQ resource\n");
> +		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 (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;
> +	default_hwif_mmiops(hwif);
> +	hwif->ultra_mask = 0x1f;	/* Ultra DMA Mode 4 Max
> +						(input clk 99MHz) */

I've just noticed that there is no cable detection for UDMA modes > UDMA2?

> +	hwif->mwdma_mask = 0x7;
> +	hwif->drives[0].autotune = 1;
> +	hwif->drives[1].autotune = 1;
> +
> +	ide_setup_dma(hwif, mem->start, 8);
> +
> +	return 0;
> +}

[...]

Otherwise the driver looks fine and will be merged once the above issues
get addressed so please recast+resubmit it.

Thanks,
Bart