Messages in this thread | | | Date | Fri, 09 Jan 2015 02:11:54 +0000 | From | Bryan O'Donoghue <> | Subject | Re: [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup |
| |
On 09/01/15 01:00, Ong, Boon Leong wrote:
>> +static void __init >> +intel_galileo_imr_sanity(unsigned long base, unsigned long size) { >> + /* Test zero zero */ >> + if (imr_add(0, 0, 0, 0, true) == 0) >> + pr_err(SANITY "zero sized IMR @ 0x00000000\n"); > > A side-discussion on imr_add(): > I would think that we should allow 1KiB IMR setting. Current imr_add() logic > is prohibiting it.
Hi Boon Leong. Ermm, it does allow 1 KiB IMR regions. The following code works on the unmodifed V1 driver.
/* Test 1 KiB works */ idx = imr_add(0, IMR_ALIGN, IMR_READ_ACCESS_ALL, IMR_WRITE_ACCESS_ALL,false); if (idx < 0) pr_err(SANITY "Couldn't add an IMR @ 0x%04x bytes\n", IMR_ALIGN);
Note IMR_ALIGN is 0x400
I'll add that test to the set of sanity tests in V2 just to put your mind at ease though.
> So, the 'size' input should be at least 1KiB and imr_add() > internal logic will adjust 'hi' by -1KiB. Please consider ..
Hmm.
Actually I had a response all typed out for you why I didn't want an API to presume to modify the size of my input from the user's POV but, thinking about it twice - I agree with you.
V2 will subtract IMR_ALIGN (0x400) bytes from the size.
It's stupid to have to subtract IMR_ALIGN bytes on the input - and assumes the user of the API understands how the hardware works - but, of course the point of an API is so that the user of it doesn't *have* to understand that.
Good call.
-- BOD
| |