Message ID | 1378399437-17615-1-git-send-email-peter.maydell@linaro.org |
---|---|
State | Accepted |
Commit | 031c44e4deedbd7829703654e381ca0b18e78a12 |
Headers | show |
Am 05.09.2013 18:43, schrieb Peter Maydell: > Clarify a comment about the ID register value presented by > the PL110 variant present on the VersatilePB board (based > on testing what the actual hardware does), to indicate that > this is not an error in our emulation, and to remove an #if-0. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > I happened to have to check the h/w behaviour in this area today > during a discussion about some kernel pl11x patches, so I thought > I might as well record the info and clean up the #if-0 in the > process. > > hw/display/pl110.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/hw/display/pl110.c b/hw/display/pl110.c > index e79ab4b..790e510 100644 > --- a/hw/display/pl110.c > +++ b/hw/display/pl110.c > @@ -94,23 +94,21 @@ static const VMStateDescription vmstate_pl110 = { > static const unsigned char pl110_id[] = > { 0x10, 0x11, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1 }; > > -/* The Arm documentation (DDI0224C) says the CLDC on the Versatile board > - has a different ID. However Linux only looks for the normal ID. */ > -#if 0 > -static const unsigned char pl110_versatile_id[] = > -{ 0x93, 0x10, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1 }; > -#else > -#define pl110_versatile_id pl110_id > -#endif > - > static const unsigned char pl111_id[] = { > 0x11, 0x11, 0x24, 0x00, 0x0d, 0xf0, 0x05, 0xb1 > }; > > + > /* Indexed by pl110_version */ > static const unsigned char *idregs[] = { > pl110_id, > - pl110_versatile_id, > + /* The ARM documentation (DDI0224C) says the CLCDC on the Versatile board > + * has a different ID (0x93, 0x10, 0x04, 0x00, ...). However the hardware > + * itself has the same ID values as a stock PL110, and guests (in > + * particular Linux) rely on this. We emulate what the hardware does, > + * rather than what the docs claim it ought to do. > + */ > + pl110_id, > pl111_id > }; > I vaguely remember us having a conversation that we might store these in the class, but me not wanting to refactor that in my 1.6 candidate patchset, right? In case you find some time, feel free to improve on this, I'll get my other pending series polished first. Andreas
On 5 September 2013 19:55, Andreas Färber <afaerber@suse.de> wrote: > Am 05.09.2013 18:43, schrieb Peter Maydell: >> /* Indexed by pl110_version */ >> static const unsigned char *idregs[] = { >> pl110_id, >> - pl110_versatile_id, >> + /* The ARM documentation (DDI0224C) says the CLCDC on the Versatile board >> + * has a different ID (0x93, 0x10, 0x04, 0x00, ...). However the hardware >> + * itself has the same ID values as a stock PL110, and guests (in >> + * particular Linux) rely on this. We emulate what the hardware does, >> + * rather than what the docs claim it ought to do. >> + */ >> + pl110_id, >> pl111_id >> }; >> > > I vaguely remember us having a conversation that we might store these in > the class, but me not wanting to refactor that in my 1.6 candidate > patchset, right? I don't feel very strongly about it one way or the other for this sort of "these two things are almost exactly the same but not quite" device; we could do it the way we do now, or with something in the class, I guess. The pl11x is not something I'm likely to do any new work on and the current code's not unacceptably ugly so unless you have a strong opinion I'd just leave it the way it is. (Possibly I leant the other way last time the issue came up: if so, that's a measure of my lack-of-strong-opinion :-)) -- PMM
diff --git a/hw/display/pl110.c b/hw/display/pl110.c index e79ab4b..790e510 100644 --- a/hw/display/pl110.c +++ b/hw/display/pl110.c @@ -94,23 +94,21 @@ static const VMStateDescription vmstate_pl110 = { static const unsigned char pl110_id[] = { 0x10, 0x11, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1 }; -/* The Arm documentation (DDI0224C) says the CLDC on the Versatile board - has a different ID. However Linux only looks for the normal ID. */ -#if 0 -static const unsigned char pl110_versatile_id[] = -{ 0x93, 0x10, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1 }; -#else -#define pl110_versatile_id pl110_id -#endif - static const unsigned char pl111_id[] = { 0x11, 0x11, 0x24, 0x00, 0x0d, 0xf0, 0x05, 0xb1 }; + /* Indexed by pl110_version */ static const unsigned char *idregs[] = { pl110_id, - pl110_versatile_id, + /* The ARM documentation (DDI0224C) says the CLCDC on the Versatile board + * has a different ID (0x93, 0x10, 0x04, 0x00, ...). However the hardware + * itself has the same ID values as a stock PL110, and guests (in + * particular Linux) rely on this. We emulate what the hardware does, + * rather than what the docs claim it ought to do. + */ + pl110_id, pl111_id };
Clarify a comment about the ID register value presented by the PL110 variant present on the VersatilePB board (based on testing what the actual hardware does), to indicate that this is not an error in our emulation, and to remove an #if-0. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- I happened to have to check the h/w behaviour in this area today during a discussion about some kernel pl11x patches, so I thought I might as well record the info and clean up the #if-0 in the process. hw/display/pl110.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)