Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
From: Vignesh Raghavendra <vigneshr@ti.com>
To: "Boris Brezillon" <boris.brezillon@collabora.com>,
	"Cédric Le Goater" <clg@kaod.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
	linux-aspeed@lists.ozlabs.org,
	Tudor Ambarus <tudor.ambarus@microchip.com>,
	Andrew Jeffery <andrew@aj.id.au>,
	Richard Weinberger <richard@nod.at>,
	Marek Vasut <marek.vasut@gmail.com>,
	linux-mtd@lists.infradead.org, Joel Stanley <joel@jms.id.au>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Brian Norris <computersforpeace@gmail.com>,
	David Woodhouse <dwmw2@infradead.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 04/16] mtd: spi-nor: aspeed: Add read training
Date: Fri, 11 Oct 2019 18:43:33 +0530
Message-ID: <a933999d-cb3d-ef3c-8a0d-a1f022817543@ti.com> (raw)
In-Reply-To: <20191011142805.6cc3905c@dhcp-172-31-174-146.wireless.concordia.ca>

Hi Cedric,

On 11/10/19 5:58 PM, Boris Brezillon wrote:
> On Fri,  4 Oct 2019 13:59:07 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> +#define ASPEED_SMC_HCLK_DIV(i) \
>> +	(aspeed_smc_hclk_divs[(i) - 1] << CONTROL_CLOCK_FREQ_SEL_SHIFT)
>> +
>> +static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip)
>> +{
>> +	/*
>> +	 * Keep the 4Byte address mode on the AST2400 SPI controller.
>> +	 * Other controllers set the 4Byte mode in the CE Control
>> +	 * Register
>> +	 */
>> +	u32 ctl_mask = chip->controller->info == &spi_2400_info ?
>> +		 CONTROL_IO_ADDRESS_4B : 0;
>> +
>> +	return (chip->ctl_val[smc_read] & ctl_mask) |
>> +		(0x00 << 28) | /* Single bit */
>> +		(0x00 << 24) | /* CE# max */
>> +		(0x03 << 16) | /* use normal reads */
>> +		(0x00 <<  8) | /* HCLK/16 */
>> +		(0x00 <<  6) | /* no dummy cycle */
>> +		(0x00);        /* normal mode */
> 
> IIUC, you're using a SPINOR_OP_READ operation to read the golden
> buffer, and if I'm right, you start reading at offset 0 of the dirmap
> window (offset 0 in the flash), so basically the first block in the NOR.
> What happens if this block is erased? In that case your golden buf will
> contain only 0xff values, and the read calibration is likely to be
> useless (how can you determine if timings are good when IO pins always
> stay high). Don't we have a command that return non-ff/non-0 data while
> still being predictable and immutable? Do you expect users to always
> flash a pattern that helps calibrating those delays?
> 

Yes, this is precisely my concern as well. I have been developing
training sequence for cadence-quadspi controller (requirements are
similar to what you have here) and found that its better to use read
only data such as SFDP table data to calibrate. Cadence-quadspi requires
training only in higher performance modes like Quad/Octal DTR mode and
needs 16 bytes of known data for calibration. Hence SFDP works well for
my case.
But the problem here is that, aspeed controller needs 16K of known data.
SFDP table is not that big and read beyond address space is not required
to wrap around.
Wondering if you really need to read 16K amount of data for calibration?

Regards
Vignesh

>> +}
>> +
>> +static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip,
>> +				    u32 max_freq)
>> +{
>> +	u8 *golden_buf, *test_buf;
>> +	int i, rc, best_div = -1;
>> +	u32 save_read_val = chip->ctl_val[smc_read];
>> +	u32 ahb_freq = chip->controller->clk_frequency;
>> +
>> +	dev_dbg(chip->nor.dev, "AHB frequency: %d MHz", ahb_freq / 1000000);
>> +
>> +	test_buf = kmalloc(CALIBRATE_BUF_SIZE * 2, GFP_KERNEL);
>> +	golden_buf = test_buf + CALIBRATE_BUF_SIZE;
>> +
>> +	/* We start with the dumbest setting (keep 4Byte bit) and read
>> +	 * some data
>> +	 */
>> +	chip->ctl_val[smc_read] = aspeed_smc_default_read(chip);
>> +
>> +	writel(chip->ctl_val[smc_read], chip->ctl);
>> +
>> +	memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE);
>> +
>> +	/* Establish our read mode with freq field set to 0 (HCLK/16) */
>> +	chip->ctl_val[smc_read] = save_read_val & 0xfffff0ff;
>> +
>> +	/* Check if calibration data is suitable */
>> +	if (!aspeed_smc_check_calib_data(golden_buf, CALIBRATE_BUF_SIZE)) {
>> +		dev_info(chip->nor.dev,
>> +			 "Calibration area too uniform, using low speed");
>> +		writel(chip->ctl_val[smc_read], chip->ctl);
>> +		kfree(test_buf);
>> +		return 0;
>> +	}
>> +
>> +	/* Now we iterate the HCLK dividers until we find our breaking point */
>> +	for (i = ARRAY_SIZE(aspeed_smc_hclk_divs); i > 0; i--) {
>> +		u32 tv, freq;
>> +
>> +		/* Compare timing to max */
>> +		freq = ahb_freq / i;
>> +		if (freq > max_freq)
>> +			continue;
>> +
>> +		/* Set the timing */
>> +		tv = chip->ctl_val[smc_read] | ASPEED_SMC_HCLK_DIV(i);
>> +		writel(tv, chip->ctl);
>> +		dev_dbg(chip->nor.dev, "Trying HCLK/%d...", i);
>> +		rc = aspeed_smc_calibrate_reads(chip, i, golden_buf, test_buf);
>> +		if (rc == 0)
>> +			best_div = i;
>> +	}
>> +	kfree(test_buf);
>> +
>> +	/* Nothing found ? */
>> +	if (best_div < 0) {
>> +		dev_warn(chip->nor.dev, "No good frequency, using dumb slow");
>> +	} else {
>> +		dev_dbg(chip->nor.dev, "Found good read timings at HCLK/%d",
>> +			best_div);
>> +		chip->ctl_val[smc_read] |= ASPEED_SMC_HCLK_DIV(best_div);
>> +	}
>> +
>> +	writel(chip->ctl_val[smc_read], chip->ctl);
>> +	return 0;
>> +}
> 
> 

