subject:"Re\: \[Qemu\-devel\] \[PATCH 06\/12\] 4byte address mode support added."

Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support added.

2016-02-04 Thread Krzeminski, Marcin (Nokia - PL/Wroclaw)



> -Original Message-
> From: EXT Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com]
> Sent: Tuesday, December 22, 2015 10:29 PM
> To: Cédric Le Goater; g...@xilinx.com
> Cc: Krzeminski, Marcin (Nokia - PL/Wroclaw); qemu-devel@nongnu.org
> Developers; Lenkow, Pawel (Nokia - PL/Wroclaw)
> Subject: Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support
> added.
> 
> On Tue, Dec 22, 2015 at 10:41 AM, Cédric Le Goater <c...@fr.ibm.com> wrote:
> > Hello Marcin,
> >
> >
> > On 12/16/2015 01:57 PM, marcin.krzemin...@nokia.com wrote:
> >> From: Marcin Krzeminski <marcin.krzemin...@nokia.com>
> >>
> >> Signed-off-by: Marcin Krzeminski <marcin.krzemin...@nokia.com>
> >> ---
> >>  hw/block/m25p80.c | 31 ---
> >>  1 file changed, 28 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index
> >> 1a547ae..6d5d90d 100644
> >> --- a/hw/block/m25p80.c
> >> +++ b/hw/block/m25p80.c
> >> @@ -237,6 +237,9 @@ typedef enum {
> >>  ERASE_32K = 0x52,
> >>  ERASE_SECTOR = 0xd8,
> >>
> >> +EN_4BYTE_ADDR = 0xB7,
> >> +EX_4BYTE_ADDR = 0xE9,
> >> +
> >>  RESET_ENABLE = 0x66,
> >>  RESET_MEMORY = 0x99,
> >>
> >> @@ -267,6 +270,7 @@ typedef struct Flash {
> >>  uint8_t cmd_in_progress;
> >>  uint64_t cur_addr;
> >>  bool write_enable;
> >> +bool four_bytes_address_mode;
> >>  bool reset_enable;
> >>  bool initialized;
> >>  uint8_t reset_pin;
> >> @@ -405,11 +409,24 @@ void flash_write8(Flash *s, uint64_t addr,
> uint8_t data)
> >>  s->dirty_page = page;
> >>  }
> >>
> >> +static inline int is_4bytes(Flash *s) {
> >> +   return s->four_bytes_address_mode;
> >> +   }
> >> +}
> >> +
> >>  static void complete_collecting_data(Flash *s)  {
> >> -s->cur_addr = s->data[0] << 16;
> >> -s->cur_addr |= s->data[1] << 8;
> >> -s->cur_addr |= s->data[2];
> >> +if (is_4bytes(s)) {
> >> +s->cur_addr = s->data[0] << 24;
> >> +s->cur_addr |= s->data[1] << 16;
> >> +s->cur_addr |= s->data[2] << 8;
> >> +s->cur_addr |= s->data[3];
> >> +} else {
> >> +s->cur_addr = s->data[0] << 16;
> >> +s->cur_addr |= s->data[1] << 8;
> >> +s->cur_addr |= s->data[2];
> >> +}
> >>
> >>  s->state = STATE_IDLE;
> >
> >
> > Don't we need to also change 'needed_bytes' in the decode_new_cmd()
> > routine to increase the number of bytes expected by some commands ?
> >
> 
> I think you are right, and it may be solved later in the series, from patch 
> 10:
> 
>  case QPP:
>  case PP:
> -s->needed_bytes = 3;
> +   s->needed_bytes = is_4bytes(s) ? 4 : 3;
>  s->pos = 0;
>  s->len = 0;
>  s->state = STATE_COLLECTING_DATA;
> 
> This hunk should be brought forward to this patch.
> 
> > If so, we could add a width attribute to 'struct Flash' and to something 
> > like :
> >
> > @@ -260,6 +263,7 @@ typedef struct Flash {
> >  uint8_t cmd_in_progress;
> >  uint64_t cur_addr;
> >  bool write_enable;
> > +uint8_t width;
> >
> >  int64_t dirty_page;
> >
> > @@ -401,6 +405,10 @@ static void complete_collecting_data(Fla
> >  s->cur_addr |= s->data[1] << 8;
> >  s->cur_addr |= s->data[2];
> >
> > +if (s->width == 4) {
> > +s->cur_addr = s->cur_addr << 8 | s->data[4];
> > +}
> > +
> >  s->state = STATE_IDLE;
> >
> >  switch (s->cmd_in_progress) {
> > @@ -446,7 +454,7 @@ static void decode_new_cmd(Flash *s, uin
> >  case DPP:
> >  case QPP:
> >  case PP:
> > -s->needed_bytes = 3;
> > +s->needed_bytes = s->width;
> >  s->pos = 0;
> >  s->len = 0;
> >  s->state = STATE_COLLECTING_DATA;
> > @@ -644,6 +658,7 @@ static int m25p80_init(SSISla

Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support added.

2015-12-22 Thread Peter Crosthwaite

On Tue, Dec 22, 2015 at 10:41 AM, Cédric Le Goater  wrote:
> Hello Marcin,
>
>
> On 12/16/2015 01:57 PM, marcin.krzemin...@nokia.com wrote:
>> From: Marcin Krzeminski 
>>
>> Signed-off-by: Marcin Krzeminski 
>> ---
>>  hw/block/m25p80.c | 31 ---
>>  1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 1a547ae..6d5d90d 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -237,6 +237,9 @@ typedef enum {
>>  ERASE_32K = 0x52,
>>  ERASE_SECTOR = 0xd8,
>>
>> +EN_4BYTE_ADDR = 0xB7,
>> +EX_4BYTE_ADDR = 0xE9,
>> +
>>  RESET_ENABLE = 0x66,
>>  RESET_MEMORY = 0x99,
>>
>> @@ -267,6 +270,7 @@ typedef struct Flash {
>>  uint8_t cmd_in_progress;
>>  uint64_t cur_addr;
>>  bool write_enable;
>> +bool four_bytes_address_mode;
>>  bool reset_enable;
>>  bool initialized;
>>  uint8_t reset_pin;
>> @@ -405,11 +409,24 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t 
>> data)
>>  s->dirty_page = page;
>>  }
>>
>> +static inline int is_4bytes(Flash *s)
>> +{
>> +   return s->four_bytes_address_mode;
>> +   }
>> +}
>> +
>>  static void complete_collecting_data(Flash *s)
>>  {
>> -s->cur_addr = s->data[0] << 16;
>> -s->cur_addr |= s->data[1] << 8;
>> -s->cur_addr |= s->data[2];
>> +if (is_4bytes(s)) {
>> +s->cur_addr = s->data[0] << 24;
>> +s->cur_addr |= s->data[1] << 16;
>> +s->cur_addr |= s->data[2] << 8;
>> +s->cur_addr |= s->data[3];
>> +} else {
>> +s->cur_addr = s->data[0] << 16;
>> +s->cur_addr |= s->data[1] << 8;
>> +s->cur_addr |= s->data[2];
>> +}
>>
>>  s->state = STATE_IDLE;
>
>
> Don't we need to also change 'needed_bytes' in the decode_new_cmd() routine
> to increase the number of bytes expected by some commands ?
>

I think you are right, and it may be solved later in the series, from patch 10:

 case QPP:
 case PP:
-s->needed_bytes = 3;
+   s->needed_bytes = is_4bytes(s) ? 4 : 3;
 s->pos = 0;
 s->len = 0;
 s->state = STATE_COLLECTING_DATA;

This hunk should be brought forward to this patch.

> If so, we could add a width attribute to 'struct Flash' and to something like 
> :
>
> @@ -260,6 +263,7 @@ typedef struct Flash {
>  uint8_t cmd_in_progress;
>  uint64_t cur_addr;
>  bool write_enable;
> +uint8_t width;
>
>  int64_t dirty_page;
>
> @@ -401,6 +405,10 @@ static void complete_collecting_data(Fla
>  s->cur_addr |= s->data[1] << 8;
>  s->cur_addr |= s->data[2];
>
> +if (s->width == 4) {
> +s->cur_addr = s->cur_addr << 8 | s->data[4];
> +}
> +
>  s->state = STATE_IDLE;
>
>  switch (s->cmd_in_progress) {
> @@ -446,7 +454,7 @@ static void decode_new_cmd(Flash *s, uin
>  case DPP:
>  case QPP:
>  case PP:
> -s->needed_bytes = 3;
> +s->needed_bytes = s->width;
>  s->pos = 0;
>  s->len = 0;
>  s->state = STATE_COLLECTING_DATA;
> @@ -644,6 +658,7 @@ static int m25p80_init(SSISlave *ss)
>  memset(s->storage, 0xFF, s->size);
>  }
>
> +s->width = 3;
>  return 0;
>  }
>
>
>
> QOR, DIOR, QIOR command also need a check. I suppose an address and some
> number of dummy bytes are expected before the fast read command is done.
> I would need to check the datasheets.
>

I just checked an n25q256 datasheet, and yes you are right. The same
logic as in the hunk above needs to apply to these commands. CC
Xilinx, this bug is in their tree as well I think.

https://github.com/Xilinx/qemu/blob/pub/2015.2.plnx/hw/block/m25p80.c

Where PP, READ and friends have the 4 byte correction logic based on
addr_4b but QIOR does not.

Nice catch :)

Regards,
Peter

> Cheers,
>
> C.
>
>> @@ -446,6 +463,7 @@ static void reset_memory(Flash *s)
>>  {
>>  s->cmd_in_progress = NOP;
>>  s->cur_addr = 0;
>> +s->four_bytes_address_mode = false;
>>  s->len = 0;
>>  s->needed_bytes = 0;
>>  s->pos = 0;
>> @@ -565,6 +583,12 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>  break;
>>  case NOP:
>>  break;
>> +case EN_4BYTE_ADDR:
>> +s->four_bytes_address_mode = true;
>> +break;
>> +case EX_4BYTE_ADDR:
>> +s->four_bytes_address_mode = false;
>> +break;
>>  case RESET_ENABLE:
>>  s->reset_enable = true;
>>  break;
>> @@ -715,6 +739,7 @@ static const VMStateDescription vmstate_m25p80 = {
>>  VMSTATE_UINT8(cmd_in_progress, Flash),
>>  VMSTATE_UINT64(cur_addr, Flash),
>>  VMSTATE_BOOL(write_enable, Flash),

Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support added.

2015-12-22 Thread Cédric Le Goater

Hello Marcin,


On 12/16/2015 01:57 PM, marcin.krzemin...@nokia.com wrote:
> From: Marcin Krzeminski 
> 
> Signed-off-by: Marcin Krzeminski 
> ---
>  hw/block/m25p80.c | 31 ---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 1a547ae..6d5d90d 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -237,6 +237,9 @@ typedef enum {
>  ERASE_32K = 0x52,
>  ERASE_SECTOR = 0xd8,
> 
> +EN_4BYTE_ADDR = 0xB7,
> +EX_4BYTE_ADDR = 0xE9,
> +
>  RESET_ENABLE = 0x66,
>  RESET_MEMORY = 0x99,
> 
> @@ -267,6 +270,7 @@ typedef struct Flash {
>  uint8_t cmd_in_progress;
>  uint64_t cur_addr;
>  bool write_enable;
> +bool four_bytes_address_mode;
>  bool reset_enable;
>  bool initialized;
>  uint8_t reset_pin;
> @@ -405,11 +409,24 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
>  s->dirty_page = page;
>  }
> 
> +static inline int is_4bytes(Flash *s)
> +{
> +   return s->four_bytes_address_mode;
> +   }
> +}
> +
>  static void complete_collecting_data(Flash *s)
>  {
> -s->cur_addr = s->data[0] << 16;
> -s->cur_addr |= s->data[1] << 8;
> -s->cur_addr |= s->data[2];
> +if (is_4bytes(s)) {
> +s->cur_addr = s->data[0] << 24;
> +s->cur_addr |= s->data[1] << 16;
> +s->cur_addr |= s->data[2] << 8;
> +s->cur_addr |= s->data[3];
> +} else {
> +s->cur_addr = s->data[0] << 16;
> +s->cur_addr |= s->data[1] << 8;
> +s->cur_addr |= s->data[2];
> +}
> 
>  s->state = STATE_IDLE;


Don't we need to also change 'needed_bytes' in the decode_new_cmd() routine
to increase the number of bytes expected by some commands ? 

If so, we could add a width attribute to 'struct Flash' and to something like :

@@ -260,6 +263,7 @@ typedef struct Flash {
 uint8_t cmd_in_progress;
 uint64_t cur_addr;
 bool write_enable;
+uint8_t width;
 
 int64_t dirty_page;
 
@@ -401,6 +405,10 @@ static void complete_collecting_data(Fla
 s->cur_addr |= s->data[1] << 8;
 s->cur_addr |= s->data[2];
 
+if (s->width == 4) {
+s->cur_addr = s->cur_addr << 8 | s->data[4];
+}
+
 s->state = STATE_IDLE;
 
 switch (s->cmd_in_progress) {
@@ -446,7 +454,7 @@ static void decode_new_cmd(Flash *s, uin
 case DPP:
 case QPP:
 case PP:
-s->needed_bytes = 3;
+s->needed_bytes = s->width;
 s->pos = 0;
 s->len = 0;
 s->state = STATE_COLLECTING_DATA;
@@ -644,6 +658,7 @@ static int m25p80_init(SSISlave *ss)
 memset(s->storage, 0xFF, s->size);
 }
 
+s->width = 3;
 return 0;
 }
 


QOR, DIOR, QIOR command also need a check. I suppose an address and some 
number of dummy bytes are expected before the fast read command is done. 
I would need to check the datasheets.

Cheers,

C.

> @@ -446,6 +463,7 @@ static void reset_memory(Flash *s)
>  {
>  s->cmd_in_progress = NOP;
>  s->cur_addr = 0;
> +s->four_bytes_address_mode = false;
>  s->len = 0;
>  s->needed_bytes = 0;
>  s->pos = 0;
> @@ -565,6 +583,12 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>  break;
>  case NOP:
>  break;
> +case EN_4BYTE_ADDR:
> +s->four_bytes_address_mode = true;
> +break;
> +case EX_4BYTE_ADDR:
> +s->four_bytes_address_mode = false;
> +break;
>  case RESET_ENABLE:
>  s->reset_enable = true;
>  break;
> @@ -715,6 +739,7 @@ static const VMStateDescription vmstate_m25p80 = {
>  VMSTATE_UINT8(cmd_in_progress, Flash),
>  VMSTATE_UINT64(cur_addr, Flash),
>  VMSTATE_BOOL(write_enable, Flash),
> +VMSTATE_BOOL(four_bytes_address_mode, Flash),
>  VMSTATE_BOOL(reset_enable, Flash),
>  VMSTATE_BOOL(initialized, Flash),
>  VMSTATE_UINT8(reset_pin, Flash),
>

Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support added.

2015-12-21 Thread Peter Crosthwaite

Commit subject subsystem prefix (block: m25p80:) needed.

On Wed, Dec 16, 2015 at 4:57 AM,   wrote:
> From: Marcin Krzeminski 
>
> Signed-off-by: Marcin Krzeminski 
> ---
>  hw/block/m25p80.c | 31 ---
>  1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 1a547ae..6d5d90d 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -237,6 +237,9 @@ typedef enum {
>  ERASE_32K = 0x52,
>  ERASE_SECTOR = 0xd8,
>
> +EN_4BYTE_ADDR = 0xB7,
> +EX_4BYTE_ADDR = 0xE9,
> +
>  RESET_ENABLE = 0x66,
>  RESET_MEMORY = 0x99,
>
> @@ -267,6 +270,7 @@ typedef struct Flash {
>  uint8_t cmd_in_progress;
>  uint64_t cur_addr;
>  bool write_enable;
> +bool four_bytes_address_mode;
>  bool reset_enable;
>  bool initialized;
>  uint8_t reset_pin;
> @@ -405,11 +409,24 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
>  s->dirty_page = page;
>  }
>
> +static inline int is_4bytes(Flash *s)
> +{
> +   return s->four_bytes_address_mode;
> +   }
> +}
> +
>  static void complete_collecting_data(Flash *s)
>  {
> -s->cur_addr = s->data[0] << 16;
> -s->cur_addr |= s->data[1] << 8;
> -s->cur_addr |= s->data[2];
> +if (is_4bytes(s)) {
> +s->cur_addr = s->data[0] << 24;
> +s->cur_addr |= s->data[1] << 16;
> +s->cur_addr |= s->data[2] << 8;
> +s->cur_addr |= s->data[3];
> +} else {
> +s->cur_addr = s->data[0] << 16;
> +s->cur_addr |= s->data[1] << 8;
> +s->cur_addr |= s->data[2];
> +}

Avoid the duped code with a loop:

s->cur_addr = 0;
for (i = 0; i < (is_4bytes(s) ? 4 : 3); i++) {
s->cur_addr <<= 8;
s->cur_addr |= s->data[i];
}

Otherwise:

Reviewed-by: Peter Crosthwaite 

Regards,
Peter

>
>  s->state = STATE_IDLE;
>
> @@ -446,6 +463,7 @@ static void reset_memory(Flash *s)
>  {
>  s->cmd_in_progress = NOP;
>  s->cur_addr = 0;
> +s->four_bytes_address_mode = false;
>  s->len = 0;
>  s->needed_bytes = 0;
>  s->pos = 0;
> @@ -565,6 +583,12 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>  break;
>  case NOP:
>  break;
> +case EN_4BYTE_ADDR:
> +s->four_bytes_address_mode = true;
> +break;
> +case EX_4BYTE_ADDR:
> +s->four_bytes_address_mode = false;
> +break;
>  case RESET_ENABLE:
>  s->reset_enable = true;
>  break;
> @@ -715,6 +739,7 @@ static const VMStateDescription vmstate_m25p80 = {
>  VMSTATE_UINT8(cmd_in_progress, Flash),
>  VMSTATE_UINT64(cur_addr, Flash),
>  VMSTATE_BOOL(write_enable, Flash),
> +VMSTATE_BOOL(four_bytes_address_mode, Flash),
>  VMSTATE_BOOL(reset_enable, Flash),
>  VMSTATE_BOOL(initialized, Flash),
>  VMSTATE_UINT8(reset_pin, Flash),
> --
> 2.5.0
>
>

Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support added.

Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support added.

Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support added.

Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support added.

4 matches

Site Navigation

Mail list logo

Footer information