Re: New IDE driver for review

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

Hi!

Looks fine.  Thanks for fixing it.

I think that the best way to push this driver is through 
Andrew Morton with the rest of cris-v32 changes.

Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

Cheers,
Bartlomiej

On 6/27/05, Mikael Starvik <mikael.starvik@axis.com> wrote:
> Hi again!
> 
> Great review comments, thanks! Fixed everything according to your remarks
> with the following comments:
> 
> >previously there was a special case for IDE_STATUS_OFFSET here,
> >is it not needed any longer?
> 
> That special case was for old crap disks but ok, I have readded the
> check just to be sure.
> 
> >There is also number of changes w.r.t. v10 support:
> >* reset and initialization sequences are changed
> 
> Yes, but that is ok
> 
> >* "PIO comment" is gone
> 
> Readded
> 
> >* RESET_DMA()/WAIT_DMA() pairs are gone
> 
> They were needed for the crap disks mentioned above, readded to be sure.
> 
> >* 65536 sg size limit is gone
> 
> Readded again with a new constant MAX_DESCR_SIZE.
> 
> >* ->speedproc() method is added
> 
> Yes, but that is a good thing.
> 
> >It would make much sense to separate these changes from addition of v32
> >support, this way testing for potential regressions would be much easier:
> 
> I agree and will do that in the future. In this particular case it would
> require that I redo some of the work and tests so I would prefer a big
> patch in this case (that is, add the new file and remove the old).
> 
> Updated file has been attached.
> 
> /Mikael
> 
> -----Original Message-----
> From: Bartlomiej Zolnierkiewicz [mailto:bzolnier@gmail.com]
> Sent: Monday, June 20, 2005 9:15 PM
> To: Mikael Starvik
> Cc: Mikael Starvik; linux-ide@vger.kernel.org
> Subject: Re: New IDE driver for review
> 
> 
> On 6/17/05, Mikael Starvik <mikael.starvik@axis.com> wrote:
> > Hi,
> 
> Hi,
> 
> > I have created a merged driver for CRIS v10 and v32. Do you think this
> > approach is better? In that case you'll also need a Makefile patch as
> > follows below.
> 
> Looks much better.  Thanks for doing this.
> 
> Few comments:
> 
> > void
> > cris_ide_outw(unsigned short data, unsigned long reg) {
> >       int timeleft;
> >
> >       LOWDB(printk("ow: data 0x%x, reg 0x%x\n", data, reg));
> >
> >       /* note the lack of handling any timeouts. we stop waiting, but we
> don't
> >        * really notify anybody.
> >        */
> >
> >       timeleft = IDE_REGISTER_TIMEOUT;
> >       /* wait for busy flag */
> >       do {
> >               timeleft--;
> >       } while(timeleft && cris_ide_busy());
> >
> >       /*
> >        * Fall through at a timeout, so the ongoing command will be
> >        * aborted by the write below, which is expected to be a dummy
> >        * command to the command register.  This happens when a faulty
> >        * drive times out on a command.  See comment on timeout in
> >        * INB.
> >        */
> >       if(!timeleft)
> >               printk("ATA timeout reg 0x%lx := 0x%x\n", reg, data);
> >
> >       cris_ide_write_command(reg|data); /* write data to the drive's
> register */
> >
> >       timeleft = IDE_REGISTER_TIMEOUT;
> >       /* wait for transmitter ready */
> >       do {
> >               timeleft--;
> >       } while(timeleft && cris_ide_busy());
> 
> previously it was:
> 
>         while(timeleft && !(*R_ATA_STATUS_DATA &
>                             IO_MASK(R_ATA_STATUS_DATA, tr_rdy)))
>                 timeleft--;
> 
> for v10, is it OK to use busy instead of tr_rdy?
> 
> > }
> 
> > unsigned short
> > cris_ide_inw(unsigned long reg) {
> >       int timeleft;
> >       unsigned short val;
> >
> >       timeleft = IDE_REGISTER_TIMEOUT;
> >       /* wait for busy flag */
> >       do {
> >               timeleft--;
> >       } while(timeleft && cris_ide_busy());
> >
> >       if(!timeleft)
> >               return 0;
> 
> previously there was a special case for IDE_STATUS_OFFSET here,
> is it not needed any longer?
> 
> >       cris_ide_write_command(reg | cris_pio_read);
> >
> >       timeleft = IDE_REGISTER_TIMEOUT;
> >       /* wait for available */
> >       do {
> >               timeleft--;
> >       } while(timeleft && !cris_ide_data_available(&val));
> >
> >       if(!timeleft)
> >               return 0;
> >
> >       LOWDB(printk("inb: 0x%x from reg 0x%x\n", val & 0xff, reg));
> >
> >       return val;
> > }
> 
> > static void
> > cris_atapi_output_bytes (ide_drive_t *drive, void *buffer, unsigned int
> bytecount)
> > {
> >       D(printk("atapi_output_bytes, dreg 0x%x, buffer 0x%x, count %d\n",
> >                0, buffer, bytecount));
> 
> dreg?
> 
> >       if(bytecount & 1) {
> >               printk("odd bytecount %d in atapi_out_bytes!\n", bytecount);
> >               bytecount++;
> >       }
> >
> >       cris_ide_fill_descriptor(&mydescr, buffer, bytecount, 1);
> >       cris_ide_start_dma(drive, &mydescr, 0, TYPE_PIO, bytecount);
> >
> >       /* wait for completion */
> >
> >       LED_DISK_WRITE(1);
> >       LED_DISK_READ(1);
> >       cris_ide_wait_dma(1);
> 
> cris_ide_wait_dma(0) ?
> 
> >       LED_DISK_WRITE(0);
> > }
> 
> > static int config_drive_for_dma (ide_drive_t *drive)
> > {
> >       struct hd_driveid *id = drive->id;
> >
> >       if (id && (id->capability & 1)) {
> >               /* Enable DMA on any drive that supports mword2 DMA */
> >               if ((id->field_valid & 2) &&
> >                   ((id->dma_mword & 0xff00) || (id->dma_ultra & 0xff00)))
> {
> 
> no checking of (id->field_valid & 4)
> 
> DMA will be used only if drive has it already enabled
> 
> >                       drive->using_dma = 1;
> 
> drive->using_dma shouldn't be touched at this layer
> 
> >                       return 0;               /* DMA enabled */
> >               }
> >       }
> >       drive->using_dma = 0;
> >       return 0;       /* DMA not enabled */
> 
> DMA whitelist/blacklist checking is missing
> 
> > }
> 
> should look more like:
> 
> static int cris_config_drive_for_dma(ide_drive_t *drive)
> {
>         u8 speed = ide_dma_speed(drive, 0); /* no UDMA for now */
> 
>         if (!speed)
>                 return 0;
> 
>         speed_cris_ide(drive, speed);
>         ide_config_drive_speed(drive, speed);
> 
>         return ide_dma_enable(drive);
> }
> 
> >  * For ATAPI devices, we just prepare for DMA and return. The caller
> should
> >  * then issue the packet command to the drive and call us again with
> >  * ide_dma_begin afterwards.
> 
> this comment got re-introduced
> s/ide_dma_begin/->dma_start()/
> 
> > static int cris_dma_check(ide_drive_t *drive)
> > {
> >       int speed = ide_dma_speed(drive, 0); /* No UDMA for now */
> 
> No UDMA?
> 
> >       speed_cris_ide(drive, speed);
> >       ide_config_drive_speed(drive, speed);
> >       return config_drive_for_dma (drive);
> > }
> 
> should look more like:
> 
> static int cris_dma_check(ide_drive_t *drive)
> {
>         ide_hwif_t *hwif = drive->hwif;
>         struct hd_driveid *id = drive->id;
> 
>         if (id && (id->capability & 1)) {
>                 if (ide_use_dma(drive) {
>                         if (cris_config_drive_for_dma(drive))
>                                 return hwif->ide_dma_on(drive);
>                 }
>         }
> 
>         return hwif->ide_dma_off_quietly(drive);
> }
> 
> > static int cris_prepare_dma(ide_drive_t *drive, int atapi, int reading)
> 
> atapi and reading arguments are unused
> 
> > {
> >       struct request *rq = drive->hwif->hwgroup->rq;
> >       if (cris_ide_build_dmatable (drive)) {
> >               ide_map_sg(drive, rq);
> >               return 1;
> >       }
> >
> >       return 0;
> > }
> >
> > static int cris_dma_setup(ide_drive_t *drive)
> > {
> >       struct request *rq = drive->hwif->hwgroup->rq;
> >       int ret;
> >       if (rq_data_dir(rq))
> >               ret = cris_prepare_dma(drive, 0, 0);
> >         else
> >               ret = cris_prepare_dma(drive, 0, 1);
> >       if (ret)
> >               return ret;
> >       drive->waiting_for_dma = 1;
> >       return 0;
> > }
> 
> should look more like:
> 
> static int cris_dma_setup(ide_drive_t *drive)
> {
>         struct request *rq = drive->hwif->hwgroup->rq;
> 
>         if (cris_ide_build_dmatable (drive)) {
>                 ide_map_sg(drive, rq);
>                 return 1;
>         }
> 
>         drive->waiting_for_dma = 1;
> 
>         return 0;
> }
> 
> 
> cris_ide_reset() and cris_ide_init() should be marked with __init
> (for both archs)
> 
> 
> There is also number of changes w.r.t. v10 support:
> * reset and initialization sequences are changed
> * "PIO comment" is gone
> * RESET_DMA()/WAIT_DMA() pairs are gone
> * 65536 sg size limit is gone
> * ->speedproc() method is added
> * ...
> 
> It would make much sense to separate these changes from addition of v32
> support, this way testing for potential regressions would be much easier:
> 
> 1. do v10 changes
> 2. add abstraction layer
> 3. add v32 support
> 4. rename driver
> 
> One big patch is also fine with me as long as it is correct
> (IMHO it is easier to achieve correctness with a patch series).
> 
> Cheers,
> Bartlomiej
>