All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] hw/integratorcp: Simplify flash remap code, fix sense of REMAP bit
@ 2011-12-19 16:45 Peter Maydell
  2011-12-20 11:00 ` Avi Kivity
  0 siblings, 1 reply; 4+ messages in thread
From: Peter Maydell @ 2011-12-19 16:45 UTC (permalink / raw)
  To: qemu-devel; +Cc: patches

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(-)

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [Qemu-devel] [PATCH] hw/integratorcp: Simplify flash remap code, fix sense of REMAP bit
  2011-12-19 16:45 [Qemu-devel] [PATCH] hw/integratorcp: Simplify flash remap code, fix sense of REMAP bit Peter Maydell
@ 2011-12-20 11:00 ` Avi Kivity
  2011-12-20 15:51   ` Peter Maydell
  0 siblings, 1 reply; 4+ messages in thread
From: Avi Kivity @ 2011-12-20 11:00 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-devel, patches

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.

-- 
error compiling committee.c: too many arguments to function

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Qemu-devel] [PATCH] hw/integratorcp: Simplify flash remap code, fix sense of REMAP bit
  2011-12-20 11:00 ` Avi Kivity
@ 2011-12-20 15:51   ` Peter Maydell
  2011-12-20 15:57     ` Avi Kivity
  0 siblings, 1 reply; 4+ messages in thread
From: Peter Maydell @ 2011-12-20 15:51 UTC (permalink / raw)
  To: Avi Kivity; +Cc: qemu-devel, patches

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Qemu-devel] [PATCH] hw/integratorcp: Simplify flash remap code, fix sense of REMAP bit
  2011-12-20 15:51   ` Peter Maydell
@ 2011-12-20 15:57     ` Avi Kivity
  0 siblings, 0 replies; 4+ messages in thread
From: Avi Kivity @ 2011-12-20 15:57 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-devel, patches

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.

-- 
error compiling committee.c: too many arguments to function

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2011-12-20 15:57 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-12-19 16:45 [Qemu-devel] [PATCH] hw/integratorcp: Simplify flash remap code, fix sense of REMAP bit Peter Maydell
2011-12-20 11:00 ` Avi Kivity
2011-12-20 15:51   ` Peter Maydell
2011-12-20 15:57     ` Avi Kivity

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.