[U-Boot] [PATCH v2] spi: kirkwood_spi: implement workaround for FE-9144572

[Chris Packham]

On Tue, Oct 17, 2017 at 10:46 AM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
> On Tue, Oct 17, 2017 at 3:07 AM, Chris Packham <judge.packham at gmail.com> wrote:
>> Erratum NO. FE-9144572: The device SPI interface supports frequencies of
>> up to 50 MHz.  However, due to this erratum, when the device core clock
>> is 250 MHz and the SPI interfaces is configured for 50MHz SPI clock and
>> CPOL=CPHA=1 there might occur data corruption on reads from the SPI
>> device.
>>
>> Implement the workaround by setting the TMISO_SAMPLE value to 0x2
>> in the timing1 register.
>>
>> Signed-off-by: Chris Packham <judge.packham at gmail.com>
>> Reviewed-by: Stefan Roese <sr at denx.de>
>> ---
>> I've based this as much as I can on the equivalent implementation in the
>> Linux kernel[1], but there are differences in the u-boot spi
>> infrastructure that means I can't be as specific when qualifying whether
>> the workaround is needed.
>>
>> [1] - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi-orion.c#n248
>>
>> Changes in v2:
>> - collect reviewed-by from Stefan
>> - remove mvebu_spi_type
>> - describe errata above mvebu_spi_50mhz_ac_timing_erratum
>>
>>  arch/arm/include/asm/arch-mvebu/spi.h |  6 ++++
>>  drivers/spi/kirkwood_spi.c            | 61 +++++++++++++++++++++++++++++++++--
>>  2 files changed, 64 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/arch-mvebu/spi.h b/arch/arm/include/asm/arch-mvebu/spi.h
>> index 3545aed17347..1de510ea6da9 100644
>> --- a/arch/arm/include/asm/arch-mvebu/spi.h
>> +++ b/arch/arm/include/asm/arch-mvebu/spi.h
>> @@ -57,6 +57,12 @@ struct kwspi_registers {
>>  #define KWSPI_TXLSBF           (1 << 13)
>>  #define KWSPI_RXLSBF           (1 << 14)
>>
>> +/* Timing Parameters 1 Register */
>> +#define KW_SPI_TMISO_SAMPLE_OFFSET     6
>> +#define KW_SPI_TMISO_SAMPLE_MASK       (0x3 << KW_SPI_TMISO_SAMPLE_OFFSET)
>> +#define KW_SPI_TMISO_SAMPLE_1          (1 << KW_SPI_TMISO_SAMPLE_OFFSET)
>> +#define KW_SPI_TMISO_SAMPLE_2          (2 << KW_SPI_TMISO_SAMPLE_OFFSET)
>> +
>>  #define KWSPI_IRQUNMASK                1 /* unmask SPI interrupt */
>>  #define KWSPI_IRQMASK          0 /* mask SPI interrupt */
>>  #define KWSPI_SMEMRDIRQ                1 /* SerMem data xfer ready irq */
>> diff --git a/drivers/spi/kirkwood_spi.c b/drivers/spi/kirkwood_spi.c
>> index 0c6bd295cde9..5b1ac0eed634 100644
>> --- a/drivers/spi/kirkwood_spi.c
>> +++ b/drivers/spi/kirkwood_spi.c
>> @@ -243,6 +243,10 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
>>
>>  /* Here now the DM part */
>>
>> +struct mvebu_spi_dev {
>> +       bool                    is_errata_50mhz_ac;
>> +};
>> +
>>  struct mvebu_spi_platdata {
>>         struct kwspi_registers *spireg;
>>  };
>> @@ -269,10 +273,38 @@ static int mvebu_spi_set_speed(struct udevice *bus, uint hz)
>>         return 0;
>>  }
>>
>> +/*
>> + * FE-9144572: The device SPI interface supports frequencies of up to 50 MHz.
>> + * However, due to this erratum, when the device core clock is 250 MHz and the
>> + * SPI interfaces is configured for 50MHz SPI clock and CPOL=CPHA=1 there might
>> + * occur data corruption on reads from the SPI device.
>> + *
>> + * Workaround: Set the TMISO_SAMPLE value to 0x2 in the SPI Timing Parameters 1
>> + * register
>> + */
>> +static void mvebu_spi_50mhz_ac_timing_erratum(struct udevice *bus, uint mode)
>> +{
>> +       struct mvebu_spi_platdata *plat = dev_get_platdata(bus);
>> +       struct kwspi_registers *reg = plat->spireg;
>> +       u32 data = readl(&reg->timing1);
>> +
>> +       data &= ~KW_SPI_TMISO_SAMPLE_MASK;
>> +
>> +       if (CONFIG_SYS_TCLK == 250000000 &&
>> +           mode & SPI_CPOL &&
>> +           mode & SPI_CPHA)
>> +               data |= KW_SPI_TMISO_SAMPLE_2;
>> +       else
>> +               data |= KW_SPI_TMISO_SAMPLE_1;
>> +
>> +       writel(data, &reg->timing1);
>> +}
>> +
>>  static int mvebu_spi_set_mode(struct udevice *bus, uint mode)
>>  {
>>         struct mvebu_spi_platdata *plat = dev_get_platdata(bus);
>>         struct kwspi_registers *reg = plat->spireg;
>> +       const struct mvebu_spi_dev *drvdata;
>>         u32 data = readl(&reg->cfg);
>>
>>         data &= ~(KWSPI_CPHA | KWSPI_CPOL | KWSPI_RXLSBF | KWSPI_TXLSBF);
>> @@ -286,6 +318,10 @@ static int mvebu_spi_set_mode(struct udevice *bus, uint mode)
>>
>>         writel(data, &reg->cfg);
>>
>> +       drvdata = (struct mvebu_spi_dev *)dev_get_driver_data(bus);
>> +       if (drvdata->is_errata_50mhz_ac)
>> +               mvebu_spi_50mhz_ac_timing_erratum(bus, mode);
>> +
>>         return 0;
>>  }
>>
>> @@ -343,10 +379,29 @@ static const struct dm_spi_ops mvebu_spi_ops = {
>>          */
>>  };
>>
>> +static const struct mvebu_spi_dev armada_xp_spi_dev_data = {
>> +};
>> +
>> +static const struct mvebu_spi_dev armada_375_spi_dev_data = {
>> +};
>
> Unused, may be we can drop these and add NULL or equivalent to data
>

Technically I'm relying on the c99 initialiser to set
is_errata_50mhz_ac to 0. I could remove them but then I'd need to add
an extra check to mvebu_spi_set_mode. I could also make the
is_errata_50mhz_ac = false explicit. Any particular preference?