[Qemu-devel] [PATCH v2 0/1] xilinx_spips dummy bytes fix

[Qemu-devel] [PATCH v2 0/1] xilinx_spips dummy bytes fix

[Sai Pavan Boddu]

For zynq-7000 boards, we saw issues when u-boot/linux accessing the qspi.
Issue is due to wrong dummy byte transfers for few of un-handled commands ex: 0x9F, 0x6, 0xe9, 0x4, 0x18, etc
There are many unhandled commands which do not require dummy bytes but they can be followed by address cycles.

To fix this issue for above commands, the next upcoming tx bytes are not considered dummies rather sent to slave directly.

Changes for V2:
	Fixed commit message
	Reordered to code, to avoid code duplication.


Sai Pavan Boddu (1):
  xilinx_spips: send dummy cycles only if cmd requires it

 hw/ssi/xilinx_spips.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/1] xilinx_spips: send dummy cycles only if cmd requires it

[Sai Pavan Boddu]

For all the commands, which do not have an entry in
xilinx_spips_num_dummies, present logic sends dummy cycles when ever we
are in SNOOP_NONE state, fix it to only send dummy cycles if the cmd
requires them.

SNOOP_NONE state handle is moved above (previously its part of default else
case), as its same as SNOOP_STRIPPING during data cycles.

Signed-off-by: Sai Pavan Boddu <saipava@xilinx.com>
---
TODO: Maybe we could delete the SNOOP_NONE state as it seems to be
same as SNOOP_STRIPPING

 hw/ssi/xilinx_spips.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index 426f971..4f6760d 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -616,7 +616,8 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
         if (fifo8_is_empty(&s->tx_fifo)) {
             xilinx_spips_update_ixr(s);
             return;
-        } else if (s->snoop_state == SNOOP_STRIPING) {
+        } else if (s->snoop_state == SNOOP_STRIPING ||
+                   s->snoop_state == SNOOP_NONE) {
             for (i = 0; i < num_effective_busses(s); ++i) {
                 tx_rx[i] = fifo8_pop(&s->tx_fifo);
             }
@@ -626,11 +627,12 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
             for (i = 0; i < num_effective_busses(s); ++i) {
                 tx_rx[i] = tx;
             }
-        } else {
+        } else if (s->cmd_dummies > 0) {
             /* Extract a dummy byte and generate dummy cycles according to the
              * link state */
             tx = fifo8_pop(&s->tx_fifo);
             dummy_cycles = 8 / s->link_state;
+            s->cmd_dummies--;
         }
 
         for (i = 0; i < num_effective_busses(s); ++i) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 1/1] xilinx_spips: send dummy cycles only if cmd requires it

[francisco iglesias]

Hi Sai,

About the subject, what do you think about changing it to below?
xilinx_spips: Correct SNOOP_NONE state when flushing the txfifo

On 18 April 2018 at 09:41, Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
wrote:

> For all the commands, which do not have an entry in
> xilinx_spips_num_dummies, present logic sends dummy cycles when ever we
> are in SNOOP_NONE state, fix it to only send dummy cycles if the cmd
> requires them.
>
> It feels like below sentence goes together with the 'fix' above so maybe
it could come directly after? (i.e. no new section but maybe with an 'also'
after 'handle is')

> SNOOP_NONE state handle is moved above (previously its part of default
> else

s/above/above in the if ladder/
s/its part of default/it was part of the default/

> case), as its same as SNOOP_STRIPPING during data cycles.

s/its/it's/

>
>
Signed-off-by: Sai Pavan Boddu <saipava@xilinx.com>
> ---
> TODO: Maybe we could delete the SNOOP_NONE state as it seems to be
> same as SNOOP_STRIPPING

I agree that a cleanup commit later on could be considered, but I think
it's better not to include the todo in the commit message of this one so
could we remove it?


