On Friday 08 May 2009 15:50:11 Bartlomiej Zolnierkiewicz wrote:
>
> On Tuesday 05 May 2009 16:53:31 Karl Hiramoto wrote:
> > Hi,
> >
> > Part of this patch (htp366.c) is commented in an #if 0 in older
> > kernels. 2.6.11 for sure.
> >
> > I have an ARM IXP4xx based board with CompactFlash slot that this patch
> > fixes some issues for. I think the case, is only with a warm boot,
> > where the drive was busy before the board was reset. There is no
> > BIOS, only the redboot loader which loads the kernel.
> >
> >
> > A few questions:
> >
> > 1. Would this be considerd to be brought into mainline?
>
> When it comes to ide-probe.c changes: yes given that you split them on
> smaller patches. hpt366.c change also seems to be correct on the first
> glance but I think that Sergei's opinion would be more suited here.
>
> > 2. How would you port something like this to using the SATA layer?
> >
> > Would it go to the ata_port_operations.prereset, softreset or hardreset?
> >
> > --
> > Karl
> >
> > diff --git a/drivers/ide/hpt366.c b/drivers/ide/hpt366.c
> > index 3377766..ba4b802 100644
> > --- a/drivers/ide/hpt366.c
> > +++ b/drivers/ide/hpt366.c
> > @@ -1280,6 +1280,31 @@ static u8 hpt3xx_cable_detect(ide_hwif_t *hwif)
> > return (scr1 & ata66) ? ATA_CBL_PATA40 : ATA_CBL_PATA80;
> > }
> >
> > +/** routine to reset disk
> > + *
> > + * Since SUN Cobalt is attempting to do this operation, I should disclose
> > + * this has been a long time ago Thu Jul 27 16:40:57 2000 was the patch date
> > + * HOTSWAP ATA Infrastructure.
> > + */
> > +
> > +static void hpt3xx_reset (ide_drive_t *drive)
> > +{
> > + ide_hwif_t *hwif = drive->hwif;
> > + struct pci_dev *dev = to_pci_dev(hwif->dev);
> > + unsigned long high_16;
> > + u8 reset;
> > + u8 reg59h = 0;
> > +
> > + printk(KERN_INFO "%s reset drive channel %d\n", __func__, hwif->channel);
> > + high_16 = pci_resource_start(dev, 4);
> > +
> > + reset = hwif->channel ? 0x80 : 0x40;
> > +
> > + pci_read_config_byte(dev, 0x59, ®59h);
> > + pci_write_config_byte(dev, 0x59, reg59h|reset);
> > + pci_write_config_byte(dev, 0x59, reg59h);
> > +}
> > +
> > static void __devinit init_hwif_hpt366(ide_hwif_t *hwif)
> > {
> > struct hpt_info *info = hpt3xx_get_info(hwif->dev);
> > @@ -1406,6 +1431,7 @@ static const struct ide_port_ops hpt3xx_port_ops = {
> > .set_pio_mode = hpt3xx_set_pio_mode,
> > .set_dma_mode = hpt3xx_set_mode,
> > .quirkproc = hpt3xx_quirkproc,
> > + .resetproc = hpt3xx_reset,
> > .maskproc = hpt3xx_maskproc,
> > .mdma_filter = hpt3xx_mdma_filter,
> > .udma_filter = hpt3xx_udma_filter,
> > diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c
> > index 7f264ed..09295b4 100644
> > --- a/drivers/ide/ide-probe.c
> > +++ b/drivers/ide/ide-probe.c
> > @@ -367,6 +367,7 @@ static int do_probe (ide_drive_t *drive, u8 cmd)
> > {
> > ide_hwif_t *hwif = drive->hwif;
> > const struct ide_tp_ops *tp_ops = hwif->tp_ops;
> > + const struct ide_port_ops *port_ops = hwif->port_ops;
> > u16 *id = drive->id;
> > int rc;
> > u8 present = !!(drive->dev_flags & IDE_DFLAG_PRESENT), stat;
> > @@ -427,9 +428,20 @@ static int do_probe (ide_drive_t *drive, u8 cmd)
> > /* ensure drive IRQ is clear */
> > stat = tp_ops->read_status(hwif);
> >
> > - if (rc == 1)
> > + if (rc == 1) {
> > printk(KERN_ERR "%s: no response (status = 0x%02x)\n",
> > drive->name, stat);
> > +
> > + if (port_ops->resetproc) {
> > + port_ops->resetproc(drive);
> > + msleep(50);
> > + }
> > + tp_ops->dev_select(drive);
> > + msleep(50);
> > + tp_ops->exec_command(hwif, ATA_CMD_DEV_RESET);
> > + (void)ide_busy_sleep(hwif, WAIT_WORSTCASE, 0);
> > + rc = ide_dev_read_id(drive, cmd, id);
> > + }
> > } else {
> > /* not present or maybe ATAPI */
> > rc = 3;
>
> Since the current code in ide-probe.c looks like this:
>
> stat = tp_ops->read_status(hwif);
>
> if (stat == (ATA_BUSY | ATA_DRDY))
> return 4;
>
> if (rc == 1 && cmd == ATA_CMD_ID_ATAPI) {
> printk(KERN_ERR "%s: no response (status = 0x%02x), "
> "resetting drive\n", drive->name, stat);
> msleep(50);
> tp_ops->dev_select(drive);
> msleep(50);
> tp_ops->exec_command(hwif, ATA_CMD_DEV_RESET);
> (void)ide_busy_sleep(hwif, WAIT_WORSTCASE, 0);
> rc = ide_dev_read_id(drive, cmd, id);
> }
>
> /* ensure drive IRQ is clear */
> stat = tp_ops->read_status(hwif);
>
> if (rc == 1)
> printk(KERN_ERR "%s: no response (status = 0x%02x)\n",
> drive->name, stat);
>
> I would really prefer fixing ATAPI case while we are at it
> (+ this would also get rid of code duplication).
>
> IOW:
>
> - in patch #1 we would add ->resetproc call
>
> - in patch #2 we would remove 'cmd == ATA_CMD_ID_ATAPI' check
[ looking a bit closer at the code in ide-probe.c ]
- in patch #2 we would move the ATAPI check to cover device selection/reset
inside 'if ()' block
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
