On 20/10/2019 13:22, Mark Cave-Ayland wrote:
On 18/10/2019 17:23, Segher Boessenkool wrote:
On Fri, Oct 18, 2019 at 04:53:26PM +0100, Mark Cave-Ayland wrote:
On 18/10/2019 10:52, Segher Boessenkool wrote:
On Fri, Oct 18, 2019 at 08:48:53AM +0100, Mark Cave-Ayland wrote:
Otherwise the compiler may not understand that a read from the assigned buffer is comming from memory-mapped IO space and fail to update accordingly.
I think you should not tell the compiler that whatever writes to ch writes the same value multiple times in sequence, instead?
Can you explain a bit more about what you mean? The issue seems to be related to this:
static volatile unsigned char *kbd_dev;
void kbd_init(phys_addr_t base) { kbd_dev = (unsigned char *)ofmem_map_io(base, 2 * 4); kbd_dev += 4; }
... ...
unsigned char keyboard_readdata(void) { volatile unsigned char ch;
while (!keyboard_dataready()) { } do { ch = kbd_dev[2] & 0xff; ...}
Without the volatile marking on ch it seems with optimisation enabled that ch gets optimised into a register causing strange intermittent keyboard behaviour. I guess since kbd_dev is a pointer to something of unknown size, the compiler doesn't understand that kbd_dev[2] should also be considered volatile?
No, the compiler understands that fine.
How is ch *used*?
[ It's a lot better to treat I/O as I/O, accessed via access functions, than to treat it as memory, exposed in C as some variables. I/O is not memory, and you cannot access it exactly the same way, normally. But that aside. ]
It's used to return a single character read from the serial port (along with some special key detection and keycode translation). The complete code looks like this:
unsigned char keyboard_readdata(void) { volatile unsigned char ch;
while (!keyboard_dataready()) { } do { ch = kbd_dev[2] & 0xff; if (ch == 99) shiftstate |= 1; else if (ch == 110) shiftstate |= 2; else if (ch == 227) shiftstate &= ~1; else if (ch == 238) shiftstate &= ~2; //printk("getch: %d\n", ch); } // If release, wait for key press while ((ch & 0x80) == 0x80 || ch == 238 || ch == 227); //printk("getch rel: %d\n", ch); ch &= 0x7f; if (shiftstate) ch = sunkbd_keycode_shifted[ch]; else ch = sunkbd_keycode[ch]; //printk("getch xlate: %d\n", ch); return ch;}
The kbd_dev pointer is allocated dynamically when mapping the physical device address.
Hi Segher,
Any further comments? I'd like to get these last couple of patchsets applied fairly soon since it's QEMU freeze starting next week.
ATB,
Mark.