Re: [PATCH 7/8] ide: add struct ide_port_info instances to legacy host drivers
On Sat, Feb 02, 2008 at 12:35:30AM +0100, Bartlomiej Zolnierkiewicz wrote: > > > Index: b/drivers/ide/arm/icside.c > > > === > > > --- a/drivers/ide/arm/icside.c > > > +++ b/drivers/ide/arm/icside.c > > > @@ -459,11 +456,19 @@ icside_register_v5(struct icside_state * > > > > > > idx[0] = hwif->index; > > > > > > - ide_device_add(idx); > > > + ide_device_add(idx, NULL); > > > > > > return 0; > > > } > > > > > > +static const struct ide_port_info icside_v6_port_info __initdata = { > > > + .host_flags = IDE_HFLAG_SERIALIZE | > > > + IDE_HFLAG_NO_DMA | /* no SFF-style DMA */ > > > + IDE_HFLAG_NO_AUTOTUNE, > > > + .mwdma_mask = ATA_MWDMA2, > > > + .swdma_mask = ATA_SWDMA2, > > > +}; > > > + > > > > Interesting... this driver's support for SWDMA0 is broken since the > > cycle > > should be 960 ns long, not 480, and SWDMA2 is underclocked using the same > > cycle as SWDMA1, 480 ns... > > Added Russell to Cc:. Underclocking the DMA is not a problem - if you look at the timing diagrams and associated tables in the ATA specifications, you'll find that they specify the *minimum* cycle timings. However, you're correct that SWDMA0 is not able to be supported. -- Russell King Linux kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/8] ide: add struct ide_port_info instances to legacy host drivers
On Mon, 11 Feb 2008 00:16:07 +0100, Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> wrote: > > It is too late for -git but not too late for 2.6.25-rc1. ;-) > > > > Could you make a patch please? > > I went ahead and made a patch (sorry, I needed it for git pull request :). > > From: Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> > Subject: [PATCH] ide: ide_init_port() bugfix Thanks! Looks sane for me :) --- Atsushi Nemoto -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/8] ide: add struct ide_port_info instances to legacy host drivers
On Sunday 10 February 2008, Bartlomiej Zolnierkiewicz wrote: > On Sunday 10 February 2008, Atsushi Nemoto wrote: > > On Sun, 06 Jan 2008 18:03:10 +0100, Bartlomiej Zolnierkiewicz <[EMAIL > > PROTECTED]> wrote: > > > + /* reset DMA masks only for SFF-style DMA controllers */ > > > + if ((d->host_flags && IDE_HFLAG_NO_DMA) == 0 && hwif->dma_base == 0) > > > + hwif->swdma_mask = hwif->mwdma_mask = hwif->ultra_mask = 0; > > > > It might be too late, but "host_flags && IDE_HFLAGS_NO_DMA" seems > > wrong for me. > > It is too late for -git but not too late for 2.6.25-rc1. ;-) > > Could you make a patch please? I went ahead and made a patch (sorry, I needed it for git pull request :). From: Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> Subject: [PATCH] ide: ide_init_port() bugfix On Sunday 10 February 2008, Atsushi Nemoto wrote: > On Sun, 06 Jan 2008 18:03:10 +0100, Bartlomiej Zolnierkiewicz <[EMAIL > PROTECTED]> wrote: > > + /* reset DMA masks only for SFF-style DMA controllers */ > > + if ((d->host_flags && IDE_HFLAG_NO_DMA) == 0 && hwif->dma_base == 0) > > + hwif->swdma_mask = hwif->mwdma_mask = hwif->ultra_mask = 0; > > It might be too late, but "host_flags && IDE_HFLAGS_NO_DMA" seems > wrong for me. Fix regression caused by commmit c413b9b94d9a8e7548cc4b2e04b7df0439ce76fd ("ide: add struct ide_port_info instances to legacy host drivers"). Reported-by: Atsushi Nemoto <[EMAIL PROTECTED]> Signed-off-by: Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> --- drivers/ide/ide-probe.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: b/drivers/ide/ide-probe.c === --- a/drivers/ide/ide-probe.c +++ b/drivers/ide/ide-probe.c @@ -1355,7 +1355,7 @@ static void ide_init_port(ide_hwif_t *hw hwif->ultra_mask = d->udma_mask; /* reset DMA masks only for SFF-style DMA controllers */ - if ((d->host_flags && IDE_HFLAG_NO_DMA) == 0 && hwif->dma_base == 0) + if ((d->host_flags & IDE_HFLAG_NO_DMA) == 0 && hwif->dma_base == 0) hwif->swdma_mask = hwif->mwdma_mask = hwif->ultra_mask = 0; if (d->host_flags & IDE_HFLAG_RQSIZE_256) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/8] ide: add struct ide_port_info instances to legacy host drivers
On Sunday 10 February 2008, Atsushi Nemoto wrote: > On Sun, 06 Jan 2008 18:03:10 +0100, Bartlomiej Zolnierkiewicz <[EMAIL > PROTECTED]> wrote: > > + /* reset DMA masks only for SFF-style DMA controllers */ > > + if ((d->host_flags && IDE_HFLAG_NO_DMA) == 0 && hwif->dma_base == 0) > > + hwif->swdma_mask = hwif->mwdma_mask = hwif->ultra_mask = 0; > > It might be too late, but "host_flags && IDE_HFLAGS_NO_DMA" seems > wrong for me. It is too late for -git but not too late for 2.6.25-rc1. ;-) Could you make a patch please? Thanks, Bart -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/8] ide: add struct ide_port_info instances to legacy host drivers
On Sun, 06 Jan 2008 18:03:10 +0100, Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> wrote: > + /* reset DMA masks only for SFF-style DMA controllers */ > + if ((d->host_flags && IDE_HFLAG_NO_DMA) == 0 && hwif->dma_base == 0) > + hwif->swdma_mask = hwif->mwdma_mask = hwif->ultra_mask = 0; It might be too late, but "host_flags && IDE_HFLAGS_NO_DMA" seems wrong for me. --- Atsushi Nemoto -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/8] ide: add struct ide_port_info instances to legacy host drivers
Hi, On Monday 28 January 2008, Sergei Shtylyov wrote: > Hello. > > Bartlomiej Zolnierkiewicz wrote: > > > * Remove 'struct pci_dev *dev' argument from ide_hwif_setup_dma(). > > > * Un-static ide_hwif_setup_dma() and add CONFIG_BLK_DEV_IDEDMA_PCI=n > > version. > > > * Add 'const struct ide_port_info *d' argument to ide_device_add[_all](). > > > * Factor out generic ports init from ide_pci_setup_ports() to > > ide_init_port(), > > move it to ide-probe.c and call it in in ide_device_add_all() instead of > > ide_pci_setup_ports(). > > > * Move ->mate setup to ide_device_add_all() from ide_port_init(). > > > * Add IDE_HFLAG_NO_AUTOTUNE host flag for host drivers that don't enable > > ->autotune currently. > > > * Setup hwif->chipset in ide_init_port() but iff pi->chipset is set > > (to not override setup done by ide_hwif_configure()). > > > * Add ETRAX host handling to ide_device_add_all(). > > > * cmd640.c: set IDE_HFLAG_ABUSE_* also for CONFIG_BLK_DEV_CMD640_ENHANCED=n. > > > * pmac.c: make pmac_ide_setup_dma() return an error value and move DMA masks > > setup to pmac_ide_setup_device(). > > > * Add 'struct ide_port_info' instances to legacy host drivers, pass them to > > ide_device_add() calls and then remove open-coded ports initialization. > > > Signed-off-by: Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> > > > Index: b/drivers/ide/arm/icside.c > > === > > --- a/drivers/ide/arm/icside.c > > +++ b/drivers/ide/arm/icside.c > > @@ -459,11 +456,19 @@ icside_register_v5(struct icside_state * > > > > idx[0] = hwif->index; > > > > - ide_device_add(idx); > > + ide_device_add(idx, NULL); > > > > return 0; > > } > > > > +static const struct ide_port_info icside_v6_port_info __initdata = { > > + .host_flags = IDE_HFLAG_SERIALIZE | > > + IDE_HFLAG_NO_DMA | /* no SFF-style DMA */ > > + IDE_HFLAG_NO_AUTOTUNE, > > + .mwdma_mask = ATA_MWDMA2, > > + .swdma_mask = ATA_SWDMA2, > > +}; > > + > > Interesting... this driver's support for SWDMA0 is broken since the cycle > should be 960 ns long, not 480, and SWDMA2 is underclocked using the same > cycle as SWDMA1, 480 ns... Added Russell to Cc:. > > Index: b/drivers/ide/cris/ide-cris.c > > === > > --- a/drivers/ide/cris/ide-cris.c > > +++ b/drivers/ide/cris/ide-cris.c > > @@ -753,6 +753,15 @@ static void cris_set_dma_mode(ide_drive_ > > cris_ide_set_speed(TYPE_DMA, 0, strobe, hold); > > } > > > > +static const struct ide_port_info cris_port_info __initdata = { > > + .chipset= ide_etrax100, > > + .host_flags = IDE_HFLAG_NO_ATAPI_DMA | > > + IDE_HFLAG_NO_DMA, /* no SFF-style DMA */ > > + .pio_mask = ATA_PIO4, > > + .udma_mask = cris_ultra_mask, > > Hm, I wonder which value it will assume, 0x07 or 0? Not sure even after > looking at the source... :-) This is tricky since it depends on the CRIS version (0x07 on CRIS V32 and 0x00 on CRIS V10). > > + .mwdma_mask = ATA_MWDMA2, > > +}; > > + > > static int __init init_e100_ide(void) > > { > > hw_regs_t hw; > > Index: b/drivers/ide/ide-probe.c > > === > > --- a/drivers/ide/ide-probe.c > > +++ b/drivers/ide/ide-probe.c > > @@ -1289,12 +1289,86 @@ static void hwif_register_devices(ide_hw > > } > > } > > > > -int ide_device_add_all(u8 *idx) > > +static void ide_init_port(ide_hwif_t *hwif, unsigned int port, > > + const struct ide_port_info *d) > > { > > - ide_hwif_t *hwif; > > + if (d->chipset != ide_etrax100) > > + hwif->channel = port; > >Hm, what's so special about ide_etrax100? The driver supports *4* channels and it didn't set ->channel at all. Since in the meantime ide-cris got marked as BROKEN I guess that a patch removing above code would be OK. > > +int ide_device_add_all(u8 idx[MAX_HWIFS], const struct ide_port_info *d) > > Function prototype doesn't match with one from which has > the > first argument as a pointer... I fixed it locally, thanks. > > +{ > > + ide_hwif_t *hwif, *mate = NULL; > > int i, rc = 0; > > > > for (i = 0; i < MAX_HWIFS; i++) { > > + if (d == NULL || idx[i] == 0xff) { > > Why check for (d == NULL) every time and not do it once and break from > the > loop or even do it before the loop? To save on some code lines and resulting code size (ide_device_add_all is not performance critical). Bart -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/8] ide: add struct ide_port_info instances to legacy host drivers
Hello. Bartlomiej Zolnierkiewicz wrote: * Remove 'struct pci_dev *dev' argument from ide_hwif_setup_dma(). * Un-static ide_hwif_setup_dma() and add CONFIG_BLK_DEV_IDEDMA_PCI=n version. * Add 'const struct ide_port_info *d' argument to ide_device_add[_all](). * Factor out generic ports init from ide_pci_setup_ports() to ide_init_port(), move it to ide-probe.c and call it in in ide_device_add_all() instead of ide_pci_setup_ports(). * Move ->mate setup to ide_device_add_all() from ide_port_init(). * Add IDE_HFLAG_NO_AUTOTUNE host flag for host drivers that don't enable ->autotune currently. * Setup hwif->chipset in ide_init_port() but iff pi->chipset is set (to not override setup done by ide_hwif_configure()). * Add ETRAX host handling to ide_device_add_all(). * cmd640.c: set IDE_HFLAG_ABUSE_* also for CONFIG_BLK_DEV_CMD640_ENHANCED=n. * pmac.c: make pmac_ide_setup_dma() return an error value and move DMA masks setup to pmac_ide_setup_device(). * Add 'struct ide_port_info' instances to legacy host drivers, pass them to ide_device_add() calls and then remove open-coded ports initialization. Signed-off-by: Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> Index: b/drivers/ide/arm/icside.c === --- a/drivers/ide/arm/icside.c +++ b/drivers/ide/arm/icside.c @@ -459,11 +456,19 @@ icside_register_v5(struct icside_state * idx[0] = hwif->index; - ide_device_add(idx); + ide_device_add(idx, NULL); return 0; } +static const struct ide_port_info icside_v6_port_info __initdata = { + .host_flags = IDE_HFLAG_SERIALIZE | + IDE_HFLAG_NO_DMA | /* no SFF-style DMA */ + IDE_HFLAG_NO_AUTOTUNE, + .mwdma_mask = ATA_MWDMA2, + .swdma_mask = ATA_SWDMA2, +}; + Interesting... this driver's support for SWDMA0 is broken since the cycle should be 960 ns long, not 480, and SWDMA2 is underclocked using the same cycle as SWDMA1, 480 ns... Index: b/drivers/ide/cris/ide-cris.c === --- a/drivers/ide/cris/ide-cris.c +++ b/drivers/ide/cris/ide-cris.c @@ -753,6 +753,15 @@ static void cris_set_dma_mode(ide_drive_ cris_ide_set_speed(TYPE_DMA, 0, strobe, hold); } +static const struct ide_port_info cris_port_info __initdata = { + .chipset= ide_etrax100, + .host_flags = IDE_HFLAG_NO_ATAPI_DMA | + IDE_HFLAG_NO_DMA, /* no SFF-style DMA */ + .pio_mask = ATA_PIO4, + .udma_mask = cris_ultra_mask, Hm, I wonder which value it will assume, 0x07 or 0? Not sure even after looking at the source... :-) + .mwdma_mask = ATA_MWDMA2, +}; + static int __init init_e100_ide(void) { hw_regs_t hw; Index: b/drivers/ide/ide-probe.c === --- a/drivers/ide/ide-probe.c +++ b/drivers/ide/ide-probe.c @@ -1289,12 +1289,86 @@ static void hwif_register_devices(ide_hw } } -int ide_device_add_all(u8 *idx) +static void ide_init_port(ide_hwif_t *hwif, unsigned int port, + const struct ide_port_info *d) { - ide_hwif_t *hwif; + if (d->chipset != ide_etrax100) + hwif->channel = port; Hm, what's so special about ide_etrax100? +int ide_device_add_all(u8 idx[MAX_HWIFS], const struct ide_port_info *d) Function prototype doesn't match with one from which has the first argument as a pointer... +{ + ide_hwif_t *hwif, *mate = NULL; int i, rc = 0; for (i = 0; i < MAX_HWIFS; i++) { + if (d == NULL || idx[i] == 0xff) { Why check for (d == NULL) every time and not do it once and break from the loop or even do it before the loop? + mate = NULL; + continue; + } + + hwif = &ide_hwifs[idx[i]]; + + if (d->chipset != ide_etrax100 && (i & 1) && mate) { + hwif->mate = mate; + mate->mate = hwif; + } + + mate = (i & 1) ? NULL : hwif; + + ide_init_port(hwif, i & 1, d); + } + + for (i = 0; i < MAX_HWIFS; i++) { if (idx[i] == 0xff) continue; MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 7/8] ide: add struct ide_port_info instances to legacy host drivers
* Remove 'struct pci_dev *dev' argument from ide_hwif_setup_dma(). * Un-static ide_hwif_setup_dma() and add CONFIG_BLK_DEV_IDEDMA_PCI=n version. * Add 'const struct ide_port_info *d' argument to ide_device_add[_all](). * Factor out generic ports init from ide_pci_setup_ports() to ide_init_port(), move it to ide-probe.c and call it in in ide_device_add_all() instead of ide_pci_setup_ports(). * Move ->mate setup to ide_device_add_all() from ide_port_init(). * Add IDE_HFLAG_NO_AUTOTUNE host flag for host drivers that don't enable ->autotune currently. * Setup hwif->chipset in ide_init_port() but iff pi->chipset is set (to not override setup done by ide_hwif_configure()). * Add ETRAX host handling to ide_device_add_all(). * cmd640.c: set IDE_HFLAG_ABUSE_* also for CONFIG_BLK_DEV_CMD640_ENHANCED=n. * pmac.c: make pmac_ide_setup_dma() return an error value and move DMA masks setup to pmac_ide_setup_device(). * Add 'struct ide_port_info' instances to legacy host drivers, pass them to ide_device_add() calls and then remove open-coded ports initialization. Signed-off-by: Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> --- drivers/ide/arm/icside.c | 25 +-- drivers/ide/arm/ide_arm.c |2 drivers/ide/arm/rapide.c |2 drivers/ide/cris/ide-cris.c | 18 drivers/ide/h8300/ide-h8300.c |2 drivers/ide/ide-generic.c |2 drivers/ide/ide-pnp.c |2 drivers/ide/ide-probe.c | 82 +++-- drivers/ide/ide.c |2 drivers/ide/legacy/ali14xx.c | 24 -- drivers/ide/legacy/buddha.c |2 drivers/ide/legacy/dtc2278.c | 24 -- drivers/ide/legacy/falconide.c|2 drivers/ide/legacy/gayle.c|2 drivers/ide/legacy/ht6560b.c | 22 - drivers/ide/legacy/ide_platform.c |2 drivers/ide/legacy/macide.c |2 drivers/ide/legacy/q40ide.c |2 drivers/ide/legacy/qd65xx.c | 23 +- drivers/ide/legacy/umc8672.c | 24 -- drivers/ide/mips/au1xxx-ide.c | 31 -- drivers/ide/mips/swarm.c |2 drivers/ide/pci/cmd640.c | 31 ++ drivers/ide/pci/cs5520.c |2 drivers/ide/pci/sgiioc4.c | 18 +--- drivers/ide/ppc/mpc8xx.c |2 drivers/ide/ppc/pmac.c| 84 +++--- drivers/ide/setup-pci.c | 81 +++- include/linux/ide.h | 13 - 29 files changed, 259 insertions(+), 271 deletions(-) Index: b/drivers/ide/arm/icside.c === --- a/drivers/ide/arm/icside.c +++ b/drivers/ide/arm/icside.c @@ -377,9 +377,6 @@ static void icside_dma_lost_irq(ide_driv static void icside_dma_init(ide_hwif_t *hwif) { - hwif->mwdma_mask= 7; /* MW0..2 */ - hwif->swdma_mask= 7; /* SW0..2 */ - hwif->dmatable_cpu = NULL; hwif->dmatable_dma = 0; hwif->set_dma_mode = icside_set_dma_mode; @@ -459,11 +456,19 @@ icside_register_v5(struct icside_state * idx[0] = hwif->index; - ide_device_add(idx); + ide_device_add(idx, NULL); return 0; } +static const struct ide_port_info icside_v6_port_info __initdata = { + .host_flags = IDE_HFLAG_SERIALIZE | + IDE_HFLAG_NO_DMA | /* no SFF-style DMA */ + IDE_HFLAG_NO_AUTOTUNE, + .mwdma_mask = ATA_MWDMA2, + .swdma_mask = ATA_SWDMA2, +}; + static int __init icside_register_v6(struct icside_state *state, struct expansion_card *ec) { @@ -472,6 +477,7 @@ icside_register_v6(struct icside_state * unsigned int sel = 0; int ret; u8 idx[4] = { 0xff, 0xff, 0xff, 0xff }; + struct ide_port_info d = icside_v6_port_info; ioc_base = ecardm_iomap(ec, ECARD_RES_IOCFAST, 0, 0); if (!ioc_base) { @@ -521,30 +527,25 @@ icside_register_v6(struct icside_state * state->hwif[1]= mate; hwif->maskproc= icside_maskproc; - hwif->channel = 0; hwif->hwif_data = state; - hwif->mate= mate; - hwif->serialized = 1; hwif->config_data = (unsigned long)ioc_base; hwif->select_data = sel; mate->maskproc= icside_maskproc; - mate->channel = 1; mate->hwif_data = state; - mate->mate= hwif; - mate->serialized = 1; mate->config_data = (unsigned long)ioc_base; mate->select_data = sel | 1; if (ec->dma != NO_DMA && !request_dma(ec->dma, hwif->name)) { icside_dma_init(hwif); icside_dma_init(mate); - } + } else + d.mwdma_mask = d.swdma_mask = 0; idx[0] = hw