sl82c105: add ->speedproc support

PATCH In review Bartlomiej Zolnierkiewicz 4 participants 9 messages Mar 10, 2007 – Mar 15, 2007 libata

lore

Share & tools

lore

Bartlomiej Zolnierkiewicz

author

2007-03-10 21:09 UTC #0 lore raw

PATCH 10/13 sl82c105: add ->speedproc support

[PATCH] sl82c105: add ->speedproc support

* add sl82c105_tunepio() wrapper for sl82c105_tune_drive() (just to get the error value)

* add sl82c105_tune_chipset() (->speedproc method) for setting transfer mode

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---

 drivers/ide/pci/sl82c105.c |   44 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

drivers/ide/pci/sl82c105.c +37 -7

Index: b/drivers/ide/pci/sl82c105.c
===================================================================
@@ -77,7 +77,7 @@ static unsigned int get_timing_sl82c105(
 /*
  * Configure the drive and chipset for PIO
  */
-static void sl82c105_tune_drive(ide_drive_t *drive, u8 pio)
+static int sl82c105_tunepio(ide_drive_t *drive, u8 pio)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
 	struct pci_dev *dev	= hwif->pci_dev;

@@ -91,7 +91,7 @@ static void sl82c105_tune_drive(ide_driv
 	xfer_mode = ide_get_best_pio_mode(drive, pio, 5, &p) + XFER_PIO_0;
 
 	if (ide_config_drive_speed(drive, xfer_mode))
-		return;
+		return 1;
 
 	drive->drive_data = drv_ctrl = get_timing_sl82c105(&p);

@@ -114,17 +114,45 @@ static void sl82c105_tune_drive(ide_driv
 	 */
 	drive->io_32bit = 1;
 	drive->unmask	= 1;
+
+	return 0;
+}
+
+static void sl82c105_tune_drive(ide_drive_t *drive, u8 pio)
+{
+	/*
+	 * TODO: find best PIO mode and set device speed here
+	 *       (requires adding helper function for getting PIO cycle time)
+	 */
+	(void)sl82c105_tunepio(drive, pio);
+}
+
+static int sl82c105_tune_chipset(ide_drive_t *drive, u8 mode)
+{
+	mode = ide_rate_filter(drive, mode);
+
+	if (mode >= XFER_PIO_0 && mode <= XFER_PIO_5)
+		return sl82c105_tunepio(drive, mode - XFER_PIO_0);
+
+	/*
+	 * TODO: add MWDMA0/1 support
+	 */
+	BUG_ON(mode != XFER_MW_DMA_2);
+
+	/*
+	 * Configure the drive for DMA.
+	 * We'll program the chipset only when DMA is actually turned on.
+	 */
+	return ide_config_drive_speed(drive, mode);
 }
 
-/*
- * Configure the drive for DMA.
- * We'll program the chipset only when DMA is actually turned on.
- */
 static int config_for_dma (ide_drive_t *drive)
 {
+	u8 mode = ide_max_dma_mode(drive);
+
 	DBG(("config_for_dma(drive:%s)\n", drive->name));
 
-	if (ide_config_drive_speed(drive, XFER_MW_DMA_2) != 0)
+	if (mode == 0 || sl82c105_tune_chipset(drive, mode))
 		return 0;
 
 	return ide_dma_enable(drive);

@@ -387,6 +415,8 @@ static void __devinit init_hwif_sl82c105
 		return;
 	}
 
+	hwif->speedproc			= &sl82c105_tune_chipset;
+
 	hwif->atapi_dma  = 1;
 	hwif->mwdma_mask = 0x04;

Bartlomiej Zolnierkiewicz

author

2007-03-10 22:25 UTC #2 lore raw

Hi,

Quoted 16 lines · Sergei Shtylyov

Hello.
Quoted 7 lines · Bartlomiej Zolnierkiewicz
[PATCH] sl82c105: add ->speedproc support
* add sl82c105_tunepio() wrapper for sl82c105_tune_drive() (just to get the error value)
* add sl82c105_tune_chipset() (->speedproc method) for setting transfer mode
Thanks for the patch!
Quoted 2 lines
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---

Quoted 14 lines

drivers/ide/pci/sl82c105.c static unsigned int get_timing_sl82c105( · line 80
Quoted 10 lines
Index: b/drivers/ide/pci/sl82c105.c
===================================================================
drivers/ide/pci/sl82c105.c +1 -1
@@ -77,7 +77,7 @@ static unsigned int get_timing_sl82c105(
 /*
  * Configure the drive and chipset for PIO
  */
-static void sl82c105_tune_drive(ide_drive_t *drive, u8 pio)
+static int sl82c105_tunepio(ide_drive_t *drive, u8 pio)
The name sl82c105_tune_pio() would have been more in line with the other
drivers but well, that's your patch (and the function behaves somewhat differently anyway :-)

I will change it.

Quoted 12 lines

drivers/ide/pci/sl82c105.c +1 -1

 {
 	ide_hwif_t *hwif	= HWIF(drive);
 	struct pci_dev *dev	= hwif->pci_dev;

@@ -91,7 +91,7 @@ static void sl82c105_tune_drive(ide_driv
 	xfer_mode = ide_get_best_pio_mode(drive, pio, 5, &p) + XFER_PIO_0;
 
 	if (ide_config_drive_speed(drive, xfer_mode))
-		return;
+		return 1;
 
 	drive->drive_data = drv_ctrl = get_timing_sl82c105(&p);

Quoted 16 lines

Quoted 4 lines
drivers/ide/pci/sl82c105.c +0 -0
@@ -114,17 +114,45 @@ static void sl82c105_tune_drive(ide_driv
 	 */
 	drive->io_32bit = 1;
 	drive->unmask	= 1;
drivers/ide/pci/sl82c105.c static void sl82c105_tune_drive(ide_driv · line 126
Quoted 10 lines
drivers/ide/pci/sl82c105.c +10 -0
+
+	return 0;
+}
+
+static void sl82c105_tune_drive(ide_drive_t *drive, u8 pio)
+{
+	/*
+	 * TODO: find best PIO mode and set device speed here
+	 *       (requires adding helper function for getting PIO cycle time)
+	 */
I thought we were doing it by calling ide_get_best_pio_mode() above...

We are also using ide_get_best_pio_mode() to get PIO cycle time so we can't move it here ATM.

Quoted 5 lines

drivers/ide/pci/sl82c105.c static void sl82c105_tune_drive(ide_driv · line 127
Quoted 1 line
drivers/ide/pci/sl82c105.c +1 -0
+	(void)sl82c105_tunepio(drive, pio);
Erm, I thought afterwards that I vainly folded one into another. I think
it's worth moving those io_32bit and unmask flag assignments above back there... May also recast my patch. :-)

Moving them to ->init_hwif where they belong would be even better... ;-)

Quoted 7 lines

drivers/ide/pci/sl82c105.c +7 -0

+}
+
+static int sl82c105_tune_chipset(ide_drive_t *drive, u8 mode)
+{
+	mode = ide_rate_filter(drive, mode);
+
+	if (mode >= XFER_PIO_0 && mode <= XFER_PIO_5)

Quoted 8 lines

drivers/ide/pci/sl82c105.c static void sl82c105_tune_drive(ide_driv · line 140
Quoted 6 lines
drivers/ide/pci/sl82c105.c +6 -0
+		return sl82c105_tunepio(drive, mode - XFER_PIO_0);
+
+	/*
+	 * TODO: add MWDMA0/1 support
+	 */
+	BUG_ON(mode != XFER_MW_DMA_2);
Well, the other drivers just return non-zero in this case...

They are are buggy and need fixing.

IDE driver itself should never ask for an unsupported mode and if user passes such through ->speedproc interface OOPSing is IMO better than possible data corruption. On the second thought ide_rate_filter() takes care of filtering UDMA modes, so it may as well take care of filtering SWDMA/MWDMA/PIO and the problem will be solved in more gracefull way... but more work is needed...

Thanks,
Bart

Sergei Shtylyov

2007-03-12 13:13 UTC #3 lore raw

Quoted 19 lines · Bartlomiej Zolnierkiewicz

Quoted 19 lines
Hello.
Quoted 7 lines · Bartlomiej Zolnierkiewicz
[PATCH] sl82c105: add ->speedproc support
* add sl82c105_tunepio() wrapper for sl82c105_tune_drive() (just to get the error value)
* add sl82c105_tune_chipset() (->speedproc method) for setting transfer mode
Thanks for the patch!
Quoted 5 lines
Index: b/drivers/ide/pci/sl82c105.c
===================================================================
drivers/ide/pci/sl82c105.c +0 -0

[...]

Quoted 26 lines

Quoted 9 lines

drivers/ide/pci/sl82c105.c +1 -1

@@ -91,7 +91,7 @@ static void sl82c105_tune_drive(ide_driv
 	xfer_mode = ide_get_best_pio_mode(drive, pio, 5, &p) + XFER_PIO_0;
 
 	if (ide_config_drive_speed(drive, xfer_mode))
-		return;
+		return 1;
 
 	drive->drive_data = drv_ctrl = get_timing_sl82c105(&p);

Quoted 16 lines

Quoted 4 lines
drivers/ide/pci/sl82c105.c +0 -0
@@ -114,17 +114,45 @@ static void sl82c105_tune_drive(ide_driv
 	 */
 	drive->io_32bit = 1;
 	drive->unmask	= 1;
drivers/ide/pci/sl82c105.c static void sl82c105_tune_drive(ide_driv · line 126
Quoted 10 lines
drivers/ide/pci/sl82c105.c +10 -0
+
+	return 0;
+}
+
+static void sl82c105_tune_drive(ide_drive_t *drive, u8 pio)
+{
+	/*
+	 * TODO: find best PIO mode and set device speed here
+	 *       (requires adding helper function for getting PIO cycle time)
+	 */
I thought we were doing it by calling ide_get_best_pio_mode() above...

Quoted 2 lines

We are also using ide_get_best_pio_mode() to get PIO cycle time so we can't move it here ATM.

I've found/used quite convenient workaround for that -- return PIO mode
actually selected from xxx_tune_pio(), then call ide_config_drive_speed() from the real tuneproc() method.

Quoted 6 lines

Quoted 4 lines
drivers/ide/pci/sl82c105.c static void sl82c105_tune_drive(ide_driv · line 127
Quoted 1 line
drivers/ide/pci/sl82c105.c +1 -0
+	(void)sl82c105_tunepio(drive, pio);
Erm, I thought afterwards that I vainly folded one into another. I think
it's worth moving those io_32bit and unmask flag assignments above back there... May also recast my patch. :-)
Moving them to ->init_hwif where they belong would be even better... ;-)

Well, I wasn't sure where they belong... :-)
So, OK to recast that patch?

Quoted 14 lines

Quoted 5 lines
Quoted 5 lines
drivers/ide/pci/sl82c105.c +5 -0
+static int sl82c105_tune_chipset(ide_drive_t *drive, u8 mode)
+{
+	mode = ide_rate_filter(drive, mode);
+
+	if (mode >= XFER_PIO_0 && mode <= XFER_PIO_5)
Quoted 8 lines
drivers/ide/pci/sl82c105.c static void sl82c105_tune_drive(ide_driv · line 140
Quoted 6 lines
drivers/ide/pci/sl82c105.c +6 -0
+		return sl82c105_tunepio(drive, mode - XFER_PIO_0);
+
+	/*
+	 * TODO: add MWDMA0/1 support
+	 */
+	BUG_ON(mode != XFER_MW_DMA_2);
Well, the other drivers just return non-zero in this case...

Quoted 1 line

They are are buggy and need fixing.

Ugh, that'll be a lot of fixing... :-)
Why BUG on being passed unsupported mode from userspace though (when
there's no filtering involved before that)?

Quoted 5 lines

IDE driver itself should never ask for an unsupported mode and if user passes such through ->speedproc interface OOPSing is IMO better than possible data corruption. On the second thought ide_rate_filter() takes care of filtering UDMA modes, so it may as well take care of filtering SWDMA/MWDMA/PIO and the problem will be solved in more gracefull way... but more work is needed...

Yeah. :-)

Quoted 2 lines

Thanks,
Bart

MBR, Sergei

Sergei Shtylyov

2007-03-14 20:51 UTC #4 lore raw

Quoted 38 lines · Hello, I wrote:

Quoted 17 lines
Quoted 17 lines
Quoted 7 lines · Bartlomiej Zolnierkiewicz
[PATCH] sl82c105: add ->speedproc support
* add sl82c105_tunepio() wrapper for sl82c105_tune_drive() (just to get the error value)
* add sl82c105_tune_chipset() (->speedproc method) for setting transfer mode
Thanks for the patch!
Quoted 5 lines
Index: b/drivers/ide/pci/sl82c105.c
===================================================================
drivers/ide/pci/sl82c105.c +0 -0
[...]
Quoted 17 lines
Quoted 17 lines
Quoted 5 lines
drivers/ide/pci/sl82c105.c +1 -0
@@ -114,17 +114,45 @@ static void sl82c105_tune_drive(ide_driv
     */
    drive->io_32bit = 1;
    drive->unmask    = 1;
+
drivers/ide/pci/sl82c105.c static void sl82c105_tune_drive(ide_driv · line 127
Quoted 10 lines
drivers/ide/pci/sl82c105.c +9 -0
+    return 0;
+}
+
+static void sl82c105_tune_drive(ide_drive_t *drive, u8 pio)
+{
+    /*
+     * TODO: find best PIO mode and set device speed here
+     *       (requires adding helper function for getting PIO cycle 
 time)
+     */
I thought we were doing it by calling ide_get_best_pio_mode() above...

Quoted 5 lines

Quoted 2 lines
We are also using ide_get_best_pio_mode() to get PIO cycle time so we can't move it here ATM.
I've found/used quite convenient workaround for that -- return PIO
mode actually selected from xxx_tune_pio(), then call ide_config_drive_speed() from the real tuneproc() method.

Works like charm with ignoring the result, since ide_get_best_pio_mode()
returns the same explicit mode as was passed to it -- so is good for the speedproc() methods also...

Quoted 9 lines

Quoted 6 lines
Quoted 4 lines
drivers/ide/pci/sl82c105.c static void sl82c105_tune_drive(ide_driv · line 127
Quoted 1 line
drivers/ide/pci/sl82c105.c +1 -0
+    (void)sl82c105_tunepio(drive, pio);
Erm, I thought afterwards that I vainly folded one into another. I
think it's worth moving those io_32bit and unmask flag assignments above back there... May also recast my patch. :-)
Moving them to ->init_hwif where they belong would be even better... ;-)
Well, I wasn't sure where they belong... :-)
So, OK to recast that patch?

Recasted it and reworked your patch atop of it (adding MWDMA 0/1 support as
a bonus! :-) -- now need to conduct some testing on a remote target...

Quoted 11 lines

drivers/ide/pci/sl82c105.c static void sl82c105_tune_drive(ide_driv · line 140

Quoted 11 lines

drivers/ide/pci/sl82c105.c +11 -0

+static int sl82c105_tune_chipset(ide_drive_t *drive, u8 mode)
+{
+    mode = ide_rate_filter(drive, mode);
+
+    if (mode >= XFER_PIO_0 && mode <= XFER_PIO_5)
+        return sl82c105_tunepio(drive, mode - XFER_PIO_0);
+
+    /*
+     * TODO: add MWDMA0/1 support
+     */
+    BUG_ON(mode != XFER_MW_DMA_2);

I wonder is there are some W83C554 users anywhere -- that chipset also
supports UltraDMA...

MBR, Sergei

Woody Suwalski

2007-03-15 14:32 UTC #5 lore raw

Quoted 78 lines · Sergei Shtylyov

Quoted 39 lines · Hello, I wrote:
Quoted 17 lines
Quoted 17 lines
Quoted 7 lines · Bartlomiej Zolnierkiewicz
[PATCH] sl82c105: add ->speedproc support
* add sl82c105_tunepio() wrapper for sl82c105_tune_drive() (just to get the error value)
* add sl82c105_tune_chipset() (->speedproc method) for setting transfer mode
Thanks for the patch!
Quoted 5 lines
Index: b/drivers/ide/pci/sl82c105.c
===================================================================
drivers/ide/pci/sl82c105.c +0 -0
[...]
Quoted 18 lines
Quoted 18 lines
Quoted 5 lines
drivers/ide/pci/sl82c105.c +1 -0
@@ -114,17 +114,45 @@ static void sl82c105_tune_drive(ide_driv
     */
    drive->io_32bit = 1;
    drive->unmask    = 1;
+
drivers/ide/pci/sl82c105.c static void sl82c105_tune_drive(ide_driv · line 127
Quoted 10 lines
drivers/ide/pci/sl82c105.c +9 -0
+    return 0;
+}
+
+static void sl82c105_tune_drive(ide_drive_t *drive, u8 pio)
+{
+    /*
+     * TODO: find best PIO mode and set device speed here
+     *       (requires adding helper function for getting PIO 
 cycle time)
+     */
I thought we were doing it by calling ide_get_best_pio_mode()
above...
Quoted 5 lines
Quoted 2 lines
We are also using ide_get_best_pio_mode() to get PIO cycle time so we can't move it here ATM.
I've found/used quite convenient workaround for that -- return PIO
mode actually selected from xxx_tune_pio(), then call ide_config_drive_speed() from the real tuneproc() method.
Works like charm with ignoring the result, since
ide_get_best_pio_mode() returns the same explicit mode as was passed to it -- so is good for the speedproc() methods also...
Quoted 9 lines
Quoted 6 lines
Quoted 4 lines
drivers/ide/pci/sl82c105.c static void sl82c105_tune_drive(ide_driv · line 127
Quoted 1 line
drivers/ide/pci/sl82c105.c +1 -0
+    (void)sl82c105_tunepio(drive, pio);
Erm, I thought afterwards that I vainly folded one into another.
I think it's worth moving those io_32bit and unmask flag assignments above back there... May also recast my patch. :-)
Moving them to ->init_hwif where they belong would be even better... ;-)
Well, I wasn't sure where they belong... :-)
So, OK to recast that patch?
Recasted it and reworked your patch atop of it (adding MWDMA 0/1
support as a bonus! :-) -- now need to conduct some testing on a remote target...
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
drivers/ide/pci/sl82c105.c static void sl82c105_tune_drive(ide_driv · line 140
Quoted 11 lines
drivers/ide/pci/sl82c105.c +11 -0
+static int sl82c105_tune_chipset(ide_drive_t *drive, u8 mode)
+{
+    mode = ide_rate_filter(drive, mode);
+
+    if (mode >= XFER_PIO_0 && mode <= XFER_PIO_5)
+        return sl82c105_tunepio(drive, mode - XFER_PIO_0);
+
+    /*
+     * TODO: add MWDMA0/1 support
+     */
+    BUG_ON(mode != XFER_MW_DMA_2);
I wonder is there are some W83C554 users anywhere -- that chipset
also supports UltraDMA...

Quoted 1 line

MBR, Sergei

Sergei,

ARM Netwinder machines are running hard disk IDE on SL82c105. Could you send me the actual source file to try (or a patch)?

Thanks, Woody

Russell King

2007-03-15 17:36 UTC #6 lore raw

Quoted 9 lines · Woody Suwalski

Quoted 4 lines · Sergei Shtylyov
I wonder is there are some W83C554 users anywhere -- that chipset
also supports UltraDMA...
MBR, Sergei
Sergei,
ARM Netwinder machines are running hard disk IDE on SL82c105. Could you send me the actual source file to try (or a patch)?

That's actually a W83C553 which does not support UDMA in any shape or form according to the documents.

I guess it's an enhancement found on W83C554, and in that case any attempt to add UDMA support to the SL82C105 driver should be done such that these older chipsets continue to work.

Signature · 3 lines

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Sergei Shtylyov

2007-03-15 17:40 UTC #7 lore raw

Hello.

Quoted 7 lines · Russell King

Quoted 6 lines
Quoted 1 line
I wonder is there are some W83C554 users anywhere -- that chipset also supports UltraDMA...
Sergei,
ARM Netwinder machines are running hard disk IDE on SL82c105. Could you send me the actual source file to try (or a patch)?

Quoted 6 lines

That's actually a W83C553 which does not support UDMA in any shape or form according to the documents.
I guess it's an enhancement found on W83C554, and in that case any attempt to add UDMA support to the SL82C105 driver should be done such that these older chipsets continue to work.

I had no plans of adding it so far since there seemed to be no designs
using that chip but decided to ask if somebody is interested just in case...

MBR, Sergei

Bartlomiej Zolnierkiewicz

author

2007-03-15 21:44 UTC #8 lore raw

In reply to Sergei Shtylyov

Quoted 57 lines · Sergei Shtylyov

Quoted 38 lines · Hello, I wrote:
Quoted 17 lines
Quoted 17 lines
Quoted 7 lines · Bartlomiej Zolnierkiewicz
[PATCH] sl82c105: add ->speedproc support
* add sl82c105_tunepio() wrapper for sl82c105_tune_drive() (just to get the error value)
* add sl82c105_tune_chipset() (->speedproc method) for setting transfer mode
Thanks for the patch!
Quoted 5 lines
Index: b/drivers/ide/pci/sl82c105.c
===================================================================
drivers/ide/pci/sl82c105.c +0 -0
[...]
Quoted 17 lines
Quoted 17 lines
Quoted 5 lines
drivers/ide/pci/sl82c105.c +1 -0
@@ -114,17 +114,45 @@ static void sl82c105_tune_drive(ide_driv
     */
    drive->io_32bit = 1;
    drive->unmask    = 1;
+
drivers/ide/pci/sl82c105.c static void sl82c105_tune_drive(ide_driv · line 127
Quoted 10 lines
drivers/ide/pci/sl82c105.c +9 -0
+    return 0;
+}
+
+static void sl82c105_tune_drive(ide_drive_t *drive, u8 pio)
+{
+    /*
+     * TODO: find best PIO mode and set device speed here
+     *       (requires adding helper function for getting PIO cycle 
 time)
+     */
I thought we were doing it by calling ide_get_best_pio_mode() above...
Quoted 6 lines
Quoted 2 lines
We are also using ide_get_best_pio_mode() to get PIO cycle time so we can't move it here ATM.
I've found/used quite convenient workaround for that -- return PIO
mode actually selected from xxx_tune_pio(), then call ide_config_drive_speed() from the real tuneproc() method.
Works like charm with ignoring the result, since ide_get_best_pio_mode()
returns the same explicit mode as was passed to it -- so is good for the speedproc() methods also...
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/ide/pci/sl82c105.c static void sl82c105_tune_drive(ide_driv · line 127
Quoted 1 line
drivers/ide/pci/sl82c105.c +1 -0
+    (void)sl82c105_tunepio(drive, pio);
Erm, I thought afterwards that I vainly folded one into another. I
think it's worth moving those io_32bit and unmask flag assignments above back there... May also recast my patch. :-)

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Moving them to ->init_hwif where they belong would be even better... ;-)
Well, I wasn't sure where they belong... :-)
So, OK to recast that patch?
Recasted it and reworked your patch atop of it (adding MWDMA 0/1 support as
a bonus! :-) -- now need to conduct some testing on a remote target...

Great. :)

Bart