Sergei Shtylyov wrote:
Hello, I wrote:
+
+ reset = hwif->channel ? 0x80 : 0x40;
These HighPoint's "soft reset" (WTF does that mean I wonder?) bits
are not the same between HPT366 and HPT370, so this code doesn't look
right...
Ah, these "soft" reset bits correspond to PRST_/SRST_ signals which
are really primary/secondary channel hard resets (RESET- signal). This
explains everything; and this patch is a pure hack then.
+
+ 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);
Karl, why are you calling resetproc() without doing a reset? What
this achieves?
To reset the drives, apparently. But it's not the function of
resetproc(). We should try soft-reset instead.
Yes, to reset the drive that does not respond.
+ }
+ 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
I would first like to hear an explanation of what it does...
I have to NAK this resetproc() addition. It tries to do things that
it's not designed to do. We should instead try soft-resetting the
channel...
Where should the soft reset be done, for this to not be called a hack?
should it be in ide_port_ops->reset_poll?
Care to revise your patch?
I see no point in "revising" this hack now...
Why not reset the drive if it does not respond? OK, you could blame
this on a bugy board, or redboot that does not reset the drive on warm
boot if it is busy.
--
Karl
--
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
