On 30 September 2011 01:20, Peter Chubb <peter.ch...@nicta.com.au> wrote:
> Thanks Peter!
>
> Here's a reworked patch.
NB: when you resend patches it's better to send them as completely
fresh emails (via git-send-email or equivalent) because otherwise they're
more of a pain to apply (you end up with random email chatter in the
commit message, usually).
> +/*
> + * 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};
I disagree with "should be" -- yes, semantically the ID registers
have a number of subfields but for practical purposes they're just
bytes; so I don't think that comment is necessary.
There's no need to split the two sets of ID registers into
different arrays, either -- it just complicates the code.
Also you have the primecell ID values in the wrong order (check
the 'register summary' table in the docs). You want:
static const uint8_t sp804_id[] = { 0x04, 0x18, 0x14, 0, 0x0d, 0xf0,
0x05, 0xb1 };
> + /*
> + * Ids are packed into a word, then accessed one byte per word.
> + */
No, they're just a set of byte registers.
> + /* TimerPeriphID */
> + if (offset >= 0xfe0 && offset <= 0xfec)
> + return sp804_id[(offset - 0xfe0) >> 2];
Coding style requires braces on if statements (plus your indentation
is wrong). If you run your patch through scripts/checkpatch.pl it
will catch this sort of thing.
> + hw_error("sp804_read: Bad offset %x\n", (int)offset);
> + return 0;
hw_error() is a fatal error -- don't use it for conditions that
can be triggered by a malicious guest. (And since it's noreturn
there's not much point putting any code after it...)
-- PMM
