| Message ID | 1382063402-30359-1-git-send-email-roy.franz@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Oct 17, 2013 at 07:30:02PM -0700, Roy Franz wrote: > For buffered writes, mask the length with the maximum supported > length. This is required for block writes to work on the ARM vexpress > platform, where the flash interface is 32 bits wide. For buffered writes > to the 2 16 bit flashes on the interface, the length is repeated in each > 16 bit word, and without this mask the two lengths are interpreted > as a single 32 bit value that is very large. > > Signed-off-by: Roy Franz <roy.franz@linaro.org> > --- > hw/block/pflash_cfi01.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index 018a967..a364cca 100644 > --- a/hw/block/pflash_cfi01.c > +++ b/hw/block/pflash_cfi01.c > @@ -378,6 +378,7 @@ static void pflash_write(pflash_t *pfl, hwaddr offset, > > break; > case 0xe8: > + value &= pfl->writeblock_size - 1; This patch feels weird. Should the 32-bit interface width be truncated down to 16 bits before dispatching pflash_write()? It's not clear to me that truncating just in this specific case is correct. But then I don't know the hardware :). Stefan
On 18 October 2013 12:38, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Thu, Oct 17, 2013 at 07:30:02PM -0700, Roy Franz wrote: >> For buffered writes, mask the length with the maximum supported >> length. This is required for block writes to work on the ARM vexpress >> platform, where the flash interface is 32 bits wide. For buffered writes >> to the 2 16 bit flashes on the interface, the length is repeated in each >> 16 bit word, and without this mask the two lengths are interpreted >> as a single 32 bit value that is very large. >> @@ -378,6 +378,7 @@ static void pflash_write(pflash_t *pfl, hwaddr offset, >> >> break; >> case 0xe8: >> + value &= pfl->writeblock_size - 1; > > This patch feels weird. Should the 32-bit interface width be truncated > down to 16 bits before dispatching pflash_write()? > > It's not clear to me that truncating just in this specific case is > correct. But then I don't know the hardware :). I think the problem may be that we're incorrectly modelling the hardware's "2 side-by-side 16 bit wide flash chips" as "one 32 bit wide flash chip", because our flash device code doesn't support doing the former. Probably instead of a single "width" property we should have two, similar to the device tree binding's pair: - bank-width : Width (in bytes) of the bank. Equal to the device width times the number of interleaved chips. - device-width : (optional) Width of a single mtd chip. If omitted, assumed to be equal to 'bank-width'. However I'm not very familiar with how flash hardware works... -- PMM
On Fri, Oct 18, 2013 at 6:36 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 18 October 2013 12:38, Stefan Hajnoczi <stefanha@gmail.com> wrote: >> On Thu, Oct 17, 2013 at 07:30:02PM -0700, Roy Franz wrote: >>> For buffered writes, mask the length with the maximum supported >>> length. This is required for block writes to work on the ARM vexpress >>> platform, where the flash interface is 32 bits wide. For buffered writes >>> to the 2 16 bit flashes on the interface, the length is repeated in each >>> 16 bit word, and without this mask the two lengths are interpreted >>> as a single 32 bit value that is very large. > >>> @@ -378,6 +378,7 @@ static void pflash_write(pflash_t *pfl, hwaddr offset, >>> >>> break; >>> case 0xe8: >>> + value &= pfl->writeblock_size - 1; >> >> This patch feels weird. Should the 32-bit interface width be truncated >> down to 16 bits before dispatching pflash_write()? >> >> It's not clear to me that truncating just in this specific case is >> correct. But then I don't know the hardware :). > > I think the problem may be that we're incorrectly modelling the > hardware's "2 side-by-side 16 bit wide flash chips" as "one 32 bit > wide flash chip", because our flash device code doesn't support > doing the former. > > Probably instead of a single "width" property we should have two, > similar to the device tree binding's pair: > - bank-width : Width (in bytes) of the bank. Equal to the > device width times the number of interleaved chips. > - device-width : (optional) Width of a single mtd chip. If > omitted, assumed to be equal to 'bank-width'. > > However I'm not very familiar with how flash hardware works... > > -- PMM Hi Peter, You are correct - we really do want to mask based on the device width, as that is what the actual flash chips will see. Lacking the device width I used the writeblock size. Thinking about this more, this will not work for 8 bit devices used together, as the mask size will be greater than 8 bits and the writeblock size will be mis-interpreted like it is now. I'll work on adding a device-size property to the pflash* implementations. It looks like this will affect about 20 platforms. For the platforms that I am not familiar with I plan just set bank-width==device-width as that should result in the unchanged behavior. Thanks, Roy
On 18 October 2013 14:54, Roy Franz <roy.franz@linaro.org> wrote: > On Fri, Oct 18, 2013 at 6:36 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> Probably instead of a single "width" property we should have two, >> similar to the device tree binding's pair: >> - bank-width : Width (in bytes) of the bank. Equal to the >> device width times the number of interleaved chips. >> - device-width : (optional) Width of a single mtd chip. If >> omitted, assumed to be equal to 'bank-width'. >> However I'm not very familiar with how flash hardware works... > You are correct - we really do want to mask based on the device > width, as that is what the > actual flash chips will see. Lacking the device width I used the > writeblock size. Thinking about this more, > this will not work for 8 bit devices used together, as the mask size > will be greater than 8 bits and the writeblock size > will be mis-interpreted like it is now. > I'll work on adding a device-size property to the pflash* > implementations. It looks like this will affect about 20 platforms. > For the platforms that I am not familiar with I plan just set > bank-width==device-width as that should result in the unchanged > behavior. Yes, you should make the default for the device-width property be to be the same as the bank-width, since that's what we currently implement; then we can just change the platforms where we know that's wrong. NB: probably best to leave the existing 'width' property with the name it has, rather than renaming it to 'bank-width'. thanks -- PMM
On Fri, Oct 18, 2013 at 7:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 18 October 2013 14:54, Roy Franz <roy.franz@linaro.org> wrote: >> On Fri, Oct 18, 2013 at 6:36 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> Probably instead of a single "width" property we should have two, >>> similar to the device tree binding's pair: >>> - bank-width : Width (in bytes) of the bank. Equal to the >>> device width times the number of interleaved chips. >>> - device-width : (optional) Width of a single mtd chip. If >>> omitted, assumed to be equal to 'bank-width'. > >>> However I'm not very familiar with how flash hardware works... >> You are correct - we really do want to mask based on the device >> width, as that is what the >> actual flash chips will see. Lacking the device width I used the >> writeblock size. Thinking about this more, >> this will not work for 8 bit devices used together, as the mask size >> will be greater than 8 bits and the writeblock size >> will be mis-interpreted like it is now. >> I'll work on adding a device-size property to the pflash* >> implementations. It looks like this will affect about 20 platforms. >> For the platforms that I am not familiar with I plan just set >> bank-width==device-width as that should result in the unchanged >> behavior. > > Yes, you should make the default for the device-width property > be to be the same as the bank-width, since that's what we > currently implement; then we can just change the platforms > where we know that's wrong. > > NB: probably best to leave the existing 'width' property with > the name it has, rather than renaming it to 'bank-width'. > > thanks > -- PMM Thanks Peter. I'm not familiar with the "properties" and how they are used. I think that the device width is likely only of interest internally, so I won't add a device-width property. Roy
On 18 October 2013 15:05, Roy Franz <roy.franz@linaro.org> wrote: > On Fri, Oct 18, 2013 at 7:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> Yes, you should make the default for the device-width property >> be to be the same as the bank-width, since that's what we >> currently implement; then we can just change the platforms >> where we know that's wrong. >> >> NB: probably best to leave the existing 'width' property with >> the name it has, rather than renaming it to 'bank-width'. > Thanks Peter. I'm not familiar with the "properties" and how they are > used. I think that > the device width is likely only of interest internally, so I won't add > a device-width property. No, you need to add a property. Properties are how all configurable device parameters are set: if you look at the implementation of the pflash_cfi01_register() function you'll see that it's just a convenience wrapper that creates the device and sets a lot of properties on it. device-width should be one of those, in the same way as the existing width property. In fact I would suggest that you don't change the utility pflash_cfi01_register() function to add a new parameter to it (that would involve editing every caller). Instead just opencode the "create / set properties / init / map" sequence in vexpress.c. I think that will actually be easier to read since you don't have to match up each unnamed argument in a long function call with what it actually does. -- PMM
On Fri, Oct 18, 2013 at 7:11 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 18 October 2013 15:05, Roy Franz <roy.franz@linaro.org> wrote: >> On Fri, Oct 18, 2013 at 7:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> Yes, you should make the default for the device-width property >>> be to be the same as the bank-width, since that's what we >>> currently implement; then we can just change the platforms >>> where we know that's wrong. >>> >>> NB: probably best to leave the existing 'width' property with >>> the name it has, rather than renaming it to 'bank-width'. > >> Thanks Peter. I'm not familiar with the "properties" and how they are >> used. I think that >> the device width is likely only of interest internally, so I won't add >> a device-width property. > > No, you need to add a property. Properties are how all configurable > device parameters are set: if you look at the implementation > of the pflash_cfi01_register() function you'll see that it's > just a convenience wrapper that creates the device and sets a > lot of properties on it. device-width should be one of those, > in the same way as the existing width property. > > In fact I would suggest that you don't change the utility > pflash_cfi01_register() function to add a new parameter to > it (that would involve editing every caller). Instead just > opencode the "create / set properties / init / map" sequence > in vexpress.c. I think that will actually be easier to read > since you don't have to match up each unnamed argument in > a long function call with what it actually does. > > -- PMM Yup, just noticed that myself :) Avoiding updating every caller for this change will be good. Thanks, Roy
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index 018a967..a364cca 100644 --- a/hw/block/pflash_cfi01.c +++ b/hw/block/pflash_cfi01.c @@ -378,6 +378,7 @@ static void pflash_write(pflash_t *pfl, hwaddr offset, break; case 0xe8: + value &= pfl->writeblock_size - 1; DPRINTF("%s: block write of %x bytes\n", __func__, value); pfl->counter = value; pfl->wcycle++;
For buffered writes, mask the length with the maximum supported length. This is required for block writes to work on the ARM vexpress platform, where the flash interface is 32 bits wide. For buffered writes to the 2 16 bit flashes on the interface, the length is repeated in each 16 bit word, and without this mask the two lengths are interpreted as a single 32 bit value that is very large. Signed-off-by: Roy Franz <roy.franz@linaro.org> --- hw/block/pflash_cfi01.c | 1 + 1 file changed, 1 insertion(+)