From: Bin Meng <bin.meng@windriver.com>

Current implementation of the imx spi controller expects the burst
length to be multiple of 8, which is the most common use case.

In case the burst length is not what we expect, log it to give user
a chance to notice it, and round it up to be multiple of 8.

Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

(no changes since v5)

Changes in v5:
- round up the burst length to be multiple of 8

Changes in v4:
- s/normal/common/ in the commit message
- log the burst length value in the log message

Changes in v3:
- new patch: log unimplemented burst length

 hw/ssi/imx_spi.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 5838bb0..3c80725 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -128,7 +128,20 @@ static uint8_t imx_spi_selected_channel(IMXSPIState *s)
 
 static uint32_t imx_spi_burst_length(IMXSPIState *s)
 {
-    return EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_BURST_LENGTH) + 1;
+    uint32_t burst;
+
+    burst = EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_BURST_LENGTH) + 1;
+    if (burst % 8) {
+        qemu_log_mask(LOG_UNIMP,
+                      "[%s]%s: burst length (%d) not multiple of 8!\n",
+                      TYPE_IMX_SPI, __func__, burst);
+        burst = ROUND_UP(burst, 8);
+        qemu_log_mask(LOG_UNIMP,
+                      "[%s]%s: burst length rounded up to %d; this may not work.\n",
+                      TYPE_IMX_SPI, __func__, burst);
+    }
+
+    return burst;
 }
 
 static bool imx_spi_is_enabled(IMXSPIState *s)
-- 
2.7.4

On Tue, 19 Jan 2021 at 13:40, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> From: Bin Meng <bin.meng@windriver.com>
>
> Current implementation of the imx spi controller expects the burst
> length to be multiple of 8, which is the most common use case.
>
> In case the burst length is not what we expect, log it to give user
> a chance to notice it, and round it up to be multiple of 8.
>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>

> @@ -128,7 +128,20 @@ static uint8_t imx_spi_selected_channel(IMXSPIState *s)
>
>  static uint32_t imx_spi_burst_length(IMXSPIState *s)
>  {
> -    return EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_BURST_LENGTH) + 1;
> +    uint32_t burst;
> +
> +    burst = EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_BURST_LENGTH) + 1;
> +    if (burst % 8) {
> +        qemu_log_mask(LOG_UNIMP,
> +                      "[%s]%s: burst length (%d) not multiple of 8!\n",
> +                      TYPE_IMX_SPI, __func__, burst);
> +        burst = ROUND_UP(burst, 8);
> +        qemu_log_mask(LOG_UNIMP,
> +                      "[%s]%s: burst length rounded up to %d; this may not work.\n",
> +                      TYPE_IMX_SPI, __func__, burst);

It's friendlier to the user to do the LOG_UNIMP when the
unsupported CONREG value is written, rather than here
where it is used. That way the warning happens once, rather
than every time the device transmits data.

Also, you could squash the warning down into one line, something
like:
   "[%s]%s: burst length %d not supported: rounding up to next multiple of 8\n"
rather than logging twice.

thanks
-- PMM