Re: [PATCH 2/3] x86: Add support for IDE on the legacy I/O ports

[Michel Stam <michel@reverze.net>]

[-- Attachment #1.1: Type: text/plain, Size: 13333 bytes --]

Hello Jean,

Forget about the ifdefs; my patches there were meant to let the code compile depending on what was selected in configuration, (this was not the case previously), but the whole thing became an ifdef mess in the process. I've completely rewritten this, splitting initialisation over multiple files which are selected from the Makefile.

>> diff --git a/include/ata_drive.h b/include/ata_drive.h
>> index 6d6cca4..44073cb 100644
>> --- a/include/ata_drive.h
>> +++ b/include/ata_drive.h
>> @@ -119,6 +119,7 @@ struct ata_ioports {
>>    /* hard reset line handling */
>>    void (*reset)(int);    /* true: assert reset, false: de-assert reset */
>>    int dataif_be;    /* true if 16 bit data register is big endian */
>> +    int mmio; /* true if memory-mapped io */
> so use a boolean
look at the line immediately preceeding the mmio line: I'm following the style of the original driver. I was not intending to do a code cleanup of the driver, the goal is to get it to work on the x86.

>> b/arch/x86/boards/x86_generic/generic_pc.c
>> @@ -27,6 +27,10 @@
>> #include <ns16550.h>
>> #include <linux/err.h>
>> +#ifdef CONFIG_DISK_INTF_PLATFORM_IDE
> why the ifdef?
i discussed that in an email last night; it is a convention I use to prevent badly behaving header files from breaking compiles.

>> secondary_ide_device = {
>> +    .name = "ide_intf",
>> +    .id = 1,
>> +    .platform_data = &ide_plat,
>> +    .resource = secondary_ide_resources,
>> +    .num_resources = ARRAY_SIZE( secondary_ide_resources ),
>> +};
> please create the device on the fly


Again I'm following the style of the original driver; the serial ports in the generic_pc.c behave identically. 

Seems to me that these files were due for an overhaul, given the number of comments I get by following the style of the original driver...

Ive found the perl script that checks patches, next version of the patches will have spacing etc fixed as well, i want to test that everything works first.

Michel Stam


> On 5 apr. 2014, at 05:11, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> 
>> On 12:12 Tue 25 Mar     , Michel Stam wrote:
>> 
>> ---
>> arch/x86/boards/x86_generic/generic_pc.c | 71 ++++++++++++++++++++++++
>> drivers/ata/ide-sff.c                    | 94
>> ++++++++++++++++++++++++++------
>> drivers/ata/intf_platform_ide.c          | 14 ++++-
>> include/ata_drive.h                      |  1 +
>> 4 files changed, 161 insertions(+), 19 deletions(-)
>> 
>> diff --git a/arch/x86/boards/x86_generic/generic_pc.c
>> b/arch/x86/boards/x86_generic/generic_pc.c
>> index 74a7224..6152afc 100644
>> --- a/arch/x86/boards/x86_generic/generic_pc.c
>> +++ b/arch/x86/boards/x86_generic/generic_pc.c
>> @@ -27,6 +27,10 @@
>> #include <ns16550.h>
>> #include <linux/err.h>
>> +#ifdef CONFIG_DISK_INTF_PLATFORM_IDE
> why the ifdef?
>> +#include <platform_ide.h>
>> +#endif
>> +
>> /*
>>  * These datas are from the MBR, created by the linker and filled by the
>>  * setup tool while installing barebox on the disk drive
>> @@ -41,6 +45,55 @@ extern uint8_t pers_env_drive;
>>  */
>> #define PATCH_AREA_PERS_SIZE_UNUSED 0x000
>> +#ifdef CONFIG_DISK_INTF_PLATFORM_IDE
>> +
>> +static struct ide_port_info ide_plat = {
>> +    .ioport_shift = 0,
>> +    .dataif_be = 0,
>> +};
>> +
>> +static struct resource primary_ide_resources[] = {
>> +    {
>> +        .name = "base",
>> +        .start = 0x1f0,
>> +        .end =  0x1f8,
>> +        .flags = IORESOURCE_IO
>> +    },
>> +    {
>> +        .name = "alt",
>> +        .start = 0x3f6,
>> +    w    .end =  0x3f8,
>> +        .flags = IORESOURCE_IO
>> +    }
>> +};
>> +
>> +static struct resource secondary_ide_resources[] = {
>> +    {
>> +        .name = "base",
>> +        .start = 0x170,
>> +        .end =  0x177,
>> +        .flags = IORESOURCE_IO
>> +    },
>> +};
>> +
>> +static struct device_d primary_ide_device = {
>> +    .name = "ide_intf",
>> +    .id = 0,
>> +    .platform_data = &ide_plat,
>> +    .resource = primary_ide_resources,
>> +    .num_resources = ARRAY_SIZE( primary_ide_resources ),
>> +};
>> +
>> +static struct device_d secondary_ide_device = {
>> +    .name = "ide_intf",
>> +    .id = 1,
>> +    .platform_data = &ide_plat,
>> +    .resource = secondary_ide_resources,
>> +    .num_resources = ARRAY_SIZE( secondary_ide_resources ),
>> +};
> please create the device on the fly
>> +
>> +#endif /* CONFIG_DISK_INTF_PLATFORM_IDE */
>> +
>> static int devices_init(void)
>> {
>>    struct cdev *cdev;
>> @@ -48,9 +101,26 @@ static int devices_init(void)
>>    /* extended memory only */
>>    add_mem_device("ram0", 0x0, bios_get_memsize() << 10,
>>               IORESOURCE_MEM_WRITEABLE);
>> +#ifdef CONFIG_DISK_BIOS
> if (IS_ENABLED(CONFxx))
>>    add_generic_device("biosdrive", DEVICE_ID_DYNAMIC, NULL, 0, 0,
>> IORESOURCE_MEM,
>>            NULL);
>> +#endif
>> +
>> +#ifdef CONFIG_DISK_INTF_PLATFORM_IDE
> ditto and no space after ( and before )
>> +    platform_device_register( &primary_ide_device );
>> +    platform_device_register( &secondary_ide_device );
>> +
>> +    if (pers_env_size != PATCH_AREA_PERS_SIZE_UNUSED) {
>> +        cdev = devfs_add_partition("ata0",
>> +                pers_env_storage * 512,
>> +                (unsigned)pers_env_size * 512,
>> +                DEVFS_PARTITION_FIXED, "env0");
>> +        printf("Partition: %ld\n", IS_ERR(cdev) ? PTR_ERR(cdev) : 0);
>> +    } else
>> +        printf("No persistent storage defined\n");
>> +#endif /* CONFIG_DISK_INTF_PLATFORM_IDE */
>> +#ifdef CONFIG_DISK_BIOS
>>    if (pers_env_size != PATCH_AREA_PERS_SIZE_UNUSED) {
>>        cdev = devfs_add_partition("biosdisk0",
>>                pers_env_storage * 512,
>> @@ -59,6 +129,7 @@ static int devices_init(void)
>>        printf("Partition: %ld\n", IS_ERR(cdev) ? PTR_ERR(cdev) : 0);
>>    } else
>>        printf("No persistent storage defined\n");
>> +#endif
>>          return 0;
>> }
>> diff --git a/drivers/ata/ide-sff.c b/drivers/ata/ide-sff.c
>> index 3d5932e..809a129 100644
>> --- a/drivers/ata/ide-sff.c
>> +++ b/drivers/ata/ide-sff.c
>> @@ -15,13 +15,71 @@
>> #define DISK_SLAVE 1
>>  /**
>> + * Read a byte from the ATA controller
>> + * @param ide IDE port structure
>> + * @param addr Register adress
>> + * @return Register's content
>> + */
>> +static inline uint8_t ata_rd_byte(struct ide_port *ide, void
>> __iomem *addr )
>> +{
>> +    if (!ide->io.mmio)
>> +        return (uint8_t) inb((int) addr);
>> +    else
>> +        return readb(addr);
>> +}
>> +
>> +/**
>> + * Write a byte to the ATA controller
>> + * @param ide IDE port structure
>> + * @param value Value to write
>> + * @param addr Register adress
>> + * @return Register's content
>> + */
>> +static inline void ata_wr_byte(struct ide_port *ide, uint8_t value,
>> void __iomem *addr )
>> +{
>> +    if (!ide->io.mmio)
>> +        outb(value, (int) addr);
>> +    else
>> +        writeb(value,addr);
>> +}
>> +
>> +/**
>> + * Read a word from the ATA controller
>> + * @param ide IDE port structure
>> + * @param addr Register adress
>> + * @return Register's content
>> + */
>> +static inline uint16_t ata_rd_word(struct ide_port *ide, void
>> __iomem *addr )
>> +{
>> +    if (!ide->io.mmio)
>> +        return (uint16_t) inw((int) addr);
>> +    else
>> +        return readw(addr);
>> +}
>> +
>> +/**
>> + * Write a word to the ATA controller
>> + * @param ide IDE port structure
>> + * @param value Value to write
>> + * @param addr Register adress
>> + * @return Register's content
>> + */
>> +static inline void ata_wr_word(struct ide_port *ide, uint16_t
>> value, void __iomem *addr )
>> +{
>> +    if (!ide->io.mmio)
>> +        outw(value, (int) addr);
>> +    else
>> +        writew(value,addr);
>> +}
>> +
>> +/**
>>  * Read the status register of the ATA drive
>>  * @param io Register file
>>  * @return Register's content
>>  */
>> static uint8_t ata_rd_status(struct ide_port *ide)
>> {
>> -    return readb(ide->io.status_addr);
>> +    return ata_rd_byte(ide,ide->io.status_addr);
>> }
>>  /**
>> @@ -83,12 +141,12 @@ static int ata_set_lba_sector(struct ide_port
>> *ide, unsigned drive, uint64_t num
>>    if (num > 0x0FFFFFFF || drive > 1)
>>        return -EINVAL;
>> -    writeb(0xA0 | LBA_FLAG | drive << 4 | num >> 24, ide->io.device_addr);
>> -    writeb(0x00, ide->io.error_addr);
>> -    writeb(0x01, ide->io.nsect_addr);
>> -    writeb(num, ide->io.lbal_addr);    /* 0 ... 7 */
>> -    writeb(num >> 8, ide->io.lbam_addr); /* 8 ... 15 */
>> -    writeb(num >> 16, ide->io.lbah_addr); /* 16 ... 23 */
>> +    ata_wr_byte(ide, 0xA0 | LBA_FLAG | drive << 4 | num >> 24,
>> ide->io.device_addr);
>> +    ata_wr_byte(ide, 0x00, ide->io.error_addr);
>> +    ata_wr_byte(ide, 0x01, ide->io.nsect_addr);
>> +    ata_wr_byte(ide, num, ide->io.lbal_addr);    /* 0 ... 7 */
>> +    ata_wr_byte(ide, num >> 8, ide->io.lbam_addr); /* 8 ... 15 */
>> +    ata_wr_byte(ide, num >> 16, ide->io.lbah_addr); /* 16 ... 23 */
>>      return 0;
>> }
>> @@ -107,7 +165,7 @@ static int ata_wr_cmd(struct ide_port *ide, uint8_t cmd)
>>    if (rc != 0)
>>        return rc;
>> -    writeb(cmd, ide->io.command_addr);
>> +    ata_wr_byte(ide, cmd, ide->io.command_addr);
>>    return 0;
>> }
>> @@ -118,7 +176,7 @@ static int ata_wr_cmd(struct ide_port *ide,
>> uint8_t cmd)
>>  */
>> static void ata_wr_dev_ctrl(struct ide_port *ide, uint8_t val)
>> {
>> -    writeb(val, ide->io.ctl_addr);
>> +    ata_wr_byte(ide, val, ide->io.ctl_addr);
>> }
>>  /**
>> @@ -133,10 +191,10 @@ static void ata_rd_sector(struct ide_port
>> *ide, void *buf)
>>      if (ide->io.dataif_be) {
>>        for (; u > 0; u--)
>> -            *b++ = be16_to_cpu(readw(ide->io.data_addr));
>> +            *b++ = be16_to_cpu(ata_rd_word(ide, ide->io.data_addr));
>>    } else {
>>        for (; u > 0; u--)
>> -            *b++ = le16_to_cpu(readw(ide->io.data_addr));
>> +            *b++ = le16_to_cpu(ata_rd_word(ide, ide->io.data_addr));
>>    }
>> }
>> @@ -152,10 +210,10 @@ static void ata_wr_sector(struct ide_port
>> *ide, const void *buf)
>>      if (ide->io.dataif_be) {
>>        for (; u > 0; u--)
>> -            writew(cpu_to_be16(*b++), ide->io.data_addr);
>> +            ata_wr_word(ide, cpu_to_be16(*b++), ide->io.data_addr);
>>    } else {
>>        for (; u > 0; u--)
>> -            writew(cpu_to_le16(*b++), ide->io.data_addr);
>> +            ata_wr_word(ide, cpu_to_le16(*b++), ide->io.data_addr);
>>    }
>> }
>> @@ -169,10 +227,10 @@ static int ide_read_id(struct ata_port *port,
>> void *buf)
>>    struct ide_port *ide = to_ata_drive_access(port);
>>    int rc;
>> -    writeb(0xA0, ide->io.device_addr);    /* FIXME drive */
>> -    writeb(0x00, ide->io.lbal_addr);
>> -    writeb(0x00, ide->io.lbam_addr);
>> -    writeb(0x00, ide->io.lbah_addr);
>> +    ata_wr_byte(ide, 0xA0, ide->io.device_addr);    /* FIXME drive */
>> +    ata_wr_byte(ide, 0x00, ide->io.lbal_addr);
>> +    ata_wr_byte(ide, 0x00, ide->io.lbam_addr);
>> +    ata_wr_byte(ide, 0x00, ide->io.lbah_addr);
>>      rc = ata_wr_cmd(ide, ATA_CMD_ID_ATA);
>>    if (rc != 0)
>> @@ -327,6 +385,8 @@ int ide_port_register(struct ide_port *ide)
>>    ide->port.ops = &ide_ops;
>>      ret = ata_port_register(&ide->port);
>> +    if ( !ret )
>> +        ata_port_detect(&ide->port);
>>      if (ret)
>>        free(ide);
>> diff --git a/drivers/ata/intf_platform_ide.c
>> b/drivers/ata/intf_platform_ide.c
>> index 8ae0f05..24f78f4 100644
>> --- a/drivers/ata/intf_platform_ide.c
>> +++ b/drivers/ata/intf_platform_ide.c
>> @@ -89,8 +89,18 @@ static int platform_ide_probe(struct device_d *dev)
>>    }
>>      ide = xzalloc(sizeof(*ide));
>> -    reg_base = dev_request_mem_region(dev, 0);
>> -    alt_base = dev_request_mem_region(dev, 1);
>> +    reg_base = dev_request_region(dev, 0, IORESOURCE_MEM);
> please check your coding style
>> +    if (reg_base != NULL)
>> +    {
>> +        alt_base = dev_request_region(dev, 1, IORESOURCE_MEM);
>> +        ide->io.mmio = 1;
>> +    }
>> +    else
>> +    {
>> +        reg_base = dev_request_region(dev, 0, IORESOURCE_IO);
>> +        alt_base = dev_request_region(dev, 1, IORESOURCE_IO);
>> +    }
>> +
>>    platform_ide_setup_port(reg_base, alt_base, &ide->io,
>> pdata->ioport_shift);
>>    ide->io.reset = pdata->reset;
>>    ide->io.dataif_be = pdata->dataif_be;
>> diff --git a/include/ata_drive.h b/include/ata_drive.h
>> index 6d6cca4..44073cb 100644
>> --- a/include/ata_drive.h
>> +++ b/include/ata_drive.h
>> @@ -119,6 +119,7 @@ struct ata_ioports {
>>    /* hard reset line handling */
>>    void (*reset)(int);    /* true: assert reset, false: de-assert reset */
>>    int dataif_be;    /* true if 16 bit data register is big endian */
>> +    int mmio; /* true if memory-mapped io */
> so use a boolean
>> };
>>  struct ata_port;
>> -- 
>> 1.8.4
>> 
>> 
>> _______________________________________________
>> barebox mailing list
>> barebox@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/barebox

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 6165 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox