>>>>> "Peter" == Peter Maydell <peter.mayd...@linaro.org> writes:
Peter> On 30 September 2011 01:20, Peter Chubb
Peter> <peter.ch...@nicta.com.au> wrote:
>> Thanks Peter!
>>
>> Here's a reworked patch.
Peter> NB: when you resend patches it's better to send them as
Peter> completely fresh emails (via git-send-email or equivalent)
Peter> because otherwise they're more of a pain to apply (you end up
Peter> with random email chatter in the commit message, usually).
Thanks for the heads-up. I'll edit the patch in Quilt.
>> +/* + * sp804_id should be: + * union { + * struct { + * uint32_t
>> PartNumber:12; == 0x804 + * uint32_t DesignerID:8; == 'A' + *
>> uint32_t Revision:4; == 1 + * uint32_t Configurations:6; == 0 + *
>> }; + * uint8_t bytes[4]; + * }; + * but that gets into too many
>> byte-ordering and packing issues. + */ +static const uint8_t
>> sp804_id[] = {0x04, 0x18, 0x14, 0}; +static const uint8_t
>> sp804_PrimeCellID[] = {0xB1, 0x05, 0xF0, 0x0D};
Peter> I disagree with "should be" -- yes, semantically the ID
Peter> registers have a number of subfields but for practical purposes
Peter> they're just bytes; so I don't think that comment is necessary.
Peter> There's no need to split the two sets of ID registers into
Peter> different arrays, either -- it just complicates the code. Also
Peter> you have the primecell ID values in the wrong order (check the
Peter> 'register summary' table in the docs). You want:
OK, and thanks.
Peter> static const uint8_t sp804_id[] = { 0x04, 0x18, 0x14, 0, 0x0d,
Peter> 0xf0, 0x05, 0xb1 };
>> + /* + * Ids are packed into a word, then accessed one byte
>> per word. + */
Peter> No, they're just a set of byte registers.
At word offsets.
>> + /* TimerPeriphID */ + if (offset >= 0xfe0 && offset <=
>> 0xfec) + return sp804_id[(offset - 0xfe0) >> 2];
Peter> Coding style requires braces on if statements (plus your
Peter> indentation is wrong). If you run your patch through
Peter> scripts/checkpatch.pl it will catch this sort of thing.
Thanks.
>> + hw_error("sp804_read: Bad offset %x\n", (int)offset); +
>> return 0;
Peter> hw_error() is a fatal error -- don't use it for conditions that
Peter> can be triggered by a malicious guest. (And since it's noreturn
Peter> there's not much point putting any code after it...)
Is there a better `tell the programmer s/he's done something stupid'
error function? The plxxx.c files all used hw_error() for bad
offsets.
I shan't be able to get to this again until Tuesday my time. Expect
another patch then.
Peter C
--
Dr Peter Chubb http://www.gelato.unsw.edu.au peterc AT gelato.unsw.edu.au
http://www.ertos.nicta.com.au ERTOS within National ICT Australia
