[U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

["Cédric Le Goater" <clg@kaod.org>]

On 09/12/2018 12:38 AM, Joel Stanley wrote:
> On Mon, 10 Sep 2018 at 23:48, Cédric Le Goater <clg@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 <clg@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 <joel@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;
>> +}
>> +