[flashrom] [PATCH] Add mediatek_pata programmer
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Mon Mar 7 02:06:36 CET 2011
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/
More information about the flashrom
mailing list