| Message ID | 20221221224022.425831-4-alistair.francis@opensource.wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,v2,01/45] target/riscv: Fix PMP propagation for tlb | expand |
Hi Alistair, On 21/12/22 23:39, Alistair Francis wrote: > From: Wilfred Mallawa <wilfred.mallawa@wdc.com> > > use the `FIELD32_1CLEAR` macro to implement register > `rw1c` functionality to `ibex_spi`. > > This change was tested by running the `SPI_HOST` from TockOS. > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > Message-Id: <20221017054950.317584-3-wilfred.mallawa@opensource.wdc.com> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > --- > hw/ssi/ibex_spi_host.c | 21 +++++++++------------ > 1 file changed, 9 insertions(+), 12 deletions(-) If a v3 is required, please s/implement/Use/ in subject.
On Thu, Dec 22, 2022 at 8:40 AM Alistair Francis <alistair.francis@opensource.wdc.com> wrote: > > From: Wilfred Mallawa <wilfred.mallawa@wdc.com> > > use the `FIELD32_1CLEAR` macro to implement register > `rw1c` functionality to `ibex_spi`. > > This change was tested by running the `SPI_HOST` from TockOS. > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > Message-Id: <20221017054950.317584-3-wilfred.mallawa@opensource.wdc.com> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > --- > hw/ssi/ibex_spi_host.c | 21 +++++++++------------ > 1 file changed, 9 insertions(+), 12 deletions(-) > > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c > index 57df462e3c..0a456cd1ed 100644 > --- a/hw/ssi/ibex_spi_host.c > +++ b/hw/ssi/ibex_spi_host.c > @@ -342,7 +342,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > { > IbexSPIHostState *s = opaque; > uint32_t val32 = val64; > - uint32_t shift_mask = 0xff, status = 0, data = 0; > + uint32_t shift_mask = 0xff, status = 0; > uint8_t txqd_len; > > trace_ibex_spi_host_write(addr, size, val64); > @@ -355,12 +355,11 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > case IBEX_SPI_HOST_INTR_STATE: > /* rw1c status register */ > if (FIELD_EX32(val32, INTR_STATE, ERROR)) { > - data = FIELD_DP32(data, INTR_STATE, ERROR, 0); > + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], INTR_STATE, ERROR); It seems that this change doesn't build on Windows (https://cirrus-ci.com/task/6444497832247296?logs=main#L2163) Maybe ERROR is reserved? Either way I'll have to drop this commit. Maybe just drop this change and keep the rest? Alistair > } > if (FIELD_EX32(val32, INTR_STATE, SPI_EVENT)) { > - data = FIELD_DP32(data, INTR_STATE, SPI_EVENT, 0); > + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], INTR_STATE, SPI_EVENT); > } > - s->regs[addr] = data; > break; > case IBEX_SPI_HOST_INTR_ENABLE: > s->regs[addr] = val32; > @@ -505,27 +504,25 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > * When an error occurs, the corresponding bit must be cleared > * here before issuing any further commands > */ > - status = s->regs[addr]; > /* rw1c status register */ > if (FIELD_EX32(val32, ERROR_STATUS, CMDBUSY)) { > - status = FIELD_DP32(status, ERROR_STATUS, CMDBUSY, 0); > + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], ERROR_STATUS, CMDBUSY); > } > if (FIELD_EX32(val32, ERROR_STATUS, OVERFLOW)) { > - status = FIELD_DP32(status, ERROR_STATUS, OVERFLOW, 0); > + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], ERROR_STATUS, OVERFLOW); > } > if (FIELD_EX32(val32, ERROR_STATUS, UNDERFLOW)) { > - status = FIELD_DP32(status, ERROR_STATUS, UNDERFLOW, 0); > + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], ERROR_STATUS, UNDERFLOW); > } > if (FIELD_EX32(val32, ERROR_STATUS, CMDINVAL)) { > - status = FIELD_DP32(status, ERROR_STATUS, CMDINVAL, 0); > + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], ERROR_STATUS, CMDINVAL); > } > if (FIELD_EX32(val32, ERROR_STATUS, CSIDINVAL)) { > - status = FIELD_DP32(status, ERROR_STATUS, CSIDINVAL, 0); > + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], ERROR_STATUS, CSIDINVAL); > } > if (FIELD_EX32(val32, ERROR_STATUS, ACCESSINVAL)) { > - status = FIELD_DP32(status, ERROR_STATUS, ACCESSINVAL, 0); > + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], ERROR_STATUS, ACCESSINVAL); > } > - s->regs[addr] = status; > break; > case IBEX_SPI_HOST_EVENT_ENABLE: > /* Controls which classes of SPI events raise an interrupt. */ > -- > 2.38.1 >
On Wed, 2023-01-04 at 22:30 +1000, Alistair Francis wrote: > On Thu, Dec 22, 2022 at 8:40 AM Alistair Francis > <alistair.francis@opensource.wdc.com> wrote: > > > > From: Wilfred Mallawa <wilfred.mallawa@wdc.com> > > > > use the `FIELD32_1CLEAR` macro to implement register > > `rw1c` functionality to `ibex_spi`. > > > > This change was tested by running the `SPI_HOST` from TockOS. > > > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > > Message-Id: > > <20221017054950.317584-3-wilfred.mallawa@opensource.wdc.com> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > > --- > > hw/ssi/ibex_spi_host.c | 21 +++++++++------------ > > 1 file changed, 9 insertions(+), 12 deletions(-) > > > > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c > > index 57df462e3c..0a456cd1ed 100644 > > --- a/hw/ssi/ibex_spi_host.c > > +++ b/hw/ssi/ibex_spi_host.c > > @@ -342,7 +342,7 @@ static void ibex_spi_host_write(void *opaque, > > hwaddr addr, > > { > > IbexSPIHostState *s = opaque; > > uint32_t val32 = val64; > > - uint32_t shift_mask = 0xff, status = 0, data = 0; > > + uint32_t shift_mask = 0xff, status = 0; > > uint8_t txqd_len; > > > > trace_ibex_spi_host_write(addr, size, val64); > > @@ -355,12 +355,11 @@ static void ibex_spi_host_write(void *opaque, > > hwaddr addr, > > case IBEX_SPI_HOST_INTR_STATE: > > /* rw1c status register */ > > if (FIELD_EX32(val32, INTR_STATE, ERROR)) { > > - data = FIELD_DP32(data, INTR_STATE, ERROR, 0); > > + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], > > INTR_STATE, ERROR); > > It seems that this change doesn't build on Windows > (https://cirrus-ci.com/task/6444497832247296?logs=main#L2163) > > Maybe ERROR is reserved? Either way I'll have to drop this commit. > Maybe just drop this change and keep the rest? > > Alistair Odd! yea I'll do that and send a new patch. Wilfred > > > } > > if (FIELD_EX32(val32, INTR_STATE, SPI_EVENT)) { > > - data = FIELD_DP32(data, INTR_STATE, SPI_EVENT, 0); > > + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], > > INTR_STATE, SPI_EVENT); > > } > > - s->regs[addr] = data; > > break; > > case IBEX_SPI_HOST_INTR_ENABLE: > > s->regs[addr] = val32; > > @@ -505,27 +504,25 @@ static void ibex_spi_host_write(void *opaque, > > hwaddr addr, > > * When an error occurs, the corresponding bit must be > > cleared > > * here before issuing any further commands > > */ > > - status = s->regs[addr]; > > /* rw1c status register */ > > if (FIELD_EX32(val32, ERROR_STATUS, CMDBUSY)) { > > - status = FIELD_DP32(status, ERROR_STATUS, CMDBUSY, 0); > > + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], > > ERROR_STATUS, CMDBUSY); > > } > > if (FIELD_EX32(val32, ERROR_STATUS, OVERFLOW)) { > > - status = FIELD_DP32(status, ERROR_STATUS, OVERFLOW, > > 0); > > + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], > > ERROR_STATUS, OVERFLOW); > > } > > if (FIELD_EX32(val32, ERROR_STATUS, UNDERFLOW)) { > > - status = FIELD_DP32(status, ERROR_STATUS, UNDERFLOW, > > 0); > > + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], > > ERROR_STATUS, UNDERFLOW); > > } > > if (FIELD_EX32(val32, ERROR_STATUS, CMDINVAL)) { > > - status = FIELD_DP32(status, ERROR_STATUS, CMDINVAL, > > 0); > > + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], > > ERROR_STATUS, CMDINVAL); > > } > > if (FIELD_EX32(val32, ERROR_STATUS, CSIDINVAL)) { > > - status = FIELD_DP32(status, ERROR_STATUS, CSIDINVAL, > > 0); > > + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], > > ERROR_STATUS, CSIDINVAL); > > } > > if (FIELD_EX32(val32, ERROR_STATUS, ACCESSINVAL)) { > > - status = FIELD_DP32(status, ERROR_STATUS, ACCESSINVAL, > > 0); > > + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], > > ERROR_STATUS, ACCESSINVAL); > > } > > - s->regs[addr] = status; > > break; > > case IBEX_SPI_HOST_EVENT_ENABLE: > > /* Controls which classes of SPI events raise an interrupt. */ > > -- > > 2.38.1 > >
diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c index 57df462e3c..0a456cd1ed 100644 --- a/hw/ssi/ibex_spi_host.c +++ b/hw/ssi/ibex_spi_host.c @@ -342,7 +342,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, { IbexSPIHostState *s = opaque; uint32_t val32 = val64; - uint32_t shift_mask = 0xff, status = 0, data = 0; + uint32_t shift_mask = 0xff, status = 0; uint8_t txqd_len; trace_ibex_spi_host_write(addr, size, val64); @@ -355,12 +355,11 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, case IBEX_SPI_HOST_INTR_STATE: /* rw1c status register */ if (FIELD_EX32(val32, INTR_STATE, ERROR)) { - data = FIELD_DP32(data, INTR_STATE, ERROR, 0); + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], INTR_STATE, ERROR); } if (FIELD_EX32(val32, INTR_STATE, SPI_EVENT)) { - data = FIELD_DP32(data, INTR_STATE, SPI_EVENT, 0); + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], INTR_STATE, SPI_EVENT); } - s->regs[addr] = data; break; case IBEX_SPI_HOST_INTR_ENABLE: s->regs[addr] = val32; @@ -505,27 +504,25 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, * When an error occurs, the corresponding bit must be cleared * here before issuing any further commands */ - status = s->regs[addr]; /* rw1c status register */ if (FIELD_EX32(val32, ERROR_STATUS, CMDBUSY)) { - status = FIELD_DP32(status, ERROR_STATUS, CMDBUSY, 0); + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], ERROR_STATUS, CMDBUSY); } if (FIELD_EX32(val32, ERROR_STATUS, OVERFLOW)) { - status = FIELD_DP32(status, ERROR_STATUS, OVERFLOW, 0); + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], ERROR_STATUS, OVERFLOW); } if (FIELD_EX32(val32, ERROR_STATUS, UNDERFLOW)) { - status = FIELD_DP32(status, ERROR_STATUS, UNDERFLOW, 0); + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], ERROR_STATUS, UNDERFLOW); } if (FIELD_EX32(val32, ERROR_STATUS, CMDINVAL)) { - status = FIELD_DP32(status, ERROR_STATUS, CMDINVAL, 0); + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], ERROR_STATUS, CMDINVAL); } if (FIELD_EX32(val32, ERROR_STATUS, CSIDINVAL)) { - status = FIELD_DP32(status, ERROR_STATUS, CSIDINVAL, 0); + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], ERROR_STATUS, CSIDINVAL); } if (FIELD_EX32(val32, ERROR_STATUS, ACCESSINVAL)) { - status = FIELD_DP32(status, ERROR_STATUS, ACCESSINVAL, 0); + s->regs[addr] = FIELD32_1CLEAR(s->regs[addr], ERROR_STATUS, ACCESSINVAL); } - s->regs[addr] = status; break; case IBEX_SPI_HOST_EVENT_ENABLE: /* Controls which classes of SPI events raise an interrupt. */