| Message ID | 1324313143-28629-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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.
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
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 --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; }
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(-)