On Thu, Nov 28, 2013 at 11:10 AM, Peter Maydell
<peter.mayd...@linaro.org> wrote:
> On 22 October 2013 17:35, Roy Franz <roy.fr...@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.fr...@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
