[RFC]hpt366/ide-probe reset drive on probe error.

[RFC]hpt366/ide-probe reset drive on probe error.

[Karl Hiramoto]

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?

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, &reg59h);
+	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;

Re: [RFC]hpt366/ide-probe reset drive on probe error.

[Bartlomiej Zolnierkiewicz]

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, &reg59h);
> +	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

Care to revise your patch?

Thanks,
Bart

Re: [RFC]hpt366/ide-probe reset drive on probe error.

[Bartlomiej Zolnierkiewicz]

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, &reg59h);
> > +	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

Re: [RFC]hpt366/ide-probe reset drive on probe error.

[Bartlomiej Zolnierkiewicz]

On Friday 08 May 2009 15:57:19 Bartlomiej Zolnierkiewicz wrote:
> 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, &reg59h);
> > > +	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

Sigh, this is even more complicated...

We should defer ->resetproc call until both drives are probed and do the
port reset after probing both drives given that:

- we didn't detect any devices

and 

- for one of devices do_probe() returned '1' (== "no response")

Also it would greatly help to clean/fix return values of do_probe()
and probe_for_drive() before doing the real changes.

Thanks,
Bart

Re: [RFC]hpt366/ide-probe reset drive on probe error.

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>Hi,

>>Part of this patch (htp366.c) is commented in an #if 0   in older 
>>kernels.   2.6.11 for sure.

    ... and for a reason.

>>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.

    Frankly speaking, I don't quite understand this patch.

[...]

>>--
>>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.

    This comment doesn't really belong to resetproc() method, it rather 
belongs to (now gone) tristate() method.

>>+ */
>>+
>>+static void hpt3xx_reset (ide_drive_t *drive)

    No spaces allowed before '('. Use scripts/checkpatch.pl please.

>>+{
>>+	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);

    Useless variable.

>>+
>>+	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...

>>+
>>+	pci_read_config_byte(dev, 0x59, &reg59h);
>>+	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?

>>+			}
>>+			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...

> - in patch #2 we would remove 'cmd == ATA_CMD_ID_ATAPI' check

    Why? Does issuing ATA_CMD_DEV_RESET to hard drives make sense?

> Care to revise your patch?

> Thanks,
> Bart

MBR, Sergei

Re: [RFC]hpt366/ide-probe reset drive on probe error.

[Sergei Shtylyov]

Hello, I wrote:

>>> 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.

>    This comment doesn't really belong to resetproc() method, it rather 
> belongs to (now gone) tristate() method.

>>> + */
>>> +
>>> +static void hpt3xx_reset (ide_drive_t *drive)

>    No spaces allowed before '('. Use scripts/checkpatch.pl please.

>>> +{
>>> +    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);

>    Useless variable.

>>> +
>>> +    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, &reg59h);
>>> +    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.

>>> +            }
>>> +            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...

>> - in patch #2 we would remove 'cmd == ATA_CMD_ID_ATAPI' check

>    Why? Does issuing ATA_CMD_DEV_RESET to hard drives make sense?

>> Care to revise your patch?

    I see no point in "revising" this hack now...

>> Thanks,
>> Bart

MBR, Sergei

Re: [RFC]hpt366/ide-probe reset drive on probe error.

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>>>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, &reg59h);
>>>>+	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

> Sigh, this is even more complicated...

> We should defer ->resetproc call until both drives are probed and do the
> port reset after probing both drives given that:

    We should just reset the port (and call resetproc() as a part of that). 
The resetproc() change should not be needed.

> - we didn't detect any devices

> and 

> - for one of devices do_probe() returned '1' (== "no response")

> Also it would greatly help to clean/fix return values of do_probe()
> and probe_for_drive() before doing the real changes.

    Sounds like the plan. :-)

> Thanks,
> Bart

MBR, Sergei

Re: [RFC]hpt366/ide-probe reset drive on probe error.

[Karl Hiramoto]

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, &reg59h);
>>>> +    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

Re: [RFC]hpt366/ide-probe reset drive on probe error.

[Bartlomiej Zolnierkiewicz]

On Saturday 09 May 2009 14:09:05 Karl Hiramoto wrote:
> Sergei Shtylyov wrote:

[...]

> >>> 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.

Please read the whole discussion between Sergei & me before getting
discouraged by our occasional use of the vivid language. ;)

Your idea is good but the current implementation needs to be reworked
into a form which doesn't duplicate the code and which would work also
for other hardware setups.

The nice starting point is sanitizing return values of do_probe()
and probe_for_drive(), and then making sure that they are propagated
correctly to higher level.  Once this is done we can use those values
in ide_probe_port() to reset and re-probe the port if necessary.

[ I think that the port reset itself would be best realized by reusing
  the existing code from do_reset1() (plese see the code for SRST near
  the end of this function). ]

Thanks,
Bart