On 6/22/19 2:25 PM, Philippe Mathieu-Daudé wrote: > Hi Stephen, > > This series haven't fall through the cracks, however it is taking me > longer than expected to review it. > > On 4/26/19 6:26 PM, Stephen Checkoway wrote: >> It's common for multiple narrow flash chips to be hooked up in parallel >> to support wider buses. For example, four 8-bit wide flash chips (x8) >> may be combined in parallel to produce a 32-bit wide device. Similarly, >> two 16-bit wide chips (x16) may be combined. >> >> This commit introduces `device-width` and `max-device-width` properties, >> similar to pflash_cfi01, with the following meanings: >> - `width`: The width of the logical, qemu device (same as before); >> - `device-width`: The width of an individual flash chip, defaulting to >> `width`; and >> - `max-device-width`: The maximum width of an individual flash chip, >> defaulting to `device-width`. >> >> Nothing needs to change to support reading such interleaved devices but >> commands (e.g., erase and programming) must be sent to all devices at >> the same time or else the various chips will be in different states. > > After some thoughts on this, I'd rather we model how hardware manage > interleaved devices: do it at the bus level, and instanciate N devices > in an interleaved config. > I believe that would drastically reduce this device complexity, and we > would match the real internal state machine. > Also this could be reused by other parallel devices used in a such config. > >> For example, a 4-byte wide logical device can be composed of four x8/x16 >> devices in x8 mode. That is, each device supports both x8 or x16 and >> they're being used in the byte, rather than word, mode. This >> configuration would have `width=4`, `device-width=1`, and >> `max-device-width=2`. > > > I'm thinking of this draft: > > FlashDevice # x8 > MemoryRegionOps > .valid.max_access_size = 1 > > FlashDevice # x16 > MemoryRegionOps > .valid.min_access_size = 2 > .valid.max_access_size = 2 > > FlashDevice # x8/x16 > MemoryRegionOps > .valid.min_access_size = 1 > .valid.max_access_size = 2 > > We might use .impl.min_access_size = 2 and consider all NOR flash using > 16-bit words internally. > .impl.max_access_size = 2 is implicit. > > So for you example we'd instanciate one: > > InterleaverDevice > Property > .bus_width = 4 # 4-byte wide logical device, `width=4` > .device_width = 1 # `device-width=1` > MemoryRegionOps > .valid.max_access_size = .bus_width # 4, set at realize() > .impl.max_access_size = .device_width # 1, set at realize() > > Then instanciate 4 pflash devices, and link them to the interleaver > using object_property_set_link(). > > typedef struct { > SysBusDevice parent_obj; > MemoryRegion iomem; > char *name; > /* > * On a 64-bit wide bus we can have at most > * 8 devices in 8-bit access mode. > */ > MemoryRegion device[8]; > unsigned device_count; > unsigned device_index_mask; > /* Properties */ > unsigned bus_width; > unsigned device_width; > } InterleaverDeviceState; > > static Property interleaver_properties[] = { > DEFINE_PROP_LINK("device[0]", InterleaverDeviceState, > device[0], > TYPE_MEMORY_REGION, MemoryRegion *), > ... > DEFINE_PROP_LINK("device[7]", InterleaverDeviceState, > device[7], > TYPE_MEMORY_REGION, MemoryRegion *), > DEFINE_PROP_END_OF_LIST(), > }; > > Then previous to call InterleaverDevice.realize(): > > In the board realize(): > > > for (i = 0; i < interleaved_devices; i++) { > pflash[i] = create_pflash(...); > ... > } > > ild = ... create InterleaverDevice ... > for (i = 0; i < interleaved_devices; i++) { > char *propname = g_strdup_printf("device[%u]", i); > > > object_property_set_link(OBJECT(&ild->device[i]), > OBJECT(pflash[i]), > propname, &err); > ... > } > > Finally, > > static void interleaved_realize(DeviceState *dev, Error **errp) > { > InterleaverDeviceState *s = INTERLEAVER_DEVICE(opaque); > > s->device_count = s->bus_width / s->device_width; > s->device_index_mask = ~(s->device_count - 1); > ... > } > > static void interleaved_write(void *opaque, hwaddr offset, > uint64_t value, unsigned size) > { > InterleaverDeviceState *s = INTERLEAVER_DEVICE(opaque); > MemoryRegion *mr; > > /* > * Since we set .impl.max_access_size = device_width, > * access_with_adjusted_size() always call this with > * size = device_width. > * > * Adjust the address (offset). > */ > offset >>= size; > /* Access the N interleaved device */ > mr = s->device[offset & s->device_index_mask]; > memory_region_dispatch_write(mr, offset, &value, size, > MEMTXATTRS_UNSPECIFIED); > } > > I'll try a PoC.
So I have a PoC, but then realize I can not use the same flash dump... I need to deinterleave is. This is easily fixed with few lines of Python, then if I want to store/share the dump (aka 'backend storage') I have to re-interleave it. I wonder if it would be possible/easy to add a 'interleave' option to blockdev to be able to have 2 pflash devices sharing the same backend... Is it worthwhile? Kevin/Markus/Max any thought on this? Thanks, Phil. >> In addition to commands being sent to all devices, guest firmware >> expects the status and CFI queries to be replicated for each device. >> (The one exception to the response replication is that each device gets >> to report its own status bit DQ7 while programming because its value >> depends on the value being programmed which will usually differ for each >> device.) >> >> Testing is limited to 16-bit wide devices due to the current inability >> to override the properties set by `pflash_cfi02_register`, but multiple >> configurations are tested. >> >> Stop using global_qtest. Instead, package the qtest variable inside the >> FlashConfig structure. >> >> Signed-off-by: Stephen Checkoway <stephen.checko...@oberlin.edu> >> Acked-by: Thomas Huth <th...@redhat.com> >> --- >> hw/block/pflash_cfi02.c | 270 +++++++++++++++------ >> tests/pflash-cfi02-test.c | 476 ++++++++++++++++++++++++++++++-------- >> 2 files changed, 576 insertions(+), 170 deletions(-)