On 09/12/2018 12:38 AM, Joel Stanley wrote:
> On Mon, 10 Sep 2018 at 23:48, Cédric Le Goater <c...@kaod.org> wrote:
>>
>> The Aspeed AST2500 SoC comes with three static memory controllers, all
>> with a similar interface :
>>
>> * Firmware SPI Memory Controller (FMC)
>> . BMC firmware
>> . 3 chip select pins (CE0 ~ CE2)
>> . supports SPI type flash memory (CE0 ~ CE1)
>> . CE2 can be of NOR type flash but this is not supported by the
>> driver
>>
>> * SPI Flash Controller (SPI1 and SPI2)
>> . host firmware
>> . 2 chip select pins (CE0 ~ CE1)
>>
>> Each controller has a defined AHB window for its registers and another
>> AHB window on which all the flash devices are mapped. Each device is
>> assigned a memory range through a set of registers called the Segment
>> Address Registers.
>>
>> Devices are controlled using two different modes: the USER command
>> mode or the READ/WRITE command mode. When in USER command mode,
>> accesses to the AHB window of the SPI flash device are translated into
>> SPI command transfers. When in READ/WRITE command mode, the HW
>> generates the SPI commands depending on the setting of the CE control
>> register.
>>
>> The following driver supports the FMC and the SPI controllers with the
>> attached SPI flash devices. When the controller is probed, the driver
>> performs a read timing calibration using specific DMA control
>> registers (FMC only). The driver's primary goal is to support the
>> first device of the FMC controller on which reside U-Boot but it
>> should support the other controllers also.
>>
>> The Aspeed FMC controller automatically detects at boot time if a
>> flash device is in 4BYTE address mode and self configures to use the
>> appropriate address width. This can be a problem for the U-Boot SPI
>> Flash layer which only supports 3 byte addresses. In such a case, a
>> warning is emitted and the address width is fixed when sent on the
>> bus.
>>
>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>
> Looks good. I compared some things against the data sheet, and read
> through the driver. There were to small questions I had, so please
> clear those up and add my:
>
> Reviewed-by: Joel Stanley <j...@jms.id.au>
>
>> +static u32 aspeed_spi_hclk_divisor(u32 hclk_rate, u32 max_hz)
>> +{
>> + /* HCLK/1 .. HCLK/16 */
>> + const u8 hclk_divisors[] = {
>> + 15, 7, 14, 6, 13, 5, 12, 4, 11, 3, 10, 2, 9, 1, 8, 0
>> + };
>> +
>> + u32 i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(hclk_divisors); i++) {
>> + if (max_hz >= (hclk_rate / (i + 1)))
>
> We spoke offline about why the values in hclk_divisors are not used in
> this calculation. I think we decided to change the name of
> hclk_divisors to something like hclk_masks?
yes. I have changed the name everywhere as it makes more sense.
>> + break;
>> + }
>> +
>> + debug("hclk=%d required=%d divisor is %d (mask %x) speed=%d\n",
>> + hclk_rate, max_hz, i + 1, hclk_divisors[i], hclk_rate / (i +
>> 1));
>> +
>> + return hclk_divisors[i];
>> +}
>
>> +static u32 aspeed_spi_read_checksum(struct aspeed_spi_priv *priv, u8 div,
>> + u8 delay)
>> +{
>> + /* TODO: the SPI controllers do not have the DMA registers.
>> + * The algorithm is the same.
>> + */
>> + if (!priv->is_fmc) {
>> + pr_warn("No timing calibration support for SPI controllers");
>> + return 0xbadc0de;
>> + }
>> +
>> + return aspeed_spi_fmc_checksum(priv, div, delay);
>> +}
>> +
>> +static int aspeed_spi_timing_calibration(struct aspeed_spi_priv *priv)
>> +{
>> + /* HCLK/5 .. HCLK/1 */
>> + const u8 hclk_divisors[] = { 13, 6, 14, 7, 15 };
>> +
>> + u32 timing_reg = 0;
>> + u32 checksum, gold_checksum;
>> + int i, j;
>> +
>> + debug("Read timing calibration :\n");
>> +
>> + /* Compute reference checksum at lowest freq HCLK/16 */
>> + gold_checksum = aspeed_spi_read_checksum(priv, 0, 0);
>> +
>> + /*
>> + * Set CE0 Control Register to FAST READ command mode. The
>> + * HCLK divisor will be set through the DMA Control Register.
>> + */
>> + writel(CE_CTRL_CMD(0xB) | CE_CTRL_DUMMY(1) | CE_CTRL_FREADMODE,
>> + &priv->regs->ce_ctrl[0]);
>> +
>> + /* Increase HCLK freq */
>> + for (i = 0; i < ARRAY_SIZE(hclk_divisors); i++) {
>> + u32 hdiv = 5 - i;
>> + u32 hshift = (hdiv - 1) << 2;
>> + bool pass = false;
>> + u8 delay;
>> +
>> + if (priv->hclk_rate / hdiv > priv->max_hz) {
>> + debug("skipping freq %ld\n", priv->hclk_rate / hdiv);
>> + continue;
>> + }
>> +
>> + /* Increase HCLK delays until read succeeds */
>> + for (j = 0; j < 6; j++) {
>
> 6 here is the number of items in hclk_divisors?
no. these are counts of HCLK cycles before input, possible values are [0-5].
I will add a define to clarify '6'
>
>> + /* Try first with a 4ns DI delay */
>> + delay = 0x8 | j;
>> + checksum = aspeed_spi_read_checksum(
>> + priv, hclk_divisors[i], delay);
>> + pass = (checksum == gold_checksum);
>> + debug(" HCLK/%d, 4ns DI delay, %d HCLK delay : %s\n",
>> + hdiv, j, pass ? "PASS" : "FAIL");
>> +
>> + /* Try again with more HCLK delays */
>> + if (!pass)
>> + continue;
>> +
>> + /* Try without the 4ns DI delay */
>> + delay = j;
>> + checksum = aspeed_spi_read_checksum(
>> + priv, hclk_divisors[i], delay);
>> + pass = (checksum == gold_checksum);
>> + debug(" HCLK/%d, 0ns DI delay, %d HCLK delay : %s\n",
>> + hdiv, j, pass ? "PASS" : "FAIL");
>> +
>> + /* All good for this freq */
>> + if (pass)
>> + break;
>> + }
>> +
>> + if (pass) {
>> + timing_reg &= ~(0xfu << hshift);
>> + timing_reg |= delay << hshift;
>> + }
>> + }
>> +
>> + debug("Timing Register set to 0x%08x\n", timing_reg);
>> + writel(timing_reg, &priv->regs->timings);
>> +
>> + /* Reset CE0 Control Register */
>> + writel(0x0, &priv->regs->ce_ctrl[0]);
>> + return 0;
>> +}
>> +
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
