> At high SPI clock frequencies, the controller must delay the sampling
> of the read data, and the right delay depends on the board. Port the
> read timing calibration from the Linux driver: golden data is read at
> HCLK/16, then the timing settings of each HCLK divider, from the
> fastest one allowed down to HCLK/5, are scanned until the reads match.
>
> Until the calibration is done, the flash is accessed at HCLK/16, as in
> Linux, since faster settings may not work without the timing
> compensation.
>
> Compared to Linux:
> - each setting is checked by reading 1KB twice, instead of 16KB ten
> times, to keep the boot time short.
> - the AST2600 setting picked in the passing window is mapped back to
> the input delay that was tested. Linux writes the column index as
> the delay, which is one step off.
>
> Based on the following Linux commits:
> - eeaec1ea05c0 ("spi: aspeed: Calibrate read timings")
> - 31dcc7e1f8a9 ("spi: aspeed: Update clock selection strategy")
> - efb79de36e94 ("spi: aspeed: Improve timing calibration algorithm
> for AST2600 platform")
>
> Link: https://git.kernel.org/torvalds/c/eeaec1ea05c0
> Link: https://git.kernel.org/torvalds/c/31dcc7e1f8a9
> Link: https://git.kernel.org/torvalds/c/efb79de36e94
> Signed-off-by: Cooper Huang <[email protected]>
>
> diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
> index e60bd77387a..57156122e85 100644
> --- a/drivers/spi/spi-aspeed-smc.c
> +++ b/drivers/spi/spi-aspeed-smc.c
*snip*
> @@ -724,6 +736,239 @@ static int aspeed_spi_exec_op_user_mode(struct
> spi_slave *slave,
> return 0;
> }
>
> +/* Read timing calibration, ported from the Linux driver */
> +static bool aspeed_spi_check_reads(struct aspeed_spi_flash *flash,
> + const u8 *golden_buf, u8 *test_buf)
> +{
> + int i;
> +
> + for (i = 0; i < CALIBRATE_REPEAT_COUNT; i++) {
> + memcpy_fromio(test_buf, (void __iomem *)flash->ahb_base,
> + CALIBRATE_BUF_SIZE);
> + if (memcmp(test_buf, golden_buf, CALIBRATE_BUF_SIZE))
> + return false;
> + }
> +
> + return true;
> +}
> +
> +/* Check that enough words are neither all 0's nor all 1's */
> +static bool aspeed_spi_check_calib_data(const u8 *buf, size_t len)
> +{
> + const u32 *buf32 = (const u32 *)buf;
Compared to Linux we only have a 1 KiB buffere here (64 out of 256 words), i.e.
25% of the data has to be neither 0x00 nor 0xff.
What if the first 1 KiB has more padding? That'd be enough to be counted as "too
uniform" and the flash would run at max_hz without calibration.
Can we scale the threshold to the buffer size maybe (len / 256 to match Linux),
or keep a bigger buffer and only shorten the repeated reads? Either way, please
mention it in the "Compared to Linux" list in the commit message.
> + u32 cnt = 0;
> + size_t i;
> +
> + for (i = 0; i < len / sizeof(u32); i++) {
> + if (buf32[i] != 0 && buf32[i] != 0xffffffff)
> + cnt++;
> + }
> +
> + return cnt >= 64;
> +}
> +
> +static u32 ast2400_fread_tpass(int i)
> +{
> + return (i / 2) | ((i & 1) ? 0 : TIMING_DELAY_DI);
> +}
> +
> +/* The timing register is shared by all CS, only CE0 is calibrated. */
> +static int ast2400_spi_calibrate(struct udevice *dev, u32 hdiv,
> + const u8 *golden_buf, u8 *test_buf)
> +{
> + struct aspeed_spi_priv *priv = dev_get_priv(dev->parent);
> + struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev);
> + u32 cs = slave_plat->cs[0];
> + struct aspeed_spi_flash *flash = &priv->flashes[cs];
> + uintptr_t timing_reg = (uintptr_t)&priv->regs->timings[0];
> + u32 shift = (hdiv - 1) * 4;
> + u32 timing_base;
> + u32 timing_val;
> + int pass_cnt = 0;
> + int i;
> +
> + /* Other CS reuse the CE0 timing, require three passes as well. */
The code reads more like that other CS can't change the shared timing register?
So maybe reword it to something like "Only check that the current CE0 setting
reads back correctly three times." or so?
> + if (cs != 0) {
> + for (i = 0; i < 3; i++) {
> + if (!aspeed_spi_check_reads(flash, golden_buf,
> + test_buf))
> + return -EIO;
> + }
> +
> + return 0;
> + }
> +
> + /* The AST2400 SPI controller has its timing register at 0x14. */
> + if (priv->info == &ast2400_spi_info)
> + timing_reg = (uintptr_t)priv->regs + 0x14;
> +
> + timing_base = readl(timing_reg) & ~(0xfu << shift);
> +
> + for (i = 0; i < (TIMING_DELAY_HCYCLE_MAX + 1) * 2; i++) {
> + timing_val = ast2400_fread_tpass(i);
> + writel(timing_base | (timing_val << shift), timing_reg);
> +
> + if (!aspeed_spi_check_reads(flash, golden_buf, test_buf)) {
> + pass_cnt = 0;
> + continue;
> + }
> +
> + /* Use the middle one of three consecutive passing settings. */
> + if (++pass_cnt == 3) {
> + timing_val = ast2400_fread_tpass(i - 1);
> + writel(timing_base | (timing_val << shift), timing_reg);
> + return 0;
> + }
> + }
> +
> + return -EIO;
> +}
> +
> +/* Column 0 is without input delay, columns 1 to 16 are input delays 0 to 15
> */
> +static u32 ast2600_timing_val(int hcycle, int col)
> +{
> + if (!col)
> + return hcycle;
> +
> + return TIMING_DELAY_DI | hcycle | ((col - 1) << 4);
> +}
> +
> +/*
> + * Each CS has its own timing register. Use the center of the longest
> + * passing window of input delays within a single HCLK cycle delay.
> + */
> +static int ast2600_spi_calibrate(struct udevice *dev, u32 hdiv,
> + const u8 *golden_buf, u8 *test_buf)
> +{
> + struct aspeed_spi_priv *priv = dev_get_priv(dev->parent);
> + struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev);
> + u32 cs = slave_plat->cs[0];
> + struct aspeed_spi_flash *flash = &priv->flashes[cs];
> + u32 *timing_reg = &priv->regs->timings[cs];
> + u32 shift = (hdiv - 2) * 8;
> + u32 timing_base;
> + u32 timing_val;
> + int best_hcycle = 0;
> + int best_col = 0;
> + int best_len = 0;
> + int hcycle;
> + int start;
> + int col;
> +
> + timing_base = readl(timing_reg) & ~(0xffu << shift);
> +
> + for (hcycle = 0; hcycle <= TIMING_DELAY_HCYCLE_MAX; hcycle++) {
> + start = 0;
> +
> + for (col = 0; col <= TIMING_DELAY_INPUT_MAX; col++) {
> + timing_val = ast2600_timing_val(hcycle, col);
> + writel(timing_base | (timing_val << shift), timing_reg);
> +
> + if (!aspeed_spi_check_reads(flash, golden_buf,
> + test_buf)) {
> + start = col + 1;
> + continue;
> + }
> +
> + if (col - start + 1 > best_len) {
> + best_len = col - start + 1;
> + best_hcycle = hcycle;
> + best_col = start + best_len / 2;
> + }
> + }
> + }
> +
> + if (best_len < TIMING_MIN_PASS_WINDOW)
> + return -EIO;
> +
> + timing_val = ast2600_timing_val(best_hcycle, best_col);
> + writel(timing_base | (timing_val << shift), timing_reg);
> +
> + return 0;
> +}
> +
> +static bool aspeed_spi_need_calibration(struct udevice *dev)
> +{
> + struct aspeed_spi_plat *plat = dev_get_plat(dev->parent);
> + struct aspeed_spi_priv *priv = dev_get_priv(dev->parent);
> + struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev);
> +
> + /* The calibration is done when the read direct mapping is created. */
> + return CONFIG_IS_ENABLED(SPI_DIRMAP) && priv->info->calibrate &&
> + slave_plat->max_hz >= plat->hclk_rate / CALIBRATE_MAX_HDIV;
> +}
> +
> +static int aspeed_spi_do_calibration(struct udevice *dev)
> +{
> + struct udevice *bus = dev->parent;
> + struct aspeed_spi_plat *plat = dev_get_plat(bus);
> + struct aspeed_spi_priv *priv = dev_get_priv(bus);
> + struct dm_spi_slave_plat *slave_plat = dev_get_parent_plat(dev);
> + const struct aspeed_spi_info *info = priv->info;
> + u32 cs = slave_plat->cs[0];
> + struct aspeed_spi_flash *flash = &priv->flashes[cs];
> + uintptr_t ce_ctrl_reg = (uintptr_t)&priv->regs->ce_ctrl[cs];
> + u8 *golden_buf;
> + u8 *test_buf;
> + u32 clk_setting = 0;
> + u32 ctrl_val;
> + u32 hdiv;
> + int ret;
> +
> + if (!aspeed_spi_need_calibration(dev))
> + return 0;
> +
> + if (info == &ast2400_spi_info)
> + ce_ctrl_reg = (uintptr_t)&priv->regs->ctrl;
> +
> + test_buf = malloc(CALIBRATE_BUF_SIZE * 2);
> + if (!test_buf)
> + return -ENOMEM;
> +
> + golden_buf = test_buf + CALIBRATE_BUF_SIZE;
> +
> + /* Read the golden data at HCLK/16. */
> + ctrl_val = flash->ce_ctrl_read & ~info->clk_ctrl_mask;
> + writel(ctrl_val, ce_ctrl_reg);
> + memcpy_fromio(golden_buf, (void __iomem *)flash->ahb_base,
> + CALIBRATE_BUF_SIZE);
> +
> + if (!aspeed_spi_check_calib_data(golden_buf, CALIBRATE_BUF_SIZE)) {
> + dev_dbg(dev, "calibration area too uniform, skipped\n");
Why's this dev_dbg? In linux it's dev_info and this information seems useful to
be reported to the user.
> + clk_setting = info->get_clk_setting(dev, slave_plat->max_hz);
> + goto out;
> + }
> +
> + for (hdiv = info->min_hdiv; hdiv <= CALIBRATE_MAX_HDIV; hdiv++) {
> + u32 hdiv_freq = plat->hclk_rate / hdiv;
> +
> + if (hdiv_freq > slave_plat->max_hz)
> + continue;
> +
> + clk_setting = info->get_clk_setting(dev, hdiv_freq);
> + writel(ctrl_val | clk_setting, ce_ctrl_reg);
> +
> + ret = info->calibrate(dev, hdiv, golden_buf, test_buf);
> + dev_dbg(dev, "HCLK/%u: %s\n", hdiv, ret ? "FAIL" : "PASS");
> + if (!ret)
> + break;
> + }
> +
> + if (hdiv > CALIBRATE_MAX_HDIV) {
> + dev_warn(dev, "no good read timing found, using HCLK/16\n");
> + clk_setting = 0;
> + }
get_clk_setting sets flash->max_freq too. If every divider fails it'd stay at
HCLK/5 while the controller runs at HCLK/16.
I mean, it _should_ have no effect as it's only called if "flash->max_freq == 0"
in claim_bus, but the stored value would be wrong.
> +
> +out:
> + flash->ce_ctrl_user &= ~info->clk_ctrl_mask;
> + flash->ce_ctrl_user |= clk_setting;
> + flash->ce_ctrl_read = ctrl_val | clk_setting;
> + writel(flash->ce_ctrl_read, ce_ctrl_reg);
> + free(test_buf);
> +
> + return 0;
> +}
> +
> static int aspeed_spi_dirmap_create(struct spi_mem_dirmap_desc *desc)
> {
> int ret = 0;
> @@ -779,6 +1024,10 @@ static int aspeed_spi_dirmap_create(struct
> spi_mem_dirmap_desc *desc)
>
> writel(priv->flashes[cs].ce_ctrl_read, ce_ctrl_reg);
>
> + ret = aspeed_spi_do_calibration(dev);
> + if (ret)
> + return ret;
> +
> dev_dbg(dev, "read bus width: %d ce_ctrl_val: 0x%08x\n",
> op_tmpl.data.buswidth, priv->flashes[cs].ce_ctrl_read);
>
*snip*
> @@ -1167,7 +1432,8 @@ static int aspeed_spi_claim_bus(struct udevice *dev)
>
> dev_dbg(bus, "%s: claim bus CS%u\n", bus->name, slave_plat->cs[0]);
>
> - if (flash->max_freq == 0) {
> + /* Stay at HCLK/16 until the read timing is calibrated. */
> + if (flash->max_freq == 0 && !aspeed_spi_need_calibration(dev)) {
If dirmap_create fails before or during the calibration (e.g.
aspeed_spi_decoded_range_config fails or aspeed_spi_do_calibration returns
-ENOMEM) then need_calibration stays true, so the clock is never raised and the
flash silently stays at HCLK/16.
Maybe add a per-flash "calibrated" bool or so for claim_bus to check instead and
in the failure path fall back to the uncalibrated max_hz or at least print a
warning?
Tan
--
Tan Siewert <[email protected]>
