| Message ID | 1449183781-2163-1-git-send-email-linux@rainbow-software.org (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
Hi Finn, Ondrej, One small question: On Fri, Dec 4, 2015 at 10:03 AM, Ondrej Zary <linux@rainbow-software.org> wrote: > Add I/O register mapping for DTC chips and enable PDMA mode. > > These chips have 16-bit wide HOST BUFFER register (counter register at > offset 0x0d increments by 2 on each HOST BUFFER read). > > Large PIO transfers crash at least the DTCT-436P chip (all reads result > in 0xFF) so this patch actually makes it work. > > The chip also crashes when we bang the C400 host status register too > heavily after PDMA write - a small udelay is needed. > > Signed-off-by: Ondrej Zary <linux@rainbow-software.org> > --- > # hdparm -t --direct /dev/sdb > > /dev/sdb: > Timing O_DIRECT disk reads: 4 MB in 3.78 seconds = 1.06 MB/sec > > > drivers/scsi/NCR5380.h | 1 + > drivers/scsi/g_NCR5380.c | 47 +++++++++++++++++++++++----------------------- > 2 files changed, 25 insertions(+), 23 deletions(-) > > diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c > index fae4332..04f6c29 100644 > --- a/drivers/scsi/g_NCR5380.c > +++ b/drivers/scsi/g_NCR5380.c > @@ -415,7 +415,8 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) > hostdata->c400_blk_cnt = 1; > hostdata->c400_host_buf = 4; > } > - if (overrides[current_override].board == BOARD_NCR53C400A) { > + if (overrides[current_override].board == BOARD_NCR53C400A || > + overrides[current_override].board == BOARD_DTC3181E) { These if statements are starting to get a bit long, would it make sense to replace them with a flag or equivalent? Thanks,
On Fri, 4 Dec 2015, Julian Calaby wrote: > > - if (overrides[current_override].board == BOARD_NCR53C400A) { > > + if (overrides[current_override].board == BOARD_NCR53C400A || > > + overrides[current_override].board == BOARD_DTC3181E) { > > These if statements are starting to get a bit long, would it make > sense to replace them with a flag or equivalent? To what end? Shorter lines? As in, if (board_is_ncr53c400a || board_is_dtc3181e) { /* ... */ } I suppose that could be an improvement if new flags would entirely replace the override.board struct member and the existing switch statement, switch (overrides[current_override].board) { /* ... */ } Or maybe you meant testing a new flag something like this, if (hostdata->ncr53c400_compatible) { /* ... */ } If your concern is the Don't Repeat Yourself rule, I'm not sure that new flag would get tested more than once (?) And it would still have to be assigned using an "objectionably" long expression, e.g. hostdata->ncr53c400_compatible = overrides[current_override].board == BOARD_NCR53C400 || overrides[current_override].board == BOARD_NCR53C400A || overrides[current_override].board == BOARD_DTC3181E; Rather than add new flags, perhaps a 'switch' statement instead of an 'if' statement would be shorter (if the size of the expression is the problem).
On Fri, 4 Dec 2015, Ondrej Zary wrote: > Add I/O register mapping for DTC chips and enable PDMA mode. > > These chips have 16-bit wide HOST BUFFER register (counter register at > offset 0x0d increments by 2 on each HOST BUFFER read). > > Large PIO transfers crash at least the DTCT-436P chip (all reads result > in 0xFF) so this patch actually makes it work. > > The chip also crashes when we bang the C400 host status register too > heavily after PDMA write - a small udelay is needed. > > Signed-off-by: Ondrej Zary <linux@rainbow-software.org> > --- > # hdparm -t --direct /dev/sdb > > /dev/sdb: > Timing O_DIRECT disk reads: 4 MB in 3.78 seconds = 1.06 MB/sec > > > drivers/scsi/NCR5380.h | 1 + > drivers/scsi/g_NCR5380.c | 47 +++++++++++++++++++++++----------------------- > 2 files changed, 25 insertions(+), 23 deletions(-) > > diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h > index 5092580..e3b8149 100644 > --- a/drivers/scsi/NCR5380.h > +++ b/drivers/scsi/NCR5380.h > @@ -222,6 +222,7 @@ > > #define FLAG_NO_DMA_FIXUP 1 /* No DMA errata workarounds */ > #define FLAG_NO_PSEUDO_DMA 8 /* Inhibit DMA */ > +#define FLAG_16BIT 16 /* 16-bit PDMA */ Can we give this a better name? FLAG_16BIT could be taken to mean "16-bit ISA card" but do we really want a flag for that? How about FLAG_16BIT_BUF_REG or FLAG_WORD_IO_BUF? All active flags appear in the console log, thanks to prepare_info(). It might be helpful to include this one; FLAG_DTC3181E is likely to disappear.
On Fri, 4 Dec 2015, Ondrej Zary wrote: > @@ -685,8 +684,10 @@ static inline int NCR5380_pwrite(struct Scsi_Host *instance, unsigned char *src, > /* All documentation says to check for this. Maybe my hardware is too > * fast. Waiting for it seems to work fine! KLL > */ > - while (!(i = NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ)) > + while (!(i = NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ)) { > + udelay(4); /* DTC436 chip hangs without this */ > ; // FIXME - no timeout > + } > > /* > * I know. i is certainly != 0 here but the loop is new. See previous > Given that you've added braces, the redundant semicolon can be removed.
On Friday 04 December 2015, Finn Thain wrote: > > On Fri, 4 Dec 2015, Ondrej Zary wrote: > > > Add I/O register mapping for DTC chips and enable PDMA mode. > > > > These chips have 16-bit wide HOST BUFFER register (counter register at > > offset 0x0d increments by 2 on each HOST BUFFER read). > > > > Large PIO transfers crash at least the DTCT-436P chip (all reads result > > in 0xFF) so this patch actually makes it work. > > > > The chip also crashes when we bang the C400 host status register too > > heavily after PDMA write - a small udelay is needed. > > > > Signed-off-by: Ondrej Zary <linux@rainbow-software.org> > > --- > > # hdparm -t --direct /dev/sdb > > > > /dev/sdb: > > Timing O_DIRECT disk reads: 4 MB in 3.78 seconds = 1.06 MB/sec > > > > > > drivers/scsi/NCR5380.h | 1 + > > drivers/scsi/g_NCR5380.c | 47 +++++++++++++++++++++++----------------------- > > 2 files changed, 25 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h > > index 5092580..e3b8149 100644 > > --- a/drivers/scsi/NCR5380.h > > +++ b/drivers/scsi/NCR5380.h > > @@ -222,6 +222,7 @@ > > > > #define FLAG_NO_DMA_FIXUP 1 /* No DMA errata workarounds */ > > #define FLAG_NO_PSEUDO_DMA 8 /* Inhibit DMA */ > > +#define FLAG_16BIT 16 /* 16-bit PDMA */ > > Can we give this a better name? FLAG_16BIT could be taken to mean "16-bit > ISA card" but do we really want a flag for that? How about > FLAG_16BIT_BUF_REG or FLAG_WORD_IO_BUF? > > All active flags appear in the console log, thanks to prepare_info(). It > might be helpful to include this one; FLAG_DTC3181E is likely to > disappear. Thinking more about this, we can probably detect the host buffer register width instead of adding another flag. Read the counter, then read once from the host buffer and read the counter again to see if it increments by 1 or 2. Or maybe even 4 for PCI cards.
Hi Finn, On Fri, Dec 4, 2015 at 7:38 PM, Finn Thain <fthain@telegraphics.com.au> wrote: > > On Fri, 4 Dec 2015, Julian Calaby wrote: > >> > - if (overrides[current_override].board == BOARD_NCR53C400A) { >> > + if (overrides[current_override].board == BOARD_NCR53C400A || >> > + overrides[current_override].board == BOARD_DTC3181E) { >> >> These if statements are starting to get a bit long, would it make >> sense to replace them with a flag or equivalent? > > To what end? Shorter lines? As in, Pretty much, each expression is quite long and they seem to be growing fairly rapidly as you and Ondrej discover similar boards. > > if (board_is_ncr53c400a || board_is_dtc3181e) { > /* ... */ > } > > I suppose that could be an improvement if new flags would entirely replace > the override.board struct member and the existing switch statement, > > switch (overrides[current_override].board) { > /* ... */ > } > > Or maybe you meant testing a new flag something like this, > > if (hostdata->ncr53c400_compatible) { > /* ... */ > } > > If your concern is the Don't Repeat Yourself rule, I'm not sure that new > flag would get tested more than once (?) And it would still have to be > assigned using an "objectionably" long expression, e.g. > > hostdata->ncr53c400_compatible = > overrides[current_override].board == BOARD_NCR53C400 || > overrides[current_override].board == BOARD_NCR53C400A || > overrides[current_override].board == BOARD_DTC3181E; > > Rather than add new flags, perhaps a 'switch' statement instead of an 'if' > statement would be shorter (if the size of the expression is the problem). I think switch statements would be cleaner in this particular instance. I was thinking something like: if (somthing->flags & NCR53C400_COMPATIBLE) { /* ... */ } but if it's only ever going to be used once, then it's pretty pointless and switch statements are cleaner. Thanks,
On Sat, 5 Dec 2015, Julian Calaby wrote: > Hi Finn, > > On Fri, Dec 4, 2015 at 7:38 PM, Finn Thain <fthain@telegraphics.com.au> wrote: > > > > On Fri, 4 Dec 2015, Julian Calaby wrote: > > > >> > - if (overrides[current_override].board == BOARD_NCR53C400A) { > >> > + if (overrides[current_override].board == BOARD_NCR53C400A || > >> > + overrides[current_override].board == BOARD_DTC3181E) { > >> > >> These if statements are starting to get a bit long, would it make > >> sense to replace them with a flag or equivalent? > > > > To what end? Shorter lines? As in, > > Pretty much, each expression is quite long and they seem to be growing > fairly rapidly as you and Ondrej discover similar boards. Each BOARD_* macro actually refers to a whole category of devices. No new boards, devices or categories of devices have been discovered. Ondrej is enabling and/or fixing PDMA functionality for three existing device categories, for which the driver already has a nominally compatible PDMA implementation.
Hi Finn, On Sat, Dec 5, 2015 at 1:12 PM, Finn Thain <fthain@telegraphics.com.au> wrote: > > On Sat, 5 Dec 2015, Julian Calaby wrote: > >> Hi Finn, >> >> On Fri, Dec 4, 2015 at 7:38 PM, Finn Thain <fthain@telegraphics.com.au> wrote: >> > >> > On Fri, 4 Dec 2015, Julian Calaby wrote: >> > >> >> > - if (overrides[current_override].board == BOARD_NCR53C400A) { >> >> > + if (overrides[current_override].board == BOARD_NCR53C400A || >> >> > + overrides[current_override].board == BOARD_DTC3181E) { >> >> >> >> These if statements are starting to get a bit long, would it make >> >> sense to replace them with a flag or equivalent? >> > >> > To what end? Shorter lines? As in, >> >> Pretty much, each expression is quite long and they seem to be growing >> fairly rapidly as you and Ondrej discover similar boards. > > Each BOARD_* macro actually refers to a whole category of devices. No new > boards, devices or categories of devices have been discovered. > > Ondrej is enabling and/or fixing PDMA functionality for three existing > device categories, for which the driver already has a nominally compatible > PDMA implementation. I meant discovering boards which are similar. Either way, I'm not sure it matters that much. Thanks,
diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h index 5092580..e3b8149 100644 --- a/drivers/scsi/NCR5380.h +++ b/drivers/scsi/NCR5380.h @@ -222,6 +222,7 @@ #define FLAG_NO_DMA_FIXUP 1 /* No DMA errata workarounds */ #define FLAG_NO_PSEUDO_DMA 8 /* Inhibit DMA */ +#define FLAG_16BIT 16 /* 16-bit PDMA */ #define FLAG_LATE_DMA_SETUP 32 /* Setup NCR before DMA H/W */ #define FLAG_TAGGED_QUEUING 64 /* as X3T9.2 spelled it */ #define FLAG_TOSHIBA_DELAY 128 /* Allow for borken CD-ROMs */ diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c index fae4332..04f6c29 100644 --- a/drivers/scsi/g_NCR5380.c +++ b/drivers/scsi/g_NCR5380.c @@ -331,7 +331,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) ports = ncr_53c400a_ports; break; case BOARD_DTC3181E: - flags = FLAG_NO_PSEUDO_DMA; + flags = FLAG_NO_DMA_FIXUP | FLAG_16BIT; ports = dtc_3181e_ports; break; } @@ -415,7 +415,8 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) hostdata->c400_blk_cnt = 1; hostdata->c400_host_buf = 4; } - if (overrides[current_override].board == BOARD_NCR53C400A) { + if (overrides[current_override].board == BOARD_NCR53C400A || + overrides[current_override].board == BOARD_DTC3181E) { hostdata->c400_ctl_status = 9; hostdata->c400_blk_cnt = 10; hostdata->c400_host_buf = 8; @@ -434,7 +435,8 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt) goto out_unregister; if (overrides[current_override].board == BOARD_NCR53C400 || - overrides[current_override].board == BOARD_NCR53C400A) + overrides[current_override].board == BOARD_NCR53C400A || + overrides[current_override].board == BOARD_DTC3181E) NCR5380_write(hostdata->c400_ctl_status, CSR_BASE); NCR5380_maybe_reset_bus(instance); @@ -561,11 +563,10 @@ static inline int NCR5380_pread(struct Scsi_Host *instance, unsigned char *dst, while (NCR5380_read(hostdata->c400_ctl_status) & CSR_HOST_BUF_NOT_RDY); #ifndef SCSI_G_NCR5380_MEM - { - int i; - for (i = 0; i < 128; i++) - dst[start + i] = NCR5380_read(hostdata->c400_host_buf); - } + if (hostdata->flags & FLAG_16BIT) + insw(instance->io_port + hostdata->c400_host_buf, dst + start, 64); + else + insb(instance->io_port + hostdata->c400_host_buf, dst + start, 128); #else /* implies SCSI_G_NCR5380_MEM */ memcpy_fromio(dst + start, @@ -582,11 +583,10 @@ static inline int NCR5380_pread(struct Scsi_Host *instance, unsigned char *dst, } #ifndef SCSI_G_NCR5380_MEM - { - int i; - for (i = 0; i < 128; i++) - dst[start + i] = NCR5380_read(hostdata->c400_host_buf); - } + if (hostdata->flags & FLAG_16BIT) + insw(instance->io_port + hostdata->c400_host_buf, dst + start, 64); + else + insb(instance->io_port + hostdata->c400_host_buf, dst + start, 128); #else /* implies SCSI_G_NCR5380_MEM */ memcpy_fromio(dst + start, @@ -645,10 +645,10 @@ static inline int NCR5380_pwrite(struct Scsi_Host *instance, unsigned char *src, while (NCR5380_read(hostdata->c400_ctl_status) & CSR_HOST_BUF_NOT_RDY) ; // FIXME - timeout #ifndef SCSI_G_NCR5380_MEM - { - for (i = 0; i < 128; i++) - NCR5380_write(hostdata->c400_host_buf, src[start + i]); - } + if (hostdata->flags & FLAG_16BIT) + outsw(instance->io_port + hostdata->c400_host_buf, src + start, 64); + else + outsb(instance->io_port + hostdata->c400_host_buf, src + start, 128); #else /* implies SCSI_G_NCR5380_MEM */ memcpy_toio(hostdata->iomem + NCR53C400_host_buffer, @@ -660,12 +660,11 @@ static inline int NCR5380_pwrite(struct Scsi_Host *instance, unsigned char *src, if (blocks) { while (NCR5380_read(hostdata->c400_ctl_status) & CSR_HOST_BUF_NOT_RDY) ; // FIXME - no timeout - #ifndef SCSI_G_NCR5380_MEM - { - for (i = 0; i < 128; i++) - NCR5380_write(hostdata->c400_host_buf, src[start + i]); - } + if (hostdata->flags & FLAG_16BIT) + outsw(instance->io_port + hostdata->c400_host_buf, src + start, 64); + else + outsb(instance->io_port + hostdata->c400_host_buf, src + start, 128); #else /* implies SCSI_G_NCR5380_MEM */ memcpy_toio(hostdata->iomem + NCR53C400_host_buffer, @@ -685,8 +684,10 @@ static inline int NCR5380_pwrite(struct Scsi_Host *instance, unsigned char *src, /* All documentation says to check for this. Maybe my hardware is too * fast. Waiting for it seems to work fine! KLL */ - while (!(i = NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ)) + while (!(i = NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ)) { + udelay(4); /* DTC436 chip hangs without this */ ; // FIXME - no timeout + } /* * I know. i is certainly != 0 here but the loop is new. See previous
Add I/O register mapping for DTC chips and enable PDMA mode. These chips have 16-bit wide HOST BUFFER register (counter register at offset 0x0d increments by 2 on each HOST BUFFER read). Large PIO transfers crash at least the DTCT-436P chip (all reads result in 0xFF) so this patch actually makes it work. The chip also crashes when we bang the C400 host status register too heavily after PDMA write - a small udelay is needed. Signed-off-by: Ondrej Zary <linux@rainbow-software.org> --- # hdparm -t --direct /dev/sdb /dev/sdb: Timing O_DIRECT disk reads: 4 MB in 3.78 seconds = 1.06 MB/sec drivers/scsi/NCR5380.h | 1 + drivers/scsi/g_NCR5380.c | 47 +++++++++++++++++++++++----------------------- 2 files changed, 25 insertions(+), 23 deletions(-)