On Fri, 12 Sep 2008 02:33:12 +0400, Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx> wrote:
> > +static void tx4939ide_dma_host_set(ide_drive_t *drive, int on)
> > +{
> > + ide_hwif_t *hwif = HWIF(drive);
> > + u8 unit = drive->dn & 1;
> > + unsigned long base = TX4939IDE_BASE(hwif);
> > + u8 dma_stat = TX4939IDE_readb(base, DMA_stat);
> > +
> > + if (on)
> > + dma_stat |= (1 << (5 + unit));
> > + else
> > + dma_stat &= ~(1 << (5 + unit));
> > +
> > + TX4939IDE_writeb(dma_stat, base, DMA_stat);
> > +}
> >
>
> BTW, you could save on using ide_dma_host_set() in LE mode if you'd
> set hwif->dma_base properly...
Yes. I like endian-free approach in general, but there is already
some ifdefs in this driver. I have no strong opinion here.
> > +static int __tx4939ide_dma_setup(ide_drive_t *drive)
> > +{
> > + ide_hwif_t *hwif = drive->hwif;
> > + struct request *rq = HWGROUP(drive)->rq;
> > + unsigned int reading;
> > + u8 dma_stat;
> > + unsigned long base = TX4939IDE_BASE(hwif);
> > +
> [...]
> > +
> > + /* read DMA status for INTR & ERROR flags */
> > + dma_stat = TX4939IDE_readb(base, DMA_stat);
> > +
> > + /* clear INTR & ERROR flags */
> > + TX4939IDE_writeb(dma_stat | 6, base, DMA_stat);
> > + /* recover intmask cleared by writing to bit2 of DMA_stat */
> > + TX4939IDE_writew(TX4939IDE_IGNORE_INTS << 8, base, int_ctl);
> >
>
> I think it might be worth factoring the BMDMA status clearing code
> into a separate function...
OK. Good idea.
> > +static int tx4939ide_dma_setup(ide_drive_t *drive)
> > +{
> > + ide_hwif_t *hwif = HWIF(drive);
> > + unsigned long base = TX4939IDE_BASE(hwif);
> > + int is_slave = drive->dn & 1;
> > + unsigned int nframes;
> > + int rc, i;
> > + unsigned int sect_size = queue_hardsect_size(drive->queue);
> > + u16 select_data;
> > +
> > + select_data = (hwif->select_data >> (is_slave ? 16 : 0)) & 0xffff;
> > + TX4939IDE_writew(select_data, base, Sys_Ctl);
> > + if (is_slave)
> > + TX4939IDE_writew(sect_size / 2, base, Xfer_Cnt_2);
> > + else
> > + TX4939IDE_writew(sect_size / 2, base, Xfer_Cnt_1);
> > +
> > + rc = __tx4939ide_dma_setup(drive);
>
> Hm, I think you need to call this earlier and do an early exit if it
> returns 1.
>
> > + if (rc == 0) {
> >
>
> Better check for !=0, return early and avoid unnecessary
> indentatiuon, isn't it?
It mighet be better. I'll try it.
> > +static void tx4939ide_dma_start(ide_drive_t *drive)
> > +{
> > + ide_hwif_t *hwif = drive->hwif;
> > + unsigned long base = TX4939IDE_BASE(hwif);
> > + u8 dma_cmd;
> > +
> > + /* Note that this is done *after* the cmd has
> > + * been issued to the drive, as per the BM-IDE spec.
> > + */
> > + dma_cmd = TX4939IDE_readb(base, DMA_Cmd);
> > + /* start DMA */
> > + TX4939IDE_writeb(dma_cmd | 1, base, DMA_Cmd);
> > +
> > + wmb();
> > +}
> >
>
> You could save by using ide_dma_start() in LE mode too...
Ditto.
> > +#ifdef __BIG_ENDIAN
> > +/* custom iops (independent from SWAP_IO_SPACE) */
> > +static u8 mm_inb(unsigned long port)
> > +{
> > + return (u8)readb((void __iomem *)port);
> > +}
> > +static void mm_outb(u8 value, unsigned long port)
> > +{
> > + writeb(value, (void __iomem *)port);
> > +}
> >
>
> I'm not sure readb()/writeb() are good substitute for
> __raw_readb()/__raw_writeb() as __swizzle_addr_b() might be actually
> changing the address...
__swizzle_addr_b() used for both readb() and __raw_readb(). ioswabb()
is used for readb() and __raw_ioswabb() is used for __raw_readb().
Hm, __raw_readb() might be better for consistency, though I cannot
imagine any custom ioswabb() ;-)
---
Atsushi Nemoto
