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

[francisco iglesias]

Hi Sai,

[PATCH v1] xilinx_spips: send dummy only if cmd requires it

s/dummy/dummy cycles/

On 17 April 2018 at 16:18, Sai Pavan Boddu <address@hidden>
wrote:

> For all the commands, which do not have an entry in
> xilinx_spips_num_dummies, present logic sends dummy byte when ever we
>

s/dummy byte/dummy cycles/


> are in SNOOP_NONE state, fix it to send only if cmd requires them.
>
> s/fix it to send only if cmd/fix it to only send dummy cycles if the
command/

Only transmit max of 1 dummy byte(i.e 8 cycles) is a single snoop cycle.
> And also convert dummy bytes to cycles (required by m25p80).
>

Maybe it is better to drop this two last lines (was already done before so
it could be misleading when reading git history).


>
>
Signed-off-by: Sai Pavan Boddu <address@hidden>
> ---
>  hw/ssi/xilinx_spips.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index 426f971..8278930 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -627,10 +627,17 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS
> *s)
>                  tx_rx[i] = tx;
>              }
>          } else {
> -            /* 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;
> +            if (s->cmd_dummies > 0) {
> +                /* Extract a dummy byte and generate dummy cycles
> according to
> +                 * the link state */
> +                 dummy_cycles = (s->cmd_dummies ? 1 : 0) * 8 /
> s->link_state;
> +                 s->cmd_dummies--;
> +            } else {
> +                for (i = 0; i < num_effective_busses(s); ++i) {
> +                    tx_rx[i] = tx;
> +                }
> +            }
>          }
>
>
Could we replace above with below in the same if ladder so we don't
complicate the code more than necessary? (Should give the same result when
num_effective_busses == 1)

-        } else if (s->snoop_state == SNOOP_STRIPING) {
+        } else if (s->snoop_state == SNOOP_STRIPING ||
+                   s->snoop_state == SNOOP_NONE) {


Thank you!

Best regards,
Francisco Iglesias




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