Auf 07.03.2011 01:05, Michael Karcher schrieb:
> Add support for flashing using CD/DVD drives with mediatek chipsets.
>
> As the driver is now, it only supports parallel ATA drives with parallel
> flash chips. The kind of drive access needed is not possible through IDE
> drivers in the OS, flashrom has to access the IDE port directly. This will
> interfere with OS drivers for the drive, so you better make sure that
> the OS does not access the drive (or another drive on the same PATA channel)
> during the flash process.
>
> On recent linux kernels (with libata), the invocation
> "-p mediatek_pata:devname=sr0" should work. It is supposed to forcibly
> unregister all drives on the IDE channel sr0 is on (you better don't have
> a file system mounted on a hard drive on the same channel), perform the
> flash job, and, if the drive successfully reboots, re-add all detectable
> IDE devices on that channel.
>
> This patch is known to lack a man page section and violate the flashrom
> coding style in some places - I send it out to catch some early reviews
> on it.
>
> Signed-off-by: Michael Karcher <[email protected]>
>
Nice! Thanks for your patch. Short review follows.
> diff --git a/Makefile b/Makefile
> index 6028485..a38740f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -158,6 +158,9 @@ CONFIG_DEDIPROG ?= no
> # Always enable Marvell SATA controllers for now.
> CONFIG_SATAMV ?= yes
>
> +# Disable Mediatek PATA programmer until cleaned up
> +CONFIG_MEDIATEK_PATA ?= no
> +
> # Disable wiki printing by default. It is only useful if you have wiki
> access.
> CONFIG_PRINT_WIKI ?= no
>
> @@ -292,6 +295,17 @@ PROGRAMMER_OBJS += satamv.o
> NEED_PCI := yes
> endif
>
> +ifeq ($(CONFIG_MEDIATEK_PATA), yes)
> +FEATURE_CFLAGS += -D'CONFIG_MEDIATEK_PATA=1'
> +PROGRAMMER_OBJS += mediatek.o
> +# actually, I/O access, not PCI
> +NEED_PCI := yes
> +ifeq ($(OS_ARCH), Linux)
> +LIBS += $(shell pkg-config --libs libsysfs)
>
"pkg-config --libs libsysfs" returns an error on my opensuse 11.3
machine, but libsysfs is installed. This is caused by a missing
libsysfs.pc file. Could you fall back to -lsysfs in that case?
> +FEATURE_CFLAGS += $(shell pkg-config --cflags libsysfs)
>
Same problem here. Fallback: -I/usr/include/sysfs
Besides that, do we want to test for libsysfs like we test for libpci?
Related side note: Should we finally kill the overly complicated libftdi
detection code, enable it always, and do a libpci-style detection
instead (with a slightly modified error message which suggests to set
CONFIG_FT2232_SPI=no to avoid the compile error)?
> +endif
> +endif
> +
> ifeq ($(NEED_SERIAL), yes)
> LIB_OBJS += serial.o
> endif
> diff --git a/flashrom.c b/flashrom.c
> index 4c6c1fa..05c0a0b 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -52,7 +52,7 @@ enum programmer programmer = PROGRAMMER_DUMMY;
> * if more than one of them is selected. If only one is selected, it is clear
> * that the user wants that one to become the default.
> */
> -#if
> CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_FT2232_SPI+CONFIG_SERPROG+CONFIG_BUSPIRATE_SPI+CONFIG_DEDIPROG+CONFIG_RAYER_SPI+CONFIG_NICINTEL_SPI+CONFIG_OGP_SPI+CONFIG_SATAMV
> > 1
> +#if
> CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_FT2232_SPI+CONFIG_SERPROG+CONFIG_BUSPIRATE_SPI+CONFIG_DEDIPROG+CONFIG_RAYER_SPI+CONFIG_NICINTEL_SPI+CONFIG_OGP_SPI+CONFIG_SATAMV+CONFIG_MEDIATEK_PATA
> > 1
> #error Please enable either CONFIG_DUMMY or CONFIG_INTERNAL or disable
> support for all programmers except one.
> #endif
> enum programmer programmer =
> @@ -102,6 +102,9 @@ enum programmer programmer =
> #if CONFIG_SATAMV == 1
> PROGRAMMER_SATAMV
> #endif
> +#if CONFIG_MEDIATEK_PATA == 1
> + PROGRAMMER_MEDIATEK_PATA
> +#endif
> ;
> #endif
>
> @@ -483,6 +486,25 @@ const struct programmer_entry programmer_table[] = {
> },
> #endif
>
> +#if CONFIG_MEDIATEK_PATA == 1
> + {
> + .name = "mediatek_pata",
> + .init = mediatek_pata_init,
> + .shutdown = mediatek_pata_shutdown,
> + .map_flash_region = fallback_map,
> + .unmap_flash_region = fallback_unmap,
> + .chip_readb = mediatek_pata_chip_readb,
> + .chip_readw = fallback_chip_readw,
> + .chip_readl = fallback_chip_readl,
> + .chip_readn = fallback_chip_readn,
> + .chip_writeb = mediatek_pata_chip_writeb,
> + .chip_writew = fallback_chip_writew,
> + .chip_writel = fallback_chip_writel,
> + .chip_writen = fallback_chip_writen,
> + .delay = internal_delay,
> + },
> +#endif
> +
> {}, /* This entry corresponds to PROGRAMMER_INVALID. */
> };
>
> diff --git a/mediatek.c b/mediatek.c
> new file mode 100644
> index 0000000..e44a261
> --- /dev/null
> +++ b/mediatek.c
> @@ -0,0 +1,350 @@
> +/*
> + * This file is part of the flashrom project.
> + *
> + * Copyright (C) 2011 Michael Karcher
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/* driver for PATA connected CD/DVD drives with mediatek chipset */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include "flash.h"
> +#include "programmer.h"
> +
> +#ifdef __linux__
> +#include <libsysfs.h>
>
sysfs/libsysfs.h instead? Not sure where it lives on Debian.
> +
> +static int grab_device(const char *devname, unsigned * portnum, int *
> isslave, char * rescanpath)
> +{
> + struct sysfs_class *blockclass;
> + struct sysfs_class_device *cdclassdevice;
> + struct sysfs_device *cddevice, *targetdevice, *hostdevice,
> *atacontroller;
> + struct dlist *targets, *hostadapters, *hosts;
> + const char *subtarget, *haname;
> + const char *resourcelist;
> + int devhostnum, hostnum;
> + int hostbus = 0, i;
> + blockclass = sysfs_open_class("block");
> + cdclassdevice = sysfs_get_class_device(blockclass,devname);
> + if (!cdclassdevice)
> + {
> + fputs("Device not found\n", stderr);
> + return -1;
> + }
> + cddevice = sysfs_get_classdev_device(cdclassdevice);
> + /* should be only 0 or 1 */
> + if (sscanf(cddevice->name,"%*d:%*d:%d",isslave) != 1)
> + {
> + fputs("can't parse device ID\n", stderr);
> + return -1;
> + }
> + targetdevice = sysfs_get_device_parent(cddevice);
> + hostdevice = sysfs_get_device_parent(targetdevice);
> + sprintf(rescanpath,"%s/scsi_host",hostdevice->path);
> + hostadapters = sysfs_open_directory_list(rescanpath);
> + dlist_for_each_data(hostadapters, haname, const char) break;
> + sprintf(rescanpath,"%s/scsi_host/%s/scan",hostdevice->path,haname);
> + sysfs_close_list(hostadapters);
> + targets = sysfs_open_directory_list(hostdevice->path);
> + dlist_for_each_data (targets, subtarget, const char) {
> + /* handle all targets on that bus */
> + if (strncmp(subtarget,"target",6) == 0)
> + {
> + struct dlist * luns;
> + const char * lundevname;
> + char lunspath[SYSFS_PATH_MAX],
> deldevpath[SYSFS_PATH_MAX];
> + sprintf(lunspath, "%s/%s", hostdevice->path, subtarget);
> + /* handle all (i.e. one) LUN on that target */
> + luns = sysfs_open_directory_list(lunspath);
> + dlist_for_each_data (luns, lundevname, const char) {
> + /* handle children, not the "power" subdir */
> + if (strchr(lundevname,':'))
> + {
> + FILE * f;
> + sprintf(deldevpath, "%s/%s/delete",
> lunspath, lundevname);
> + f = fopen(deldevpath,"w");
> + if (!f)
> + {
> + fprintf(stderr, "Can't open %s
> for grabbing device\n", deldevpath);
> + return -1;
> + }
> + fputs("1\n", f);
> + fclose(f);
> + }
> + }
> + sysfs_close_list(luns);
> + }
> + }
> + sysfs_close_list(targets);
> + if (sscanf(hostdevice->name, "host%d", &devhostnum) != 1)
> + {
> + fputs("Can't parse host number\n",stderr);
> + return -1;
> + }
> + atacontroller = sysfs_get_device_parent(hostdevice);
> + hosts = sysfs_open_directory_list(atacontroller->path);
> + dlist_for_each_data (hosts, haname, const char) {
> + if (sscanf(haname, "host%d", &hostnum) == 1 &&
> + hostnum < devhostnum)
> + hostbus++;
> + }
> + sysfs_close_list(hosts);
> + resourcelist = sysfs_get_device_attr(atacontroller, "resource")->value;
> + for (i = 0; i < 2*hostbus; i++)
> + resourcelist = strchr(resourcelist, '\n') + 1;
> + if (sscanf(resourcelist,"%i",portnum) != 1)
> + {
> + fputs("Can't parse port number\n", stderr);
> + return 1;
> + }
> + sysfs_close_class(blockclass);
> + return 0;
> +}
> +
> +static void release_device(void *rescanpath)
> +{
> + FILE * f;
> + f = fopen((const char*)rescanpath, "w");
> + if (!f)
> + {
> + fputs("Can't open rescanning metafile\n", stderr);
> + return;
> + }
> + fputs("- - -\n",f);
> + fclose(f);
> +}
> +
> +#endif
> +
> +struct ataintf {
>
A comment what "intf" means would be appreciated.
> + unsigned portbase;
> + int slavebit;
> +} mtk_intf;
> +
> +static int ide_submit(struct ataintf *ata, unsigned char *command)
> +{
> + int i, status, ret = 0;
> + command[6] &= ~0x10;
> + command[6] |= ata->slavebit;
> + for (i = 1; i < 7; i++)
> + outb(command[i], ata->portbase+i);
>
OUTB please, otherwise non-Linux platforms will fail.
> + outb(command[0], ata->portbase+7);
> + programmer_delay(10000);
>
A comment explaining the delay would be nice.
> + i = 0;
> + while ((status = inb(ata->portbase+7)) & 0x80 && ++i < 200)
>
INB
Magic numbers. Please use #defines instead where possible.
> + programmer_delay(10000);
> + if (i == 200)
> + ret = -1;
> + command[0] = status;
> + for (i = 1; i < 7; i++)
> + command[i] = inb(ata->portbase + i);
> + return ret;
> +}
> +
> +static int mtk_open(unsigned portbase, int is_slave, struct ataintf * ata)
>
Whitespace after *.
> +{
> + ata->portbase = portbase;
> + if (is_slave)
> + ata->slavebit = 0x10;
>
#define for 0x10 please.
> + else
> + ata->slavebit = 0;
> + return 0;
> +}
> +
> +#define MTK_A19toA17 1
> +#define MTK_CONTROL 2
> +#define MTK_DATA 3
> +#define MTK_A7toA0 4
> +#define MTK_A15toA8 5
> +
> +#define MTK_CTL_RAISE_nWE 0x01
> +#define MTK_CTL_LOWER_nWE 0x02
> +#define MTK_CTL_RAISE_nOE 0x04
> +#define MTK_CTL_LOWER_nOE 0x08
> +#define MTK_CTL_RAISE_nCS 0x10
> +#define MTK_CTL_LOWER_nCS 0x20 /* latches low 8 address bits */
> +#define MTK_CTL_DRIVE_DATA 0x40
> +#define MTK_CTL_ADDRESS_A16 0x80
>
Some #defines have lowercase chars, but I don't really think that
readability would benefit from making them uppercase. Hm.
> +
> +static int mtk_open_flash(struct ataintf * ata)
> +{
> + unsigned char task[7];
> + task[0] = 0x80; /* Vendor specific */
> + task[3] = 0x2A; /* mediatek: start flash access */
> + ide_submit(ata, task);
>
I think Linux once supported ATA command filters for security reasons.
Do we have to catch something like that, and does that security feature
even impact us?
> + msg_pdbg("status: %x\n", task[0]);
> + if ((task[0] & 0xFC) != 0x70) {
> + msg_perr("Mediatek flash mode signature didn't appear. Most
> likely "
> + "unsupported device\n");
> + return 1;
> + }
> + if (task[0] == 0x70) /* parallel flash */
> + {
> + outb(MTK_CTL_RAISE_nCS | MTK_CTL_RAISE_nOE | MTK_CTL_RAISE_nWE,
> ata->portbase + MTK_CONTROL);
> + return 0;
> + }
> + else
> + {
> + msg_perr("Signature for parallel flash not found, maybe SPI
> flash?\n");
> + return 1;
> + }
> +}
> +
> +static int mtk_close_flash(struct ataintf * ata)
> +{
> + unsigned char task[8];
> + task[0] = 0x81; /* Vendor specific */
> + task[2] = 8;
> + task[3] = 0;
> + if (ide_submit(ata, task) == -1)
> + {
> + msg_perr("error rebooting CD drive\n");
> + return -1;
> + }
> + return 0;
> +}
> +
> +static unsigned char mtk_readb(struct ataintf * ata, unsigned address)
> +{
> + unsigned char val;
> + unsigned char A16bit = address & 0x10000 ? MTK_CTL_ADDRESS_A16 : 0;
> + outb(A16bit, ata->portbase + MTK_CONTROL);
> + outb(address >> 17, ata->portbase + MTK_A19toA17);
> + outb(address >> 8, ata->portbase + MTK_A15toA8);
> + outb(address >> 0, ata->portbase + MTK_A7toA0);
> + outb(MTK_CTL_LOWER_nCS | MTK_CTL_LOWER_nOE | A16bit, ata->portbase +
> MTK_CONTROL);
> + val = inb(ata->portbase + MTK_DATA);
> + outb(MTK_CTL_RAISE_nCS | MTK_CTL_RAISE_nOE, ata->portbase +
> MTK_CONTROL);
> + return val;
> +}
> +
> +static void mtk_writeb(struct ataintf * ata, unsigned char val, unsigned
> address)
> +{
> + unsigned char A16bit = address & 0x10000 ? MTK_CTL_ADDRESS_A16 : 0;
> + outb(A16bit, ata->portbase + MTK_CONTROL);
> + outb(address >> 17, ata->portbase + MTK_A19toA17);
> + outb(address >> 8, ata->portbase + MTK_A15toA8);
> + outb(address >> 0, ata->portbase + MTK_A7toA0);
> + outb(val, ata->portbase + MTK_DATA);
> + outb(MTK_CTL_LOWER_nCS | MTK_CTL_DRIVE_DATA | A16bit, ata->portbase +
> MTK_CONTROL);
> + outb(MTK_CTL_LOWER_nWE | MTK_CTL_DRIVE_DATA | A16bit, ata->portbase +
> MTK_CONTROL);
> + outb(MTK_CTL_RAISE_nCS | MTK_CTL_RAISE_nWE, ata->portbase +
> MTK_CONTROL);
> +}
> +
> +#ifdef __linux__
> +static char rescanpath[SYSFS_PATH_MAX] = "";
> +#endif
> +
> +int mediatek_pata_init(void)
> +{
> + unsigned iobase;
> + int isslave;
> + char *portpos = NULL, *devpos = NULL;
> +
> + /* IDE port specified */
> + if ((portpos = extract_programmer_param("iobase"))) {
> + char *endptr = NULL;
> + char *slaveptr;
> + unsigned long tmp;
> + tmp = strtoul(portpos, &endptr, 0);
> + free(portpos);
>
We free portpos here.
> + /* Port 0, port >0x10000, unaligned ports and garbage strings
> + * are rejected.
> + */
> + if (!tmp || (tmp >= 0x10000) || (tmp & 0x7) ||
>
Is the alignment really 0x8, or something different?
> + (*endptr != '\0')) {
>
And dereference portpos here. Boom. (If this is copy-pasted, this means
we access freed memory in another driver as well.)
> + /* Using ports below 0x100 is a really bad idea, and
> + * should only be done if no port between 0x100 and
> + * 0xfffc works due to routing issues.
> + */
> + msg_perr("Error: iobase= specified, but the I/O base "
> + "given was invalid.\nIt must be a multiple of "
> + "0x8 and lie between 0x100 and 0xfff8.\n");
> + return 1;
> + }
> + iobase = tmp;
> + slaveptr = extract_programmer_param("slave");
>
Mh, so anything starting with slave= will match?
> + isslave = slaveptr != NULL;
> + free(slaveptr);
> + } else if ((devpos = extract_programmer_param("devname"))) {
> +#ifdef __linux__
> + if(grab_device(devpos, &iobase, &isslave, rescanpath) == 0)
> + {
> + msg_pinfo("device grabbed: %s at port %x\n",
> + isslave?"slave":"master", iobase);
> + }
> + else
> + {
> + msg_perr("can't get access to device %s\n", devpos);
> + return -1;
> + }
> +#else
> + msg_perr("Open by devname is not implemented on this OS yet\n");
> + return -1;
> +#endif
> + } else {
> + msg_perr("Need to specify either iobase or devname for the "
> + "mediatek_pata driver\n");
> + return -1;
> + }
> +
> + get_io_perms();
> + if (mtk_open(iobase, isslave, &mtk_intf) < 0)
> + {
> + msg_perr("can't access that device, aborting\n");
> +// return -1;
>
Why is this commented out?
> + }
> + if (mtk_open_flash(&mtk_intf) < 0)
> + {
> + msg_perr("can't get access to flash chip, aborting\n");
> + return -1;
> + }
> + return 0;
> +}
> +
> +void mediatek_pata_chip_writeb(uint8_t val, chipaddr addr)
> +{
> + mtk_writeb(&mtk_intf, val, addr);
> +}
> +
> +uint8_t mediatek_pata_chip_readb(const chipaddr addr)
> +{
> + return mtk_readb(&mtk_intf, addr);
> +}
> +
> +int mediatek_pata_shutdown(void)
> +{
> + int res;
> + res = mtk_close_flash(&mtk_intf);
> +#ifdef __linux__
> + if (rescanpath[0]) {
> + if (res >= 0) /* don't rescan if the drive is broken now */
> + release_device(rescanpath);
> + else {
> + msg_perr("probably bad flash contents, won't"
> + " rescan ATA bus\n");
> + msg_perr("use \"echo - - - > %s\" to rescan after"
> + " fixing\n",rescanpath);
> + msg_perr("use \"-p mediatek_pata:iobase=0x%x%s\" until"
> + " the drive is working again\n",
> + mtk_intf.portbase,
> + mtk_intf.slavebit ? ":slave" : "");
> + }
> + }
> + rescanpath[0] = 0;
> +#endif
> + return 0;
> +}
> diff --git a/programmer.h b/programmer.h
> index 7a497f8..b62b86f 100644
> --- a/programmer.h
> +++ b/programmer.h
> @@ -82,6 +82,9 @@ enum programmer {
> #if CONFIG_SATAMV == 1
> PROGRAMMER_SATAMV,
> #endif
> +#if CONFIG_MEDIATEK_PATA == 1
> + PROGRAMMER_MEDIATEK_PATA,
> +#endif
> PROGRAMMER_INVALID /* This must always be the last entry. */
> };
>
> @@ -516,6 +519,11 @@ char *extract_programmer_param(char *param_name);
> /* layout.c */
> int show_id(uint8_t *bios, int size, int force);
>
> +int mediatek_pata_init(void);
> +int mediatek_pata_shutdown(void);
> +void mediatek_pata_chip_writeb(uint8_t val, chipaddr addr);
> +uint8_t mediatek_pata_chip_readb(const chipaddr addr);
> +
> /* spi.c */
> enum spi_controller {
> SPI_CONTROLLER_NONE,
>
I did not review any Linux-specific stuff, but the rest looks OK apart
from the coding style issues you mentioned.
Someone with more sysfs/libsysfs knowledge should take a look at the
sysfs related stuff.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
