[Qemu-devel] [PATCH v2] hw/integratorcp: Fix bugs in writes to CM_CTRL system register

[Qemu-devel] [PATCH v2] hw/integratorcp: Fix bugs in writes to CM_CTRL system register

[Peter Maydell]

Fix a number of bugs in the implementation of writes to the CM_CTRL
system register:
 * write to cm_ctrl, not cm_init !
 * an '&' vs '^' typo meant we would write the inverse of the bits
 * handling the LED via printf() meant we spew lots of output
   to stdout when Linux uses the LED as a heartbeat indicator
 * we would hw_error() if a reset was requested rather than
   actually resetting

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This is just a retransmit rebased following some of Avi's MemoryRegion
patches landing, no other changes from v1.

 hw/integratorcp.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/integratorcp.c b/hw/integratorcp.c
index 3c8982e..9a289b4 100644
--- a/hw/integratorcp.c
+++ b/hw/integratorcp.c
@@ -14,6 +14,7 @@
 #include "arm-misc.h"
 #include "net.h"
 #include "exec-memory.h"
+#include "sysemu.h"
 
 typedef struct {
     SysBusDevice busdev;
@@ -126,15 +127,20 @@ static void integratorcm_do_remap(integratorcm_state *s, int flash)
 static void integratorcm_set_ctrl(integratorcm_state *s, uint32_t value)
 {
     if (value & 8) {
-        hw_error("Board reset\n");
+        qemu_system_reset_request();
     }
-    if ((s->cm_init ^ value) & 4) {
+    if ((s->cm_ctrl ^ value) & 4) {
         integratorcm_do_remap(s, (value & 4) == 0);
     }
-    if ((s->cm_init ^ value) & 1) {
-        printf("Green LED %s\n", (value & 1) ? "on" : "off");
+    if ((s->cm_ctrl ^ value) & 1) {
+        /* (value & 1) != 0 means the green "MISC LED" is lit.
+         * We don't have any nice place to display LEDs. printf is a bad
+         * idea because Linux uses the LED as a heartbeat and the output
+         * will swamp anything else on the terminal.
+         */
     }
-    s->cm_init = (s->cm_init & ~ 5) | (value ^ 5);
+    /* Note that the RESET bit [3] always reads as zero */
+    s->cm_ctrl = (s->cm_ctrl & ~5) | (value & 5);
 }
 
 static void integratorcm_update(integratorcm_state *s)
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2] hw/integratorcp: Fix bugs in writes to CM_CTRL system register

[Anthony Liguori]

On 09/12/2011 09:43 AM, Peter Maydell wrote:
> Fix a number of bugs in the implementation of writes to the CM_CTRL
> system register:
>   * write to cm_ctrl, not cm_init !
>   * an '&' vs '^' typo meant we would write the inverse of the bits
>   * handling the LED via printf() meant we spew lots of output
>     to stdout when Linux uses the LED as a heartbeat indicator
>   * we would hw_error() if a reset was requested rather than
>     actually resetting
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
> This is just a retransmit rebased following some of Avi's MemoryRegion
> patches landing, no other changes from v1.
>
>   hw/integratorcp.c |   16 +++++++++++-----
>   1 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/hw/integratorcp.c b/hw/integratorcp.c
> index 3c8982e..9a289b4 100644
> --- a/hw/integratorcp.c
> +++ b/hw/integratorcp.c
> @@ -14,6 +14,7 @@
>   #include "arm-misc.h"
>   #include "net.h"
>   #include "exec-memory.h"
> +#include "sysemu.h"
>
>   typedef struct {
>       SysBusDevice busdev;
> @@ -126,15 +127,20 @@ static void integratorcm_do_remap(integratorcm_state *s, int flash)
>   static void integratorcm_set_ctrl(integratorcm_state *s, uint32_t value)
>   {
>       if (value&  8) {
> -        hw_error("Board reset\n");
> +        qemu_system_reset_request();
>       }
> -    if ((s->cm_init ^ value)&  4) {
> +    if ((s->cm_ctrl ^ value)&  4) {
>           integratorcm_do_remap(s, (value&  4) == 0);
>       }
> -    if ((s->cm_init ^ value)&  1) {
> -        printf("Green LED %s\n", (value&  1) ? "on" : "off");
> +    if ((s->cm_ctrl ^ value)&  1) {
> +        /* (value&  1) != 0 means the green "MISC LED" is lit.
> +         * We don't have any nice place to display LEDs. printf is a bad
> +         * idea because Linux uses the LED as a heartbeat and the output
> +         * will swamp anything else on the terminal.
> +         */
>       }
> -    s->cm_init = (s->cm_init&  ~ 5) | (value ^ 5);
> +    /* Note that the RESET bit [3] always reads as zero */
> +    s->cm_ctrl = (s->cm_ctrl&  ~5) | (value&  5);
>   }
>
>   static void integratorcm_update(integratorcm_state *s)

Re: [Qemu-devel] [PATCH v2] hw/integratorcp: Fix bugs in writes to CM_CTRL system register

[Peter Maydell]

On 16 September 2011 15:08, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 09/12/2011 09:43 AM, Peter Maydell wrote:
>>
>> Fix a number of bugs in the implementation of writes to the CM_CTRL
>> system register:
>>  * write to cm_ctrl, not cm_init !
>>  * an '&' vs '^' typo meant we would write the inverse of the bits
>>  * handling the LED via printf() meant we spew lots of output
>>    to stdout when Linux uses the LED as a heartbeat indicator
>>  * we would hw_error() if a reset was requested rather than
>>    actually resetting
>>
>> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
>
> Applied.  Thanks.

Thanks. (This is my 250th committed qemu patch, so a personal
milestone :-))

-- PMM