On Saturday 31 January 2009, Sergei Shtylyov wrote:
> Hello.
>
> Bartlomiej Zolnierkiewicz wrote:
>
> >>> +enum {
> >>> + MSR_IDE_CFG = 0x51300010,
> >>> + PCI_IDE_CFG = 0x40,
> >>> +
> >>> + CFG = 0,
> >>> + DTC = 2,
> >>> + CAST = 3,
> >>> + ETC = 4,
> >>> +
> >>> + IDE_CFG_CHANEN = (1 << 1),
> >>> + IDE_CFG_CABLE = (1 << 17) | (1 << 16),
> >>> +
> >>> + IDE_D0_SHIFT = 24,
> >>> + IDE_D1_SHIFT = 16,
> >>> + IDE_DRV_MASK = 0xff,
> >>> +
> >>> + IDE_CAST_D0_SHIFT = 6,
> >>> + IDE_CAST_D1_SHIFT = 4,
> >>> + IDE_CAST_DRV_MASK = 0x3,
> >>> +
> >>> + IDE_CAST_CMD_SHIFT = 24,
> >>> + IDE_CAST_CMD_MASK = 0xff,
> >>> +};
> >>>
> >>>
> >> Declaring a lot of semi-related constants is not what enum was
> >> intended for I think...
> >>
> >
> > Don't know about that but it is still better than using defines.
> >
>
> That's what I doubt...
>
> >>> + cs5536_read(dev, CFG, &cfg);
> >>> +
> >>> + if ((cfg & IDE_CFG_CHANEN) == 0) {
> >>> + printk(KERN_ERR DRV_NAME ": disabled by BIOS\n");
> >>> + return -ENODEV;
> >>>
> >>>
> >> Eh, why not do it via the usual .enablebits mechanism?
> >>
> >
> > Because we may be using MSR access instead of PCI one.
> >
>
> Ah...
>
> > From: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
> > Subject: [PATCH] ide: add CS5536 host driver (v2)
> >
> > This is a port of libata's pata_cs5536.c (written by Martin K. Petersen)
> > to IDE subsystem.
> >
> > Changes done while at it:
> >
> > * Reprogram PIO/MWDMA timings if needed before and after DMA transfer
> > (chipset uses shared PIO/MWDMA timings).
> >
> > * Fix cable detection to report 80-wires cable if BIOS set it for any
> > device on a port (IDE core will do drive-side cable detection later).
> >
> > * Don't disable UDMA while programming PIO timings.
> >
> > * Simplify PCI/MSR support.
> >
> > Pros of having IDE host driver in addition to libata's one:
> >
> > * IDE is much lighter than SCSI+libata, the host driver itself is also
> > a bit smaller:
> >
> > text data bss dec hex filename
> > 1237 500 4 1741 6cd drivers/ata/pata_cs5536.o
> > 1214 128 4 1346 542 drivers/ide/cs5536.o
> >
> > * This allows use of IDE features which are unavailable under libata.
> >
> > v2:
> > * Fixes per review from Sergei:
> > - simplify dependency check in Kconfig
> > - use IDE_DRV_MASK also for ->drive_data
> > - disable UDMA when programming MWDMA
> > - program new DTC timings only when necessary
> > - fix printk() level in cs5536_init_one()
> >
> > * Fix patch description according to comments from Alan and Sergei.
> >
> > Cc: Martin K. Petersen <mkp@xxxxxxx>
> > Cc: Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx>
> > Cc: Karl Auerbach <karl@xxxxxxx>
> > Cc: Alan Cox <alan@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
> > ---
> > v1->v2 interdiff only
> >
>
> Acked-by: Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx>
>
>
> > diff -u b/drivers/ide/cs5536.c b/drivers/ide/cs5536.c
> > --- b/drivers/ide/cs5536.c
> > +++ b/drivers/ide/cs5536.c
> > @@ -61,6 +61,9 @@
> >
> > IDE_CAST_CMD_SHIFT = 24,
> > IDE_CAST_CMD_MASK = 0xff,
> > +
> > + IDE_ETC_UDMA_RELSHIFT = 6,
> > + IDE_ETC_UDMA_MASK = 0x3,
> >
>
> Too many shifts and mask values to my taste, why not just use 0xC0?
Fixed, I have no idea why I did it this way originally... :)
> > @@ -186,22 +189,24 @@
> > int dshift = (drive->dn & 1) ? IDE_D1_SHIFT : IDE_D0_SHIFT;
> > u32 etc;
> >
> > - if (mode >= XFER_UDMA_0) {
> > - cs5536_read(pdev, ETC, &etc);
> > + cs5536_read(pdev, ETC, &etc);
> >
> > + if (mode >= XFER_UDMA_0) {
> > etc &= ~(IDE_DRV_MASK << dshift);
> >
>
> Er, I'm not sure using IDE_DRV_MASK here is completely correct as
> only the mask 0xC7 actually controls UltraDMA enables and cycle time...
Bits 0x38 are described to be hardwired to 0 in the datasheet so this
should be OK (also pata_cs5536.c does the same thing)...
v2->v3 interdiff:
...
v3:
* Smarter masking of UDMA bits per Sergei's suggestion.
...
diff -u b/drivers/ide/cs5536.c b/drivers/ide/cs5536.c
--- b/drivers/ide/cs5536.c
+++ b/drivers/ide/cs5536.c
@@ -62,8 +62,7 @@
IDE_CAST_CMD_SHIFT = 24,
IDE_CAST_CMD_MASK = 0xff,
- IDE_ETC_UDMA_RELSHIFT = 6,
- IDE_ETC_UDMA_MASK = 0x3,
+ IDE_ETC_UDMA_MASK = 0xc0,
};
static int use_msr;
@@ -195,7 +194,7 @@
etc &= ~(IDE_DRV_MASK << dshift);
etc |= udma_timings[mode - XFER_UDMA_0] << dshift;
} else { /* MWDMA */
- etc &= ~(IDE_ETC_UDMA_MASK << (dshift + IDE_ETC_UDMA_RELSHIFT));
+ etc &= ~(IDE_ETC_UDMA_MASK << dshift);
drive->drive_data &= IDE_DRV_MASK;
drive->drive_data |= mwdma_timings[mode - XFER_MW_DMA_0] << 8;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
