The attached patch enables flashing on the Iwill DK8-HTX board. Basically, it configures the SuperIO to set the right GPIO pins, so write protection is disabled.
Signed-off-by: Mondrian Nuessle nuessle@uni-mannheim.de ---
So, this is the second try :-) Only pci vendor id/device id is given, no subsystem id. Therefor, flashing when booted from factory BIOS will only work, if -m iwill:dk8_htx switch is given. I guess this could go in the wiki doc to this board...
Regards, Mondrian
On Thu, Apr 26, 2007 at 06:24:06PM +0200, Mondrian Nuessle wrote:
The attached patch enables flashing on the Iwill DK8-HTX board. Basically, it configures the SuperIO to set the right GPIO pins, so write protection is disabled.
Signed-off-by: Mondrian Nuessle nuessle@uni-mannheim.de
So, this is the second try :-) Only pci vendor id/device id is given, no subsystem id. Therefor, flashing when booted from factory BIOS will only work, if -m iwill:dk8_htx switch is given. I guess this could go in the wiki doc to this board...
Regards, Mondrian
Ok, first some whitespace gestapoing.
There are 3 trailing whitespaces on empty lines. You probably also want to review comments. There's one place (GPIO inversion reg) where you start the line with spaces instead of a tab. Also, get a space in after a ',' in your in/outbs (activate logical device).
Is it really necessary to write the index register twice on the w83627?
Also:
{ 0x1022, 0x7468, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,"iwill", "dk8_htx", "IWILL DK8-HTX", board_iwill_dk8htx },
There is a hard strcmp in the linuxbios name matching. For the agami aruma, the linuxbios name is in all caps, but here it is in small letters. Is this correct, as in, does this correspond with the linuxbios config? Should board_enable be using strcasecmp?
As for documenting, README should be touched as well. You might want to add the AGAMI:ARUMA.
This name matching is rather safe now, as you have to match at least one set of main pci-ids. This will stop joe simple from simply running down the list of board enables.
Luc Verhaegen.
On Thu, Apr 26, 2007 at 10:12:33PM +0200, Luc Verhaegen wrote:
Also:
{ 0x1022, 0x7468, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,"iwill", "dk8_htx", "IWILL DK8-HTX", board_iwill_dk8htx },There is a hard strcmp in the linuxbios name matching. For the agami aruma, the linuxbios name is in all caps, but here it is in small letters. Is this correct, as in, does this correspond with the linuxbios config? Should board_enable be using strcasecmp?
Good question. We should definately make sure to consistently use only _one_ of the variants. IMHO the one which is the most "correct", as in: the vendor uses that name in that exact variant (lower-/uppercase, hyphens, etc.)
Uwe.
The attached patch enables flashing on the Iwill DK8-HTX board. Basically, it configures the SuperIO to set the right GPIO pins, so write protection is disabled.
Signed-off-by: Mondrian Nuessle nuessle@uni-mannheim.de ---
Ok, this is the third try :-)
* I changed the name argument to "const char*" (as suggested by Uwe, applied that to all boards, of course) * I fixed indent, as specified by the coding styles, notice that this bloats the patch considerably, since a lot of existing code is touched * fixed several typos, comments etc * I tested this patch on real hardware, both booted by factory BIOS and LinuxBIOS * regarding the naming of the boards, these are the names as used in the LinuxBIOS config and therefore correct (i.e. LinuxBIOS identifies the dk8-htx board as iwill:dk8_htx, I did never check the the ARUMA...) * added some explanation to the README
Regards, Mondrian
On Fri, Apr 27, 2007 at 01:01:19PM +0200, Mondrian Nuessle wrote:
The attached patch enables flashing on the Iwill DK8-HTX board. Basically, it configures the SuperIO to set the right GPIO pins, so write protection is disabled.
Signed-off-by: Mondrian Nuessle nuessle@uni-mannheim.de
Ok, this is the third try :-)
Regards, Mondrian
- /* set GPIO regs... */
- outb(0x2b, EFIR); /* GPIO multiplexed pin reg. */
- b = inb(EFDR) | 0xd0;
- outb(0x2b, EFIR);
- outb(b, EFDR);
Is it really necessary to write the index register twice here? Not that it matters much in codesize.
/* Disable the flash write protect. The flash write protect is* connected to the WinBond w83627hf GPIO 24.*/- outb(0x87, EFIR); /* sequence to unlock extended functions */
- /* Disable the flash write protect. The flash write protect is
* connected to the WinBond w83627hf GPIO 24.*/- outb(0x87, EFIR); /* sequence to unlock extended functions */ outb(0x87, EFIR);
Ah, my old spaces for tabs setting, removed now. I had assumed that this was handled earlier, when stepan committed my code when i was too busy to fix things then. Sorry for that.
It will probably still exist in other places of flashrom too, nothing you should worry about though.
Luc Verhaegen.
On Fri, Apr 27, 2007 at 01:17:27PM +0200, Luc Verhaegen wrote:
On Fri, Apr 27, 2007 at 01:01:19PM +0200, Mondrian Nuessle wrote:
The attached patch enables flashing on the Iwill DK8-HTX board. Basically, it configures the SuperIO to set the right GPIO pins, so write protection is disabled.
Signed-off-by: Mondrian Nuessle nuessle@uni-mannheim.de
Ok, this is the third try :-)
Regards, Mondrian
- /* set GPIO regs... */
- outb(0x2b, EFIR); /* GPIO multiplexed pin reg. */
- b = inb(EFDR) | 0xd0;
- outb(0x2b, EFIR);
- outb(b, EFDR);
Is it really necessary to write the index register twice here? Not that it matters much in codesize.
I would like an answer there, just for reference, so don't worry about it patchwise :)
Acked-by: Luc Verhaegen libv@skynet.be
Luc Verhaegen.
On Fri, Apr 27, 2007 at 01:25:11PM +0200, Luc Verhaegen wrote:
Acked-by: Luc Verhaegen libv@skynet.be
One more revision, please :)
I fixed some more cosmetic issues and reduced the patch to just the relevant code. Let's fix the random other coding style changes in an extra patch.
(patch untested on hardware, please take this with a grain of salt)
Uwe.
* Uwe Hermann uwe@hermann-uwe.de [070428 01:59]:
Signed-off-by: Mondrian Nuessle nuessle@uni-mannheim.de Signed-off-by: Uwe Hermann uwe@hermann-uwe.de
Acked-by: Stefan Reinauer stepan@coresystems.de
On Thu, May 03, 2007 at 03:46:43AM +0200, Stefan Reinauer wrote:
- Uwe Hermann uwe@hermann-uwe.de [070428 01:59]:
Signed-off-by: Mondrian Nuessle nuessle@uni-mannheim.de Signed-off-by: Uwe Hermann uwe@hermann-uwe.de
Acked-by: Stefan Reinauer stepan@coresystems.de
Committed in r2624.
Uwe.
Actually you do not need to set the index reg twice. I just used the code pattern from the Agami Aruma board, which does the same thing...
Mondrian
Luc Verhaegen wrote:
On Fri, Apr 27, 2007 at 01:01:19PM +0200, Mondrian Nuessle wrote:
The attached patch enables flashing on the Iwill DK8-HTX board. Basically, it configures the SuperIO to set the right GPIO pins, so write protection is disabled.
Signed-off-by: Mondrian Nuessle nuessle@uni-mannheim.de
Ok, this is the third try :-)
Regards, Mondrian
- /* set GPIO regs... */
- outb(0x2b, EFIR); /* GPIO multiplexed pin reg. */
- b = inb(EFDR) | 0xd0;
- outb(0x2b, EFIR);
- outb(b, EFDR);
Is it really necessary to write the index register twice here? Not that it matters much in codesize.
/* Disable the flash write protect. The flash write protect is* connected to the WinBond w83627hf GPIO 24.*/- outb(0x87, EFIR); /* sequence to unlock extended functions */
- /* Disable the flash write protect. The flash write protect is
* connected to the WinBond w83627hf GPIO 24.*/- outb(0x87, EFIR); /* sequence to unlock extended functions */ outb(0x87, EFIR);
Ah, my old spaces for tabs setting, removed now. I had assumed that this was handled earlier, when stepan committed my code when i was too busy to fix things then. Sorry for that.
It will probably still exist in other places of flashrom too, nothing you should worry about though.
Luc Verhaegen.
On Fri, Apr 27, 2007 at 04:44:10PM +0200, Mondrian Nuessle wrote:
Actually you do not need to set the index reg twice. I just used the code pattern from the Agami Aruma board, which does the same thing...
In that case, we should introduce functions like:
void w83xxx_ext_enter(void); void w83xxx_ext_leave(void);
/* Enter extended functions before using these! */ unsigned char w83xxx_ext_read(unsigned char index); void w83xxx_ext_write(unsigned char index, unsigned char data); void w83xxx_ext_mask(unsigned char index, unsigned char data, unsigned char mask);
A _mask function is not that common, but this plainly provides a bitmask for data to be written. I don't see it around much, but i very much enjoy having them around in my code.
This replaces:
/* set GPIO regs... */ outb(0x2b, EFIR); /* GPIO multiplexed pin reg. */ b = inb(EFDR) | 0xd0; outb(0x2b, EFIR); outb(b, EFDR);
which would've been changed to:
/* set GPIO regs... */ b= w83xxx_ext_read(0x2b); /* GPIO multiplexed pin reg. */ b |= 0xd0; w83xxx_ext_write(0x2b, b);
with: w83xxx_ext_mask(0x2B, 0xD0, 0xD0); /* Set GPIO regs... */
I don't think that these functions need to live outside board_enable.c The size of them will already largely be made up for by the code we save with this.
Luc Verhaegen.
PS: Mondrian, there is no need for your patch to also implement this. With the whitespace comments, i didn't expect you to go over the whole file, but looking back, it's good that you did so though, as i was unaware of the spaces still being there.
On Fri, Apr 27, 2007 at 10:33:53PM +0200, Luc Verhaegen wrote:
On Fri, Apr 27, 2007 at 04:44:10PM +0200, Mondrian Nuessle wrote:
Actually you do not need to set the index reg twice. I just used the code pattern from the Agami Aruma board, which does the same thing...
In that case, we should introduce functions like:
void w83xxx_ext_enter(void); void w83xxx_ext_leave(void);
/* Enter extended functions before using these! */ unsigned char w83xxx_ext_read(unsigned char index); void w83xxx_ext_write(unsigned char index, unsigned char data); void w83xxx_ext_mask(unsigned char index, unsigned char data, unsigned char mask);
Hm, not sure. Looks a bit like overkill in this case.
Uwe.
On Sat, Apr 28, 2007 at 02:00:42AM +0200, Uwe Hermann wrote:
On Fri, Apr 27, 2007 at 10:33:53PM +0200, Luc Verhaegen wrote:
On Fri, Apr 27, 2007 at 04:44:10PM +0200, Mondrian Nuessle wrote:
Actually you do not need to set the index reg twice. I just used the code pattern from the Agami Aruma board, which does the same thing...
In that case, we should introduce functions like:
void w83xxx_ext_enter(void); void w83xxx_ext_leave(void);
/* Enter extended functions before using these! */ unsigned char w83xxx_ext_read(unsigned char index); void w83xxx_ext_write(unsigned char index, unsigned char data); void w83xxx_ext_mask(unsigned char index, unsigned char data, unsigned char mask);
Hm, not sure. Looks a bit like overkill in this case.
Uwe.
Why?
Three out of four functions in there (when mondrians patch finally gets in) would make use of this already. That says something about what future board enables will look like too.
This isn't overkill in any way, it's formalizing what's there.
Without mondrians code: board_enable.c | 137 +++++++++++++++++++++++++++++++-------------------------- 1 file changed, 76 insertions(+), 61 deletions(-)
So when mondrians patch is in there, this is made up for completely, and the board enables really are a lot friendlier.
Luc Verhaegen.
While I think that a clean function interface is nice, I can live without it... So I could go with the patch, as Uwe provided it.
Mondrian
Luc Verhaegen wrote:
On Sat, Apr 28, 2007 at 02:00:42AM +0200, Uwe Hermann wrote:
On Fri, Apr 27, 2007 at 10:33:53PM +0200, Luc Verhaegen wrote:
On Fri, Apr 27, 2007 at 04:44:10PM +0200, Mondrian Nuessle wrote:
Actually you do not need to set the index reg twice. I just used the code pattern from the Agami Aruma board, which does the same thing...
In that case, we should introduce functions like:
void w83xxx_ext_enter(void); void w83xxx_ext_leave(void);
/* Enter extended functions before using these! */ unsigned char w83xxx_ext_read(unsigned char index); void w83xxx_ext_write(unsigned char index, unsigned char data); void w83xxx_ext_mask(unsigned char index, unsigned char data, unsigned char mask);
Hm, not sure. Looks a bit like overkill in this case.
Uwe.
Why?
Three out of four functions in there (when mondrians patch finally gets in) would make use of this already. That says something about what future board enables will look like too.
This isn't overkill in any way, it's formalizing what's there.
Without mondrians code: board_enable.c | 137 +++++++++++++++++++++++++++++++-------------------------- 1 file changed, 76 insertions(+), 61 deletions(-)
So when mondrians patch is in there, this is made up for completely, and the board enables really are a lot friendlier.
Luc Verhaegen.
On Wed, May 02, 2007 at 06:45:42PM +0200, Mondrian Nuessle wrote:
While I think that a clean function interface is nice, I can live without it... So I could go with the patch, as Uwe provided it.
Mondrian
Well, as soon as this code is in, i'll put up a patch for the functions, and you'll see that it really makes a lot of difference, even on such a small file. 3 out of 4 board enables make use of it though.
Luc Verhaegen.
On Wed, May 02, 2007 at 10:14:00PM +0200, Luc Verhaegen wrote:
On Wed, May 02, 2007 at 06:45:42PM +0200, Mondrian Nuessle wrote:
While I think that a clean function interface is nice, I can live without it... So I could go with the patch, as Uwe provided it.
Mondrian
Well, as soon as this code is in, i'll put up a patch for the functions, and you'll see that it really makes a lot of difference, even on such a small file. 3 out of 4 board enables make use of it though.
Yep, thanks! It sounded like overkill when I read about it the first time, but I think you may be right. Please post a patch...
As for _this_ patch: someone just needs to ACK it, IHMO it can be committed.
Uwe.
Well, as soon as this code is in, i'll put up a patch for the functions, and you'll see that it really makes a lot of difference, even on such a small file. 3 out of 4 board enables make use of it though.
Yep, thanks! It sounded like overkill when I read about it the first time, but I think you may be right. Please post a patch...
I agree with you two. Great.
On Thu, Apr 26, 2007 at 06:24:06PM +0200, Mondrian Nuessle wrote:
The attached patch enables flashing on the Iwill DK8-HTX board. Basically, it configures the SuperIO to set the right GPIO pins, so write protection is disabled.
Signed-off-by: Mondrian Nuessle nuessle@uni-mannheim.de
So, this is the second try :-) Only pci vendor id/device id is given, no subsystem id. Therefor, flashing when booted from factory BIOS will only work, if -m iwill:dk8_htx switch is given. I guess this could go in the wiki doc to this board...
And in the flashrom README, yes.
Index: util/flashrom/board_enable.c
--- util/flashrom/board_enable.c (revision 2618) +++ util/flashrom/board_enable.c (working copy) @@ -23,6 +23,56 @@ #include "flash.h" #include "debug.h"
+static int board_iwill_dk8htx(char *name)
I think this can be const char *name as you never modify 'name' (or use it at all).
+{
- /* Extended function index register, either 0x2e or 0x4e */
On what does this depend? Can it be detected at runtime? If so, we should "probe" the value and handle both cases.
+#define EFIR 0x2e
- /* Extended function data register, one plus the index reg. */
+#define EFDR EFIR + 1
- char b;
/* Disable the flash write protect. The flash write protect is* connected to the WinBond w83627hf GPIOs*/- outb(0x87, EFIR); /* sequence to unlock extended functions */
- outb(0x87, EFIR);
- /* activate logical devie */
device
- outb(0x7,EFIR);
- outb(8,EFDR);
Space after comma (and other stuff Luc mentioned in his mail). Please use the Linux kernel coding style and/or run your code through indent, see http://linuxbios.org/Development_Guidelines#Coding_Style.
(yes, I know, lots of code currently in LinuxBIOS does not yet follow those guidelines, but we're slowly moving towards that direction)
- /*lock extended function sagain */
Typo.
Otherwise the patch looks good, but I cannot test it on real hardware, of course.
Uwe.
I just want to thank the author for the patch, and the folks who chimed in for the helpful corrections :-)
we've needed this patch for a long time. Thanks again
ron
There are some serious problems with this code. And things only become transparent when you introduce those helper functions.
This is what your function is reduced to:
/* * Disable the flash write protect (which is connected to the * Winbond W83627HF GPIOs). * * There are some severe discrepancies in this code! */ static int board_iwill_dk8_htx(const char *name) { w836xx_ext_enter();
/* set pins to SCL or GPIO. */ wbsio_mask(0x2B, 0xD0, 0xD0); /* GPIO 21,22,24 */
wbsio_write(0x07, 0x08); /* select logical device 8: GPIO port 2. */
wbsio_mask(0x30, 0x01, 0x01); /* Activate logical device */
wbsio_mask(0xF0, 0xEF, 0xEF); /* GPIO 20,21,22,23,25,26,27 = input */
wbsio_mask(0xF1, 0x16, 0x16); /* Raise GPIO 21,22,24 */
#if 0 wbsio_mask(0xF2, 0x00, 0x00); /* GPIO inversion reg */ #endif
w836xx_ext_leave();
return 0; }
Let me run down through things one by one.
First of all, which GPIO pin is the culprit here? Which one needs to be raised/dropped for your rom to be writeable? Do you know this? Or are you not sure which one it is.
Secondly, you are using | <value> all the time on all but one register access. Clearly this is not what is wanted in all cases and it leads to some pretty crappy results when using the helper functions.
Let me step through the code:
/* set pins to SCL or GPIO. */ wbsio_mask(0x2B, 0xD0, 0xD0); /* GPIO 21,22,24 */
This was moved up. You don't need to select a logical device for this to work. Aruma code was correct here. But which GPIO is it?
wbsio_write(0x07, 0x08); /* select logical device 8: GPIO port 2. */
This is fully correct.
wbsio_mask(0x30, 0x01, 0x01); /* Activate logical device */
This is correct, but could've just as well been a write. I prefer this over what the aruma code uses as it clearly marks this as a single bit to be raised.
wbsio_mask(0xF0, 0xEF, 0xEF); /* GPIO 20,21,22,23,25,26,27 = input */
It seems as if you wanted to set GPIO24 as an output. But instead you did | 0xEF. Which is quite the opposite of & 0xEF or & ~0x10.
This line should be replaced by: wbsio_mask(0xF0, 0x00, 0x10); /* GPIO24 = output */
So is your gpio line GPIO24 like the aruma?
Next line:
wbsio_mask(0xF1, 0x16, 0x16); /* Raise GPIO 21,22,24 */
Which one is it?
And this: #if 0 wbsio_mask(0xF2, 0x00, 0x00); /* GPIO inversion reg */ #endif
This was the conversion of the useless | 0x00 you did there. Were you trying to do this: wbsio_write(0xF2, 0x00, 0x10); /* Clear GPIO24 inversion */
All in all, the way this patch was handled was very very poor. We've all been talking about formatting yet no-one bothered to look at what actually was going on (and i too acked this code).
This is not how code should get handled.
I've attached the full patch here. Except for the ordering of the 0x2B write, this is a direct translation, with just some comments added.
I will look into writing static int W83627HFGPIORaise(int Port); which handles the raising of GPIO20-27 on this very superIO.
This would reduce the two relevant board enables and stop mistakes like this from happening ever again.
Luc Verhaegen.
Ok, I looked over everything again. I was blind!
Luc, you are absolutely right. And we do not need a dk8_htx enable function. Just use the ARUMA one.
The code I posted was based on reverse engineering registers, it worked for us very well for months. So I never bothered checking the other boards. Sorry.
I tested using the Aruma function on real hw and it works as expected.
After your patch, everything is A LOT better readable.
Sorry for the hassle, Mondrian
On Thu, May 03, 2007 at 08:11:43PM +0200, Mondrian Nuessle wrote:
Ok, I looked over everything again. I was blind!
I acked your code, and i didn't really bother to look what you actually were doing. The old register accesses don't invite one to look over them. You were no blinder than i was.
Luc, you are absolutely right. And we do not need a dk8_htx enable function. Just use the ARUMA one.
Smashing. New patch deletes your function, slightly alters the aruma one and renames it to w83627hf_gpio24_raise.
The code I posted was based on reverse engineering registers, it worked for us very well for months. So I never bothered checking the other boards. Sorry.
I tested using the Aruma function on real hw and it works as expected.
After your patch, everything is A LOT better readable.
Yeah. I wouldn't have a single hair on my head left if i hadn't done this very early on for vga io in my unichrome driver :)
Sorry for the hassle, Mondrian
Don't apologise. You saw this through to the end. Also, the code is cleaner, and we have what currently seems the best solution in place. The result is there, that's what counts.
New patch will be sent in shortly.
Luc Verhaegen.
-
Luc Verhaegen -
Mondrian Nuessle -
ron minnich -
Stefan Reinauer -
Uwe Hermann