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:address@hidden
Sent: Thursday, April 19, 2018 3:39 AM
To: Sai Pavan Boddu <address@hidden>
Cc: address@hidden; Peter Crosthwaite <address@hidden>; Peter Maydell 
<address@hidden>; Edde <address@hidden>; address@hidden Developers 
<address@hidden>
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 
<address@hidden<mailto:address@hidden>> 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 <address@hidden<mailto:address@hidden>>
---
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