Re: [flashrom] [PATCH] Add mediatek_pata programmer

[Carl-Daniel Hailfinger]

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

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