Re: [PATCH v3 3/3] hw/ssi/pnv_spi: Use local var seq_index instead of get_seq_index().

[Chalapathi V <chalapathi.v@linux.ibm.com>]

On 08-10-2024 13:43, Nicholas Piggin wrote:
> On Thu Sep 19, 2024 at 2:50 AM AEST, Chalapathi V wrote:
>> Use a local variable seq_index instead of repeatedly caling
>> get_seq_index() method.
>>
>> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
>> ---
>>   hw/ssi/pnv_spi.c | 61 ++++++++++++++++++++++++------------------------
>>   1 file changed, 31 insertions(+), 30 deletions(-)
>>
>> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
>> index 2fd5aa0a96..962115f40f 100644
>> --- a/hw/ssi/pnv_spi.c
>> +++ b/hw/ssi/pnv_spi.c
>> @@ -210,15 +210,8 @@ static void transfer(PnvSpi *s)
>>       fifo8_reset(&s->rx_fifo);
>>   }
>>   
>> -static inline uint8_t get_seq_index(PnvSpi *s)
>> -{
>> -    return GETFIELD(SPI_STS_SEQ_INDEX, s->status);
>> -}
>> -
>>   static inline void next_sequencer_fsm(PnvSpi *s)
>>   {
>> -    uint8_t seq_index = get_seq_index(s);
>> -    s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, (seq_index + 1));
>>       s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_INDEX_INCREMENT);
>>   }
>>   
>> @@ -647,6 +640,7 @@ static void operation_sequencer(PnvSpi *s)
>>       bool stop = false; /* Flag to stop the sequencer */
>>       uint8_t opcode = 0;
>>       uint8_t masked_opcode = 0;
>> +    uint8_t seq_index;
>>   
>>       /*
>>        * Clear the sequencer FSM error bit - general_SPI_status[3]
>> @@ -660,12 +654,13 @@ static void operation_sequencer(PnvSpi *s)
>>       if (GETFIELD(SPI_STS_SEQ_FSM, s->status) == SEQ_STATE_IDLE) {
>>           s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, 0);
>>       }
>> +    seq_index = GETFIELD(SPI_STS_SEQ_INDEX, s->status);
>>       /*
>>        * There are only 8 possible operation IDs to iterate through though
>>        * some operations may cause more than one frame to be sequenced.
>>        */
>> -    while (get_seq_index(s) < NUM_SEQ_OPS) {
>> -        opcode = s->seq_op[get_seq_index(s)];
>> +    while (seq_index < NUM_SEQ_OPS) {
>> +        opcode = s->seq_op[seq_index];
>>           /* Set sequencer state to decode */
>>           s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_DECODE);
>>           /*
>> @@ -682,7 +677,7 @@ static void operation_sequencer(PnvSpi *s)
>>           case SEQ_OP_STOP:
>>               s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
>>               /* A stop operation in any position stops the sequencer */
>> -            trace_pnv_spi_sequencer_op("STOP", get_seq_index(s));
>> +            trace_pnv_spi_sequencer_op("STOP", seq_index);
>>   
>>               stop = true;
>>               s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_IDLE);
>> @@ -693,7 +688,7 @@ static void operation_sequencer(PnvSpi *s)
>>   
>>           case SEQ_OP_SELECT_SLAVE:
>>               s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
>> -            trace_pnv_spi_sequencer_op("SELECT_SLAVE", get_seq_index(s));
>> +            trace_pnv_spi_sequencer_op("SELECT_SLAVE", seq_index);
>>               /*
>>                * This device currently only supports a single responder
>>                * connection at position 0.  De-selecting a responder is fine
>> @@ -704,8 +699,7 @@ static void operation_sequencer(PnvSpi *s)
>>               if (s->responder_select == 0) {
>>                   trace_pnv_spi_shifter_done();
>>                   qemu_set_irq(s->cs_line[0], 1);
>> -                s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
>> -                                (get_seq_index(s) + 1));
>> +                seq_index++;
>>                   s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_DONE);
>>               } else if (s->responder_select != 1) {
>>                   qemu_log_mask(LOG_GUEST_ERROR, "Slave selection other than 1 "
>> @@ -732,13 +726,14 @@ static void operation_sequencer(PnvSpi *s)
>>                    * applies once a valid responder select has occurred.
>>                    */
>>                   s->shift_n1_done = false;
>> +                seq_index++;
>>                   next_sequencer_fsm(s);
> Maybe could just open-code next_sequencer_fsm() now, since a bunch of
> other FSM fields seem to be open-coded?
Sure.
>
>>               }
>>               break;
>>   
>>           case SEQ_OP_SHIFT_N1:
>>               s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
>> -            trace_pnv_spi_sequencer_op("SHIFT_N1", get_seq_index(s));
>> +            trace_pnv_spi_sequencer_op("SHIFT_N1", seq_index);
>>               /*
>>                * Only allow a shift_n1 when the state is not IDLE or DONE.
>>                * In either of those two cases the sequencer is not in a proper
>> @@ -770,8 +765,9 @@ static void operation_sequencer(PnvSpi *s)
>>                    * transmission to the responder without requiring a refill of
>>                    * the TDR between the two operations.
>>                    */
>> -                if (PNV_SPI_MASKED_OPCODE(s->seq_op[get_seq_index(s) + 1])
>> -                                == SEQ_OP_SHIFT_N2) {
>> +                if ((seq_index != 7) &&
>> +                    PNV_SPI_MASKED_OPCODE(s->seq_op[(seq_index + 1)]) ==
>> +                    SEQ_OP_SHIFT_N2) {
>>                       send_n1_alone = false;
>>                   }
>>                   s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
> The seq_index != 7 is a new test? Is that a separate fix, I'm not
> seeing how it's related to the seq_index change.
Not a new test but to make sure array index of seq_op doesn't overflow 
due to seq_index + 1.
>
> Thanks,
> Nick