Re: [Qemu-devel] [PATCH 18/19] aspeed/smc: inject errors in DMA checksum

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Cédric,

On 5/25/19 5:12 PM, Cédric Le Goater wrote:
> Emulate read errors in the DMA Checksum Register for high frequencies
> and optimistic settings of the Read Timing Compensation Register. This
> will help in tuning the SPI timing calibration algorithm.
> 
> The values below are those to expect from the first flash device of
> the FMC controller of a palmetto-bmc machine.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ssi/aspeed_smc.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 406c30c60b3f..4c162912cf62 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -866,6 +866,30 @@ static void aspeed_smc_dma_calibration(AspeedSMCState *s)
>      s->regs[s->r_ctrl0 + cs] |= CE_CTRL_CLOCK_FREQ(hclk_div);
>  }
>  

Can you add a comment (like the patch description) here?

> +static bool aspeed_smc_inject_read_failure(AspeedSMCState *s)
> +{
> +    uint8_t delay =
> +        (s->regs[R_DMA_CTRL] >> DMA_CTRL_DELAY_SHIFT) & DMA_CTRL_DELAY_MASK;
> +    uint8_t hclk_mask =
> +        (s->regs[R_DMA_CTRL] >> DMA_CTRL_FREQ_SHIFT) & DMA_CTRL_FREQ_MASK;
> +
> +    /*
> +     * Typical values of a palmetto-bmc machine.
> +     */
> +    switch (aspeed_smc_hclk_divisor(hclk_mask)) {
> +    case 4 ... 16:
> +        return false;
> +    case 3: /* at least one HCLK cycle delay */
> +        return (delay & 0x7) < 1;
> +    case 2: /* at least two HCLK cycle delay */
> +        return (delay & 0x7) < 2;
> +    case 1: /* (> 100MHz) is above the max freq of the controller */
> +        return true;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
>  /*
>   * Accumulate the result of the reads to provide a checksum that will
>   * be used to validate the read timing settings.
> @@ -903,6 +927,11 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
>          s->regs[R_DMA_FLASH_ADDR] += 4;
>          s->regs[R_DMA_LEN] -= 4;
>      }
> +
> +    if (aspeed_smc_inject_read_failure(s)) {

So this model real world where noise eventually triggers errors. Don't
we want this to be enable by the user (or a QMP command)?

> +        s->regs[R_DMA_CHECKSUM] = 0xbadc0de;
> +    }
> +
>  }
>  
>  static void aspeed_smc_dma_rw(AspeedSMCState *s)
>