On Tue, Dec 3, 2013 at 12:27 PM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 3 December 2013 20:12, Roy Franz <roy.fr...@linaro.org> wrote:
>> On Thu, Nov 28, 2013 at 11:03 AM, Peter Maydell
>> <peter.mayd...@linaro.org> wrote:
>>> Other than this and the status (which you deal with in
>>> the other patch) the accesses I wonder about whether we
>>> have correct are:
>>> * manufacturer & device ID code read
>>> * cfi_table[] accesses ("query mode")
>>
>> I'm pretty sure these are not correct when device width
>> is not equal to bank width. The manufacturer and device ID code
>> looks completely broken, doing:
>>
>> ret = pfl->ident0 << 8 | pfl->ident1;
>>
>> with ident0 and ident1 being 16 bit values.
>>
>> I can update the device ID and cfi_table accesses to take into account
>> device width, and test that with UEFI, but my concern is that this code is
>> tricky to get right, and won't be well tested. The UEFI code doesn't
>> care that these
>> values are wrong, so my test case will likely continue to work whether the
>> updates I make are correct or not. Also, all other platforms will
>> continue to see
>> the current behavior, as they don't set the device width, so they
>> won't be testing
>> the new code either.
>>
>> Let me know how you would like me to proceed on this. This is the last issue
>> for me to resolve and I will have another version of the patch series ready.
>
> I'd prefer us to have some untested code which we believe to
> be correct, rather than the current approach, which is to have
> untested code which we know to be wrong :-)
>
> Cross-checking against what the real hardware reads these
> ident/ID accesses as would be nice, if you have the setup to
> hand (ie stuff some temporary test code into a uefi build or
> something). At least then we can be reasonably sure the 16:16 case
> is correct.
>
> -- PMM
OK, I'll take a stab at this. I don't have VExpress hardware, but
should be able to get someone
to run some test code on it to check how actual hardware behaves.
Roy
