On 2012-03-30 22:18, Arnd Bergmann wrote:

quoted

+static u16 ep93xx_pata_read(struct ep93xx_pata_data *drv_data,
+			    void *addr_io,
+			    const struct ata_timing *t)
+{
+	unsigned long addr = (unsigned long) addr_io & 0x1f;
+	void __iomem *base = drv_data->ide_base;
+	u16 ret;
+
+	writel(IDECtrl_DIOWn | IDECtrl_DIORn | addr, base + IDECtrl);
+	ndelay(t->setup);
+	writel(IDECtrl_DIOWn | addr, base + IDECtrl);

The pointer arithmetic that you do here on addr_io looks really evil.
Basically your registers are indirect and cannot be accessed through
pointer dereference. Maybe you should not be trying to do that then
and not use the ata_port->ioaddr structure.

Yes, I already prepared a version of the driver in which IDE register
addresses are coded as enums instead of using ata_port->ioaddr structure. 
I will post updated version, when I get feedback from Ryan if enums
or defines are preferred.

quoted

+	ndelay(t->act8b);

I'm not too familar with ata drivers, but I don't think you're supposed
to have delays in the code for the timings, rather than programming
the timings into the controller registers. Are you sure this is the
right thing here?

EP93xx IDE controller is quite unusual - in PIO mode it's a GPIO like pin
interface.

quoted

+	if (drv_data->iordy) {
+		/* min. 1s timeout (at cpu cycle = 5ns) */
+		unsigned int timeout = 200000;
+		while (!ep93xx_pata_check_iordy(base) && --timeout)
+			cpu_relax();
+	}

This is not a reliable way to implement a timeout. Instead, you should
use time_before() to check if hte timeout has expired.

Fixed.

quoted

+static void ep93xx_pata_setup_port(struct ata_ioports *ioaddr)
+{
+	/*
+	 * the device IDE register to be accessed is selected through
+	 * IDECTRL register's specific bitfields 'DA', 'CS1n' and 'CS0n':
+	 *   b4   b3   b2    b1     b0
+	 *   A2   A1   A0   CS1n   CS0n
+	 * the values filled in this structure allows the value to be directly
+	 * ORed to the IDECTRL register, hence giving directly the A[2:0] and
+	 * CS1n/CS0n values for each IDE register.
+	 * The values correspond to the transformation:
+	 *   ((real IDE address) << 2) | CS1n value << 1 | CS0n value
+	 */
+	ioaddr->cmd_addr	= (void __iomem *) 0 + 2; /* CS1 */
+
+	ioaddr->data_addr	= (void __iomem *) (ATA_REG_DATA << 2) + 2;
+	ioaddr->error_addr	= (void __iomem *) (ATA_REG_ERR << 2) + 2;
+	ioaddr->feature_addr	= (void __iomem *) (ATA_REG_FEATURE << 2) + 2;
+	ioaddr->nsect_addr	= (void __iomem *) (ATA_REG_NSECT << 2) + 2;
+	ioaddr->lbal_addr	= (void __iomem *) (ATA_REG_LBAL << 2) + 2;
+	ioaddr->lbam_addr	= (void __iomem *) (ATA_REG_LBAM << 2) + 2;
+	ioaddr->lbah_addr	= (void __iomem *) (ATA_REG_LBAH << 2) + 2;
+	ioaddr->device_addr	= (void __iomem *) (ATA_REG_DEVICE << 2) + 2;
+	ioaddr->status_addr	= (void __iomem *) (ATA_REG_STATUS << 2) + 2;
+	ioaddr->command_addr	= (void __iomem *) (ATA_REG_CMD << 2) + 2;
+
+	ioaddr->altstatus_addr	= (void __iomem *) (0x06 << 2) + 1; /* CS0 */
+	ioaddr->ctl_addr	= (void __iomem *) (0x06 << 2) + 1; /* CS0 */
+}

As mentioned above, this seems to make no sense.

Removed.

quoted

===================================================================

--- linux-2.6.orig/drivers/ata/Kconfig
+++ linux-2.6/drivers/ata/Kconfig

@@ -416,6 +416,15 @@ config PATA_EFAR
 
 	  If unsure, say N.
 
+config PATA_EP93XX
+	tristate "Cirrus Logic EP93xx PATA support"
+	depends on ARCH_EP93XX
+	help
+	  This option enables support for the PATA controller in
+	  the Cirrus Logic EP9312 and EP9315 ARM CPU.
+
+	  If unsure, say N.
+
 config PATA_HPT366
 	tristate "HPT 366/368 PATA support"
 	depends on PCI

And I think this should be consequently be sorted below nonstandard ports, instead of the SFF list.

	Arnd

I selected "PATA SFF controllers with BMDMA" list because the driver
inherits .ata_bmdma_port_ops (this is in libata-sff.c, which is compiled
only if ATA_SFF is set).

Thanks for comments.
RP