From: Sergei Shtylyov <sshtylyov@ru.mvista.com>
To: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Cc: linux-ide@vger.kernel.org, Tejun Heo <htejun@gmail.com>
Subject: Re: [PATCH 6/12] ide: add struct ide_taskfile
Date: Wed, 17 Oct 2007 21:38:28 +0400 [thread overview]
Message-ID: <47164894.9080007@ru.mvista.com> (raw)
In-Reply-To: <200710082311.10925.bzolnier@gmail.com>
Bartlomiej Zolnierkiewicz wrote:
> * Don't set write-only ide_task_t.hobRegister[6] and ide_task_t.hobRegister[7]
> in idedisk_set_max_address_ext().
> * Add struct ide_taskfile and use it in ide_task_t instead of tfRegister[]
> and hobRegister[].
> * Remove no longer needed IDE_CONTROL_OFFSET_HOB define.
> * Add #ifndef/#endif __KERNEL__ around definitions of {task,hob}_struct_t.
> While at it:
> * Use ATA_LBA define for LBA bit (0x40) as suggested by Tejun Heo.
> There should be no functionality changes caused by this patch.
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Nearly-acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> Index: b/drivers/ide/ide-disk.c
> ===================================================================
> --- a/drivers/ide/ide-disk.c
> +++ b/drivers/ide/ide-disk.c
[...]
> + if ((tf->command & 0x01) == 0) {
> + u32 high, low;
Isn't newline needed after declarations?
> + high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) | tf->hob_lbal;
> + low = (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
> addr = ((__u64)high << 24) | low;
> addr++; /* since the return value is (maxlba - 1), we add 1 */
> }
[...]
> @@ -422,33 +417,29 @@ static unsigned long idedisk_set_max_add
[...]
> /* if OK, compute maximum address value */
> - if ((args.tfRegister[IDE_STATUS_OFFSET] & 0x01) == 0) {
> - u32 high = (args.hobRegister[IDE_HCYL_OFFSET] << 16) |
> - (args.hobRegister[IDE_LCYL_OFFSET] << 8) |
> - args.hobRegister[IDE_SECTOR_OFFSET];
> - u32 low = ((args.tfRegister[IDE_HCYL_OFFSET])<<16) |
> - ((args.tfRegister[IDE_LCYL_OFFSET])<<8) |
> - (args.tfRegister[IDE_SECTOR_OFFSET]);
> + if ((tf->command & 0x01) == 0) {
> + u32 high, low;
Again missing newline...
> + high = (tf->hob_lbah << 16) | (tf->hob_lbam << 8) | tf->hob_lbal;
> + low = (tf->lbah << 16) | (tf->lbam << 8) | tf->lbal;
> addr_set = ((__u64)high << 24) | low;
> addr_set++;
> }
> @@ -582,12 +573,13 @@ static sector_t idedisk_capacity (ide_dr
> static int smart_enable(ide_drive_t *drive)
> {
> ide_task_t args;
> + struct ide_taskfile *tf = &args.tf;
>
> memset(&args, 0, sizeof(ide_task_t));
> - args.tfRegister[IDE_FEATURE_OFFSET] = SMART_ENABLE;
> - args.tfRegister[IDE_LCYL_OFFSET] = SMART_LCYL_PASS;
> - args.tfRegister[IDE_HCYL_OFFSET] = SMART_HCYL_PASS;
> - args.tfRegister[IDE_COMMAND_OFFSET] = WIN_SMART;
> + tf->feature = SMART_ENABLE;
> + tf->lbam = SMART_LCYL_PASS;
> + tf->lbah = SMART_HCYL_PASS;
> + tf->command = WIN_SMART;
> args.command_type = IDE_DRIVE_TASK_NO_DATA;
> args.handler = &task_no_data_intr;
> return ide_raw_taskfile(drive, &args, NULL);
> @@ -596,13 +588,14 @@ static int smart_enable(ide_drive_t *dri
> static int get_smart_data(ide_drive_t *drive, u8 *buf, u8 sub_cmd)
> {
> ide_task_t args;
> + struct ide_taskfile *tf = &args.tf;
>
> memset(&args, 0, sizeof(ide_task_t));
> - args.tfRegister[IDE_FEATURE_OFFSET] = sub_cmd;
> - args.tfRegister[IDE_NSECTOR_OFFSET] = 0x01;
> - args.tfRegister[IDE_LCYL_OFFSET] = SMART_LCYL_PASS;
> - args.tfRegister[IDE_HCYL_OFFSET] = SMART_HCYL_PASS;
> - args.tfRegister[IDE_COMMAND_OFFSET] = WIN_SMART;
> + tf->feature = sub_cmd;
> + tf->nsect = 0x01;
> + tf->lbam = SMART_LCYL_PASS;
> + tf->lbah = SMART_HCYL_PASS;
> + tf->command = WIN_SMART;
I guess the code above and below were menat to have the same = indentation...
> args.command_type = IDE_DRIVE_TASK_IN;
> args.data_phase = TASKFILE_IN;
> args.handler = &task_in_intr;
> @@ -808,9 +801,9 @@ static int write_cache(ide_drive_t *driv
>
> if (ide_id_has_flush_cache(drive->id)) {
> memset(&args, 0, sizeof(ide_task_t));
> - args.tfRegister[IDE_FEATURE_OFFSET] = (arg) ?
> + args.tf.feature = arg ?
> SETFEATURES_EN_WCACHE : SETFEATURES_DIS_WCACHE;
> - args.tfRegister[IDE_COMMAND_OFFSET] = WIN_SETFEATURES;
> + args.tf.command = WIN_SETFEATURES;
Same here...
> args.command_type = IDE_DRIVE_TASK_NO_DATA;
> args.handler = &task_no_data_intr;
> err = ide_raw_taskfile(drive, &args, NULL);
> @@ -829,9 +822,9 @@ static int do_idedisk_flushcache (ide_dr
>
> memset(&args, 0, sizeof(ide_task_t));
> if (ide_id_has_flush_cache_ext(drive->id))
> - args.tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE_EXT;
> + args.tf.command = WIN_FLUSH_CACHE_EXT;
> else
> - args.tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE;
> + args.tf.command = WIN_FLUSH_CACHE;
... and here...
> args.command_type = IDE_DRIVE_TASK_NO_DATA;
> args.handler = &task_no_data_intr;
> return ide_raw_taskfile(drive, &args, NULL);
> @@ -845,10 +838,9 @@ static int set_acoustic (ide_drive_t *dr
> return -EINVAL;
>
> memset(&args, 0, sizeof(ide_task_t));
> - args.tfRegister[IDE_FEATURE_OFFSET] = (arg) ? SETFEATURES_EN_AAM :
> - SETFEATURES_DIS_AAM;
> - args.tfRegister[IDE_NSECTOR_OFFSET] = arg;
> - args.tfRegister[IDE_COMMAND_OFFSET] = WIN_SETFEATURES;
> + args.tf.feature = arg ? SETFEATURES_EN_AAM : SETFEATURES_DIS_AAM;
> + args.tf.nsect = arg;
> + args.tf.command = WIN_SETFEATURES;
... and here...
> args.command_type = IDE_DRIVE_TASK_NO_DATA;
> args.handler = &task_no_data_intr;
> ide_raw_taskfile(drive, &args, NULL);
> @@ -1115,7 +1107,7 @@ static int idedisk_open(struct inode *in
> if (drive->removable && idkp->openers == 1) {
> ide_task_t args;
> memset(&args, 0, sizeof(ide_task_t));
> - args.tfRegister[IDE_COMMAND_OFFSET] = WIN_DOORLOCK;
> + args.tf.command = WIN_DOORLOCK;
... and here...
> args.command_type = IDE_DRIVE_TASK_NO_DATA;
> args.handler = &task_no_data_intr;
> check_disk_change(inode->i_bdev);
> @@ -1142,7 +1134,7 @@ static int idedisk_release(struct inode
> if (drive->removable && idkp->openers == 1) {
> ide_task_t args;
> memset(&args, 0, sizeof(ide_task_t));
> - args.tfRegister[IDE_COMMAND_OFFSET] = WIN_DOORUNLOCK;
> + args.tf.command = WIN_DOORUNLOCK;
... and here...
> args.command_type = IDE_DRIVE_TASK_NO_DATA;
> args.handler = &task_no_data_intr;
> if (drive->doorlocking && ide_raw_taskfile(drive, &args, NULL))
> Index: b/drivers/ide/ide-io.c
> ===================================================================
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
> @@ -189,15 +189,15 @@ static ide_startstop_t ide_start_power_s
> return ide_stopped;
> }
> if (ide_id_has_flush_cache_ext(drive->id))
> - args->tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE_EXT;
> + args->tf.command = WIN_FLUSH_CACHE_EXT;
> else
> - args->tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE;
> + args->tf.command = WIN_FLUSH_CACHE;
> args->command_type = IDE_DRIVE_TASK_NO_DATA;
> args->handler = &task_no_data_intr;
> return do_rw_taskfile(drive, args);
>
> case idedisk_pm_standby: /* Suspend step 2 (standby) */
> - args->tfRegister[IDE_COMMAND_OFFSET] = WIN_STANDBYNOW1;
> + args->tf.command = WIN_STANDBYNOW1;
... and here...
> args->command_type = IDE_DRIVE_TASK_NO_DATA;
> args->handler = &task_no_data_intr;
> return do_rw_taskfile(drive, args);
[...]
> @@ -387,28 +387,30 @@ void ide_end_drive_cmd (ide_drive_t *dri
> rq->errors = !OK_STAT(stat,READY_STAT,BAD_STAT);
>
> if (args) {
> + struct ide_taskfile *tf = &args->tf;
> +
> if (args->tf_in_flags.b.data) {
> - u16 data = hwif->INW(IDE_DATA_REG);
> - args->tfRegister[IDE_DATA_OFFSET] = (data) & 0xFF;
> - args->hobRegister[IDE_DATA_OFFSET] = (data >> 8) & 0xFF;
> + u16 data = hwif->INW(IDE_DATA_REG);
Again, no newline after declaration block.
[...]
> @@ -707,28 +709,28 @@ static ide_startstop_t drive_cmd_intr (i
>
> static void ide_init_specify_cmd(ide_drive_t *drive, ide_task_t *task)
> {
> - task->tfRegister[IDE_NSECTOR_OFFSET] = drive->sect;
> - task->tfRegister[IDE_SECTOR_OFFSET] = drive->sect;
> - task->tfRegister[IDE_LCYL_OFFSET] = drive->cyl;
> - task->tfRegister[IDE_HCYL_OFFSET] = drive->cyl>>8;
> - task->tfRegister[IDE_SELECT_OFFSET] = ((drive->head-1)|drive->select.all)&0xBF;
> - task->tfRegister[IDE_COMMAND_OFFSET] = WIN_SPECIFY;
> + task->tf.nsect = drive->sect;
> + task->tf.lbal = drive->sect;
> + task->tf.lbam = drive->cyl;
> + task->tf.lbah = drive->cyl >> 8;
> + task->tf.device = ((drive->head - 1) | drive->select.all) & 0xBF;
Well, if you started using ATA_LBA, s/0xBF/~ATA_LBA.
> + task->tf.command = WIN_SPECIFY;
[...]
> Index: b/drivers/ide/ide-taskfile.c
> ===================================================================
> --- a/drivers/ide/ide-taskfile.c
> +++ b/drivers/ide/ide-taskfile.c
[...]
> /* (ks) send hob registers first */
> if (task->tf_out_flags.b.nsector_hob)
> - hwif->OUTB(hobfile->sector_count, IDE_NSECTOR_REG);
> + hwif->OUTB(tf->hob_nsect, IDE_NSECTOR_REG);
> if (task->tf_out_flags.b.sector_hob)
> - hwif->OUTB(hobfile->sector_number, IDE_SECTOR_REG);
> + hwif->OUTB(tf->hob_lbal, IDE_SECTOR_REG);
> if (task->tf_out_flags.b.lcyl_hob)
> - hwif->OUTB(hobfile->low_cylinder, IDE_LCYL_REG);
> + hwif->OUTB(tf->hob_lbam, IDE_LCYL_REG);
> if (task->tf_out_flags.b.hcyl_hob)
> - hwif->OUTB(hobfile->high_cylinder, IDE_HCYL_REG);
> + hwif->OUTB(tf->hob_lbah, IDE_HCYL_REG);
>
> /* (ks) Send now the standard registers */
> if (task->tf_out_flags.b.error_feature)
> - hwif->OUTB(taskfile->feature, IDE_FEATURE_REG);
> + hwif->OUTB(tf->feature, IDE_FEATURE_REG);
> /* refers to number of sectors to transfer */
> if (task->tf_out_flags.b.nsector)
> - hwif->OUTB(taskfile->sector_count, IDE_NSECTOR_REG);
> + hwif->OUTB(tf->nsect, IDE_NSECTOR_REG);
> /* refers to sector offset or start sector */
> if (task->tf_out_flags.b.sector)
> - hwif->OUTB(taskfile->sector_number, IDE_SECTOR_REG);
> + hwif->OUTB(tf->lbal, IDE_SECTOR_REG);
> if (task->tf_out_flags.b.lcyl)
> - hwif->OUTB(taskfile->low_cylinder, IDE_LCYL_REG);
> + hwif->OUTB(tf->lbam, IDE_LCYL_REG);
> if (task->tf_out_flags.b.hcyl)
> - hwif->OUTB(taskfile->high_cylinder, IDE_HCYL_REG);
> + hwif->OUTB(tf->lbah, IDE_HCYL_REG);
>
> /*
> * (ks) In the flagged taskfile approch, we will use all specified
Well, maybe it time to fix typo in approAch? :-)
> Index: b/include/linux/ide.h
> ===================================================================
> --- a/include/linux/ide.h
> +++ b/include/linux/ide.h
> @@ -103,8 +103,6 @@ typedef unsigned char byte; /* used ever
> #define IDE_FEATURE_OFFSET IDE_ERROR_OFFSET
> #define IDE_COMMAND_OFFSET IDE_STATUS_OFFSET
>
> -#define IDE_CONTROL_OFFSET_HOB (7)
> -
> #define IDE_DATA_REG (HWIF(drive)->io_ports[IDE_DATA_OFFSET])
> #define IDE_ERROR_REG (HWIF(drive)->io_ports[IDE_ERROR_OFFSET])
> #define IDE_NSECTOR_REG (HWIF(drive)->io_ports[IDE_NSECTOR_OFFSET])
> @@ -1072,15 +1070,33 @@ extern void ide_end_drive_cmd(ide_drive_
> */
> extern int ide_wait_cmd(ide_drive_t *, u8, u8, u8, u8, u8 *);
>
> +struct ide_taskfile {
> + u8 hob_data; /* 0: high data byte (for TASKFILE IOCTL) */
> +
> + u8 hob_feature; /* 1-5: additional data to support LBA48 */
> + u8 hob_nsect;
> + u8 hob_lbal;
> + u8 hob_lbam;
> + u8 hob_lbah;
> +
> + u8 data; /* 6: low data byte (for TASKFILE IOCTL) */
I wonder who needs to read/write the data reg. (even with HOB)? :-O
MBR, Sergei
next prev parent reply other threads:[~2007-10-17 17:38 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-10-08 21:11 [PATCH 6/12] ide: add struct ide_taskfile Bartlomiej Zolnierkiewicz
2007-10-17 17:38 ` Sergei Shtylyov [this message]
2007-10-24 22:16 ` Bartlomiej Zolnierkiewicz
2007-10-25 16:37 ` Sergei Shtylyov
2007-10-19 16:47 ` Sergei Shtylyov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=47164894.9080007@ru.mvista.com \
--to=sshtylyov@ru.mvista.com \
--cc=bzolnier@gmail.com \
--cc=htejun@gmail.com \
--cc=linux-ide@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).