diff mbox

m25p80: Fix QIOR/DIOR handling for Winbond

Message ID 1467809036-6986-1-git-send-email-marcin.krzeminski@nokia.com (mailing list archive)
State New, archived
Headers show

Commit Message

marcin.krzeminski@nokia.com July 6, 2016, 12:43 p.m. UTC 
From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Winbond also support continuous read mode, but as an opposite for other
flash type read mode clock cycles are included to dummy cycles number.
This path add proper handling of read mode byte and update needed
dummy cycles. QPI mode and dummy cycles configuration are not supported.

Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
---
 hw/block/m25p80.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Cédric Le Goater July 7, 2016, 12:57 p.m. UTC | #1 
Hello Marcin,

On 07/06/2016 02:43 PM, marcin.krzeminski@nokia.com wrote:
> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> 
> Winbond also support continuous read mode, but as an opposite for other
> flash type read mode clock cycles are included to dummy cycles number.
> This path add proper handling of read mode byte and update needed
> dummy cycles. QPI mode and dummy cycles configuration are not supported.
> 
> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> ---
>  hw/block/m25p80.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index d9b2793..a349544 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -149,6 +149,7 @@ typedef struct FlashPartInfo {
>  */
>  
>  #define SPANSION_CONTINUOUS_READ_MODE_CMD_LEN 1
> +#define WINBOND_CONTINUOUS_READ_MODE_CMD_LEN 1

I checked the W25Q256FV datasheet and

>  static const FlashPartInfo known_devices[] = {
>      /* Atmel -- some are (confusingly) marketed as "DataFlash" */
> @@ -775,7 +776,7 @@ static void decode_dio_read_cmd(Flash *s)
>      /* Dummy cycles modeled with bytes writes instead of bits */
>      switch (get_man(s)) {
>      case MAN_WINBOND:
> -        s->needed_bytes += 8;
> +        s->needed_bytes += WINBOND_CONTINUOUS_READ_MODE_CMD_LEN;

this looks fine,

>          break;
>      case MAN_SPANSION:
>          s->needed_bytes += SPANSION_CONTINUOUS_READ_MODE_CMD_LEN;
> @@ -814,7 +815,8 @@ static void decode_qio_read_cmd(Flash *s)
>      /* Dummy cycles modeled with bytes writes instead of bits */
>      switch (get_man(s)) {
>      case MAN_WINBOND:
> -        s->needed_bytes += 8;
> +        s->needed_bytes += WINBOND_CONTINUOUS_READ_MODE_CMD_LEN;
> +        s->needed_bytes += 4;

but I don't understand the above. I see the address + M7-0 + 2 dummies.

Thanks,

C. 

>          break;
>      case MAN_SPANSION:
>          s->needed_bytes += SPANSION_CONTINUOUS_READ_MODE_CMD_LEN;
>
marcin.krzeminski@nokia.com July 7, 2016, 1:04 p.m. UTC | #2 
> -----Original Message-----
> From: Cédric Le Goater [mailto:clg@kaod.org]
> Sent: Thursday, July 07, 2016 2:57 PM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <marcin.krzeminski@nokia.com>; qemu-devel@nongnu.org
> Cc: crosthwaitepeter@gmail.com; rfsw-patches@mlist.nokia.com;
> peter.maydell@linaro.org; clg@fr.ibm.com
> Subject: Re: [PATCH] m25p80: Fix QIOR/DIOR handling for Winbond
> 
> Hello Marcin,
> 
> On 07/06/2016 02:43 PM, marcin.krzeminski@nokia.com wrote:
> > From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> >
> > Winbond also support continuous read mode, but as an opposite for
> > other flash type read mode clock cycles are included to dummy cycles
> number.
> > This path add proper handling of read mode byte and update needed
> > dummy cycles. QPI mode and dummy cycles configuration are not
> supported.
> >
> > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> > ---
> >  hw/block/m25p80.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index
> > d9b2793..a349544 100644
> > --- a/hw/block/m25p80.c
> > +++ b/hw/block/m25p80.c
> > @@ -149,6 +149,7 @@ typedef struct FlashPartInfo {  */
> >
> >  #define SPANSION_CONTINUOUS_READ_MODE_CMD_LEN 1
> > +#define WINBOND_CONTINUOUS_READ_MODE_CMD_LEN 1
> 
> I checked the W25Q256FV datasheet and
> 
> >  static const FlashPartInfo known_devices[] = {
> >      /* Atmel -- some are (confusingly) marketed as "DataFlash" */ @@
> > -775,7 +776,7 @@ static void decode_dio_read_cmd(Flash *s)
> >      /* Dummy cycles modeled with bytes writes instead of bits */
> >      switch (get_man(s)) {
> >      case MAN_WINBOND:
> > -        s->needed_bytes += 8;
> > +        s->needed_bytes +=
> WINBOND_CONTINUOUS_READ_MODE_CMD_LEN;
> 
> this looks fine,
> 
> >          break;
> >      case MAN_SPANSION:
> >          s->needed_bytes +=
> SPANSION_CONTINUOUS_READ_MODE_CMD_LEN;
> > @@ -814,7 +815,8 @@ static void decode_qio_read_cmd(Flash *s)
> >      /* Dummy cycles modeled with bytes writes instead of bits */
> >      switch (get_man(s)) {
> >      case MAN_WINBOND:
> > -        s->needed_bytes += 8;
> > +        s->needed_bytes +=
> WINBOND_CONTINUOUS_READ_MODE_CMD_LEN;
> > +        s->needed_bytes += 4;
> 
> but I don't understand the above. I see the address + M7-0 + 2 dummies.
> 
Hello Cedric,

This is because the problematic interface in here. Generally you need
to send 2 dummy bytes, but this is four clocks in this mode (Fig 24a).
I want to send a proposal to change this dummy clock interface
after 2.7 will be released, because now it is really confusing and unclear.

Thanks,
Marcin

> Thanks,
> 
> C.
> 
> >          break;
> >      case MAN_SPANSION:
> >          s->needed_bytes +=
> SPANSION_CONTINUOUS_READ_MODE_CMD_LEN;
> >
Peter Maydell July 11, 2016, 6:05 p.m. UTC | #3 
On 7 July 2016 at 14:04, Krzeminski, Marcin (Nokia - PL/Wroclaw)
<marcin.krzeminski@nokia.com> wrote:
>
>
>> -----Original Message-----
>> From: Cédric Le Goater [mailto:clg@kaod.org]

>> On 07/06/2016 02:43 PM, marcin.krzeminski@nokia.com wrote:
>>
>> >          break;
>> >      case MAN_SPANSION:
>> >          s->needed_bytes +=
>> SPANSION_CONTINUOUS_READ_MODE_CMD_LEN;
>> > @@ -814,7 +815,8 @@ static void decode_qio_read_cmd(Flash *s)
>> >      /* Dummy cycles modeled with bytes writes instead of bits */
>> >      switch (get_man(s)) {
>> >      case MAN_WINBOND:
>> > -        s->needed_bytes += 8;
>> > +        s->needed_bytes +=
>> WINBOND_CONTINUOUS_READ_MODE_CMD_LEN;
>> > +        s->needed_bytes += 4;
>>
>> but I don't understand the above. I see the address + M7-0 + 2 dummies.
>>
> Hello Cedric,
>
> This is because the problematic interface in here. Generally you need
> to send 2 dummy bytes, but this is four clocks in this mode (Fig 24a).
> I want to send a proposal to change this dummy clock interface
> after 2.7 will be released, because now it is really confusing and unclear.

Cedric, could you let me know if you're happy with this patch now?
If you are you can provide a reviewed-by tag and I can apply it to
target-arm.next...

thanks
-- PMM
Cédric Le Goater July 12, 2016, 8:24 a.m. UTC | #4 
Hello Peter,

On 07/11/2016 08:05 PM, Peter Maydell wrote:
> On 7 July 2016 at 14:04, Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <marcin.krzeminski@nokia.com> wrote:
>>
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater [mailto:clg@kaod.org]
> 
>>> On 07/06/2016 02:43 PM, marcin.krzeminski@nokia.com wrote:
>>>
>>>>          break;
>>>>      case MAN_SPANSION:
>>>>          s->needed_bytes +=
>>> SPANSION_CONTINUOUS_READ_MODE_CMD_LEN;
>>>> @@ -814,7 +815,8 @@ static void decode_qio_read_cmd(Flash *s)
>>>>      /* Dummy cycles modeled with bytes writes instead of bits */
>>>>      switch (get_man(s)) {
>>>>      case MAN_WINBOND:
>>>> -        s->needed_bytes += 8;
>>>> +        s->needed_bytes +=
>>> WINBOND_CONTINUOUS_READ_MODE_CMD_LEN;
>>>> +        s->needed_bytes += 4;
>>>
>>> but I don't understand the above. I see the address + M7-0 + 2 dummies.
>>>
>> Hello Cedric,
>>
>> This is because the problematic interface in here. Generally you need
>> to send 2 dummy bytes, but this is four clocks in this mode (Fig 24a).
>> I want to send a proposal to change this dummy clock interface
>> after 2.7 will be released, because now it is really confusing and unclear.
> 
> Cedric, could you let me know if you're happy with this patch now?
> If you are you can provide a reviewed-by tag and I can apply it to
> target-arm.next...

Yes. It should be fine and Marcin has plans for 2.8 to clarify all that. 
Let's move forward :

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.
Peter Maydell July 12, 2016, 2:16 p.m. UTC | #5 
On 12 July 2016 at 09:24, Cédric Le Goater <clg@kaod.org> wrote:
> Yes. It should be fine and Marcin has plans for 2.8 to clarify all that.
> Let's move forward :
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>



Applied to target-arm.next, thanks.

-- PMM
diff mbox

Patch

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index d9b2793..a349544 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -149,6 +149,7 @@  typedef struct FlashPartInfo {
 */
 
 #define SPANSION_CONTINUOUS_READ_MODE_CMD_LEN 1
+#define WINBOND_CONTINUOUS_READ_MODE_CMD_LEN 1
 
 static const FlashPartInfo known_devices[] = {
     /* Atmel -- some are (confusingly) marketed as "DataFlash" */
@@ -775,7 +776,7 @@  static void decode_dio_read_cmd(Flash *s)
     /* Dummy cycles modeled with bytes writes instead of bits */
     switch (get_man(s)) {
     case MAN_WINBOND:
-        s->needed_bytes += 8;
+        s->needed_bytes += WINBOND_CONTINUOUS_READ_MODE_CMD_LEN;
         break;
     case MAN_SPANSION:
         s->needed_bytes += SPANSION_CONTINUOUS_READ_MODE_CMD_LEN;
@@ -814,7 +815,8 @@  static void decode_qio_read_cmd(Flash *s)
     /* Dummy cycles modeled with bytes writes instead of bits */
     switch (get_man(s)) {
     case MAN_WINBOND:
-        s->needed_bytes += 8;
+        s->needed_bytes += WINBOND_CONTINUOUS_READ_MODE_CMD_LEN;
+        s->needed_bytes += 4;
         break;
     case MAN_SPANSION:
         s->needed_bytes += SPANSION_CONTINUOUS_READ_MODE_CMD_LEN;