Re: [PATCH] ide: Add tx4939ide driver (v3)

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

Hello.

Atsushi Nemoto wrote:

> This is the driver for the Toshiba TX4939 SoC ATA controller.
>
> This controller has standard ATA taskfile registers and DMA
> command/status registers, but the register layout is swapped on big
> endian.  There are some other endian issue and some special registers
> which requires many custom dma_ops/tp_ops routines and build_dmatable.
>
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

   Mostly ACK but there's still a few minor nits...

> diff --git a/drivers/ide/mips/Makefile b/drivers/ide/mips/Makefile
> index 677c7b2..1e0ad98 100644
> --- a/drivers/ide/mips/Makefile
> +++ b/drivers/ide/mips/Makefile
> @@ -1,4 +1,5 @@
>  obj-$(CONFIG_BLK_DEV_IDE_SWARM)		+= swarm.o
>   

  The context have changed here but I guess Bart handled that...

> diff --git a/drivers/ide/mips/tx4939ide.c b/drivers/ide/mips/tx4939ide.c
> new file mode 100644
> index 0000000..6671d64
> --- /dev/null
> +++ b/drivers/ide/mips/tx4939ide.c
> @@ -0,0 +1,775 @@
>   
[...]
> +/* ATA Shadow Registers (8-bit except for DATA which is 16-bit) */
> +#define TX4939IDE_DATA			0x000
>   

   Not sure whether the data register deserves more respect than the 
others. :-)

> +/* H/W DMA Registers  */
> +#define TX4939IDE_DMA_Cmd	0x800	/* 8-bit */
> +#define TX4939IDE_DMA_stat	0x802	/* 8-bit */
>   

   Symbol case still inconsistent...