-- 
Regards
Vignesh

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

  reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-04 11:59 [PATCH 00/16] mtd: spi-nor: aspeed: AST2600 support and extensions Cédric Le Goater
2019-10-04 11:59 ` [PATCH 01/16] mtd: spi-nor: aspeed: Use command mode for reads Cédric Le Goater
2019-10-04 11:59 ` [PATCH 02/16] mtd: spi-nor: aspeed: Add support for SPI dual IO read mode Cédric Le Goater
2019-10-04 11:59 ` [PATCH 03/16] mtd: spi-nor: aspeed: Link controller with the ahb clock Cédric Le Goater
2019-10-04 11:59 ` [PATCH 04/16] mtd: spi-nor: aspeed: Add read training Cédric Le Goater
2019-10-11 12:28   ` Boris Brezillon
2019-10-11 13:13     ` Vignesh Raghavendra [this message]
2019-10-11 14:03       ` Cédric Le Goater
2019-10-11 13:55     ` Cédric Le Goater
2019-10-11 14:29       ` Boris Brezillon
2019-10-11 14:37         ` Cédric Le Goater
2019-10-04 11:59 ` [PATCH 05/16] mtd: spi-nor: aspeed: Limit the maximum SPI frequency Cédric Le Goater
2019-10-04 11:59 ` [PATCH 06/16] mtd: spi-nor: fix options for mx66l51235f Cédric Le Goater
2019-10-04 16:23   ` Cédric Le Goater
2019-10-04 11:59 ` [PATCH 07/16] mtd: spi-nor: aspeed: Add support for the 4B opcodes Cédric Le Goater
2019-10-04 11:59 ` [PATCH 08/16] mtd: spi-nor: Add support for w25q512jv Cédric Le Goater
2019-10-04 11:59 ` [PATCH 09/16] mtd: spi-nor: aspeed: Introduce a field for the AHB physical address Cédric Le Goater
2019-10-04 11:59 ` [PATCH 10/16] mtd: spi-nor: aspeed: Introduce segment operations Cédric Le Goater
2019-10-04 11:59 ` [PATCH 11/16] dt-bindings: mtd: aspeed-smc: Add new comptatible for AST2600 Cédric Le Goater
2019-10-15 19:26   ` Rob Herring
2019-10-04 11:59 ` [PATCH 12/16] mtd: spi-nor: aspeed: Add initial support for the AST2600 Cédric Le Goater
2019-10-04 11:59 ` [PATCH 13/16] mtd: spi-nor: aspeed: Check for disabled segments on " Cédric Le Goater
2019-10-04 12:09 ` [PATCH 14/16] mtd: spi-nor: aspeed: Introduce training operations per platform Cédric Le Goater
2019-10-04 12:09   ` [PATCH 15/16] mtd: spi-nor: aspeed: Introduce a HCLK mask for training Cédric Le Goater
2019-10-04 12:09   ` [PATCH 16/16] mtd: spi-nor: aspeed: Add read training support for the AST2600 Cédric Le Goater
2019-10-09 20:55 ` [PATCH 00/16] mtd: spi-nor: aspeed: AST2600 support and extensions Boris Brezillon
2019-10-10 23:47   ` Joel Stanley
2019-10-11  6:45     ` Boris Brezillon
2019-10-11  9:29       ` Cédric Le Goater
2019-10-11  9:51         ` Boris Brezillon
2019-10-11 11:47           ` Cédric Le Goater
2019-10-11 12:07             ` Boris Brezillon
2019-10-11 13:07               ` Cédric Le Goater
2019-10-11 14:01                 ` Boris Brezillon

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a933999d-cb3d-ef3c-8a0d-a1f022817543@ti.com \
    --to=vigneshr@ti.com \
    --cc=andrew@aj.id.au \
    --cc=boris.brezillon@collabora.com \
    --cc=clg@kaod.org \
    --cc=computersforpeace@gmail.com \
    --cc=dwmw2@infradead.org \
    --cc=joel@jms.id.au \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-aspeed@lists.ozlabs.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=marek.vasut@gmail.com \
    --cc=mark.rutland@arm.com \
    --cc=miquel.raynal@bootlin.com \
    --cc=richard@nod.at \
    --cc=tudor.ambarus@microchip.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git