diff mbox

hw/integratorcp: Simplify flash remap code, fix sense of REMAP bit

Message ID 1324313143-28629-1-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show

Commit Message

Peter Maydell Dec. 19, 2011, 4:45 p.m. UTC
Use the new memory mutator API to simplify the flash remap code;
this allows us to drop the flash_mapped flag.

This patch also fixes the sense of the REMAP bit, which was
reversed.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/integratorcp.c |   26 +++++++++-----------------
 1 files changed, 9 insertions(+), 17 deletions(-)

Comments

Avi Kivity Dec. 20, 2011, 11 a.m. UTC | #1
On 12/19/2011 06:45 PM, Peter Maydell wrote:
> Use the new memory mutator API to simplify the flash remap code;
> this allows us to drop the flash_mapped flag.
>
> This patch also fixes the sense of the REMAP bit, which was
> reversed.
>

I'm surprised the word "also" doesn't cause the maintainers' scripts to
auto-reject the patch.  It makes the patch hard to review, and also
makes cherry picking for updating stable releases harder.
Peter Maydell Dec. 20, 2011, 3:51 p.m. UTC | #2
On 20 December 2011 11:00, Avi Kivity <avi@redhat.com> wrote:
> On 12/19/2011 06:45 PM, Peter Maydell wrote:
>> Use the new memory mutator API to simplify the flash remap code;
>> this allows us to drop the flash_mapped flag.
>>
>> This patch also fixes the sense of the REMAP bit, which was
>> reversed.
>>
>
> I'm surprised the word "also" doesn't cause the maintainers' scripts to
> auto-reject the patch.  It makes the patch hard to review, and also
> makes cherry picking for updating stable releases harder.

Well, if you like I could split it into one patch which just
changes the "if (flash) {" in integratorcm_do_remap() to
"if (!flash) {", and then another patch which was exactly
this one, but I don't think that makes this patch easier to
review, because it's exactly the same size.

I do kinda agree that it was a bit borderline putting both
things into one patch, though.

-- PMM
Avi Kivity Dec. 20, 2011, 3:57 p.m. UTC | #3
On 12/20/2011 05:51 PM, Peter Maydell wrote:
> On 20 December 2011 11:00, Avi Kivity <avi@redhat.com> wrote:
> > On 12/19/2011 06:45 PM, Peter Maydell wrote:
> >> Use the new memory mutator API to simplify the flash remap code;
> >> this allows us to drop the flash_mapped flag.
> >>
> >> This patch also fixes the sense of the REMAP bit, which was
> >> reversed.
> >>
> >
> > I'm surprised the word "also" doesn't cause the maintainers' scripts to
> > auto-reject the patch.  It makes the patch hard to review, and also
> > makes cherry picking for updating stable releases harder.
>
> Well, if you like I could split it into one patch which just
> changes the "if (flash) {" in integratorcm_do_remap() to
> "if (!flash) {", and then another patch which was exactly
> this one, but I don't think that makes this patch easier to
> review, because it's exactly the same size.

It's easier to review because the smaller patch just fixes a bug, and is
thus very visible.  When reviewing the larger patch you verify that it
doesn't change behaviour in any way, only simplifies the code.  This
helps people with limited cache in their brains.
diff mbox

Patch

diff --git a/hw/integratorcp.c b/hw/integratorcp.c
index 2551236..fb243e1 100644
--- a/hw/integratorcp.c
+++ b/hw/integratorcp.c
@@ -21,7 +21,6 @@  typedef struct {
     MemoryRegion iomem;
     uint32_t memsz;
     MemoryRegion flash;
-    bool flash_mapped;
     uint32_t cm_osc;
     uint32_t cm_ctrl;
     uint32_t cm_lock;
@@ -110,20 +109,16 @@  static uint64_t integratorcm_read(void *opaque, target_phys_addr_t offset,
     }
 }
 
-static void integratorcm_do_remap(integratorcm_state *s, int flash)
+static void integratorcm_do_remap(integratorcm_state *s)
 {
-    if (flash) {
-        if (s->flash_mapped) {
-            sysbus_del_memory(&s->busdev, &s->flash);
-            s->flash_mapped = false;
-        }
+    /* Sync memory region state with CM_CTRL REMAP bit:
+     * bit 0 => flash at address 0; bit 1 => RAM
+     */
+    if (s->cm_ctrl & 4) {
+        memory_region_set_enabled(&s->flash, 0);
     } else {
-        if (!s->flash_mapped) {
-            sysbus_add_memory_overlap(&s->busdev, 0, &s->flash, 1);
-            s->flash_mapped = true;
-        }
+        memory_region_set_enabled(&s->flash, 1);
     }
-    //??? tlb_flush (cpu_single_env, 1);
 }
 
 static void integratorcm_set_ctrl(integratorcm_state *s, uint32_t value)
@@ -131,9 +126,6 @@  static void integratorcm_set_ctrl(integratorcm_state *s, uint32_t value)
     if (value & 8) {
         qemu_system_reset_request();
     }
-    if ((s->cm_ctrl ^ value) & 4) {
-        integratorcm_do_remap(s, (value & 4) == 0);
-    }
     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
@@ -143,6 +135,7 @@  static void integratorcm_set_ctrl(integratorcm_state *s, uint32_t value)
     }
     /* Note that the RESET bit [3] always reads as zero */
     s->cm_ctrl = (s->cm_ctrl & ~5) | (value & 5);
+    integratorcm_do_remap(s);
 }
 
 static void integratorcm_update(integratorcm_state *s)
@@ -262,13 +255,12 @@  static int integratorcm_init(SysBusDevice *dev)
     memcpy(integrator_spd + 73, "QEMU-MEMORY", 11);
     s->cm_init = 0x00000112;
     memory_region_init_ram(&s->flash, NULL, "integrator.flash", 0x100000);
-    s->flash_mapped = false;
 
     memory_region_init_io(&s->iomem, &integratorcm_ops, s,
                           "integratorcm", 0x00800000);
     sysbus_init_mmio(dev, &s->iomem);
 
-    integratorcm_do_remap(s, 1);
+    integratorcm_do_remap(s);
     /* ??? Save/restore.  */
     return 0;
 }