Re: [PATCH] Resurrect IT8172 IDE controller driver

From: Sergei Shtylyov
Date: Mon Dec 08 2008 - 07:02:40 EST

Hello.

Shane McDonald wrote:

I have made some of the required changes, but I have some questions.
See in-line for my resolution / questions for each comment.

On Mon, Nov 24, 2008 at 4:33 AM, Sergei Shtylyov
<sshtylyov@xxxxxxxxxxxxx> wrote:
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@xxxxxxxxxx or source@xxxxxxxxxx

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.

OK, I will update accordingly.

+/*
+ * Prototypes
+ */
I see no prototypes here...

Ah yes, the prototypes were removed, but not the comment. I will remove this.

+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...

I don't believe it can be dropped entirely -- although the
controller only has a single channel, it does support two drives on
that channel. Unless I understand incorrectly, I will make the
"drive->dn & 1" change.

Believe it or not, drive->dn will be 0 or 1 in this case, so & is superfluous. :-)
I don't insist on dropping it though...

+ /*
+ * 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 just don't undestand what this bit is good for in IT8172G... Perhaps by clearing it once could achieve PIO0 timings, just like by clearing the fast timing bits on PIIX?

I suggest that you take a close look at drivers/ide/piix.c...

The specs I have indicate that it is not the same. For example, bits
13:12 in the IDETIM register at offset 0x40-0x41 of the PIIX give the
IORDY Sample Point, but the same function is found in bits 19-17 in
the SLVT register at offset 0x44-0x47 in the IT8172. Perhaps better
comments are in order, or perhaps these bitfields should be defined in
a .h file?

No, IT8172G just implements them differently. But you can still find in this driver a good example of how things should be done...

+ 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

OK, I will update accordingly. Am I correct that prefetch should be
enabled if "drive->media == ide_disk", and not otherwise?

Yes, you're correct.

I am not sure how to determine if IORDY sampling is supported by a

I think Bart has replied to thia already...

drive. If I'm reading the code correctly, other drivers only check
that the PIO mode is > 2 (not > 1 as in my driver) -- that seems to be
the case for at least piix.c, siimage.c, and it8213.c.

That's because PIO modes above 2 necessiate IORDY, while for mode 2 the drive can require it optionally.

+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...

Do you think I should keep this code in here? It kind of seems silly
to run drivers at UDMA0 speeds just because of a few bad drives back
in 2000 when the driver was originally written.

If was only happening for some drives, you probably shouldn't keep it...

Should this not be
handled in userspace by hdparm? Other drivers don't seem to do
something similar.

This is usually handled by blacklisting the drives for the user's convenience.

+ * 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.

"On reads the drive dictatees the timings" I was going to write. :-)


+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.

I will remove this.

You'll probably need to add this as a quirk to drivers/pci/quirks.c -- unless the bootloader sets the controller to native mode itself.

+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...

Er, I was wrong here -- you actiually can't do that as the channel will remain disabled.

Well, mine is clearly wrong. Am I correct that {0x41, 0x80, 0x80} is
checking that the IDE Decode Enable bit is set? This bit is in the
same location in both the PIIX and the IT8172.

Yes, you're correct. You should add the code to force it enabled if the bootloader doesn't do that.

.host_flags = IDE_HFLAG_SINGLE,
+ .pio_mask = ATA_PIO4,

I'm not seeing how PIO mode 0 is supportable.

+ .swdma_mask = ATA_SWDMA2_ONLY,

The SWDMA support could be dropped altogether -- these modes are slow and long obsolete.

+ .mwdma_mask = ATA_MWDMA12_ONLY,
More modes are supportable...

I will update accordingly.

These masks were stolen from the piix.c driver and are determined by the limitation of the timing register fileds being only 2-bit wide. IT8172G has these bits implemented differently, hence the limitation shouldn't apply. You shouldn't just update the masks without doing anythiung about programming the modes...

+MODULE_AUTHOR("SteveL@xxxxxxxxxx");
I wonder why Steve didn't specify his full name... :-)

I will change this to my name.

I don't think it would be a legitimate change...

Thank you for your time!

If you could give me more free time instead... :-)

MBR, Sergei


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/