lkml.org 
[lkml]   [2008]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] Resurrect IT8172 IDE controller driver
    Hello.

    Shane McDonald wrote:

    > diff -uprN a/drivers/ide/it8172.c b/drivers/ide/it8172.c
    > --- a/drivers/ide/it8172.c 1969-12-31 18:00:00.000000000 -0600
    > +++ b/drivers/ide/it8172.c 2008-11-23 01:06:01.000000000 -0600
    > @@ -0,0 +1,205 @@
    > +/*
    > + *
    > + * BRIEF MODULE DESCRIPTION
    > + * IT8172 IDE controller support
    > + *
    > + * Copyright 2000 MontaVista Software Inc.
    > + * Author: MontaVista Software, Inc.
    > + * stevel@mvista.com or source@mvista.com
    >

    You can remove the former address, Steve Longerbeam is no longer with
    MV (quit long ago). And I think you may add your own copyright now.

    > +/*
    > + * Prototypes
    > + */
    >

    I see no prototypes here...

    > +
    > +static void it8172_set_pio_mode(ide_drive_t *drive, const u8 pio)
    > +{
    > + ide_hwif_t *hwif = HWIF(drive);
    > + struct pci_dev *dev = to_pci_dev(hwif->dev);
    > + int is_slave = (&hwif->drives[1] == drive);
    >

    Use simpler "drive->dn & 1" (and drop the useless parens please) --
    & can be dropped though as the controller has only a single channel...

    > + spin_lock_irqsave(&ide_lock, flags);
    >

    Don't use ide_lock here please. Moreover, since the controller has
    only one channel, you don't need any spinlock here.

    > + pci_read_config_word(dev, 0x40, &drive_enables);
    > + pci_read_config_dword(dev, 0x44, &drive_timing);
    > +
    > + /*
    > + * FIX! The DIOR/DIOW pulse width and recovery times in port 0x44
    >

    It's usually FIXME. :-)

    > + * are being left at the default values of 8 PCI clocks (242 nsec
    > + * for a 33 MHz clock).

    It's 240, not 242 ns as 33 is actually 33.333.

    > These can be safely shortened at higher
    > + * PIO modes. The DIOR/DIOW pulse width and recovery times only
    > + * apply to PIO modes, not to the DMA modes.
    > + */
    >

    Not true. They do apply to SW/MW DMA modes -- and if you look at your
    own code you'll see that.

    > +
    > + /*
    > + * Enable port 0x44. The IT8172 spec is confused; it calls
    > + * this register the "Slave IDE Timing Register", but in fact,
    > + * it controls timing for both master and slave drives.
    > + */
    > + drive_enables |= 0x4000;
    >

    This is strange because this IDE controller seems to be a clone of
    the one in the Intel PIIX chip. However, the spec. I have tellm it's not
    an exact clone.
    I suggest that you take a close look at drivers/ide/piix.c...

    > +
    > + if (is_slave) {
    > + drive_enables &= 0xc006;
    > + if (pio > 1)
    > + /* enable prefetch and IORDY sample-point */
    > + drive_enables |= 0x0060;
    >

    IORDY sampling shouldn't be enabled for PIO mode 2 always, only if
    the drive supports it.
    Prefetch should only be enabled for ATA disks, not the ATAPI devices

    > +static void it8172_set_dma_mode(ide_drive_t *drive, const u8 speed)
    > +{
    > + ide_hwif_t *hwif = HWIF(drive);
    > + struct pci_dev *dev = to_pci_dev(hwif->dev);
    > + int a_speed = 3 << (drive->dn * 4);
    > + int u_flag = 1 << drive->dn;
    > + int u_speed = 0;
    > + u8 reg48, reg4a;
    > +
    > + const u8 mwdma_to_pio[] = { 0, 3, 4 };
    > + u8 pio;
    > +
    > + pci_read_config_byte(dev, 0x48, &reg48);
    > + pci_read_config_byte(dev, 0x4a, &reg4a);
    > +
    > + if (speed >= XFER_UDMA_0) {
    > +
    > + /* Setting the DMA cycle time to 2 or 3 PCI clocks
    > + * (60 and 91 nsec at 33 MHz PCI clock) seems to cause
    > + * BadCRC errors during DMA transfers on some drives,
    >

    Sigh, such drives should have been blacklisted...

    > + * even though both numbers meet the minimum ATAPI-4 spec
    > + * of 73 and 54 nsec for UDMA 1 and 2 respectively.
    > + * So the faster times are not implemented here.
    > + * The good news is that the slower cycle time has
    > + * very little affect on transfer performance.
    >

    This should only affect the write performance, reads.

    > + */
    > +
    > + u_speed = 0 << (drive->dn * 4);
    >

    I think you're actually setting UltraDMA mode 2 timing regardless of
    the mode selected.

    The below code only applies to non-UltraDMA modes.

    > +
    > + if (speed >= XFER_MW_DMA_0)
    > + pio = mwdma_to_pio[speed - XFER_MW_DMA_0];
    > + else
    > + pio = 2;
    > +
    > + it8172_set_pio_mode(drive, pio);
    >

    Moreover, I'd suggest to reimplement it as the timing register layout
    is different from the original PIIX and additional DMA modes can be
    supported: MWDMA0 and SWDMA1.

    > +static unsigned int __devinit init_chipset_it8172(struct pci_dev *dev)
    > +{
    > + unsigned char progif;
    > +
    > + /*
    > + * Place both IDE interfaces into PCI "native" mode
    > + */
    > + pci_read_config_byte(dev, PCI_CLASS_PROG, &progif);
    > + pci_write_config_byte(dev, PCI_CLASS_PROG, progif | 0x05);
    > +
    > + return dev->irq;
    > +}
    >

    As Alan have said, you can't do that here.

    > +static const struct ide_port_info it8172_chipset __devinitdata = {
    > + .name = DRV_NAME,
    > + .init_chipset = init_chipset_it8172,
    > + .port_ops = &it8172_port_ops,
    > + .enablebits = {{0x00, 0x00, 0x00}, {0x40, 0x00, 0x01} },
    >

    Wrong, should be:

    .enablebits = {{0x41, 0x80, 0x80}, {0x00, 0x00, 0x00}},

    If that doesn't work (firmware doesn't enable the channel), you can leave it all 0s...

    > .host_flags = IDE_HFLAG_SINGLE,
    > + .pio_mask = ATA_PIO4,
    > + .swdma_mask = ATA_SWDMA2_ONLY,
    > + .mwdma_mask = ATA_MWDMA12_ONLY,
    >

    More modes are supportable...

    > +static const struct ide_port_info it8172_chipset __devinitdata = {

    Please rename this variable to it8172_port_info.

    > +};
    > +
    > +static int __devinit it8172_init_one(struct pci_dev *dev,
    > + const struct pci_device_id *id)
    > +{
    > + if ((!(PCI_FUNC(dev->devfn) & 1) ||
    > + (!((dev->class >> 8) == PCI_CLASS_STORAGE_IDE))))
    >

    Please, don't abuse parens, it should be just:

    if (!(PCI_FUNC(dev->devfn) & 1) ||
    (dev->class >> 8) != PCI_CLASS_STORAGE_IDE)


    > + return -ENODEV; /* IT8172 is more than an IDE controller */
    >

    And I don't get it: why the first check is needed? Is there another
    IDE controller on function 1 that you want to ignore? I doubt it.

    > +MODULE_AUTHOR("SteveL@mvista.com");
    >

    I wonder why Steve didn't specify his full name... :-)

    MBR, Sergei




    \
     
     \ /
      Last update: 2008-11-24 11:37    [W:4.060 / U:1.120 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site