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

[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>
>   

   Again, mostly ACK but there are some things that I haven't noticed 
before...

> diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
> index 6c6dd2f..c23ff28 100644
> --- a/drivers/ide/Kconfig
> +++ b/drivers/ide/Kconfig
> @@ -746,6 +746,12 @@ config BLK_DEV_IDE_AU1XXX_SEQTS_PER_RQ
>         default "128"
>         depends on BLK_DEV_IDE_AU1XXX
>  
> +config BLK_DEV_IDE_TX4939
> +	tristate "TX4939 internal IDE support"
> +	depends on SOC_TX4939
> +	select BLK_DEV_IDEDMA_SFF
> +	select IDE_TIMINGS
>   

   BTW, are you really using anything from ide-timings.c?

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

   Speaking about cnsistency, "data" is not an acronym. :-)

> +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 {
> +		BUG_ON(mode < XFER_MW_DMA_0);
>   

   Should be no need for that as it's filtered out at the higher level...

> +static int tx4939ide_dma_setup(ide_drive_t *drive)
> +{
> +	ide_hwif_t *hwif = drive->hwif;
> +	void __iomem *base = TX4939IDE_BASE(hwif);
> +	struct request *rq = hwif->hwgroup->rq;
> +	unsigned int reading;
>   

   BTW, shoudn't it be of type 'u8'?

> +	int nent;
> +
> +	if (rq_data_dir(rq))
> +		reading = 0;
> +	else
> +		reading = 1 << 3;
>   

   I think it's time to start using ATA_DMA_WR instead of the bare 
number; maybe I'll submit a patch to do this for ide-dma-sff.c...

> +static int tx4939ide_dma_end(ide_drive_t *drive)
> +{
>   
[...]
> +	tx4939ide_writeb(dma_cmd & ~1, base, TX4939IDE_DMA_Cmd);
>   

   Suggesting s/1/ATA_DMA_START/...
   OTOH, might be addressed by a followup patch converting every 
SFF-8038i compatible driver, if I (or Bart) ever get to it...

> +/* returns 1 if dma irq issued, 0 otherwise */
>   

   OTOH, DMA and IRQ are acronyms... :-)

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

   Er, need spaces around | for consistency...

> +			found = 1;
> +		else
> +			/* Wait for XFEREND (Mask HOST and unmask XFEREND) */
> +			ctl &= ~TX4939IDE_INT_XFEREND << 8;
> +		ctl |= ide_int << 8;
> +		break;
> +	case TX4939IDE_INT_HOST | TX4939IDE_INT_XFEREND:
> +		dma_stat = tx4939ide_readb(base, TX4939IDE_DMA_Stat);
> +		if (!(dma_stat & 4))
>   

   s/4/ATA_DMA_INTR/?

> +static void tx4939ide_init_hwif(ide_hwif_t *hwif)
> +{
> +	void __iomem *base = TX4939IDE_BASE(hwif);
> +
> +	/* Soft Reset */
> +	tx4939ide_writew(0x8000, base, TX4939IDE_Sys_Ctl);
> +	mmiowb();
> +	/* at least 20 UPSCLK (typ. 100ns @ GBUS200MHz, max 450ns) */
>   

   Not 20 GBUSCLK?

> +static int tx4939ide_init_dma(ide_hwif_t *hwif, const struct ide_port_info *d)
> +{
> +	hwif->dma_base = (unsigned long)TX4939IDE_BASE(hwif) +
>   

   Hum, casting 'hwif->extra_base' to 'void __iomem *' and then back to 
'unsigned long' is too much, don't you think?
 
> +#ifdef __BIG_ENDIAN
> +
> +static u8 tx4939ide_read_sff_dma_status(ide_hwif_t *hwif)
> +{
> +	void __iomem *base = TX4939IDE_BASE(hwif);
>   

   No new line after declaration...

> +	return tx4939ide_readb(base, TX4939IDE_DMA_Stat);
> +}
> +
> +/* custom iops (independent from SWAP_IO_SPACE) */
> +static u8 tx4939ide_inb(unsigned long port)
> +{
> +	return (u8)__raw_readb((void __iomem *)port);
>   

   Doesn't __raw_readb() return 'u8'?

> +static void tx4939ide_tf_load(ide_drive_t *drive, ide_task_t *task)
> +{
>   
[...]
> +	if (task->tf_flags & IDE_TFLAG_OUT_DEVICE)
> +		tx4939ide_outb((tf->device & HIHI) | drive->select,
> +			       io_ports->device_addr);
> +
> +	tx4939ide_tf_load_fixup(drive, task);
>   

   Might be worth calling tx4939ide_tf_load_fixup() under the preceding 
*if* and removing the corresponding *if* from that function...

> +static void tx4939ide_tf_load(ide_drive_t *drive, ide_task_t *task)
> +{
> +	ide_tf_load(drive, task);
> +	tx4939ide_tf_load_fixup(drive, task);
>   

   ... and adding *if* here.

> +static int __init tx4939ide_probe(struct platform_device *pdev)
> +{
> +	hw_regs_t hw;
> +	hw_regs_t *hws[] = { &hw, NULL, NULL, NULL };
> +	struct ide_host *host;
> +	struct resource *res;
> +	int irq;
> +	unsigned long mapbase;
> +	int ret;
>   

   Variables 'írq' and 'ret' could be on the same line...

> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return -ENODEV;
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	mapbase = (unsigned long)devm_ioremap(&pdev->dev, res->start,
> +					      res->end - res->start + 1);
>   

   Looks like you've forgotten to call request_mem_region()...

MBR, Sergei