On 2012-03-30 22:18, Arnd Bergmann wrote:
>> +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.
>> + 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.
>> + 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.
>> +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.
>> ===================================================================
>> --- 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
--
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
