Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
From: "Cédric Le Goater" <clg@kaod.org>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Vignesh Raghavendra <vigneshr@ti.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 15:55:25 +0200
Message-ID: <3244b1ce-587c-6f12-cc9c-7eee0354e76b@kaod.org> (raw)
In-Reply-To: <20191011142805.6cc3905c@dhcp-172-31-174-146.wireless.concordia.ca>

On 11/10/2019 14:28, 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.

Yes.

> 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 

yes. that is why we have the aspeed_smc_check_calib_data() routine to
check that the data read makes some sense. If this is not the case, then :

	 "Calibration area too uniform, using low speed"

> (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? 

Not that I know of on these controllers.

> Do you expect users to always
> flash a pattern that helps calibrating those delays?

This is the case on the OpenBMC systems, AFAICT. 

u-boot.bin should be the data read on the FMC controller and the 
SPI controller contains the host Firmware which is as random.   

> 
>> +}
>> +
>> +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;
>> +}
> 
> 


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

  parent 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
2019-10-11 14:03       ` Cédric Le Goater
2019-10-11 13:55     ` Cédric Le Goater [this message]
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=3244b1ce-587c-6f12-cc9c-7eee0354e76b@kaod.org \
    --to=clg@kaod.org \
    --cc=andrew@aj.id.au \
    --cc=boris.brezillon@collabora.com \
    --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 \
    --cc=vigneshr@ti.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 linux-mtd@archiver.kernel.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