> +static void tx4939ide_set_dma_mode(ide_drive_t *drive, const u8 mode)
> +{
> +	ide_hwif_t *hwif = drive->hwif;
> +	u32 mask, val;
> +
> +	/* Update Data Transfer Mode for this drive. */
> +	if (mode >= XFER_UDMA_0)
> +		val = mode - XFER_UDMA_0 + 8;
> +	else if (mode >= XFER_MW_DMA_0)
> +		val = mode - XFER_MW_DMA_0 + 5;
> +	else
> +		val = mode - XFER_PIO_0;
>   

   I must've missed that in the previous review but you don't need to 
handle PIO modes in this method.

> +static u16 tx4939ide_check_error_ints(ide_hwif_t *hwif)
> +{
> +	void __iomem *base = TX4939IDE_BASE(hwif);
> +	u16 ctl = tx4939ide_readw(base, TX4939IDE_Int_Ctl);
> +
> +	if (ctl & TX4939IDE_INT_BUSERR) {
> +		/* reset FIFO */
> +		u16 sysctl = tx4939ide_readw(base, TX4939IDE_Sys_Ctl);
> +		tx4939ide_writew(sysctl | 0x4000, base, TX4939IDE_Sys_Ctl);
> +		mmiowb();
> +		/* wait 12GBUSCLK (typ. 60ns @ GBUS200MHz, max 270ns) */
> +		ndelay(270);
> +		tx4939ide_writew(sysctl, base, TX4939IDE_Sys_Ctl);
> +	}
> +	if (ctl & (TX4939IDE_INT_ADDRERR |
> +		    TX4939IDE_INT_DEVTIMING | TX4939IDE_INT_BUSERR))
>   

   Hm, why not line up TX4939IDE_INT_DEVTIMING with TX4939IDE_INT_ADDRERR?

> +static void tx4939ide_clear_irq(ide_drive_t *drive)
> +{
> +	ide_hwif_t *hwif;
> +	void __iomem *base;
> +	u16 ctl;
> +
> +	/*
> +	 * tx4939ide_dma_test_irq() and tx4939ide_dma_end() do all
> +	 * jobs for DMA case.
>   

   Shouldn't it be "job", singular?

> +#ifdef __BIG_ENDIAN
> +static void tx4939ide_dma_host_set(ide_drive_t *drive, int on)
> +{
> +	ide_hwif_t *hwif	= drive->hwif;
> +	u8 unit			= drive->dn & 1;
> +	void __iomem *base = TX4939IDE_BASE(hwif);
> +	u8 dma_stat = tx4939ide_readb(base, TX4939IDE_DMA_stat);
>   

   Hm, why not line up all the initializers? Either do line up all or do 
not line up any...

> +static u8 tx4939ide_read_and_clear_dma_status(void __iomem *base)
>   

   Hum, that's a long name... :-)

> +#ifdef __BIG_ENDIAN
> +/* custom ide_build_dmatable to handle swapped layout */
> +static int tx4939ide_build_dmatable(ide_drive_t *drive, struct request *rq)
> +{
>   
[...]
> +			xcount = bcount & 0xffff;
> +			if (xcount == 0x0000) {
>   

   Hm, I'm not sure this is necessary here... although I didn't see an 
explicit mention that zero count means 64 KB in the datasheet -- which 
it must mean if the BMIDE spec. was followed).
In ide-dma.c this check was added because of CS5530's brain damage...

> +static int tx4939ide_dma_end(ide_drive_t *drive)
> +{
>   
[...]
> +	if ((dma_stat & 7) == 0 &&
> +	    (ctl & (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST)) ==
> +	    (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST))
>   

   Parens around & and | are hardly needed...

> +/* returns 1 if dma irq issued, 0 otherwise */
> +static int tx4939ide_dma_test_irq(ide_drive_t *drive)
> +{
> +	ide_hwif_t *hwif = drive->hwif;
> +	void __iomem *base = TX4939IDE_BASE(hwif);
> +	u16 ctl;
> +	u8 dma_stat, stat;
> +	u16 ide_int;
> +	int found = 0;
> +
> +	ctl = tx4939ide_check_error_ints(hwif);
> +	ide_int = ctl & (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST);
> +	switch (ide_int) {
> +	case TX4939IDE_INT_HOST:
> +		/* On error, XFEREND might not be asserted. */
> +		stat = tx4939ide_readb(base, TX4939IDE_AltStat_DevCtl);
> +		if ((stat & (ATA_BUSY|ATA_DRQ|ATA_ERR)) == ATA_ERR)
> +			found = 1;
> +		else {
> +			/* Wait for XFERINT (Mask HOST and unmask XFERINT) */
>   

   s/XFERINT/XFEREND/

> +			ctl &= ~TX4939IDE_INT_XFEREND << 8;
> +			ctl |= TX4939IDE_INT_HOST << 8;
>   

  The last statement seems superfluous given that the same is achieved 
by the following statement.

> +		}
> +		ctl |= ide_int << 8;
> +		break;
> +	case TX4939IDE_INT_HOST | TX4939IDE_INT_XFEREND:
> +		dma_stat = tx4939ide_readb(base, TX4939IDE_DMA_stat);
> +		if (!(dma_stat & 4))
> +			pr_warning("%s: weird interrupt status. "
> +				   "DMA_stat %#02x int_ctl %#04x\n",
> +				   hwif->name, dma_stat, ctl);
> +		found = 1;
> +		break;
> +	}
> +	/*
> +	 * Do not clear XFERINT, HOST now.  They will be cleared by
>   

   s/XFERINT/XFEREND/

> +static u8 tx4939ide_read_sff_dma_status(ide_hwif_t *hwif)
> +{
> +	void __iomem *base = TX4939IDE_BASE(hwif);
> +	return tx4939ide_readb(base, TX4939IDE_DMA_stat);
> +}
> +
>   

   Can't ide_read_sff_dma_status() be used in LE mode now that you set 
hwif->dma_base?

> +static void tx4939ide_insw_swap(unsigned long port, void *addr, u32 count)
>   
[...]
> +static void tx4939ide_outsw_swap(unsigned long port, void *addr, u32 count)
>   

   Shouldn't these be inline (if you really need them)?

> +static int __init tx4939ide_probe(struct platform_device *pdev)
> +{
>   
[...]
> +	pr_info("TX4939 IDE interface (%lx,%d)\n", mapbase, irq);
>   

   Hm, the bare numbers in the log won't be informative, could you add 
"base" and "IRQ"?

MBR, Sergei