Re: [Qemu-devel] [PATCH 3/4 v4] block: return status for each device

[Roy Franz]

On Thu, Nov 28, 2013 at 11:10 AM, Peter Maydell
<address@hidden> wrote:
> On 22 October 2013 17:35, Roy Franz <address@hidden> 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 <address@hidden>
>> ---
>>  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