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

login
register
mail settings
Submitter Peter Maydell
Date Dec. 19, 2011, 4:45 p.m.
Message ID <1324313143-28629-1-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/132285/
State New
Headers show

Comments

Peter Maydell - Dec. 19, 2011, 4:45 p.m.
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(-)
Avi Kivity - Dec. 20, 2011, 11 a.m.
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.
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.
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.

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;
 }