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);
}
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,
@@ -793,8 +789,7 @@ static void operation_sequencer(PnvSpi *s)
s->shift_n1_done = true;
s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
FSM_SHIFT_N2);
- s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
- (get_seq_index(s) + 1));
+ seq_index++;
} else {
/*
* This is case (1) or (2) so the sequencer needs to
@@ -806,6 +801,7 @@ static void operation_sequencer(PnvSpi *s)
} else {
/* Ok to move on to the next index */
s->shift_n1_done = true;
+ seq_index++;
next_sequencer_fsm(s);
}
}
@@ -813,7 +809,7 @@ static void operation_sequencer(PnvSpi *s)
case SEQ_OP_SHIFT_N2:
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
- trace_pnv_spi_sequencer_op("SHIFT_N2", get_seq_index(s));
+ trace_pnv_spi_sequencer_op("SHIFT_N2", seq_index);
if (!s->shift_n1_done) {
qemu_log_mask(LOG_GUEST_ERROR, "Shift_N2 is not allowed if a "
"Shift_N1 is not done, shifter state = 0x%llx",
@@ -841,6 +837,7 @@ static void operation_sequencer(PnvSpi *s)
FSM_WAIT);
} else {
/* Ok to move on to the next index */
+ seq_index++;
next_sequencer_fsm(s);
}
}
@@ -848,7 +845,7 @@ static void operation_sequencer(PnvSpi *s)
case SEQ_OP_BRANCH_IFNEQ_RDR:
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
- trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_RDR", get_seq_index(s));
+ trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_RDR", seq_index);
/*
* The memory mapping register RDR match value is compared against
* the 16 rightmost bytes of the RDR (potentially with masking).
@@ -864,6 +861,7 @@ static void operation_sequencer(PnvSpi *s)
if (rdr_matched) {
trace_pnv_spi_RDR_match("success");
/* A match occurred, increment the sequencer index. */
+ seq_index++;
next_sequencer_fsm(s);
} else {
trace_pnv_spi_RDR_match("failed");
@@ -871,8 +869,7 @@ static void operation_sequencer(PnvSpi *s)
* Branch the sequencer to the index coded into the op
* code.
*/
- s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
- PNV_SPI_OPCODE_LO_NIBBLE(opcode));
+ seq_index = PNV_SPI_OPCODE_LO_NIBBLE(opcode);
}
/*
* Regardless of where the branch ended up we want the
@@ -891,12 +888,13 @@ static void operation_sequencer(PnvSpi *s)
case SEQ_OP_TRANSFER_TDR:
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
qemu_log_mask(LOG_GUEST_ERROR, "Transfer TDR is not supported\n");
+ seq_index++;
next_sequencer_fsm(s);
break;
case SEQ_OP_BRANCH_IFNEQ_INC_1:
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
- trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_INC_1", get_seq_index(s));
+ trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_INC_1", seq_index);
/*
* The spec says the loop should execute count compare + 1 times.
* However we learned from engineering that we really only loop
@@ -910,18 +908,18 @@ static void operation_sequencer(PnvSpi *s)
* mask off all but the first three bits so we don't try to
* access beyond the sequencer_operation_reg boundary.
*/
- s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status,
- PNV_SPI_OPCODE_LO_NIBBLE(opcode));
+ seq_index = PNV_SPI_OPCODE_LO_NIBBLE(opcode);
s->loop_counter_1++;
} else {
/* Continue to next index if loop counter is reached */
+ seq_index++;
next_sequencer_fsm(s);
}
break;
case SEQ_OP_BRANCH_IFNEQ_INC_2:
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
- trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_INC_2", get_seq_index(s));
+ trace_pnv_spi_sequencer_op("BRANCH_IFNEQ_INC_2", seq_index);
uint8_t condition2 = GETFIELD(SPI_CTR_CFG_CMP2,
s->regs[SPI_CTR_CFG_REG]);
/*
@@ -936,11 +934,11 @@ static void operation_sequencer(PnvSpi *s)
* mask off all but the first three bits so we don't try to
* access beyond the sequencer_operation_reg boundary.
*/
- s->status = SETFIELD(SPI_STS_SEQ_INDEX,
- s->status, PNV_SPI_OPCODE_LO_NIBBLE(opcode));
+ seq_index = PNV_SPI_OPCODE_LO_NIBBLE(opcode);
s->loop_counter_2++;
} else {
/* Continue to next index if loop counter is reached */
+ seq_index++;
next_sequencer_fsm(s);
}
break;
@@ -948,6 +946,7 @@ static void operation_sequencer(PnvSpi *s)
default:
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_EXECUTE);
/* Ignore unsupported operations. */
+ seq_index++;
next_sequencer_fsm(s);
break;
} /* end of switch */
@@ -956,10 +955,10 @@ static void operation_sequencer(PnvSpi *s)
* we need to go ahead and end things as if there was a STOP at the
* end.
*/
- if (get_seq_index(s) == NUM_SEQ_OPS) {
+ if (seq_index == NUM_SEQ_OPS) {
/* All 8 opcodes completed, sequencer idling */
s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status, FSM_IDLE);
- s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, 0);
+ seq_index = 0;
s->loop_counter_1 = 0;
s->loop_counter_2 = 0;
s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, SEQ_STATE_IDLE);
@@ -970,6 +969,8 @@ static void operation_sequencer(PnvSpi *s)
break;
}
} /* end of while */
+ /* Update sequencer index field in status.*/
+ s->status = SETFIELD(SPI_STS_SEQ_INDEX, s->status, seq_index);
return;
} /* end of operation_sequencer() */
--
2.39.5On 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?
> }
> 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.
Thanks,
Nick
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
© 2016 - 2026 Red Hat, Inc.