[3/4,v4] block: return status for each device

Message ID 1382459756-6853-4-git-send-email-roy.franz@linaro.org
State New
Headers show

Commit Message

Roy Franz Oct. 22, 2013, 4:35 p.m.
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(-)

Comments

Peter Maydell Nov. 28, 2013, 7:10 p.m. | #1
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
Roy Franz Dec. 3, 2013, 2:27 a.m. | #2
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

Patch

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);