libata: Add Intel SCH PATA Support
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 1 line · Alek Du
This patch adds Intel SCH chipsets
Looks good, yet there's some nits...
Quoted 1 line
(US15W, US15L, UL11L) PATA controller support.
From the Intel's documentation I got the impression that the part numbers
start with AF82 (82 being the usual Intel's chip number prefix).
drivers/ata/Makefile obj-$(CONFIG_ATA) += libata.o · line 7
Quoted 11 lines
Signed-off-by Alek Du <[email protected]>drivers/ata/Makefile +1
-0@@ -4,6 +4,7 @@ obj-$(CONFIG_ATA) += libata.o obj-$(CONFIG_SATA_AHCI) += ahci.o obj-$(CONFIG_SATA_SVW) += sata_svw.o obj-$(CONFIG_ATA_PIIX) += ata_piix.o +obj-$(CONFIG_ATA_SCH) += ata_sch.o
The name of the drivers for the pure PATA chips are prefixed with 'pata_',
not 'ata_'.
Quoted 9 lines
drivers/ata/Makefile +0
-0obj-$(CONFIG_SATA_PROMISE) += sata_promise.o obj-$(CONFIG_SATA_QSTOR) += sata_qstor.o obj-$(CONFIG_SATA_SIL) += sata_sil.odrivers/ata/ata_sch.c new file +0
-0@@ -0,0 +1,271 @@
[...]
drivers/ata/ata_sch.c
Quoted 5 lines
drivers/ata/ata_sch.c new file +5
-0+/* see SCH datasheet page 351 */ +enum { +D0TIM =0x80 ,/* Device 0 Timing Register */ +D1TIM =0x84 ,/* Device 1 Timing Register */ +PIO =0x07 ,/* PIO Mode Bit Mask */
The SCH datasheet calls this field PM and since you've got all other names
form there, why not call it PM too?
drivers/ata/ata_sch.c
Quoted 10 lines
drivers/ata/ata_sch.c new file +9
-0+MDM = (0x03 <<8 ),/* Multi-word DMA Mode Bit Mask */ +UDM = (0x07 <<16 ),/* Ultra DMA Mode Bit Mask */ +PPE = (1 <<30 ),/* Prefetch/Post Enable */ + USD = (1 << 31), /* Use Synchronous DMA */ +}; +enum sch_controller_ids { +/* controller IDs */ + sch_pata_100,/* SCH up to UDMA 100 */ +};
I don't see why you need the above.
drivers/ata/ata_sch.c
Quoted 8 lines
drivers/ata/ata_sch.c new file +8
-0+static unsigned int in_module_init =1 ; + +static struct ata_port_operations sch_pata_ops = { + .inherits = &ata_bmdma_port_ops, + .cable_detect = ata_cable_unknown, + .set_piomode = sch_set_piomode, + .set_dmamode = sch_set_dmamode, + .prereset = ata_sff_prereset,
ata_sff_prereset is the default method, why not just inherit it?
Quoted 11 lines
drivers/ata/ata_sch.c new file +11
-0+/** +* sch_set_piomode - Initialize host controller PATA PIO timings +* @ap: Port whose timings we are configuring +* @adev: um +* +* Set PIO mode for device, in host controller PCI config space. +* +* LOCKING: +* None (inherited from caller). +*/ +
drivers/ata/ata_sch.c
Quoted 10 lines
drivers/ata/ata_sch.c new file +10
-0+static void sch_set_piomode (struct ata_port *ap,struct ata_device *adev) +{ +unsigned int pio = adev->pio_mode -XFER_PIO_0 ; +struct pci_dev *dev =to_pci_dev (ap->host->dev); +unsigned int port = adev->devno ?D1TIM :D0TIM ; +unsigned int data; + +/* SCH only support primary channel with one master and one slave*/ +if (ap->port_no !=0 ) +return ;
There should be no references to the secondary channel from libata since
you should explicitly specify that there is no secondary channel.
drivers/ata/ata_sch.c
Quoted 4 lines
drivers/ata/ata_sch.c new file +4
-0+ +pci_read_config_dword (dev, port, &data); +/* see SCH datasheet page 351 */ +/* enable PIO with PPE*/
No space before */...
drivers/ata/ata_sch.c
Quoted 1 line
drivers/ata/ata_sch.c new file +1
-0+ data &= ~(PIO );
Unneeded parens.
drivers/ata/ata_sch.c
Quoted 1 line
drivers/ata/ata_sch.c new file +1
-0+ data |= (PPE |pio);
You shouldn't enable prefetch for ATAPI deives I think -- look at whatata_piix does...
Also, unneeded parens, no space after | operator. Be consistent please.
drivers/ata/ata_sch.c
Quoted 1 line
drivers/ata/ata_sch.c new file +1
-0+/* mask reserved bits */
Why?
drivers/ata/ata_sch.c
Quoted 1 line
drivers/ata/ata_sch.c new file +1
-0+ data = data & (USD |PPE |UDM |MDM |PIO );
No spaces between | operator and operands.
Quoted 14 lines
drivers/ata/ata_sch.c new file +14
-0+pci_write_config_dword (dev, port, data); +} + +/** +* sch_set_dmamode - Initialize host controller PATA DMA timings +* @ap: Port whose timings we are configuring +* @adev: um +* +* Set MW/UDMA mode for device, in host controller PCI config space. +* +* LOCKING: +* None (inherited from caller). +*/ +
drivers/ata/ata_sch.c
Quoted 10 lines
drivers/ata/ata_sch.c new file +10
-0+static void sch_set_dmamode (struct ata_port *ap,struct ata_device *adev) +{ +unsigned int dma_mode = adev->dma_mode; +struct pci_dev *dev =to_pci_dev (ap->host->dev); +unsigned int port = adev->devno ?D1TIM :D0TIM ; +unsigned int data; + +/* SCH only support primary channel with one master and one slave*/ +if (ap->port_no !=0 ) +return ;
Same comment as in sch_set_piomode().
drivers/ata/ata_sch.c
Quoted 7 lines
drivers/ata/ata_sch.c new file +7
-0+ +pci_read_config_dword (dev, port, &data); +/* see SCH datasheet page 351 */ +if (dma_mode >=XFER_UDMA_0 ) { +/* enable Synchronous DMA mode */ + data |=USD ; + data &= ~(UDM );
Unneeded parens.
drivers/ata/ata_sch.c
Quoted 3 lines
drivers/ata/ata_sch.c new file +3
-0+ data |= (dma_mode -XFER_UDMA_0 ) <<16 ; + }else {/* must be MWDMA mode, since we masked SWDMA already */ + data &= ~(USD |MDM );
No spaces between | and operands.
drivers/ata/ata_sch.c
Quoted 3 lines
drivers/ata/ata_sch.c new file +3
-0+ data |= (dma_mode -XFER_MW_DMA_0 ) <<8 ; + } +/* mask reserved bits */
Why again?
drivers/ata/ata_sch.c
Quoted 1 line
drivers/ata/ata_sch.c new file +1
-0+ data = data & (USD |PPE |UDM |MDM |PIO );
No spaces between | and operands.
MBR, Sergei
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 7 lines · Alek Du
Quoted 4 lines
Quoted 1 line
(US15W, US15L, UL11L) PATA controller support.
From the Intel's documentation I got the impression that the part numbers
start with AF82 (82 being the usual Intel's chip number prefix).From documents I got, no 82xxx part number assigned. The formal names for SCH are just like US15W, US15L and UL11L.
Look into the spec. update, "Identification Information" section.
Quoted 4 lines
Quoted 2 lines
You shouldn't enable prefetch for ATAPI deives I think -- look at what
ata_piixdoes...Why?
Prefetch usually doesn't work with ATAPI devices since the prefetch logic
operates in units of 512-byte sectors and not all ATAPI data transfers are a multiple of 512 bytes.
MBR, Sergei
PATCH 1/1 libata: Add Intel SCH PATA Support
Hi Sergei and All,
I modified the driver again according to comments from Sergei, please help to review it.
Quoted 1 line
From
9a318d7ff2fc55811374b5b959524f5afcb8d5a6Mon Sep 17 00:00:00 2001
From: Alek Du <[email protected]>
Date: Mon, 5 May 2008 14:32:04 +0800
Subject: [PATCH] libata: Add Intel SCH PATA Support
This patch adds Intel SCH chipsets (AF82US15W, AF82US15L, AF82UL11L) PATA controller support.
--- drivers/ata/Kconfig | 9 ++ drivers/ata/Makefile | 1 + drivers/ata/pata_sch.c | 244 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 254 insertions(+), 0 deletions(-) create mode 100644 drivers/ata/pata_sch.c
drivers/ata/Kconfig +9 -0
@@ -697,6 +697,15 @@ config PATA_SCC If unsure, say N. +config PATA_SCH + tristate "Intel SCH PATA support" + depends on PCI + help + This option enables support for Intel SCH PATA on the Intel + SCH (US15W, US15L, UL11L) series host controllers. + + If unsure, say N. + config PATA_BF54X tristate "Blackfin 54x ATAPI support" depends on BF542 || BF548 || BF549
drivers/ata/Makefile +1 -0
@@ -67,6 +67,7 @@ obj-$(CONFIG_PATA_SIS) += pata_sis.o obj-$(CONFIG_PATA_TRIFLEX) += pata_triflex.o obj-$(CONFIG_PATA_IXP4XX_CF) += pata_ixp4xx_cf.o obj-$(CONFIG_PATA_SCC) += pata_scc.o +obj-$(CONFIG_PATA_SCH) += pata_sch.o obj-$(CONFIG_PATA_BF54X) += pata_bf54x.o obj-$(CONFIG_PATA_PLATFORM) += pata_platform.o obj-$(CONFIG_PATA_OF_PLATFORM) += pata_of_platform.o
drivers/ata/pata_sch.c new file +244 -0
244 lines not on this page · raw message
Signature · 4 lines
1.5.2.5 BR, Alek
Hello.
Quoted 1 line · Alek Du
I modified the driver again according to comments from Sergei, please help to review it.
You even were somewhat over-zealous. ;-)
Quoted 11 lines
This patch adds Intel SCH chipsets (AF82US15W, AF82US15L, AF82UL11L) PATA controller support.
Signed-off-by Alek Du <[email protected]>drivers/ata/pata_sch.c new file +0
-0@@ -0,0 +1,244 @@
[...]
Quoted 4 lines
+/**
+ *sch_set_piomode- Initialize host controller PATA PIO timings
+ * @ap: Port whose timings we are configuring
+ * @adev: um
Hm, I'm seing the same "um" un ata_piix, and wondering what it means. :-)do_pata_set_dmamode at least calls this "drive in question"...
Quoted 9 lines
+ *
+ * Set PIO mode for device, in host controller PCI config space.
+ *
+ * LOCKING:
+ * None (inherited from caller).
+ */drivers/ata/pata_sch.c new file +3
-0+ +static void sch_set_piomode (struct ata_port *ap,struct ata_device *adev) +{
drivers/ata/pata_sch.c line 126
Quoted 10 lines
drivers/ata/pata_sch.c new file +10
-0+unsigned int pio = adev->pio_mode -XFER_PIO_0 ; +struct pci_dev *dev =to_pci_dev (ap->host->dev); +unsigned int port = adev->devno ?D1TIM :D0TIM ; +unsigned int data; + +pci_read_config_dword (dev, port, &data); +/* see SCH datasheet page 351 */ +/* set PIO mode without PPE */ + data &= ~(PM |PPE ); + data |= pio;
I didn't mean you shouldn't ever set PPE, what I meant was:
if (adev->class == ATA_DEV_ATA)
data |= PPE;MBR, Sergei
drivers/ata/pata_sch.c line 215
Quoted 3 lines
drivers/ata/pata_sch.c new file +3
-0+/* no hotplugging support (FIXME) */ +if (!in_module_init) +return -ENODEV ;
I see no reason for this check ? AHCI/PIIX sometimes need this because they rely on BIOS set values [and nobody has fixed it to only check on those chips]. Your new driver seems clean of such cases.
Alan
Quoted 13 lines · Alek Du
Hi Sergei and All,
I modified the driver again according to comments from Sergei, please help to review it.
Quoted 1 line
From
9a318d7ff2fc55811374b5b959524f5afcb8d5a6Mon Sep 17 00:00:00 2001From: Alek Du <[email protected]>
Date: Mon, 5 May 2008 14:32:04 +0800
Subject: [PATCH] libata: Add Intel SCH PATA SupportThis patch adds Intel SCH chipsets (AF82US15W, AF82US15L, AF82UL11L) PATA controller support.
Quoted 7 lines
Signed-off-by Alek Du <[email protected]>--- drivers/ata/Kconfig | 9 ++ drivers/ata/Makefile | 1 + drivers/ata/pata_sch.c | 244 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 254 insertions(+), 0 deletions(-) create mode 100644 drivers/ata/pata_sch.c
overall, looks good.
your suspend/resume routines are copies of the in-kernel ata_pci_device_suspend/resume, and should be removed (just refer directly to the generic functions)
agree w/ Alan and Sergei's comments, too
Jeff
Also...
Quoted 4 lines · Alek Du
+/*
+ * Supports:
+ * Intel SCH (AF82US15W, AF82US15L, AF82UL11L) chipsets
+ */
a link to the public datasheet would be nice, somewhere in a comment
drivers/ata/pata_sch.c line 64
Quoted 3 lines
drivers/ata/pata_sch.c new file +3
-0+static const struct pci_device_id sch_pci_tbl[] = { +/* Intel SCH PATA Controller */ + {0x8086 ,0x811A ,PCI_ANY_ID ,PCI_ANY_ID ,0 ,0 ,0 },
recommend using PCI_VDEVICE() macro
[and it would be nice if someone converted ata_piix.c, too]
drivers/ata/pata_sch.c line 236
Quoted 12 lines
drivers/ata/pata_sch.c new file +11
-0+ { }/* terminate list */ +static int __init sch_init (void ) +{ +int rc; + + rc =pci_register_driver (&sch_pci_driver); +if (rc) +return rc; + in_module_init =0 ; +return 0 ; +}
with the in_module_init thing removed (per Alan's comment), this can become 'return pci_register_driver(...)' nicely
again: [PATCH] libata: Add Intel SCH PATA Support
Hi Jeff, Alan, Sergei, Bartlomiej
Modified again according to your good comments! Hope this time it is ok. :-)
Quoted 1 line
From
7a7f6630e32b135f6d659db8e189748b4e7fd1a4Mon Sep 17 00:00:00 2001
From: Alek Du <[email protected]>
Date: Mon, 5 May 2008 14:32:04 +0800
Subject: [PATCH] libata: Add Intel SCH PATA Support
This patch adds Intel SCH chipsets (AF82US15W, AF82US15L, AF82UL11L) PATA controller support.
--- drivers/ata/Kconfig | 9 ++ drivers/ata/Makefile | 1 + drivers/ata/pata_sch.c | 206 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 216 insertions(+), 0 deletions(-) create mode 100644 drivers/ata/pata_sch.c
drivers/ata/Kconfig +9 -0
@@ -697,6 +697,15 @@ config PATA_SCC If unsure, say N. +config PATA_SCH + tristate "Intel SCH PATA support" + depends on PCI + help + This option enables support for Intel SCH PATA on the Intel + SCH (US15W, US15L, UL11L) series host controllers. + + If unsure, say N. + config PATA_BF54X tristate "Blackfin 54x ATAPI support" depends on BF542 || BF548 || BF549
drivers/ata/Makefile +1 -0
@@ -67,6 +67,7 @@ obj-$(CONFIG_PATA_SIS) += pata_sis.o obj-$(CONFIG_PATA_TRIFLEX) += pata_triflex.o obj-$(CONFIG_PATA_IXP4XX_CF) += pata_ixp4xx_cf.o obj-$(CONFIG_PATA_SCC) += pata_scc.o +obj-$(CONFIG_PATA_SCH) += pata_sch.o obj-$(CONFIG_PATA_BF54X) += pata_bf54x.o obj-$(CONFIG_PATA_PLATFORM) += pata_platform.o obj-$(CONFIG_PATA_OF_PLATFORM) += pata_of_platform.o
drivers/ata/pata_sch.c new file +206 -0
206 lines not on this page · raw message
Signature · 4 lines
1.5.2.5 BR, Alek
Jeff Garzik
AppliedRe: again: [PATCH] libata: Add Intel SCH PATA Support
Quoted 13 lines · Alek Du
Hi Jeff, Alan, Sergei, Bartlomiej
Modified again according to your good comments! Hope this time it is ok. :-)
From
7a7f6630e32b135f6d659db8e189748b4e7fd1a4Mon Sep 17 00:00:00 2001
From: Alek Du <[email protected]>
Date: Mon, 5 May 2008 14:32:04 +0800
Subject: [PATCH] libata: Add Intel SCH PATA SupportThis patch adds Intel SCH chipsets (AF82US15W, AF82US15L, AF82UL11L) PATA controller support.
Quoted 7 lines
Signed-off-by Alek Du <[email protected]>--- drivers/ata/Kconfig | 9 ++ drivers/ata/Makefile | 1 + drivers/ata/pata_sch.c | 206 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 216 insertions(+), 0 deletions(-) create mode 100644 drivers/ata/pata_sch.c
applied
for future reference, multiple revisions of patches are typically done like
Subject: [PATCH v2] libata: Add Intel SCH PATA Support
Subject: [PATCH v3] libata: Add Intel SCH PATA Support etc.
Re: again: [PATCH] libata: Add Intel SCH PATA Support
In reply to Alek DuQuoted 3 lines · Alek Du
Hi Jeff, Alan, Sergei, Bartlomiej
Modified again according to your good comments! Hope this time it is ok. :-)
Hi,
Looks good, are you going to submit IDE version soon?
Thanks,
Bart
Re: again: [PATCH] libata: Add Intel SCH PATA Support
Quoted 8 lines · On Tue, 6 May 2008 18:20:42 +0200 Bartlomiej Zolnierkiewicz…
Quoted 3 lines · Alek Du
Hi Jeff, Alan, Sergei, Bartlomiej
Modified again according to your good comments! Hope this time it is ok. :-)
Hi,
Looks good, are you going to submit IDE version soon?
Is there any point adding new drivers to the old IDE layer nowdays ?
Alan
Re: again: [PATCH] libata: Add Intel SCH PATA Support
Hi,
Quoted 6 lines · On Tue, 6 May 2008 16:59:37 +0100 Alan Cox <[email protected]> wrote:
Quoted 3 lines · On Tue, 6 May 2008 18:20:42 +0200 Bartlomiej Zolnierkiewicz…
Quoted 2 lines · Alek Du
Hi Jeff, Alan, Sergei, Bartlomiej
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Modified again according to your good comments! Hope this time it is ok. :-)
Hi,
Looks good, are you going to submit IDE version soon?
Is there any point adding new drivers to the old IDE layer nowdays ?
Yeah, I really was hesitating to write the corresponding IDE driver for a while. If it really could help people who have good reason to use IDE layer instead of libata, I will do that.
Thanks,
Alek
Re: again: [PATCH] libata: Add Intel SCH PATA Support
In reply to Alan CoxQuoted 6 lines · Alan Cox
Quoted 3 lines · On Tue, 6 May 2008 18:20:42 +0200 Bartlomiej Zolnierkiewicz…
Quoted 2 lines · Alek Du
Hi Jeff, Alan, Sergei, Bartlomiej
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Modified again according to your good comments! Hope this time it is ok. :-)
Hi,
Looks good, are you going to submit IDE version soon?
Is there any point adding new drivers to the old IDE layer nowdays ?
Not all distros want to use libata SCSI to ATA emulation layer for PATA, and IDE/ATA subsystem is more embedded friendly (code-size wise etc.).
Thanks,
Bart