| Message ID | 1382459756-6853-4-git-send-email-roy.franz@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 22 October 2013 17:35, Roy Franz <roy.franz@linaro.org> wrote: > Now that we know how wide each flash device that makes up the bank is, > return status for each device in the bank. Leave existing code > that treats 32 bit wide banks as composed of two 16 bit devices as otherwise > we may break configurations that do not set the device_width propery. > > Signed-off-by: Roy Franz <roy.franz@linaro.org> > --- > hw/block/pflash_cfi01.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index cda8289..aa2cbbc 100644 > --- a/hw/block/pflash_cfi01.c > +++ b/hw/block/pflash_cfi01.c > @@ -191,9 +191,21 @@ static uint32_t pflash_read (pflash_t *pfl, hwaddr offset, > case 0x60: /* Block /un)lock */ > case 0x70: /* Status Register */ > case 0xe8: /* Write block */ > - /* Status register read */ > + /* Status register read. Return status from each device in > + * bank. > + */ > ret = pfl->status; > - if (width > 2) { > + if (width > pfl->device_width) { > + int shift = pfl->device_width * 8; > + while (shift + pfl->device_width * 8 <= width * 8) { > + ret |= pfl->status << (shift); The brackets here are superfluous. > + shift += pfl->device_width * 8; > + } If we end up with several things that all want to get this "replicate into all lanes" behaviour then a helper function is probably going to be worthwhile (see comments on other patch). > + } else if (width > 2) { > + /* Handle 32 bit flash cases where device width may be > + * improperly set. (Existing behavior before device width > + * added.) > + */ > ret |= pfl->status << 16; Maybe we should have 'device_width == 0' mean 'same as bank width and with all the legacy workaround code', so device_width can be specifically set same as bank_width for new platforms to get the actual correct behaviour for a full 32 bit wide flash device. I don't care very much though: up to you. > } > DPRINTF("%s: status %x\n", __func__, ret); > -- > 1.7.10.4 > thanks -- PMM
On Thu, Nov 28, 2013 at 11:10 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 22 October 2013 17:35, Roy Franz <roy.franz@linaro.org> wrote: >> Now that we know how wide each flash device that makes up the bank is, >> return status for each device in the bank. Leave existing code >> that treats 32 bit wide banks as composed of two 16 bit devices as otherwise >> we may break configurations that do not set the device_width propery. >> >> Signed-off-by: Roy Franz <roy.franz@linaro.org> >> --- >> hw/block/pflash_cfi01.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >> index cda8289..aa2cbbc 100644 >> --- a/hw/block/pflash_cfi01.c >> +++ b/hw/block/pflash_cfi01.c >> @@ -191,9 +191,21 @@ static uint32_t pflash_read (pflash_t *pfl, hwaddr offset, >> case 0x60: /* Block /un)lock */ >> case 0x70: /* Status Register */ >> case 0xe8: /* Write block */ >> - /* Status register read */ >> + /* Status register read. Return status from each device in >> + * bank. >> + */ >> ret = pfl->status; >> - if (width > 2) { >> + if (width > pfl->device_width) { >> + int shift = pfl->device_width * 8; >> + while (shift + pfl->device_width * 8 <= width * 8) { >> + ret |= pfl->status << (shift); > > The brackets here are superfluous. fixed. > >> + shift += pfl->device_width * 8; >> + } > > If we end up with several things that all want to get > this "replicate into all lanes" behaviour then a helper > function is probably going to be worthwhile (see comments > on other patch). > >> + } else if (width > 2) { >> + /* Handle 32 bit flash cases where device width may be >> + * improperly set. (Existing behavior before device width >> + * added.) >> + */ >> ret |= pfl->status << 16; > > Maybe we should have 'device_width == 0' mean 'same as > bank width and with all the legacy workaround code', so > device_width can be specifically set same as bank_width > for new platforms to get the actual correct behaviour for > a full 32 bit wide flash device. > > I don't care very much though: up to you. I changed it as you describe. Even though I have never seen a 32 bit wide NOR device in the wild, it would be good to allow proper support for them. > >> } >> DPRINTF("%s: status %x\n", __func__, ret); >> -- >> 1.7.10.4 >> > > thanks > -- PMM
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index cda8289..aa2cbbc 100644 --- a/hw/block/pflash_cfi01.c +++ b/hw/block/pflash_cfi01.c @@ -191,9 +191,21 @@ static uint32_t pflash_read (pflash_t *pfl, hwaddr offset, case 0x60: /* Block /un)lock */ case 0x70: /* Status Register */ case 0xe8: /* Write block */ - /* Status register read */ + /* Status register read. Return status from each device in + * bank. + */ ret = pfl->status; - if (width > 2) { + if (width > pfl->device_width) { + int shift = pfl->device_width * 8; + while (shift + pfl->device_width * 8 <= width * 8) { + ret |= pfl->status << (shift); + shift += pfl->device_width * 8; + } + } else if (width > 2) { + /* Handle 32 bit flash cases where device width may be + * improperly set. (Existing behavior before device width + * added.) + */ ret |= pfl->status << 16; } DPRINTF("%s: status %x\n", __func__, ret);
Now that we know how wide each flash device that makes up the bank is, return status for each device in the bank. Leave existing code that treats 32 bit wide banks as composed of two 16 bit devices as otherwise we may break configurations that do not set the device_width propery. Signed-off-by: Roy Franz <roy.franz@linaro.org> --- hw/block/pflash_cfi01.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)