From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:47030) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QwYQK-0008Dv-44 for qemu-devel@nongnu.org; Thu, 25 Aug 2011 07:48:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QwYQI-0000rC-Vx for qemu-devel@nongnu.org; Thu, 25 Aug 2011 07:48:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51811) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QwYQI-0000r0-MH for qemu-devel@nongnu.org; Thu, 25 Aug 2011 07:48:38 -0400 Message-ID: <4E563691.1030906@redhat.com> Date: Thu, 25 Aug 2011 14:48:33 +0300 From: Avi Kivity MIME-Version: 1.0 References: <1314193259-27092-1-git-send-email-avi@redhat.com> <1314193259-27092-15-git-send-email-avi@redhat.com> <4E5633E0.3010400@siemens.com> In-Reply-To: <4E5633E0.3010400@siemens.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 14/22] pflash_cfi01/pflash_cfi02: convert to memory API List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: qemu-devel@nongnu.org On 08/25/2011 02:37 PM, Jan Kiszka wrote: > On 2011-08-24 15:40, Avi Kivity wrote: > > cfi02 is annoying in that is ignores some address bits; we probably > > want explicit support in the memory API for that. > > ... > > > diff --git a/hw/musicpal.c b/hw/musicpal.c > > index 63dd391..5e74ee7 100644 > > --- a/hw/musicpal.c > > +++ b/hw/musicpal.c > > @@ -1502,6 +1502,7 @@ static void musicpal_init(ram_addr_t ram_size, > > unsigned long flash_size; > > DriveInfo *dinfo; > > ram_addr_t sram_off; > > + MemoryRegion *flash = g_new(MemoryRegion, 1); > > > > if (!cpu_model) { > > cpu_model = "arm926"; > > @@ -1565,21 +1566,23 @@ static void musicpal_init(ram_addr_t ram_size, > > * image is smaller than 32 MB. > > */ > > #ifdef TARGET_WORDS_BIGENDIAN > > - pflash_cfi02_register(0-MP_FLASH_SIZE_MAX, qemu_ram_alloc(NULL, > > - "musicpal.flash", flash_size), > > + memory_region_init_rom_device(flash,&pflash_cfi02_ops_be, > > + NULL, "musicpal.flash", flash_size); > > + pflash_cfi02_register(0-MP_FLASH_SIZE_MAX, flash, > > dinfo->bdrv, 0x10000, > > (flash_size + 0xffff)>> 16, > > MP_FLASH_SIZE_MAX / flash_size, > > 2, 0x00BF, 0x236D, 0x0000, 0x0000, > > - 0x5555, 0x2AAA, 1); > > + 0x5555, 0x2AAA); > > #else > > - pflash_cfi02_register(0-MP_FLASH_SIZE_MAX, qemu_ram_alloc(NULL, > > - "musicpal.flash", flash_size), > > + memory_region_init_rom_device(flash,&pflash_cfi02_ops_le, > > + NULL, "musicpal.flash", flash_size); > > + pflash_cfi02_register(0-MP_FLASH_SIZE_MAX, flash, > > dinfo->bdrv, 0x10000, > > (flash_size + 0xffff)>> 16, > > MP_FLASH_SIZE_MAX / flash_size, > > 2, 0x00BF, 0x236D, 0x0000, 0x0000, > > - 0x5555, 0x2AAA, 0); > > + 0x5555, 0x2AAA); > > #endif > > This would deserve some reordering to reduce the code under #ifdef. Sure, but I try to avoid refactoring in these conversions. We already have too many regressions. > Anyway, it also breaks the musicpal (and presumably all other flash > users) because memory_region_init_rom_device is broken. It assumes > mr->ops is set, but I guess it rather should set that field itself, no? > Right, thanks. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.