| Message ID | 1382459756-6853-3-git-send-email-roy.franz@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 22 October 2013 17:35, Roy Franz <roy.franz@linaro.org> wrote: > The width of the devices that make up the flash interface > is required to mask certain commands, in particular the > write length for buffered writes. This length will be presented > to each device on the interface by the program writing the flash, > and the flash emulation code needs to be able to determine > the length of the write as recieved by each flash device. > The device-width defaults to the bank width which should > maintain existing behavior for platforms that don't need > this change. > This change is required to support buffered writes on the > vexpress platform that has a 32 bit flash interface with 2 > 16 bit devices on it. > > Signed-off-by: Roy Franz <roy.franz@linaro.org> > --- > hw/block/pflash_cfi01.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index d5e366d..cda8289 100644 > --- a/hw/block/pflash_cfi01.c > +++ b/hw/block/pflash_cfi01.c > @@ -72,6 +72,7 @@ struct pflash_t { > uint32_t nb_blocs; > uint64_t sector_len; > uint8_t bank_width; > + uint8_t device_width; > uint8_t be; > uint8_t wcycle; /* if 0, the flash is read normally */ > int ro; > @@ -378,6 +379,8 @@ static void pflash_write(pflash_t *pfl, hwaddr offset, > > break; > case 0xe8: > + /* Mask writeblock size based on device width */ > + value &= (1ULL << (pfl->device_width * 8)) - 1; value = extract32(value, 0, pfl->device_width * 8); (you'll need to #include "qemu/bitops.h".) Other than this and the status (which you deal with in the other patch) the accesses I wonder about whether we have correct are: * manufacturer & device ID code read * cfi_table[] accesses ("query mode") http://www.delorie.com/agenda/specs/29220404.pdf table 1 only talks about using 2 8x devices for a 16 bit bus, but it definitely seems to imply that the QRY reads from the cfi_table[] behave differently for paired devices than for a single full width one (and that's logically what you'd expect since a single device will just return the one character but a paired device will return that one character mirrored up into each of the byte lanes). Basically these two things, like status read, are returning fixed-values which will be output by both the parallel devices; it's only pure data accesses that behave the same way regardless. > DPRINTF("%s: block write of %x bytes\n", __func__, value); > pfl->counter = value; > pfl->wcycle++; > @@ -707,6 +710,7 @@ static Property pflash_cfi01_properties[] = { > DEFINE_PROP_UINT32("num-blocks", struct pflash_t, nb_blocs, 0), > DEFINE_PROP_UINT64("sector-length", struct pflash_t, sector_len, 0), > DEFINE_PROP_UINT8("width", struct pflash_t, bank_width, 0), > + DEFINE_PROP_UINT8("device-width", struct pflash_t, device_width, 0), > DEFINE_PROP_UINT8("big-endian", struct pflash_t, be, 0), > DEFINE_PROP_UINT16("id0", struct pflash_t, ident0, 0), > DEFINE_PROP_UINT16("id1", struct pflash_t, ident1, 0), > @@ -757,6 +761,7 @@ pflash_t *pflash_cfi01_register(hwaddr base, > qdev_prop_set_uint32(dev, "num-blocks", nb_blocs); > qdev_prop_set_uint64(dev, "sector-length", sector_len); > qdev_prop_set_uint8(dev, "width", bank_width); > + qdev_prop_set_uint8(dev, "device-width", bank_width); This doesn't look right. We should just leave the property at its default value. (In pflash_cfi01_realize you can catch the 'device_width == 0' case and set it to bank_width there.) > qdev_prop_set_uint8(dev, "big-endian", !!be); > qdev_prop_set_uint16(dev, "id0", id0); > qdev_prop_set_uint16(dev, "id1", id1); thanks -- PMM
On Thu, Nov 28, 2013 at 11:03 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 22 October 2013 17:35, Roy Franz <roy.franz@linaro.org> wrote: >> The width of the devices that make up the flash interface >> is required to mask certain commands, in particular the >> write length for buffered writes. This length will be presented >> to each device on the interface by the program writing the flash, >> and the flash emulation code needs to be able to determine >> the length of the write as recieved by each flash device. >> The device-width defaults to the bank width which should >> maintain existing behavior for platforms that don't need >> this change. >> This change is required to support buffered writes on the >> vexpress platform that has a 32 bit flash interface with 2 >> 16 bit devices on it. >> >> Signed-off-by: Roy Franz <roy.franz@linaro.org> >> --- >> hw/block/pflash_cfi01.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >> index d5e366d..cda8289 100644 >> --- a/hw/block/pflash_cfi01.c >> +++ b/hw/block/pflash_cfi01.c >> @@ -72,6 +72,7 @@ struct pflash_t { >> uint32_t nb_blocs; >> uint64_t sector_len; >> uint8_t bank_width; >> + uint8_t device_width; >> uint8_t be; >> uint8_t wcycle; /* if 0, the flash is read normally */ >> int ro; >> @@ -378,6 +379,8 @@ static void pflash_write(pflash_t *pfl, hwaddr offset, >> >> break; >> case 0xe8: >> + /* Mask writeblock size based on device width */ >> + value &= (1ULL << (pfl->device_width * 8)) - 1; > > value = extract32(value, 0, pfl->device_width * 8); > > (you'll need to #include "qemu/bitops.h".) > > Other than this and the status (which you deal with in > the other patch) the accesses I wonder about whether we > have correct are: > * manufacturer & device ID code read > * cfi_table[] accesses ("query mode") I'm pretty sure these are not correct when device width is not equal to bank width. The manufacturer and device ID code looks completely broken, doing: ret = pfl->ident0 << 8 | pfl->ident1; with ident0 and ident1 being 16 bit values. I can update the device ID and cfi_table accesses to take into account device width, and test that with UEFI, but my concern is that this code is tricky to get right, and won't be well tested. The UEFI code doesn't care that these values are wrong, so my test case will likely continue to work whether the updates I make are correct or not. Also, all other platforms will continue to see the current behavior, as they don't set the device width, so they won't be testing the new code either. Let me know how you would like me to proceed on this. This is the last issue for me to resolve and I will have another version of the patch series ready. Thanks, Roy > > http://www.delorie.com/agenda/specs/29220404.pdf > table 1 only talks about using 2 8x devices for a 16 > bit bus, but it definitely seems to imply that the QRY > reads from the cfi_table[] behave differently for > paired devices than for a single full width one > (and that's logically what you'd expect since a > single device will just return the one character but > a paired device will return that one character > mirrored up into each of the byte lanes). > > Basically these two things, like status read, are > returning fixed-values which will be output by both > the parallel devices; it's only pure data accesses > that behave the same way regardless. > >> DPRINTF("%s: block write of %x bytes\n", __func__, value); >> pfl->counter = value; >> pfl->wcycle++; >> @@ -707,6 +710,7 @@ static Property pflash_cfi01_properties[] = { >> DEFINE_PROP_UINT32("num-blocks", struct pflash_t, nb_blocs, 0), >> DEFINE_PROP_UINT64("sector-length", struct pflash_t, sector_len, 0), >> DEFINE_PROP_UINT8("width", struct pflash_t, bank_width, 0), >> + DEFINE_PROP_UINT8("device-width", struct pflash_t, device_width, 0), >> DEFINE_PROP_UINT8("big-endian", struct pflash_t, be, 0), >> DEFINE_PROP_UINT16("id0", struct pflash_t, ident0, 0), >> DEFINE_PROP_UINT16("id1", struct pflash_t, ident1, 0), >> @@ -757,6 +761,7 @@ pflash_t *pflash_cfi01_register(hwaddr base, >> qdev_prop_set_uint32(dev, "num-blocks", nb_blocs); >> qdev_prop_set_uint64(dev, "sector-length", sector_len); >> qdev_prop_set_uint8(dev, "width", bank_width); >> + qdev_prop_set_uint8(dev, "device-width", bank_width); > > This doesn't look right. We should just leave the property > at its default value. (In pflash_cfi01_realize you can catch > the 'device_width == 0' case and set it to bank_width there.) > >> qdev_prop_set_uint8(dev, "big-endian", !!be); >> qdev_prop_set_uint16(dev, "id0", id0); >> qdev_prop_set_uint16(dev, "id1", id1); > > thanks > -- PMM
On 3 December 2013 20:12, Roy Franz <roy.franz@linaro.org> wrote: > On Thu, Nov 28, 2013 at 11:03 AM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> Other than this and the status (which you deal with in >> the other patch) the accesses I wonder about whether we >> have correct are: >> * manufacturer & device ID code read >> * cfi_table[] accesses ("query mode") > > I'm pretty sure these are not correct when device width > is not equal to bank width. The manufacturer and device ID code > looks completely broken, doing: > > ret = pfl->ident0 << 8 | pfl->ident1; > > with ident0 and ident1 being 16 bit values. > > I can update the device ID and cfi_table accesses to take into account > device width, and test that with UEFI, but my concern is that this code is > tricky to get right, and won't be well tested. The UEFI code doesn't > care that these > values are wrong, so my test case will likely continue to work whether the > updates I make are correct or not. Also, all other platforms will > continue to see > the current behavior, as they don't set the device width, so they > won't be testing > the new code either. > > Let me know how you would like me to proceed on this. This is the last issue > for me to resolve and I will have another version of the patch series ready. I'd prefer us to have some untested code which we believe to be correct, rather than the current approach, which is to have untested code which we know to be wrong :-) Cross-checking against what the real hardware reads these ident/ID accesses as would be nice, if you have the setup to hand (ie stuff some temporary test code into a uefi build or something). At least then we can be reasonably sure the 16:16 case is correct. -- PMM
On Tue, Dec 3, 2013 at 12:27 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 3 December 2013 20:12, Roy Franz <roy.franz@linaro.org> wrote: >> On Thu, Nov 28, 2013 at 11:03 AM, Peter Maydell >> <peter.maydell@linaro.org> wrote: >>> Other than this and the status (which you deal with in >>> the other patch) the accesses I wonder about whether we >>> have correct are: >>> * manufacturer & device ID code read >>> * cfi_table[] accesses ("query mode") >> >> I'm pretty sure these are not correct when device width >> is not equal to bank width. The manufacturer and device ID code >> looks completely broken, doing: >> >> ret = pfl->ident0 << 8 | pfl->ident1; >> >> with ident0 and ident1 being 16 bit values. >> >> I can update the device ID and cfi_table accesses to take into account >> device width, and test that with UEFI, but my concern is that this code is >> tricky to get right, and won't be well tested. The UEFI code doesn't >> care that these >> values are wrong, so my test case will likely continue to work whether the >> updates I make are correct or not. Also, all other platforms will >> continue to see >> the current behavior, as they don't set the device width, so they >> won't be testing >> the new code either. >> >> Let me know how you would like me to proceed on this. This is the last issue >> for me to resolve and I will have another version of the patch series ready. > > I'd prefer us to have some untested code which we believe to > be correct, rather than the current approach, which is to have > untested code which we know to be wrong :-) > > Cross-checking against what the real hardware reads these > ident/ID accesses as would be nice, if you have the setup to > hand (ie stuff some temporary test code into a uefi build or > something). At least then we can be reasonably sure the 16:16 case > is correct. > > -- PMM OK, I'll take a stab at this. I don't have VExpress hardware, but should be able to get someone to run some test code on it to check how actual hardware behaves. Roy
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index d5e366d..cda8289 100644 --- a/hw/block/pflash_cfi01.c +++ b/hw/block/pflash_cfi01.c @@ -72,6 +72,7 @@ struct pflash_t { uint32_t nb_blocs; uint64_t sector_len; uint8_t bank_width; + uint8_t device_width; uint8_t be; uint8_t wcycle; /* if 0, the flash is read normally */ int ro; @@ -378,6 +379,8 @@ static void pflash_write(pflash_t *pfl, hwaddr offset, break; case 0xe8: + /* Mask writeblock size based on device width */ + value &= (1ULL << (pfl->device_width * 8)) - 1; DPRINTF("%s: block write of %x bytes\n", __func__, value); pfl->counter = value; pfl->wcycle++; @@ -707,6 +710,7 @@ static Property pflash_cfi01_properties[] = { DEFINE_PROP_UINT32("num-blocks", struct pflash_t, nb_blocs, 0), DEFINE_PROP_UINT64("sector-length", struct pflash_t, sector_len, 0), DEFINE_PROP_UINT8("width", struct pflash_t, bank_width, 0), + DEFINE_PROP_UINT8("device-width", struct pflash_t, device_width, 0), DEFINE_PROP_UINT8("big-endian", struct pflash_t, be, 0), DEFINE_PROP_UINT16("id0", struct pflash_t, ident0, 0), DEFINE_PROP_UINT16("id1", struct pflash_t, ident1, 0), @@ -757,6 +761,7 @@ pflash_t *pflash_cfi01_register(hwaddr base, qdev_prop_set_uint32(dev, "num-blocks", nb_blocs); qdev_prop_set_uint64(dev, "sector-length", sector_len); qdev_prop_set_uint8(dev, "width", bank_width); + qdev_prop_set_uint8(dev, "device-width", bank_width); qdev_prop_set_uint8(dev, "big-endian", !!be); qdev_prop_set_uint16(dev, "id0", id0); qdev_prop_set_uint16(dev, "id1", id1);
The width of the devices that make up the flash interface is required to mask certain commands, in particular the write length for buffered writes. This length will be presented to each device on the interface by the program writing the flash, and the flash emulation code needs to be able to determine the length of the write as recieved by each flash device. The device-width defaults to the bank width which should maintain existing behavior for platforms that don't need this change. This change is required to support buffered writes on the vexpress platform that has a 32 bit flash interface with 2 16 bit devices on it. Signed-off-by: Roy Franz <roy.franz@linaro.org> --- hw/block/pflash_cfi01.c | 5 +++++ 1 file changed, 5 insertions(+)