* [PATCH 12/15] ide: remove ->dma_master field from ide_hwif_t
@ 2007-10-01 21:39 Bartlomiej Zolnierkiewicz
2007-10-01 21:48 ` Jeff Garzik
0 siblings, 1 reply; 12+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2007-10-01 21:39 UTC (permalink / raw)
To: linux-ide
* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.
* Remove no longer needed ->dma_master field from ide_hwif_t.
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
drivers/ide/ide-dma.c | 7 -------
drivers/ide/ide.c | 2 --
drivers/ide/pci/cmd64x.c | 8 +++++---
drivers/ide/pci/hpt366.c | 21 +++++++++++----------
drivers/ide/pci/pdc202xx_old.c | 12 ++++++------
include/linux/ide.h | 1 -
6 files changed, 22 insertions(+), 29 deletions(-)
Index: b/drivers/ide/ide-dma.c
===================================================================
--- a/drivers/ide/ide-dma.c
+++ b/drivers/ide/ide-dma.c
@@ -1004,11 +1004,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
hwif->dma_base = base;
- if (hwif->mate)
- hwif->dma_master = hwif->channel ? hwif->mate->dma_base : base;
- else
- hwif->dma_master = base;
-
if (!(hwif->dma_command))
hwif->dma_command = hwif->dma_base;
if (!(hwif->dma_vendor1))
@@ -1050,8 +1045,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
hwif->drives[1].name, (dma_stat & 0x40) ? "DMA" : "pio");
}
printk("\n");
-
- BUG_ON(!hwif->dma_master);
}
EXPORT_SYMBOL_GPL(ide_setup_dma);
Index: b/drivers/ide/ide.c
===================================================================
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -470,7 +470,6 @@ static void ide_hwif_restore(ide_hwif_t
#endif
hwif->dma_base = tmp_hwif->dma_base;
- hwif->dma_master = tmp_hwif->dma_master;
hwif->dma_command = tmp_hwif->dma_command;
hwif->dma_vendor1 = tmp_hwif->dma_vendor1;
hwif->dma_status = tmp_hwif->dma_status;
@@ -604,7 +603,6 @@ void ide_unregister(unsigned int index)
(void) ide_release_dma(hwif);
hwif->dma_base = 0;
- hwif->dma_master = 0;
hwif->dma_command = 0;
hwif->dma_vendor1 = 0;
hwif->dma_status = 0;
Index: b/drivers/ide/pci/cmd64x.c
===================================================================
--- a/drivers/ide/pci/cmd64x.c
+++ b/drivers/ide/pci/cmd64x.c
@@ -333,13 +333,14 @@ static void cmd64x_set_dma_mode(ide_driv
static int cmd648_ide_dma_end (ide_drive_t *drive)
{
ide_hwif_t *hwif = HWIF(drive);
+ unsigned long addr = pci_resource_start(hwif->pci_dev, 4) + 0x01;
int err = __ide_dma_end(drive);
u8 irq_mask = hwif->channel ? MRDMODE_INTR_CH1 :
MRDMODE_INTR_CH0;
- u8 mrdmode = inb(hwif->dma_master + 0x01);
+ u8 mrdmode = inb(addr);
/* clear the interrupt bit */
- outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+ outb(mrdmode | irq_mask, addr);
return err;
}
@@ -364,10 +365,11 @@ static int cmd64x_ide_dma_end (ide_drive
static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
{
ide_hwif_t *hwif = HWIF(drive);
+ unsigned long addr = pci_resource_start(hwif->pci_dev, 4) + 0x01;
u8 irq_mask = hwif->channel ? MRDMODE_INTR_CH1 :
MRDMODE_INTR_CH0;
u8 dma_stat = inb(hwif->dma_status);
- u8 mrdmode = inb(hwif->dma_master + 0x01);
+ u8 mrdmode = inb(addr);
#ifdef DEBUG
printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",
Index: b/drivers/ide/pci/hpt366.c
===================================================================
--- a/drivers/ide/pci/hpt366.c
+++ b/drivers/ide/pci/hpt366.c
@@ -827,32 +827,33 @@ static int hpt374_ide_dma_end(ide_drive_
static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
{
- u8 scr2 = inb(hwif->dma_master + 0x7b);
+ unsigned long addr = pci_resource_start(hwif->pci_dev, 4);
+ u8 scr2 = inb(addr + 0x7b);
if ((scr2 & 0x7f) == mode)
return;
/* Tristate the bus */
- outb(0x80, hwif->dma_master + 0x73);
- outb(0x80, hwif->dma_master + 0x77);
+ outb(0x80, addr + 0x73);
+ outb(0x80, addr + 0x77);
/* Switch clock and reset channels */
- outb(mode, hwif->dma_master + 0x7b);
- outb(0xc0, hwif->dma_master + 0x79);
+ outb(mode, addr + 0x7b);
+ outb(0xc0, addr + 0x79);
/*
* Reset the state machines.
* NOTE: avoid accidentally enabling the disabled channels.
*/
- outb(inb(hwif->dma_master + 0x70) | 0x32, hwif->dma_master + 0x70);
- outb(inb(hwif->dma_master + 0x74) | 0x32, hwif->dma_master + 0x74);
+ outb(inb(addr + 0x70) | 0x32, addr + 0x70);
+ outb(inb(addr + 0x74) | 0x32, addr + 0x74);
/* Complete reset */
- outb(0x00, hwif->dma_master + 0x79);
+ outb(0x00, addr + 0x79);
/* Reconnect channels to bus */
- outb(0x00, hwif->dma_master + 0x73);
- outb(0x00, hwif->dma_master + 0x77);
+ outb(0x00, addr + 0x73);
+ outb(0x00, addr + 0x77);
}
/**
Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================
--- a/drivers/ide/pci/pdc202xx_old.c
+++ b/drivers/ide/pci/pdc202xx_old.c
@@ -162,7 +162,7 @@ static u8 pdc202xx_old_cable_detect (ide
*/
static void pdc_old_enable_66MHz_clock(ide_hwif_t *hwif)
{
- unsigned long clock_reg = hwif->dma_master + 0x11;
+ unsigned long clock_reg = pci_resource_start(hwif->pci_dev, 4) + 0x11;
u8 clock = inb(clock_reg);
outb(clock | (hwif->channel ? 0x08 : 0x02), clock_reg);
@@ -170,7 +170,7 @@ static void pdc_old_enable_66MHz_clock(i
static void pdc_old_disable_66MHz_clock(ide_hwif_t *hwif)
{
- unsigned long clock_reg = hwif->dma_master + 0x11;
+ unsigned long clock_reg = pci_resource_start(hwif->pci_dev, 4) + 0x11;
u8 clock = inb(clock_reg);
outb(clock & ~(hwif->channel ? 0x08 : 0x02), clock_reg);
@@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
if (drive->media != ide_disk || drive->addressing == 1) {
struct request *rq = HWGROUP(drive)->rq;
ide_hwif_t *hwif = HWIF(drive);
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = pci_resource_start(hwif->pci_dev, 4);
unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
u32 word_count = 0;
u8 clock = inb(high_16 + 0x11);
@@ -212,7 +212,7 @@ static int pdc202xx_old_ide_dma_end(ide_
{
if (drive->media != ide_disk || drive->addressing == 1) {
ide_hwif_t *hwif = HWIF(drive);
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = pci_resource_start(hwif->pci_dev, 4);
unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
u8 clock = 0;
@@ -228,7 +228,7 @@ static int pdc202xx_old_ide_dma_end(ide_
static int pdc202xx_old_ide_dma_test_irq(ide_drive_t *drive)
{
ide_hwif_t *hwif = HWIF(drive);
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = pci_resource_start(hwif->pci_dev, 4);
u8 dma_stat = inb(hwif->dma_status);
u8 sc1d = inb(high_16 + 0x001d);
@@ -271,7 +271,7 @@ static void pdc202xx_dma_timeout(ide_dri
static void pdc202xx_reset_host (ide_hwif_t *hwif)
{
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = pci_resource_start(hwif->pci_dev, 4);
u8 udma_speed_flag = inb(high_16 | 0x001f);
outb(udma_speed_flag | 0x10, high_16 | 0x001f);
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -764,7 +764,6 @@ typedef struct hwif_s {
int rqsize; /* max sectors per request */
int irq; /* our irq number */
- unsigned long dma_master; /* reference base addr dmabase */
unsigned long dma_base; /* base addr for dma ports */
unsigned long dma_command; /* dma command register */
unsigned long dma_vendor1; /* dma vendor 1 register */
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 12/15] ide: remove ->dma_master field from ide_hwif_t
2007-10-01 21:39 [PATCH 12/15] ide: remove ->dma_master field from ide_hwif_t Bartlomiej Zolnierkiewicz
@ 2007-10-01 21:48 ` Jeff Garzik
2007-10-01 22:06 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 12+ messages in thread
From: Jeff Garzik @ 2007-10-01 21:48 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz; +Cc: linux-ide
Bartlomiej Zolnierkiewicz wrote:
> * Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
> pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.
Before using pci_resource_start(), the code should check
pci_resource_len() to ensure it is the appropriate size.
It would be very wise to ensure that pci_resource_flags()&IORESOURCE_IO
is true, too.
Jeff
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 12/15] ide: remove ->dma_master field from ide_hwif_t
2007-10-01 21:48 ` Jeff Garzik
@ 2007-10-01 22:06 ` Bartlomiej Zolnierkiewicz
2007-10-01 22:31 ` Jeff Garzik
0 siblings, 1 reply; 12+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2007-10-01 22:06 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-ide
On Monday 01 October 2007, Jeff Garzik wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > * Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
> > pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.
>
> Before using pci_resource_start(), the code should check
> pci_resource_len() to ensure it is the appropriate size.
>
> It would be very wise to ensure that pci_resource_flags()&IORESOURCE_IO
> is true, too.
Sure but since the old code hasn't been doing these checks (old code
just did pci_resource_start() -> hwif->dma_base -> hwif->dma_master)
this is a separate issue from what the patch tries to achieve.
Bart
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 12/15] ide: remove ->dma_master field from ide_hwif_t
2007-10-01 22:06 ` Bartlomiej Zolnierkiewicz
@ 2007-10-01 22:31 ` Jeff Garzik
2007-10-02 21:24 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 12+ messages in thread
From: Jeff Garzik @ 2007-10-01 22:31 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz; +Cc: linux-ide
Bartlomiej Zolnierkiewicz wrote:
> On Monday 01 October 2007, Jeff Garzik wrote:
>> Bartlomiej Zolnierkiewicz wrote:
>>> * Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
>>> pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.
>> Before using pci_resource_start(), the code should check
>> pci_resource_len() to ensure it is the appropriate size.
>>
>> It would be very wise to ensure that pci_resource_flags()&IORESOURCE_IO
>> is true, too.
>
> Sure but since the old code hasn't been doing these checks (old code
> just did pci_resource_start() -> hwif->dma_base -> hwif->dma_master)
> this is a separate issue from what the patch tries to achieve.
Nonetheless this is duplicating an existing bug $N times... not ideal :)
Jeff
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 12/15] ide: remove ->dma_master field from ide_hwif_t
2007-10-01 22:31 ` Jeff Garzik
@ 2007-10-02 21:24 ` Bartlomiej Zolnierkiewicz
2007-10-03 12:50 ` Sergei Shtylyov
0 siblings, 1 reply; 12+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2007-10-02 21:24 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-ide
On Tuesday 02 October 2007, Jeff Garzik wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > On Monday 01 October 2007, Jeff Garzik wrote:
> >> Bartlomiej Zolnierkiewicz wrote:
> >>> * Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
> >>> pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.
> >> Before using pci_resource_start(), the code should check
> >> pci_resource_len() to ensure it is the appropriate size.
> >>
> >> It would be very wise to ensure that pci_resource_flags()&IORESOURCE_IO
> >> is true, too.
> >
> > Sure but since the old code hasn't been doing these checks (old code
> > just did pci_resource_start() -> hwif->dma_base -> hwif->dma_master)
> > this is a separate issue from what the patch tries to achieve.
>
> Nonetheless this is duplicating an existing bug $N times... not ideal :)
The existing bug is that the methods converted to use pci_resource_start()
may be set in hwif instance and used even if the BAR4 is invalid. Therefore
the above patch is correct and it isn't duplicating the existing bug. :)
However I agree that all these pci_resource_start()s could be a bit confusing
so here is 'take 2'.
[PATCH] ide: remove ->dma_master field from ide_hwif_t (take 2)
* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.
* Remove no longer needed ->dma_master field from ide_hwif_t.
v2:
* Use the more readable 'hwif->dma_base - (hwif->channel * 8)' instead of
pci_resource_start(hwif->pci_dev, 4).
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
drivers/ide/ide-dma.c | 7 -------
drivers/ide/ide.c | 2 --
drivers/ide/pci/cmd64x.c | 8 +++++---
drivers/ide/pci/hpt366.c | 21 +++++++++++----------
drivers/ide/pci/pdc202xx_old.c | 12 ++++++------
include/linux/ide.h | 1 -
6 files changed, 22 insertions(+), 29 deletions(-)
Index: b/drivers/ide/ide-dma.c
===================================================================
--- a/drivers/ide/ide-dma.c
+++ b/drivers/ide/ide-dma.c
@@ -1006,11 +1006,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
hwif->dma_base = base;
- if (hwif->mate)
- hwif->dma_master = hwif->channel ? hwif->mate->dma_base : base;
- else
- hwif->dma_master = base;
-
if (!(hwif->dma_command))
hwif->dma_command = hwif->dma_base;
if (!(hwif->dma_vendor1))
@@ -1052,8 +1047,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
hwif->drives[1].name, (dma_stat & 0x40) ? "DMA" : "pio");
}
printk("\n");
-
- BUG_ON(!hwif->dma_master);
}
EXPORT_SYMBOL_GPL(ide_setup_dma);
Index: b/drivers/ide/ide.c
===================================================================
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -470,7 +470,6 @@ static void ide_hwif_restore(ide_hwif_t
#endif
hwif->dma_base = tmp_hwif->dma_base;
- hwif->dma_master = tmp_hwif->dma_master;
hwif->dma_command = tmp_hwif->dma_command;
hwif->dma_vendor1 = tmp_hwif->dma_vendor1;
hwif->dma_status = tmp_hwif->dma_status;
@@ -604,7 +603,6 @@ void ide_unregister(unsigned int index)
(void) ide_release_dma(hwif);
hwif->dma_base = 0;
- hwif->dma_master = 0;
hwif->dma_command = 0;
hwif->dma_vendor1 = 0;
hwif->dma_status = 0;
Index: b/drivers/ide/pci/cmd64x.c
===================================================================
--- a/drivers/ide/pci/cmd64x.c
+++ b/drivers/ide/pci/cmd64x.c
@@ -333,13 +333,14 @@ static void cmd64x_set_dma_mode(ide_driv
static int cmd648_ide_dma_end (ide_drive_t *drive)
{
ide_hwif_t *hwif = HWIF(drive);
+ unsigned long addr = hwif->dma_base - (hwif->channel * 8) + 0x01;
int err = __ide_dma_end(drive);
u8 irq_mask = hwif->channel ? MRDMODE_INTR_CH1 :
MRDMODE_INTR_CH0;
- u8 mrdmode = inb(hwif->dma_master + 0x01);
+ u8 mrdmode = inb(addr);
/* clear the interrupt bit */
- outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+ outb(mrdmode | irq_mask, addr);
return err;
}
@@ -364,10 +365,11 @@ static int cmd64x_ide_dma_end (ide_drive
static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
{
ide_hwif_t *hwif = HWIF(drive);
+ unsigned long addr = hwif->dma_base - (hwif->channel * 8) + 0x01;
u8 irq_mask = hwif->channel ? MRDMODE_INTR_CH1 :
MRDMODE_INTR_CH0;
u8 dma_stat = inb(hwif->dma_status);
- u8 mrdmode = inb(hwif->dma_master + 0x01);
+ u8 mrdmode = inb(addr);
#ifdef DEBUG
printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",
Index: b/drivers/ide/pci/hpt366.c
===================================================================
--- a/drivers/ide/pci/hpt366.c
+++ b/drivers/ide/pci/hpt366.c
@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
{
- u8 scr2 = inb(hwif->dma_master + 0x7b);
+ unsigned long addr = hwif->dma_base - (hwif->channel * 8);
+ u8 scr2 = inb(addr + 0x7b);
if ((scr2 & 0x7f) == mode)
return;
/* Tristate the bus */
- outb(0x80, hwif->dma_master + 0x73);
- outb(0x80, hwif->dma_master + 0x77);
+ outb(0x80, addr + 0x73);
+ outb(0x80, addr + 0x77);
/* Switch clock and reset channels */
- outb(mode, hwif->dma_master + 0x7b);
- outb(0xc0, hwif->dma_master + 0x79);
+ outb(mode, addr + 0x7b);
+ outb(0xc0, addr + 0x79);
/*
* Reset the state machines.
* NOTE: avoid accidentally enabling the disabled channels.
*/
- outb(inb(hwif->dma_master + 0x70) | 0x32, hwif->dma_master + 0x70);
- outb(inb(hwif->dma_master + 0x74) | 0x32, hwif->dma_master + 0x74);
+ outb(inb(addr + 0x70) | 0x32, addr + 0x70);
+ outb(inb(addr + 0x74) | 0x32, addr + 0x74);
/* Complete reset */
- outb(0x00, hwif->dma_master + 0x79);
+ outb(0x00, addr + 0x79);
/* Reconnect channels to bus */
- outb(0x00, hwif->dma_master + 0x73);
- outb(0x00, hwif->dma_master + 0x77);
+ outb(0x00, addr + 0x73);
+ outb(0x00, addr + 0x77);
}
/**
Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================
--- a/drivers/ide/pci/pdc202xx_old.c
+++ b/drivers/ide/pci/pdc202xx_old.c
@@ -162,7 +162,7 @@ static u8 pdc202xx_old_cable_detect (ide
*/
static void pdc_old_enable_66MHz_clock(ide_hwif_t *hwif)
{
- unsigned long clock_reg = hwif->dma_master + 0x11;
+ unsigned long clock_reg = hwif->dma_base - (hwif->channel * 8) + 0x11;
u8 clock = inb(clock_reg);
outb(clock | (hwif->channel ? 0x08 : 0x02), clock_reg);
@@ -170,7 +170,7 @@ static void pdc_old_enable_66MHz_clock(i
static void pdc_old_disable_66MHz_clock(ide_hwif_t *hwif)
{
- unsigned long clock_reg = hwif->dma_master + 0x11;
+ unsigned long clock_reg = hwif->dma_base - (hwif->channel * 8) + 0x11;
u8 clock = inb(clock_reg);
outb(clock & ~(hwif->channel ? 0x08 : 0x02), clock_reg);
@@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
if (drive->media != ide_disk || drive->addressing == 1) {
struct request *rq = HWGROUP(drive)->rq;
ide_hwif_t *hwif = HWIF(drive);
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->dma_base - (hwif->channel * 8);
unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
u32 word_count = 0;
u8 clock = inb(high_16 + 0x11);
@@ -212,7 +212,7 @@ static int pdc202xx_old_ide_dma_end(ide_
{
if (drive->media != ide_disk || drive->addressing == 1) {
ide_hwif_t *hwif = HWIF(drive);
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->dma_base - (hwif->channel * 8);
unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
u8 clock = 0;
@@ -228,7 +228,7 @@ static int pdc202xx_old_ide_dma_end(ide_
static int pdc202xx_old_ide_dma_test_irq(ide_drive_t *drive)
{
ide_hwif_t *hwif = HWIF(drive);
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->dma_base - (hwif->channel * 8);
u8 dma_stat = inb(hwif->dma_status);
u8 sc1d = inb(high_16 + 0x001d);
@@ -271,7 +271,7 @@ static void pdc202xx_dma_timeout(ide_dri
static void pdc202xx_reset_host (ide_hwif_t *hwif)
{
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->dma_base - (hwif->channel * 8);
u8 udma_speed_flag = inb(high_16 | 0x001f);
outb(udma_speed_flag | 0x10, high_16 | 0x001f);
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -764,7 +764,6 @@ typedef struct hwif_s {
int rqsize; /* max sectors per request */
int irq; /* our irq number */
- unsigned long dma_master; /* reference base addr dmabase */
unsigned long dma_base; /* base addr for dma ports */
unsigned long dma_command; /* dma command register */
unsigned long dma_vendor1; /* dma vendor 1 register */
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 12/15] ide: remove ->dma_master field from ide_hwif_t
2007-10-02 21:24 ` Bartlomiej Zolnierkiewicz
@ 2007-10-03 12:50 ` Sergei Shtylyov
2007-10-03 22:27 ` Bartlomiej Zolnierkiewicz
2007-10-04 13:27 ` Sergei Shtylyov
0 siblings, 2 replies; 12+ messages in thread
From: Sergei Shtylyov @ 2007-10-03 12:50 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz; +Cc: Jeff Garzik, linux-ide
Hello.
Bartlomiej Zolnierkiewicz wrote:
>>>>>* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
>>>>> pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.
>>>>Before using pci_resource_start(), the code should check
>>>>pci_resource_len() to ensure it is the appropriate size.
>>>>It would be very wise to ensure that pci_resource_flags()&IORESOURCE_IO
>>>>is true, too.
>>>Sure but since the old code hasn't been doing these checks (old code
>>>just did pci_resource_start() -> hwif->dma_base -> hwif->dma_master)
>>>this is a separate issue from what the patch tries to achieve.
>>Nonetheless this is duplicating an existing bug $N times... not ideal :)
> The existing bug is that the methods converted to use pci_resource_start()
> may be set in hwif instance and used even if the BAR4 is invalid. Therefore
> the above patch is correct and it isn't duplicating the existing bug. :)
> However I agree that all these pci_resource_start()s could be a bit confusing
> so here is 'take 2'.
Yeah, they are. Actually, I was thinking about such patch ~1.5 years ago
*but* using a different approach -- namely, hwif->extra_base.
> [PATCH] ide: remove ->dma_master field from ide_hwif_t (take 2)
> * Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
> pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.
> * Remove no longer needed ->dma_master field from ide_hwif_t.
> v2:
> * Use the more readable 'hwif->dma_base - (hwif->channel * 8)' instead of
> pci_resource_start(hwif->pci_dev, 4).
s/readable/clumsy/? ;-)
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Index: b/drivers/ide/ide-dma.c
> ===================================================================
> --- a/drivers/ide/ide-dma.c
> +++ b/drivers/ide/ide-dma.c
> @@ -1006,11 +1006,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
>
> hwif->dma_base = base;
>
> - if (hwif->mate)
> - hwif->dma_master = hwif->channel ? hwif->mate->dma_base : base;
> - else
> - hwif->dma_master = base;
> -
> if (!(hwif->dma_command))
> hwif->dma_command = hwif->dma_base;
> if (!(hwif->dma_vendor1))
Ugh, these extra parens... :-)
[...]
> Index: b/drivers/ide/pci/cmd64x.c
> ===================================================================
> --- a/drivers/ide/pci/cmd64x.c
> +++ b/drivers/ide/pci/cmd64x.c
> @@ -333,13 +333,14 @@ static void cmd64x_set_dma_mode(ide_driv
> static int cmd648_ide_dma_end (ide_drive_t *drive)
> {
> ide_hwif_t *hwif = HWIF(drive);
> + unsigned long addr = hwif->dma_base - (hwif->channel * 8) + 0x01;
Dunno... pci_resource_start() looked better to my taste. Except I'd
probably called the variable 'base' and wouldn't + 0x01 right away.
> int err = __ide_dma_end(drive);
> u8 irq_mask = hwif->channel ? MRDMODE_INTR_CH1 :
> MRDMODE_INTR_CH0;
> - u8 mrdmode = inb(hwif->dma_master + 0x01);
> + u8 mrdmode = inb(addr);
>
> /* clear the interrupt bit */
> - outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
> + outb(mrdmode | irq_mask, addr);
>
> return err;
> }
> @@ -364,10 +365,11 @@ static int cmd64x_ide_dma_end (ide_drive
> static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
> {
> ide_hwif_t *hwif = HWIF(drive);
> + unsigned long addr = hwif->dma_base - (hwif->channel * 8) + 0x01;
Same comment here...
> u8 irq_mask = hwif->channel ? MRDMODE_INTR_CH1 :
> MRDMODE_INTR_CH0;
> u8 dma_stat = inb(hwif->dma_status);
> - u8 mrdmode = inb(hwif->dma_master + 0x01);
> + u8 mrdmode = inb(addr);
>
> #ifdef DEBUG
> printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",
> Index: b/drivers/ide/pci/hpt366.c
> ===================================================================
> --- a/drivers/ide/pci/hpt366.c
> +++ b/drivers/ide/pci/hpt366.c
> @@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
This just asks to be:
> static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
> {
unsigned long base = hwif->extra_base;
u8 scr2 = inb(base + 0x5b);
if ((scr2 & 0x7f) == mode)
return;
/* Tristate the bus */
outb(0x80, base + 0x53);
outb(0x80, base + 0x57);
/* Switch clock and reset channels */
outb(mode, base + 0x5b);
outb(0xc0, base + 0x59);
/*
* Reset the state machines.
* NOTE: avoid accidentally enabling the disabled channels.
*/
outb(inb(base + 0x50) | 0x32, base + 0x50);
outb(inb(base + 0x54) | 0x32, base + 0x54);
/* Complete reset */
outb(0x00, base + 0x59);
/* Reconnect channels to bus */
outb(0x00, base + 0x53);
outb(0x00, base + 0x57);
}
> Index: b/drivers/ide/pci/pdc202xx_old.c
> ===================================================================
> --- a/drivers/ide/pci/pdc202xx_old.c
> +++ b/drivers/ide/pci/pdc202xx_old.c
> @@ -162,7 +162,7 @@ static u8 pdc202xx_old_cable_detect (ide
And this asks to be:
> */
> static void pdc_old_enable_66MHz_clock(ide_hwif_t *hwif)
> {
> - unsigned long clock_reg = hwif->dma_master + 0x11;
+ unsigned long clock_reg = hwif->extra_base + 0x01;
> u8 clock = inb(clock_reg);
>
> outb(clock | (hwif->channel ? 0x08 : 0x02), clock_reg);
> @@ -170,7 +170,7 @@ static void pdc_old_enable_66MHz_clock(i
>
> static void pdc_old_disable_66MHz_clock(ide_hwif_t *hwif)
> {
> - unsigned long clock_reg = hwif->dma_master + 0x11;
+ unsigned long clock_reg = hwif->extra_base + 0x01;
> u8 clock = inb(clock_reg);
>
> outb(clock & ~(hwif->channel ? 0x08 : 0x02), clock_reg);
> @@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
> if (drive->media != ide_disk || drive->addressing == 1) {
> struct request *rq = HWGROUP(drive)->rq;
> ide_hwif_t *hwif = HWIF(drive);
> - unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->extra_base;
> unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
> u32 word_count = 0;
> u8 clock = inb(high_16 + 0x11);
> @@ -212,7 +212,7 @@ static int pdc202xx_old_ide_dma_end(ide_
> {
> if (drive->media != ide_disk || drive->addressing == 1) {
> ide_hwif_t *hwif = HWIF(drive);
> - unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->extra_base;
> unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
> u8 clock = 0;
>
> @@ -228,7 +228,7 @@ static int pdc202xx_old_ide_dma_end(ide_
> static int pdc202xx_old_ide_dma_test_irq(ide_drive_t *drive)
> {
> ide_hwif_t *hwif = HWIF(drive);
> - unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->extra_base;
> u8 dma_stat = inb(hwif->dma_status);
> u8 sc1d = inb(high_16 + 0x001d);
>
> @@ -271,7 +271,7 @@ static void pdc202xx_dma_timeout(ide_dri
>
> static void pdc202xx_reset_host (ide_hwif_t *hwif)
> {
> - unsigned long high_16 = hwif->dma_master;
> + unsigned long high_16 = hwif->dma_base - (hwif->channel * 8);
unsigned long high_16 = hwif->extra_base;
> u8 udma_speed_flag = inb(high_16 | 0x001f);
MBR, Sergei
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 12/15] ide: remove ->dma_master field from ide_hwif_t
2007-10-03 12:50 ` Sergei Shtylyov
@ 2007-10-03 22:27 ` Bartlomiej Zolnierkiewicz
2007-10-04 13:27 ` Sergei Shtylyov
1 sibling, 0 replies; 12+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2007-10-03 22:27 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: Jeff Garzik, linux-ide
On Wednesday 03 October 2007, Sergei Shtylyov wrote:
> Hello.
>
> Bartlomiej Zolnierkiewicz wrote:
>
> >>>>>* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
> >>>>> pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.
>
> >>>>Before using pci_resource_start(), the code should check
> >>>>pci_resource_len() to ensure it is the appropriate size.
>
> >>>>It would be very wise to ensure that pci_resource_flags()&IORESOURCE_IO
> >>>>is true, too.
>
> >>>Sure but since the old code hasn't been doing these checks (old code
> >>>just did pci_resource_start() -> hwif->dma_base -> hwif->dma_master)
> >>>this is a separate issue from what the patch tries to achieve.
>
> >>Nonetheless this is duplicating an existing bug $N times... not ideal :)
>
> > The existing bug is that the methods converted to use pci_resource_start()
> > may be set in hwif instance and used even if the BAR4 is invalid. Therefore
> > the above patch is correct and it isn't duplicating the existing bug. :)
>
> > However I agree that all these pci_resource_start()s could be a bit confusing
> > so here is 'take 2'.
>
> Yeah, they are. Actually, I was thinking about such patch ~1.5 years ago
> *but* using a different approach -- namely, hwif->extra_base.
good idea!
> [...]
> > Index: b/drivers/ide/pci/cmd64x.c
> > ===================================================================
> > --- a/drivers/ide/pci/cmd64x.c
> > +++ b/drivers/ide/pci/cmd64x.c
> > @@ -333,13 +333,14 @@ static void cmd64x_set_dma_mode(ide_driv
> > static int cmd648_ide_dma_end (ide_drive_t *drive)
> > {
> > ide_hwif_t *hwif = HWIF(drive);
> > + unsigned long addr = hwif->dma_base - (hwif->channel * 8) + 0x01;
>
> Dunno... pci_resource_start() looked better to my taste. Except I'd
> probably called the variable 'base' and wouldn't + 0x01 right away.
fixed
> > int err = __ide_dma_end(drive);
> > u8 irq_mask = hwif->channel ? MRDMODE_INTR_CH1 :
> > MRDMODE_INTR_CH0;
> > - u8 mrdmode = inb(hwif->dma_master + 0x01);
> > + u8 mrdmode = inb(addr);
> >
> > /* clear the interrupt bit */
> > - outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
> > + outb(mrdmode | irq_mask, addr);
> >
> > return err;
> > }
> > @@ -364,10 +365,11 @@ static int cmd64x_ide_dma_end (ide_drive
> > static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
> > {
> > ide_hwif_t *hwif = HWIF(drive);
> > + unsigned long addr = hwif->dma_base - (hwif->channel * 8) + 0x01;
>
> Same comment here...
ditto
> > u8 irq_mask = hwif->channel ? MRDMODE_INTR_CH1 :
> > MRDMODE_INTR_CH0;
> > u8 dma_stat = inb(hwif->dma_status);
> > - u8 mrdmode = inb(hwif->dma_master + 0x01);
> > + u8 mrdmode = inb(addr);
> >
> > #ifdef DEBUG
> > printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",
> > Index: b/drivers/ide/pci/hpt366.c
> > ===================================================================
> > --- a/drivers/ide/pci/hpt366.c
> > +++ b/drivers/ide/pci/hpt366.c
> > @@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
>
> This just asks to be:
>
> > static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
> > {
> unsigned long base = hwif->extra_base;
> u8 scr2 = inb(base + 0x5b);
>
> if ((scr2 & 0x7f) == mode)
> return;
>
> /* Tristate the bus */
> outb(0x80, base + 0x53);
> outb(0x80, base + 0x57);
>
> /* Switch clock and reset channels */
> outb(mode, base + 0x5b);
> outb(0xc0, base + 0x59);
>
> /*
> * Reset the state machines.
> * NOTE: avoid accidentally enabling the disabled channels.
> */
> outb(inb(base + 0x50) | 0x32, base + 0x50);
> outb(inb(base + 0x54) | 0x32, base + 0x54);
>
> /* Complete reset */
> outb(0x00, base + 0x59);
>
> /* Reconnect channels to bus */
> outb(0x00, base + 0x53);
> outb(0x00, base + 0x57);
> }
ditto
> > Index: b/drivers/ide/pci/pdc202xx_old.c
> > ===================================================================
> > --- a/drivers/ide/pci/pdc202xx_old.c
> > +++ b/drivers/ide/pci/pdc202xx_old.c
> > @@ -162,7 +162,7 @@ static u8 pdc202xx_old_cable_detect (ide
>
> And this asks to be:
>
> > */
> > static void pdc_old_enable_66MHz_clock(ide_hwif_t *hwif)
> > {
> > - unsigned long clock_reg = hwif->dma_master + 0x11;
> + unsigned long clock_reg = hwif->extra_base + 0x01;
ditto
> > u8 clock = inb(clock_reg);
> >
> > outb(clock | (hwif->channel ? 0x08 : 0x02), clock_reg);
> > @@ -170,7 +170,7 @@ static void pdc_old_enable_66MHz_clock(i
> >
> > static void pdc_old_disable_66MHz_clock(ide_hwif_t *hwif)
> > {
> > - unsigned long clock_reg = hwif->dma_master + 0x11;
>
> + unsigned long clock_reg = hwif->extra_base + 0x01;
ditto
> > u8 clock = inb(clock_reg);
> >
> > outb(clock & ~(hwif->channel ? 0x08 : 0x02), clock_reg);
> > @@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
> > if (drive->media != ide_disk || drive->addressing == 1) {
> > struct request *rq = HWGROUP(drive)->rq;
> > ide_hwif_t *hwif = HWIF(drive);
> > - unsigned long high_16 = hwif->dma_master;
>
> + unsigned long high_16 = hwif->extra_base;
fixed but with s/hwif->extra_base/hwif->extra_base - 16/
> > unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
> > u32 word_count = 0;
> > u8 clock = inb(high_16 + 0x11);
> > @@ -212,7 +212,7 @@ static int pdc202xx_old_ide_dma_end(ide_
> > {
> > if (drive->media != ide_disk || drive->addressing == 1) {
> > ide_hwif_t *hwif = HWIF(drive);
> > - unsigned long high_16 = hwif->dma_master;
>
> + unsigned long high_16 = hwif->extra_base;
ditto
> > unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
> > u8 clock = 0;
> >
> > @@ -228,7 +228,7 @@ static int pdc202xx_old_ide_dma_end(ide_
> > static int pdc202xx_old_ide_dma_test_irq(ide_drive_t *drive)
> > {
> > ide_hwif_t *hwif = HWIF(drive);
> > - unsigned long high_16 = hwif->dma_master;
>
> + unsigned long high_16 = hwif->extra_base;
ditto
> > u8 dma_stat = inb(hwif->dma_status);
> > u8 sc1d = inb(high_16 + 0x001d);
> >
> > @@ -271,7 +271,7 @@ static void pdc202xx_dma_timeout(ide_dri
> >
> > static void pdc202xx_reset_host (ide_hwif_t *hwif)
> > {
> > - unsigned long high_16 = hwif->dma_master;
> > + unsigned long high_16 = hwif->dma_base - (hwif->channel * 8);
>
> unsigned long high_16 = hwif->extra_base;
ditto
thanks for all hints, new version of the patch below...
[PATCH] ide: remove ->dma_master field from ide_hwif_t (take 3)
* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.
* Remove no longer needed ->dma_master field from ide_hwif_t.
v2:
* Use the more readable 'hwif->dma_base - (hwif->channel * 8)' instead of
pci_resource_start(hwif->pci_dev, 4).
v3:
* Use hwif->extra_base in hpt366/pdc20xx_old + some cosmetic fixups over v2
(suggested by Sergei).
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
Cc: Jeff Garzik <jeff@garzik.org>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
drivers/ide/ide-dma.c | 7 -------
drivers/ide/ide.c | 2 --
drivers/ide/pci/cmd64x.c | 8 +++++---
drivers/ide/pci/hpt366.c | 21 +++++++++++----------
drivers/ide/pci/pdc202xx_old.c | 12 ++++++------
include/linux/ide.h | 1 -
6 files changed, 22 insertions(+), 29 deletions(-)
Index: b/drivers/ide/ide-dma.c
===================================================================
--- a/drivers/ide/ide-dma.c
+++ b/drivers/ide/ide-dma.c
@@ -1002,11 +1002,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
hwif->dma_base = base;
- if (hwif->mate)
- hwif->dma_master = hwif->channel ? hwif->mate->dma_base : base;
- else
- hwif->dma_master = base;
-
if (!(hwif->dma_command))
hwif->dma_command = hwif->dma_base;
if (!(hwif->dma_vendor1))
@@ -1048,8 +1043,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
hwif->drives[1].name, (dma_stat & 0x40) ? "DMA" : "pio");
}
printk("\n");
-
- BUG_ON(!hwif->dma_master);
}
EXPORT_SYMBOL_GPL(ide_setup_dma);
Index: b/drivers/ide/ide.c
===================================================================
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -468,7 +468,6 @@ static void ide_hwif_restore(ide_hwif_t
#endif
hwif->dma_base = tmp_hwif->dma_base;
- hwif->dma_master = tmp_hwif->dma_master;
hwif->dma_command = tmp_hwif->dma_command;
hwif->dma_vendor1 = tmp_hwif->dma_vendor1;
hwif->dma_status = tmp_hwif->dma_status;
@@ -602,7 +601,6 @@ void ide_unregister(unsigned int index)
(void) ide_release_dma(hwif);
hwif->dma_base = 0;
- hwif->dma_master = 0;
hwif->dma_command = 0;
hwif->dma_vendor1 = 0;
hwif->dma_status = 0;
Index: b/drivers/ide/pci/cmd64x.c
===================================================================
--- a/drivers/ide/pci/cmd64x.c
+++ b/drivers/ide/pci/cmd64x.c
@@ -333,13 +333,14 @@ static void cmd64x_set_dma_mode(ide_driv
static int cmd648_ide_dma_end (ide_drive_t *drive)
{
ide_hwif_t *hwif = HWIF(drive);
+ unsigned long base = hwif->dma_base - (hwif->channel * 8);
int err = __ide_dma_end(drive);
u8 irq_mask = hwif->channel ? MRDMODE_INTR_CH1 :
MRDMODE_INTR_CH0;
- u8 mrdmode = inb(hwif->dma_master + 0x01);
+ u8 mrdmode = inb(base + 1);
/* clear the interrupt bit */
- outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+ outb(mrdmode | irq_mask, base + 1);
return err;
}
@@ -364,10 +365,11 @@ static int cmd64x_ide_dma_end (ide_drive
static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
{
ide_hwif_t *hwif = HWIF(drive);
+ unsigned long base = hwif->dma_base - (hwif->channel * 8);
u8 irq_mask = hwif->channel ? MRDMODE_INTR_CH1 :
MRDMODE_INTR_CH0;
u8 dma_stat = inb(hwif->dma_status);
- u8 mrdmode = inb(hwif->dma_master + 0x01);
+ u8 mrdmode = inb(base + 1);
#ifdef DEBUG
printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",
Index: b/drivers/ide/pci/hpt366.c
===================================================================
--- a/drivers/ide/pci/hpt366.c
+++ b/drivers/ide/pci/hpt366.c
@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
{
- u8 scr2 = inb(hwif->dma_master + 0x7b);
+ unsigned long base = hwif->dma_base - (hwif->channel * 8);
+ u8 scr2 = inb(base + 0x7b);
if ((scr2 & 0x7f) == mode)
return;
/* Tristate the bus */
- outb(0x80, hwif->dma_master + 0x73);
- outb(0x80, hwif->dma_master + 0x77);
+ outb(0x80, base + 0x73);
+ outb(0x80, base + 0x77);
/* Switch clock and reset channels */
- outb(mode, hwif->dma_master + 0x7b);
- outb(0xc0, hwif->dma_master + 0x79);
+ outb(mode, base + 0x7b);
+ outb(0xc0, base + 0x79);
/*
* Reset the state machines.
* NOTE: avoid accidentally enabling the disabled channels.
*/
- outb(inb(hwif->dma_master + 0x70) | 0x32, hwif->dma_master + 0x70);
- outb(inb(hwif->dma_master + 0x74) | 0x32, hwif->dma_master + 0x74);
+ outb(inb(base + 0x70) | 0x32, base + 0x70);
+ outb(inb(base + 0x74) | 0x32, base + 0x74);
/* Complete reset */
- outb(0x00, hwif->dma_master + 0x79);
+ outb(0x00, base + 0x79);
/* Reconnect channels to bus */
- outb(0x00, hwif->dma_master + 0x73);
- outb(0x00, hwif->dma_master + 0x77);
+ outb(0x00, base + 0x73);
+ outb(0x00, base + 0x77);
}
/**
Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================
--- a/drivers/ide/pci/pdc202xx_old.c
+++ b/drivers/ide/pci/pdc202xx_old.c
@@ -162,7 +162,7 @@ static u8 pdc202xx_old_cable_detect (ide
*/
static void pdc_old_enable_66MHz_clock(ide_hwif_t *hwif)
{
- unsigned long clock_reg = hwif->dma_master + 0x11;
+ unsigned long clock_reg = hwif->extra_base + 0x01;
u8 clock = inb(clock_reg);
outb(clock | (hwif->channel ? 0x08 : 0x02), clock_reg);
@@ -170,7 +170,7 @@ static void pdc_old_enable_66MHz_clock(i
static void pdc_old_disable_66MHz_clock(ide_hwif_t *hwif)
{
- unsigned long clock_reg = hwif->dma_master + 0x11;
+ unsigned long clock_reg = hwif->extra_base + 0x01;
u8 clock = inb(clock_reg);
outb(clock & ~(hwif->channel ? 0x08 : 0x02), clock_reg);
@@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
if (drive->media != ide_disk || drive->addressing == 1) {
struct request *rq = HWGROUP(drive)->rq;
ide_hwif_t *hwif = HWIF(drive);
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->extra_base - 16;
unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
u32 word_count = 0;
u8 clock = inb(high_16 + 0x11);
@@ -212,7 +212,7 @@ static int pdc202xx_old_ide_dma_end(ide_
{
if (drive->media != ide_disk || drive->addressing == 1) {
ide_hwif_t *hwif = HWIF(drive);
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->extra_base - 16;
unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
u8 clock = 0;
@@ -228,7 +228,7 @@ static int pdc202xx_old_ide_dma_end(ide_
static int pdc202xx_old_ide_dma_test_irq(ide_drive_t *drive)
{
ide_hwif_t *hwif = HWIF(drive);
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->extra_base - 16;
u8 dma_stat = inb(hwif->dma_status);
u8 sc1d = inb(high_16 + 0x001d);
@@ -271,7 +271,7 @@ static void pdc202xx_dma_timeout(ide_dri
static void pdc202xx_reset_host (ide_hwif_t *hwif)
{
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->extra_base - 16;
u8 udma_speed_flag = inb(high_16 | 0x001f);
outb(udma_speed_flag | 0x10, high_16 | 0x001f);
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -764,7 +764,6 @@ typedef struct hwif_s {
int rqsize; /* max sectors per request */
int irq; /* our irq number */
- unsigned long dma_master; /* reference base addr dmabase */
unsigned long dma_base; /* base addr for dma ports */
unsigned long dma_command; /* dma command register */
unsigned long dma_vendor1; /* dma vendor 1 register */
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 12/15] ide: remove ->dma_master field from ide_hwif_t
2007-10-03 12:50 ` Sergei Shtylyov
2007-10-03 22:27 ` Bartlomiej Zolnierkiewicz
@ 2007-10-04 13:27 ` Sergei Shtylyov
2007-10-08 21:03 ` Bartlomiej Zolnierkiewicz
1 sibling, 1 reply; 12+ messages in thread
From: Sergei Shtylyov @ 2007-10-04 13:27 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: Bartlomiej Zolnierkiewicz, Jeff Garzik, linux-ide
Hello, I wrote:
>>>>>> * Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
>>>>>> pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.
>>>>> Before using pci_resource_start(), the code should check
>>>>> pci_resource_len() to ensure it is the appropriate size.
>>>>> It would be very wise to ensure that
>>>>> pci_resource_flags()&IORESOURCE_IO is true, too.
>>>> Sure but since the old code hasn't been doing these checks (old code
>>>> just did pci_resource_start() -> hwif->dma_base -> hwif->dma_master)
>>>> this is a separate issue from what the patch tries to achieve.
>>> Nonetheless this is duplicating an existing bug $N times... not ideal :)
>> The existing bug is that the methods converted to use
>> pci_resource_start()
>> may be set in hwif instance and used even if the BAR4 is invalid.
>> Therefore
>> the above patch is correct and it isn't duplicating the existing bug. :)
>> However I agree that all these pci_resource_start()s could be a bit
>> confusing
>> so here is 'take 2'.
> Yeah, they are. Actually, I was thinking about such patch ~1.5 years
> ago *but* using a different approach -- namely, hwif->extra_base.
Yes, the code introducing this field was written with replacing dma_master
by it in mind... It's a pity I haven't gotten around to it.
[...]
>> Index: b/drivers/ide/pci/hpt366.c
>> ===================================================================
>> --- a/drivers/ide/pci/hpt366.c
>> +++ b/drivers/ide/pci/hpt366.c
>> @@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
> This just asks to be:
No, it doesn't cause that way one would only break it.
>> static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
>> {
> unsigned long base = hwif->extra_base;
Sorry for a thinko. All offsets below shoud be really 0x6x, not 0x5x...
> u8 scr2 = inb(base + 0x5b);
>
> if ((scr2 & 0x7f) == mode)
> return;
>
> /* Tristate the bus */
> outb(0x80, base + 0x53);
> outb(0x80, base + 0x57);
>
> /* Switch clock and reset channels */
> outb(mode, base + 0x5b);
> outb(0xc0, base + 0x59);
>
> /*
> * Reset the state machines.
> * NOTE: avoid accidentally enabling the disabled channels.
> */
> outb(inb(base + 0x50) | 0x32, base + 0x50);
> outb(inb(base + 0x54) | 0x32, base + 0x54);
>
> /* Complete reset */
> outb(0x00, base + 0x59);
>
> /* Reconnect channels to bus */
> outb(0x00, base + 0x53);
> outb(0x00, base + 0x57);
> }
MBR, Sergei
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 12/15] ide: remove ->dma_master field from ide_hwif_t
2007-10-04 13:27 ` Sergei Shtylyov
@ 2007-10-08 21:03 ` Bartlomiej Zolnierkiewicz
2007-10-18 17:08 ` Sergei Shtylyov
0 siblings, 1 reply; 12+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2007-10-08 21:03 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: Jeff Garzik, linux-ide
Hi,
On Thursday 04 October 2007, Sergei Shtylyov wrote:
> [...]
>
> >> Index: b/drivers/ide/pci/hpt366.c
> >> ===================================================================
> >> --- a/drivers/ide/pci/hpt366.c
> >> +++ b/drivers/ide/pci/hpt366.c
> >> @@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
>
> > This just asks to be:
>
> No, it doesn't cause that way one would only break it.
>
> >> static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
> >> {
> > unsigned long base = hwif->extra_base;
>
> Sorry for a thinko. All offsets below shoud be really 0x6x, not 0x5x...
Here we go again... :)
[PATCH] ide: remove ->dma_master field from ide_hwif_t (take 4)
* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.
* Remove no longer needed ->dma_master field from ide_hwif_t.
v2:
* Use the more readable 'hwif->dma_base - (hwif->channel * 8)' instead of
pci_resource_start(hwif->pci_dev, 4).
v3:
* Use hwif->extra_base in hpt366/pdc20xx_old + some cosmetic fixups over v2
(suggested by Sergei).
v4:
* Correct offsets in hpt3xxn_set_clock().
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
Cc: Jeff Garzik <jeff@garzik.org>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
drivers/ide/ide-dma.c | 7 -------
drivers/ide/ide.c | 2 --
drivers/ide/pci/cmd64x.c | 8 +++++---
drivers/ide/pci/hpt366.c | 21 +++++++++++----------
drivers/ide/pci/pdc202xx_old.c | 12 ++++++------
include/linux/ide.h | 1 -
6 files changed, 22 insertions(+), 29 deletions(-)
Index: b/drivers/ide/ide-dma.c
===================================================================
--- a/drivers/ide/ide-dma.c
+++ b/drivers/ide/ide-dma.c
@@ -1002,11 +1002,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
hwif->dma_base = base;
- if (hwif->mate)
- hwif->dma_master = hwif->channel ? hwif->mate->dma_base : base;
- else
- hwif->dma_master = base;
-
if (!(hwif->dma_command))
hwif->dma_command = hwif->dma_base;
if (!(hwif->dma_vendor1))
@@ -1048,8 +1043,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
hwif->drives[1].name, (dma_stat & 0x40) ? "DMA" : "pio");
}
printk("\n");
-
- BUG_ON(!hwif->dma_master);
}
EXPORT_SYMBOL_GPL(ide_setup_dma);
Index: b/drivers/ide/ide.c
===================================================================
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -468,7 +468,6 @@ static void ide_hwif_restore(ide_hwif_t
#endif
hwif->dma_base = tmp_hwif->dma_base;
- hwif->dma_master = tmp_hwif->dma_master;
hwif->dma_command = tmp_hwif->dma_command;
hwif->dma_vendor1 = tmp_hwif->dma_vendor1;
hwif->dma_status = tmp_hwif->dma_status;
@@ -602,7 +601,6 @@ void ide_unregister(unsigned int index)
(void) ide_release_dma(hwif);
hwif->dma_base = 0;
- hwif->dma_master = 0;
hwif->dma_command = 0;
hwif->dma_vendor1 = 0;
hwif->dma_status = 0;
Index: b/drivers/ide/pci/cmd64x.c
===================================================================
--- a/drivers/ide/pci/cmd64x.c
+++ b/drivers/ide/pci/cmd64x.c
@@ -333,13 +333,14 @@ static void cmd64x_set_dma_mode(ide_driv
static int cmd648_ide_dma_end (ide_drive_t *drive)
{
ide_hwif_t *hwif = HWIF(drive);
+ unsigned long base = hwif->dma_base - (hwif->channel * 8);
int err = __ide_dma_end(drive);
u8 irq_mask = hwif->channel ? MRDMODE_INTR_CH1 :
MRDMODE_INTR_CH0;
- u8 mrdmode = inb(hwif->dma_master + 0x01);
+ u8 mrdmode = inb(base + 1);
/* clear the interrupt bit */
- outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+ outb(mrdmode | irq_mask, base + 1);
return err;
}
@@ -364,10 +365,11 @@ static int cmd64x_ide_dma_end (ide_drive
static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
{
ide_hwif_t *hwif = HWIF(drive);
+ unsigned long base = hwif->dma_base - (hwif->channel * 8);
u8 irq_mask = hwif->channel ? MRDMODE_INTR_CH1 :
MRDMODE_INTR_CH0;
u8 dma_stat = inb(hwif->dma_status);
- u8 mrdmode = inb(hwif->dma_master + 0x01);
+ u8 mrdmode = inb(base + 1);
#ifdef DEBUG
printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",
Index: b/drivers/ide/pci/hpt366.c
===================================================================
--- a/drivers/ide/pci/hpt366.c
+++ b/drivers/ide/pci/hpt366.c
@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
{
- u8 scr2 = inb(hwif->dma_master + 0x7b);
+ unsigned long base = hwif->dma_base - (hwif->channel * 8);
+ u8 scr2 = inb(base + 0x6b);
if ((scr2 & 0x7f) == mode)
return;
/* Tristate the bus */
- outb(0x80, hwif->dma_master + 0x73);
- outb(0x80, hwif->dma_master + 0x77);
+ outb(0x80, base + 0x63);
+ outb(0x80, base + 0x67);
/* Switch clock and reset channels */
- outb(mode, hwif->dma_master + 0x7b);
- outb(0xc0, hwif->dma_master + 0x79);
+ outb(mode, base + 0x6b);
+ outb(0xc0, base + 0x69);
/*
* Reset the state machines.
* NOTE: avoid accidentally enabling the disabled channels.
*/
- outb(inb(hwif->dma_master + 0x70) | 0x32, hwif->dma_master + 0x70);
- outb(inb(hwif->dma_master + 0x74) | 0x32, hwif->dma_master + 0x74);
+ outb(inb(base + 0x60) | 0x32, base + 0x60);
+ outb(inb(base + 0x64) | 0x32, base + 0x64);
/* Complete reset */
- outb(0x00, hwif->dma_master + 0x79);
+ outb(0x00, base + 0x69);
/* Reconnect channels to bus */
- outb(0x00, hwif->dma_master + 0x73);
- outb(0x00, hwif->dma_master + 0x77);
+ outb(0x00, base + 0x63);
+ outb(0x00, base + 0x67);
}
/**
Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================
--- a/drivers/ide/pci/pdc202xx_old.c
+++ b/drivers/ide/pci/pdc202xx_old.c
@@ -162,7 +162,7 @@ static u8 pdc202xx_old_cable_detect (ide
*/
static void pdc_old_enable_66MHz_clock(ide_hwif_t *hwif)
{
- unsigned long clock_reg = hwif->dma_master + 0x11;
+ unsigned long clock_reg = hwif->extra_base + 0x01;
u8 clock = inb(clock_reg);
outb(clock | (hwif->channel ? 0x08 : 0x02), clock_reg);
@@ -170,7 +170,7 @@ static void pdc_old_enable_66MHz_clock(i
static void pdc_old_disable_66MHz_clock(ide_hwif_t *hwif)
{
- unsigned long clock_reg = hwif->dma_master + 0x11;
+ unsigned long clock_reg = hwif->extra_base + 0x01;
u8 clock = inb(clock_reg);
outb(clock & ~(hwif->channel ? 0x08 : 0x02), clock_reg);
@@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
if (drive->media != ide_disk || drive->addressing == 1) {
struct request *rq = HWGROUP(drive)->rq;
ide_hwif_t *hwif = HWIF(drive);
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->extra_base - 16;
unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
u32 word_count = 0;
u8 clock = inb(high_16 + 0x11);
@@ -212,7 +212,7 @@ static int pdc202xx_old_ide_dma_end(ide_
{
if (drive->media != ide_disk || drive->addressing == 1) {
ide_hwif_t *hwif = HWIF(drive);
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->extra_base - 16;
unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
u8 clock = 0;
@@ -228,7 +228,7 @@ static int pdc202xx_old_ide_dma_end(ide_
static int pdc202xx_old_ide_dma_test_irq(ide_drive_t *drive)
{
ide_hwif_t *hwif = HWIF(drive);
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->extra_base - 16;
u8 dma_stat = inb(hwif->dma_status);
u8 sc1d = inb(high_16 + 0x001d);
@@ -271,7 +271,7 @@ static void pdc202xx_dma_timeout(ide_dri
static void pdc202xx_reset_host (ide_hwif_t *hwif)
{
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->extra_base - 16;
u8 udma_speed_flag = inb(high_16 | 0x001f);
outb(udma_speed_flag | 0x10, high_16 | 0x001f);
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -764,7 +764,6 @@ typedef struct hwif_s {
int rqsize; /* max sectors per request */
int irq; /* our irq number */
- unsigned long dma_master; /* reference base addr dmabase */
unsigned long dma_base; /* base addr for dma ports */
unsigned long dma_command; /* dma command register */
unsigned long dma_vendor1; /* dma vendor 1 register */
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 12/15] ide: remove ->dma_master field from ide_hwif_t
2007-10-08 21:03 ` Bartlomiej Zolnierkiewicz
@ 2007-10-18 17:08 ` Sergei Shtylyov
2007-10-24 21:24 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 12+ messages in thread
From: Sergei Shtylyov @ 2007-10-18 17:08 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz; +Cc: Jeff Garzik, linux-ide
Hello.
Bartlomiej Zolnierkiewicz wrote:
>>>>Index: b/drivers/ide/pci/hpt366.c
>>>>===================================================================
>>>>--- a/drivers/ide/pci/hpt366.c
>>>>+++ b/drivers/ide/pci/hpt366.c
>>>>@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
>>> This just asks to be:
>> No, it doesn't cause that way one would only break it.
>>>> static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
>>>> {
>>> unsigned long base = hwif->extra_base;
>> Sorry for a thinko. All offsets below shoud be really 0x6x, not 0x5x...
> Here we go again... :)
> [PATCH] ide: remove ->dma_master field from ide_hwif_t (take 4)
> * Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
> pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.
> * Remove no longer needed ->dma_master field from ide_hwif_t.
> v2:
> * Use the more readable 'hwif->dma_base - (hwif->channel * 8)' instead of
> pci_resource_start(hwif->pci_dev, 4).
> v3:
> * Use hwif->extra_base in hpt366/pdc20xx_old + some cosmetic fixups over v2
> (suggested by Sergei).
Huh? I don't see where you're using extra_base in hpt366.c
> v4:
> * Correct offsets in hpt3xxn_set_clock().
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Unfortunately, I have to NAK the patch... :-/
> Index: b/drivers/ide/pci/hpt366.c
> ===================================================================
> --- a/drivers/ide/pci/hpt366.c
> +++ b/drivers/ide/pci/hpt366.c
> @@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
>
> static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
> {
> - u8 scr2 = inb(hwif->dma_master + 0x7b);
> + unsigned long base = hwif->dma_base - (hwif->channel * 8);
Should be hwif->extra_base - 0x10 or else the following code will touch
all the wrong/reserved registers; one also can use hwif->extra_base ISO 'base'
and change all the offsets to 0x6x...
> + u8 scr2 = inb(base + 0x6b);
>
> if ((scr2 & 0x7f) == mode)
> return;
>
> /* Tristate the bus */
> - outb(0x80, hwif->dma_master + 0x73);
> - outb(0x80, hwif->dma_master + 0x77);
> + outb(0x80, base + 0x63);
> + outb(0x80, base + 0x67);
> /* Switch clock and reset channels */
> - outb(mode, hwif->dma_master + 0x7b);
> - outb(0xc0, hwif->dma_master + 0x79);
> + outb(mode, base + 0x6b);
> + outb(0xc0, base + 0x69);
>
> /*
> * Reset the state machines.
> * NOTE: avoid accidentally enabling the disabled channels.
> */
> - outb(inb(hwif->dma_master + 0x70) | 0x32, hwif->dma_master + 0x70);
> - outb(inb(hwif->dma_master + 0x74) | 0x32, hwif->dma_master + 0x74);
> + outb(inb(base + 0x60) | 0x32, base + 0x60);
> + outb(inb(base + 0x64) | 0x32, base + 0x64);
>
> /* Complete reset */
> - outb(0x00, hwif->dma_master + 0x79);
> + outb(0x00, base + 0x69);
>
> /* Reconnect channels to bus */
> - outb(0x00, hwif->dma_master + 0x73);
> - outb(0x00, hwif->dma_master + 0x77);
> + outb(0x00, base + 0x63);
> + outb(0x00, base + 0x67);
> }
Huh? That's wrong -- if you're setting base to the value of the former
value of hwof->dma_master, the register offsets shouldn't change!
> Index: b/drivers/ide/pci/pdc202xx_old.c
> ===================================================================
> --- a/drivers/ide/pci/pdc202xx_old.c
> +++ b/drivers/ide/pci/pdc202xx_old.c
> @@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
> if (drive->media != ide_disk || drive->addressing == 1) {
> struct request *rq = HWGROUP(drive)->rq;
> ide_hwif_t *hwif = HWIF(drive);
> - unsigned long high_16 = hwif->dma_master;
> + unsigned long high_16 = hwif->extra_base - 16;
> unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
That 'high_16' is *not* a good name in this case -- it would've been if we
didn't subtract 16. So, I'd wrote:
unsigned long atapi_reg = hwif->extra_base +
(hwif->channel ? 0x14 : 0x10);
> @@ -212,7 +212,7 @@ static int pdc202xx_old_ide_dma_end(ide_
> {
> if (drive->media != ide_disk || drive->addressing == 1) {
> ide_hwif_t *hwif = HWIF(drive);
> - unsigned long high_16 = hwif->dma_master;
> + unsigned long high_16 = hwif->extra_base - 16;
> unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
> u8 clock = 0;
Same here...
> @@ -228,7 +228,7 @@ static int pdc202xx_old_ide_dma_end(ide_
> static int pdc202xx_old_ide_dma_test_irq(ide_drive_t *drive)
> {
> ide_hwif_t *hwif = HWIF(drive);
> - unsigned long high_16 = hwif->dma_master;
> + unsigned long high_16 = hwif->extra_base - 16;
> u8 dma_stat = inb(hwif->dma_status);
> u8 sc1d = inb(high_16 + 0x001d);
... and here...
> @@ -271,7 +271,7 @@ static void pdc202xx_dma_timeout(ide_dri
>
> static void pdc202xx_reset_host (ide_hwif_t *hwif)
> {
> - unsigned long high_16 = hwif->dma_master;
> + unsigned long high_16 = hwif->extra_base - 16;
> u8 udma_speed_flag = inb(high_16 | 0x001f);
... and here too.
> outb(udma_speed_flag | 0x10, high_16 | 0x001f);
MBR, Sergei
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 12/15] ide: remove ->dma_master field from ide_hwif_t
2007-10-18 17:08 ` Sergei Shtylyov
@ 2007-10-24 21:24 ` Bartlomiej Zolnierkiewicz
2007-10-25 15:01 ` Sergei Shtylyov
0 siblings, 1 reply; 12+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2007-10-24 21:24 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: Jeff Garzik, linux-ide
Hi,
On Thursday 18 October 2007, Sergei Shtylyov wrote:
> Hello.
>
> Bartlomiej Zolnierkiewicz wrote:
>
> >>>>Index: b/drivers/ide/pci/hpt366.c
> >>>>===================================================================
> >>>>--- a/drivers/ide/pci/hpt366.c
> >>>>+++ b/drivers/ide/pci/hpt366.c
> >>>>@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
>
> >>> This just asks to be:
>
> >> No, it doesn't cause that way one would only break it.
>
> >>>> static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
> >>>> {
>
> >>> unsigned long base = hwif->extra_base;
>
> >> Sorry for a thinko. All offsets below shoud be really 0x6x, not 0x5x...
>
> > Here we go again... :)
>
> > [PATCH] ide: remove ->dma_master field from ide_hwif_t (take 4)
>
> > * Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
> > pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.
>
> > * Remove no longer needed ->dma_master field from ide_hwif_t.
>
> > v2:
> > * Use the more readable 'hwif->dma_base - (hwif->channel * 8)' instead of
> > pci_resource_start(hwif->pci_dev, 4).
>
> > v3:
> > * Use hwif->extra_base in hpt366/pdc20xx_old + some cosmetic fixups over v2
> > (suggested by Sergei).
>
> Huh? I don't see where you're using extra_base in hpt366.c
Doh, somebody hand me brown paper bag, please...
> > v4:
> > * Correct offsets in hpt3xxn_set_clock().
>
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>
> Unfortunately, I have to NAK the patch... :-/
>
> > Index: b/drivers/ide/pci/hpt366.c
> > ===================================================================
> > --- a/drivers/ide/pci/hpt366.c
> > +++ b/drivers/ide/pci/hpt366.c
> > @@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
> >
> > static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
> > {
> > - u8 scr2 = inb(hwif->dma_master + 0x7b);
> > + unsigned long base = hwif->dma_base - (hwif->channel * 8);
>
> Should be hwif->extra_base - 0x10 or else the following code will touch
> all the wrong/reserved registers; one also can use hwif->extra_base ISO 'base'
> and change all the offsets to 0x6x...
Well, I updated all offsets but I somehow forgot about updating 'base'. :)
> > + u8 scr2 = inb(base + 0x6b);
> >
> > if ((scr2 & 0x7f) == mode)
> > return;
> >
> > /* Tristate the bus */
> > - outb(0x80, hwif->dma_master + 0x73);
> > - outb(0x80, hwif->dma_master + 0x77);
> > + outb(0x80, base + 0x63);
> > + outb(0x80, base + 0x67);
>
> > /* Switch clock and reset channels */
> > - outb(mode, hwif->dma_master + 0x7b);
> > - outb(0xc0, hwif->dma_master + 0x79);
> > + outb(mode, base + 0x6b);
> > + outb(0xc0, base + 0x69);
> >
> > /*
> > * Reset the state machines.
> > * NOTE: avoid accidentally enabling the disabled channels.
> > */
> > - outb(inb(hwif->dma_master + 0x70) | 0x32, hwif->dma_master + 0x70);
> > - outb(inb(hwif->dma_master + 0x74) | 0x32, hwif->dma_master + 0x74);
> > + outb(inb(base + 0x60) | 0x32, base + 0x60);
> > + outb(inb(base + 0x64) | 0x32, base + 0x64);
> >
> > /* Complete reset */
> > - outb(0x00, hwif->dma_master + 0x79);
> > + outb(0x00, base + 0x69);
> >
> > /* Reconnect channels to bus */
> > - outb(0x00, hwif->dma_master + 0x73);
> > - outb(0x00, hwif->dma_master + 0x77);
> > + outb(0x00, base + 0x63);
> > + outb(0x00, base + 0x67);
> > }
>
> Huh? That's wrong -- if you're setting base to the value of the former
> value of hwof->dma_master, the register offsets shouldn't change!
>
> > Index: b/drivers/ide/pci/pdc202xx_old.c
> > ===================================================================
> > --- a/drivers/ide/pci/pdc202xx_old.c
> > +++ b/drivers/ide/pci/pdc202xx_old.c
> > @@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
> > if (drive->media != ide_disk || drive->addressing == 1) {
> > struct request *rq = HWGROUP(drive)->rq;
> > ide_hwif_t *hwif = HWIF(drive);
> > - unsigned long high_16 = hwif->dma_master;
> > + unsigned long high_16 = hwif->extra_base - 16;
> > unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
>
> That 'high_16' is *not* a good name in this case -- it would've been if we
> didn't subtract 16. So, I'd wrote:
>
> unsigned long atapi_reg = hwif->extra_base +
> (hwif->channel ? 0x14 : 0x10);
'high_16' is used for more than 'atapi_reg' (same is true for other places
defining 'high_16' local variable). With five revisions of this patch
needed to hopefully, finally get it right I prefer to defer futher cleanups
to the incremental patches (*hint*).
[PATCH] ide: remove ->dma_master field from ide_hwif_t (take 5)
* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.
* Remove no longer needed ->dma_master field from ide_hwif_t.
v2:
* Use the more readable 'hwif->dma_base - (hwif->channel * 8)' instead of
pci_resource_start(hwif->pci_dev, 4).
v3:
* Use hwif->extra_base in hpt366/pdc20xx_old + some cosmetic fixups over v2
(suggested by Sergei).
v4:
* Correct offsets in hpt3xxn_set_clock().
v5:
* Use hwif->extra_base in hpt366 for _real_ this time. (Noticed by Sergei)
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
Cc: Jeff Garzik <jeff@garzik.org>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
drivers/ide/ide-dma.c | 7 -------
drivers/ide/ide.c | 2 --
drivers/ide/pci/cmd64x.c | 8 +++++---
drivers/ide/pci/hpt366.c | 21 +++++++++++----------
drivers/ide/pci/pdc202xx_old.c | 12 ++++++------
include/linux/ide.h | 1 -
6 files changed, 22 insertions(+), 29 deletions(-)
Index: b/drivers/ide/ide-dma.c
===================================================================
--- a/drivers/ide/ide-dma.c
+++ b/drivers/ide/ide-dma.c
@@ -1002,11 +1002,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
hwif->dma_base = base;
- if (hwif->mate)
- hwif->dma_master = hwif->channel ? hwif->mate->dma_base : base;
- else
- hwif->dma_master = base;
-
if (!(hwif->dma_command))
hwif->dma_command = hwif->dma_base;
if (!(hwif->dma_vendor1))
@@ -1048,8 +1043,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
hwif->drives[1].name, (dma_stat & 0x40) ? "DMA" : "pio");
}
printk("\n");
-
- BUG_ON(!hwif->dma_master);
}
EXPORT_SYMBOL_GPL(ide_setup_dma);
Index: b/drivers/ide/ide.c
===================================================================
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -468,7 +468,6 @@ static void ide_hwif_restore(ide_hwif_t
#endif
hwif->dma_base = tmp_hwif->dma_base;
- hwif->dma_master = tmp_hwif->dma_master;
hwif->dma_command = tmp_hwif->dma_command;
hwif->dma_vendor1 = tmp_hwif->dma_vendor1;
hwif->dma_status = tmp_hwif->dma_status;
@@ -602,7 +601,6 @@ void ide_unregister(unsigned int index)
(void) ide_release_dma(hwif);
hwif->dma_base = 0;
- hwif->dma_master = 0;
hwif->dma_command = 0;
hwif->dma_vendor1 = 0;
hwif->dma_status = 0;
Index: b/drivers/ide/pci/cmd64x.c
===================================================================
--- a/drivers/ide/pci/cmd64x.c
+++ b/drivers/ide/pci/cmd64x.c
@@ -333,13 +333,14 @@ static void cmd64x_set_dma_mode(ide_driv
static int cmd648_ide_dma_end (ide_drive_t *drive)
{
ide_hwif_t *hwif = HWIF(drive);
+ unsigned long base = hwif->dma_base - (hwif->channel * 8);
int err = __ide_dma_end(drive);
u8 irq_mask = hwif->channel ? MRDMODE_INTR_CH1 :
MRDMODE_INTR_CH0;
- u8 mrdmode = inb(hwif->dma_master + 0x01);
+ u8 mrdmode = inb(base + 1);
/* clear the interrupt bit */
- outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+ outb(mrdmode | irq_mask, base + 1);
return err;
}
@@ -364,10 +365,11 @@ static int cmd64x_ide_dma_end (ide_drive
static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
{
ide_hwif_t *hwif = HWIF(drive);
+ unsigned long base = hwif->dma_base - (hwif->channel * 8);
u8 irq_mask = hwif->channel ? MRDMODE_INTR_CH1 :
MRDMODE_INTR_CH0;
u8 dma_stat = inb(hwif->dma_status);
- u8 mrdmode = inb(hwif->dma_master + 0x01);
+ u8 mrdmode = inb(base + 1);
#ifdef DEBUG
printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",
Index: b/drivers/ide/pci/hpt366.c
===================================================================
--- a/drivers/ide/pci/hpt366.c
+++ b/drivers/ide/pci/hpt366.c
@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
{
- u8 scr2 = inb(hwif->dma_master + 0x7b);
+ unsigned long base = hwif->extra_base;
+ u8 scr2 = inb(base + 0x6b);
if ((scr2 & 0x7f) == mode)
return;
/* Tristate the bus */
- outb(0x80, hwif->dma_master + 0x73);
- outb(0x80, hwif->dma_master + 0x77);
+ outb(0x80, base + 0x63);
+ outb(0x80, base + 0x67);
/* Switch clock and reset channels */
- outb(mode, hwif->dma_master + 0x7b);
- outb(0xc0, hwif->dma_master + 0x79);
+ outb(mode, base + 0x6b);
+ outb(0xc0, base + 0x69);
/*
* Reset the state machines.
* NOTE: avoid accidentally enabling the disabled channels.
*/
- outb(inb(hwif->dma_master + 0x70) | 0x32, hwif->dma_master + 0x70);
- outb(inb(hwif->dma_master + 0x74) | 0x32, hwif->dma_master + 0x74);
+ outb(inb(base + 0x60) | 0x32, base + 0x60);
+ outb(inb(base + 0x64) | 0x32, base + 0x64);
/* Complete reset */
- outb(0x00, hwif->dma_master + 0x79);
+ outb(0x00, base + 0x69);
/* Reconnect channels to bus */
- outb(0x00, hwif->dma_master + 0x73);
- outb(0x00, hwif->dma_master + 0x77);
+ outb(0x00, base + 0x63);
+ outb(0x00, base + 0x67);
}
/**
Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================
--- a/drivers/ide/pci/pdc202xx_old.c
+++ b/drivers/ide/pci/pdc202xx_old.c
@@ -162,7 +162,7 @@ static u8 pdc202xx_old_cable_detect (ide
*/
static void pdc_old_enable_66MHz_clock(ide_hwif_t *hwif)
{
- unsigned long clock_reg = hwif->dma_master + 0x11;
+ unsigned long clock_reg = hwif->extra_base + 0x01;
u8 clock = inb(clock_reg);
outb(clock | (hwif->channel ? 0x08 : 0x02), clock_reg);
@@ -170,7 +170,7 @@ static void pdc_old_enable_66MHz_clock(i
static void pdc_old_disable_66MHz_clock(ide_hwif_t *hwif)
{
- unsigned long clock_reg = hwif->dma_master + 0x11;
+ unsigned long clock_reg = hwif->extra_base + 0x01;
u8 clock = inb(clock_reg);
outb(clock & ~(hwif->channel ? 0x08 : 0x02), clock_reg);
@@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
if (drive->media != ide_disk || drive->addressing == 1) {
struct request *rq = HWGROUP(drive)->rq;
ide_hwif_t *hwif = HWIF(drive);
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->extra_base - 16;
unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
u32 word_count = 0;
u8 clock = inb(high_16 + 0x11);
@@ -212,7 +212,7 @@ static int pdc202xx_old_ide_dma_end(ide_
{
if (drive->media != ide_disk || drive->addressing == 1) {
ide_hwif_t *hwif = HWIF(drive);
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->extra_base - 16;
unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
u8 clock = 0;
@@ -228,7 +228,7 @@ static int pdc202xx_old_ide_dma_end(ide_
static int pdc202xx_old_ide_dma_test_irq(ide_drive_t *drive)
{
ide_hwif_t *hwif = HWIF(drive);
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->extra_base - 16;
u8 dma_stat = inb(hwif->dma_status);
u8 sc1d = inb(high_16 + 0x001d);
@@ -271,7 +271,7 @@ static void pdc202xx_dma_timeout(ide_dri
static void pdc202xx_reset_host (ide_hwif_t *hwif)
{
- unsigned long high_16 = hwif->dma_master;
+ unsigned long high_16 = hwif->extra_base - 16;
u8 udma_speed_flag = inb(high_16 | 0x001f);
outb(udma_speed_flag | 0x10, high_16 | 0x001f);
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -766,7 +766,6 @@ typedef struct hwif_s {
int rqsize; /* max sectors per request */
int irq; /* our irq number */
- unsigned long dma_master; /* reference base addr dmabase */
unsigned long dma_base; /* base addr for dma ports */
unsigned long dma_command; /* dma command register */
unsigned long dma_vendor1; /* dma vendor 1 register */
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 12/15] ide: remove ->dma_master field from ide_hwif_t
2007-10-24 21:24 ` Bartlomiej Zolnierkiewicz
@ 2007-10-25 15:01 ` Sergei Shtylyov
0 siblings, 0 replies; 12+ messages in thread
From: Sergei Shtylyov @ 2007-10-25 15:01 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz; +Cc: Jeff Garzik, linux-ide
Hello.
Bartlomiej Zolnierkiewicz wrote:
>>>Index: b/drivers/ide/pci/pdc202xx_old.c
>>>===================================================================
>>>--- a/drivers/ide/pci/pdc202xx_old.c
>>>+++ b/drivers/ide/pci/pdc202xx_old.c
>>>@@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
>>> if (drive->media != ide_disk || drive->addressing == 1) {
>>> struct request *rq = HWGROUP(drive)->rq;
>>> ide_hwif_t *hwif = HWIF(drive);
>>>- unsigned long high_16 = hwif->dma_master;
>>>+ unsigned long high_16 = hwif->extra_base - 16;
>>> unsigned long atapi_reg = high_16 + (hwif->channel ? 0x24 : 0x20);
>> That 'high_16' is *not* a good name in this case --
It's not a good name in any case, because there's more than 16 extra
registers.
>> it would've been if we
>>didn't subtract 16. So, I'd wrote:
>> unsigned long atapi_reg = hwif->extra_base +
>> (hwif->channel ? 0x14 : 0x10);
> 'high_16' is used for more than 'atapi_reg' (same is true for other places
> defining 'high_16' local variable). With five revisions of this patch
> needed to hopefully, finally get it right I prefer to defer futher cleanups
> to the incremental patches (*hint*).
Hint gotten. I'm still neck deep in other stuff but the IDE cleanups can
wait anyway.
> [PATCH] ide: remove ->dma_master field from ide_hwif_t (take 5)
> * Convert cmd64x, hpt366 and pdc202xx_old host drivers to use
> pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.
> * Remove no longer needed ->dma_master field from ide_hwif_t.
> v2:
> * Use the more readable 'hwif->dma_base - (hwif->channel * 8)' instead of
> pci_resource_start(hwif->pci_dev, 4).
> v3:
> * Use hwif->extra_base in hpt366/pdc20xx_old + some cosmetic fixups over v2
> (suggested by Sergei).
> v4:
> * Correct offsets in hpt3xxn_set_clock().
> v5:
> * Use hwif->extra_base in hpt366 for _real_ this time. (Noticed by Sergei)
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
MBR, Sergei
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2007-10-25 15:01 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-10-01 21:39 [PATCH 12/15] ide: remove ->dma_master field from ide_hwif_t Bartlomiej Zolnierkiewicz
2007-10-01 21:48 ` Jeff Garzik
2007-10-01 22:06 ` Bartlomiej Zolnierkiewicz
2007-10-01 22:31 ` Jeff Garzik
2007-10-02 21:24 ` Bartlomiej Zolnierkiewicz
2007-10-03 12:50 ` Sergei Shtylyov
2007-10-03 22:27 ` Bartlomiej Zolnierkiewicz
2007-10-04 13:27 ` Sergei Shtylyov
2007-10-08 21:03 ` Bartlomiej Zolnierkiewicz
2007-10-18 17:08 ` Sergei Shtylyov
2007-10-24 21:24 ` Bartlomiej Zolnierkiewicz
2007-10-25 15:01 ` Sergei Shtylyov
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).