| Message ID | 20240918165045.21298-4-chalapathi.v@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/ssi/pnv_spi: Remove PnvXferBuffer and get_seq_index() | expand |
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? > } > 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
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() */
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(-)