[flashrom] [PATCH] Add mediatek_pata programmer

[Carl-Daniel Hailfinger]

Auf 07.03.2011 02:42, Michael Karcher schrieb:
> Hello Carl-Daniel,
>
>   
>>> +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?
>>     
> libsysfs-devel or the like is also installed? If yes, we need the
> fallback.
>   

On opensuse 11.3, all needed files are part of the sysfsutils package
(headers + lib).


>>> +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?
>>     
> Probably we need to. The sysfs code (for kicking the linux driver and
> reenabling it) might even be made optional.
>
>   
>> 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)?
>>     
> Didn't look at it yet, but auto-setting to "no" is more user friendly
> than requiring some magic option.
>   

Hm. You're right, autodetection is more user friendly.


>>> +#include <libsysfs.h>
>>>       
>> sysfs/libsysfs.h instead? Not sure where it lives on Debian.
>>     
> No, not <sysfs/libsysfs.h>. The file resides in /usr/include/sysfs, but
> that is passed to the "-I" option, so just <libsysfs.h> needs to be
> specified here.
>   

OK.


>>> +struct ataintf {
>>>       
>> A comment what "intf" means would be appreciated.
>>     
> "interface". OK, I might make a comment.
>   

Thanks.


>>> +		outb(command[i], ata->portbase+i);
>>>       
>> OUTB please, otherwise non-Linux platforms will fail.
>>     
> Good point.
>
>   
>>> +	outb(command[0], ata->portbase+7);
>>> +	programmer_delay(10000);
>>>       
>> A comment explaining the delay would be nice.
>>     
> [..]
>   
>> INB
>> Magic numbers. Please use #defines instead where possible.
>>     
> Yep, will change that.
>
>   
>>> +{
>>> +	ata->portbase = portbase;
>>> +	if (is_slave)
>>> +		ata->slavebit = 0x10;
>>>       
>> #define for 0x10 please.
>>     
> OK, that looks so obvious to IDE-knowing people that you don't think
> about naming it, but you are right.
>
>
>   
>>> +#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.
>>     
> I even suspect that readability is improved by using this small amount
> of lowercase letters, just like a small amount of uppercase letters can
> improve readability of variable names. If there is a strict "all defines
> pure uppercase" policy, I will reconsider the names, otherwise, I prefer
> to keep them as is.
>   

Keep them. There is no hard policy AFAIK, and even if there was one,
code readability wins.


>>> +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?
>>     
> If I were using the Linux ATA command submission function, you might
> raise a valid point, but "ide_submit" is the direct I/O function some
> lines above. In fact, this is the only command which could be sent
> through the linux interface, as it generates an IRQ. But most likely
> only if the drive currently has a valid firmware. If the drive has no
> valid firmware, the linux command layer can't be used anyway.
>   

Ah OK.


>>> +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?
>>     
> For IDE, alignment is 8.
>
>   
>>> +		    (*endptr != '\0')) {
>>>       
>> And dereference portpos here. Boom. (If this is copy-pasted, this means
>> we access freed memory in another driver as well.)
>>     
> The freeing error is my fault. I felt clever moving the free to the top
> to not have it in the error case as well as in the success case.
> Obviously I outsmarted myself. Excellent catch.
>   

Thanks!


>>> +		slaveptr = extract_programmer_param("slave");
>>>       
>> Mh, so anything starting with slave= will match?
>>     
> The intention was that the pure mentioning of "slave" as option will
> match (a boolean option toggled by presence), but it turned out that
> "slave=" will be needed, so I will change that to
> "drive=master"/"drive=slave".
>   

Looks good.


>>> +	if (mtk_open(iobase, isslave, &mtk_intf) < 0)
>>> +	{
>>> +		msg_perr("can't access that device, aborting\n");
>>> +//		return -1;
>>>       
>> Why is this commented out?
>>     
> Because that's a leftover from a test. mtk_open currently never fails,
> and I am considering merging it with mtk_open_flash. Previously, it
> checked for a valid ATAPI device on that port. But that check fails if
> there is no valid firmware, so I disabled it.
>   

It might be interesting to print a diagnostic message in that case, e.g.
"It seems this drive does not have a valid flash image matching the
drive. Continuing anyway."
Your choice, though.


>> 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.
>>     
> Yeah, that's a good idea. Maybe that code profits from some comments
> telling the expected position in the device tree.
>
> Thank you for the review. I will send an improved patch soon.
>   

Thank you for reverse engineering and coding this patch!

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/