>
>  hw/ssi/xilinx_spips.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index 426f971..4f6760d 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -616,7 +616,8 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
>          if (fifo8_is_empty(&s->tx_fifo)) {
>              xilinx_spips_update_ixr(s);
>              return;
> -        } else if (s->snoop_state == SNOOP_STRIPING) {
> +        } else if (s->snoop_state == SNOOP_STRIPING ||
> +                   s->snoop_state == SNOOP_NONE) {
>              for (i = 0; i < num_effective_busses(s); ++i) {
>                  tx_rx[i] = fifo8_pop(&s->tx_fifo);
>              }
> @@ -626,11 +627,12 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS
> *s)
>              for (i = 0; i < num_effective_busses(s); ++i) {
>                  tx_rx[i] = tx;
>              }
> -        } else {
> +        } else if (s->cmd_dummies > 0) {
>
The variable snoop_state already keeps track of the dummy cycles, the same
the 'else' to 'else if' change above together with the 'cmd_dummies'
decrementation below does, could we undo these two changes since it is
already handled?

Thank you!

Best regards,
Francisco Iglesias

             /* Extract a dummy byte and generate dummy cycles according to
> the
>               * link state */
>              tx = fifo8_pop(&s->tx_fifo);
>              dummy_cycles = 8 / s->link_state;
> +            s->cmd_dummies--;
>          }
>
>          for (i = 0; i < num_effective_busses(s); ++i) {
> --
> 2.7.4
>
>
>

Re: [Qemu-devel] [PATCH v2 1/1] xilinx_spips: send dummy cycles only if cmd requires it

[Sai Pavan Boddu]

HI Franciso,

From: francisco iglesias [mailto:frasse.iglesias@gmail.com]
Sent: Thursday, April 19, 2018 3:39 AM
To: Sai Pavan Boddu <saipava@xilinx.com>
Cc: alistair@alistair23.me; Peter Crosthwaite <crosthwaite.peter@gmail.com>; Peter Maydell <peter.maydell@linaro.org>; Edde <edgar.iglesias@gmail.com>; qemu-devel@nongnu.org Developers <qemu-devel@nongnu.org>
Subject: Re: [Qemu-devel] [PATCH v2 1/1] xilinx_spips: send dummy cycles only if cmd requires it

Hi Sai,

About the subject, what do you think about changing it to below?
xilinx_spips: Correct SNOOP_NONE state when flushing the txfifo

On 18 April 2018 at 09:41, Sai Pavan Boddu <sai.pavan.boddu@xilinx.com<mailto:sai.pavan.boddu@xilinx.com>> wrote:
For all the commands, which do not have an entry in
xilinx_spips_num_dummies, present logic sends dummy cycles when ever we
are in SNOOP_NONE state, fix it to only send dummy cycles if the cmd
requires them.
It feels like below sentence goes together with the 'fix' above so maybe it could come directly after? (i.e. no new section but maybe with an 'also' after 'handle is')
SNOOP_NONE state handle is moved above (previously its part of default else
s/above/above in the if ladder/
s/its part of default/it was part of the default/
case), as its same as SNOOP_STRIPPING during data cycles.
s/its/it's/

Signed-off-by: Sai Pavan Boddu <saipava@xilinx.com<mailto:saipava@xilinx.com>>
---
TODO: Maybe we could delete the SNOOP_NONE state as it seems to be
same as SNOOP_STRIPPING
I agree that a cleanup commit later on could be considered, but I think it's better not to include the todo in the commit message of this one so could we remove it?


 hw/ssi/xilinx_spips.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index 426f971..4f6760d 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -616,7 +616,8 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
         if (fifo8_is_empty(&s->tx_fifo)) {
             xilinx_spips_update_ixr(s);
             return;
-        } else if (s->snoop_state == SNOOP_STRIPING) {
+        } else if (s->snoop_state == SNOOP_STRIPING ||
+                   s->snoop_state == SNOOP_NONE) {
             for (i = 0; i < num_effective_busses(s); ++i) {
                 tx_rx[i] = fifo8_pop(&s->tx_fifo);
             }
@@ -626,11 +627,12 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
             for (i = 0; i < num_effective_busses(s); ++i) {
                 tx_rx[i] = tx;
             }
-        } else {
+        } else if (s->cmd_dummies > 0) {
The variable snoop_state already keeps track of the dummy cycles, the same the 'else' to 'else if' change above together with the 'cmd_dummies' decrementation below does, could we undo these two changes since it is already handled?
[Sai Pavan Boddu] Yes, you are right. After we moved SNOOP_NONE above. We don’t need an of these changes here. Wonderful that it patch got minimized to single line now.

Regards,
Sai Pavan

Thank you!

Best regards,
Francisco Iglesias

             /* Extract a dummy byte and generate dummy cycles according to the
              * link state */
             tx = fifo8_pop(&s->tx_fifo);
             dummy_cycles = 8 / s->link_state;
+            s->cmd_dummies--;
         }

         for (i = 0; i < num_effective_busses(s); ++i) {
--
2.7.4