[U-Boot] [PATCH v5] dm: at91: Add driver model support for the spi driver

[Jagan Teki]

On 25 July 2016 at 13:34,  <Wenyou.Yang at microchip.com> wrote:
> Hi Jagan,
>
>> -----Original Message-----
>> From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
>> Sent: 2016年7月21日 15:10
>> To: Wenyou Yang <wenyou.yang at atmel.com>
>> Cc: U-Boot Mailing List <u-boot at lists.denx.de>
>> Subject: Re: [U-Boot] [PATCH v5] dm: at91: Add driver model support for the spi
>> driver
>>
>> On 20 July 2016 at 15:05, Wenyou Yang <wenyou.yang at atmel.com> wrote:
>> > Add driver model support while retaining the existing legacy code.
>> > This allows the driver to support boards that have converted to driver
>> > model as well as those that have not.
>> >
>> > Signed-off-by: Wenyou Yang <wenyou.yang at atmel.com>
>> > Reviewed-by: Simon Glass <sjg at chromium.org>
>> > ---
>> >
>> > Changes in v5:
>> >  - Change clk_client.h -> clk.h to adapt to clk API conversion.
>> >
>> > Changes in v4:
>> >  - Collect Reviewed-by tag.
>> >  - Update the clk API based on [PATCH] clk: convert API to match
>> >    reset/mailbox fstyle (http://patchwork.ozlabs.org/patch/625342/).
>> >  - Remove check on dev_get_parent() return.
>> >  - Fixed the return value, -ENODEV->-EINVAL.
>> >  - Retain #include <asm/arch/clk.h> line.
>> >
>> > Changes in v3:
>> >  - Remove redundant log print.
>> >
>> > Changes in v2:
>> >  - Add clock support.
>> >
>> >  drivers/spi/Kconfig     |   9 ++
>> >  drivers/spi/atmel_spi.c | 303
>> > ++++++++++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 312 insertions(+)
>> >
>> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index
>> > aca385d..e015d1d 100644
>> > --- a/drivers/spi/Kconfig
>> > +++ b/drivers/spi/Kconfig
>> > @@ -32,6 +32,15 @@ config ATH79_SPI
>> >           uses driver model and requires a device tree binding to operate.
>> >           please refer to doc/device-tree-bindings/spi/spi-ath79.txt.
>> >
>> > +config ATMEL_SPI
>> > +       bool "Atmel SPI driver"
>> > +       depends on ARCH_AT91
>> > +       select SPI_FLASH
>> > +       select SPI_FLASH_ATMEL
>>
>> These two configs's are flash related don't add these in spi side,  and
>
> Accepted.
>
>> > +       help
>> > +         Enable the Atmel SPI driver. This driver can be used to access
>> > +         the SPI Flash, such as AT25DF321.
>> > +
>> >  config CADENCE_QSPI
>> >         bool "Cadence QSPI driver"
>> >         help
>> > diff --git a/drivers/spi/atmel_spi.c b/drivers/spi/atmel_spi.c index
>> > ed6278b..98bee35 100644
>> > --- a/drivers/spi/atmel_spi.c
>> > +++ b/drivers/spi/atmel_spi.c
>> > @@ -4,6 +4,9 @@
>> >   * SPDX-License-Identifier:    GPL-2.0+
>> >   */
>> >  #include <common.h>
>> > +#include <clk.h>
>> > +#include <dm.h>
>> > +#include <fdtdec.h>
>> >  #include <spi.h>
>> >  #include <malloc.h>
>> >
>> > @@ -11,9 +14,15 @@
>> >
>> >  #include <asm/arch/clk.h>
>> >  #include <asm/arch/hardware.h>
>> > +#include <asm/arch/at91_spi.h>
>> > +#include <asm/gpio.h>
>> >
>> >  #include "atmel_spi.h"
>> >
>> > +DECLARE_GLOBAL_DATA_PTR;
>> > +
>> > +#ifndef CONFIG_DM_SPI
>> > +
>> >  static int spi_has_wdrbt(struct atmel_spi_slave *slave)  {
>> >         unsigned int ver;
>> > @@ -209,3 +218,297 @@ out:
>> >
>> >         return 0;
>> >  }
>> > +
>> > +#else
>> > +
>> > +#define MAX_CS_COUNT   4
>> > +
>> > +struct atmel_spi_platdata {
>> > +       struct at91_spi *regs;
>> > +};
>> > +
>> > +struct atmel_spi_priv {
>> > +       unsigned int freq;              /* Default frequency */
>> > +       unsigned int mode;
>> > +       ulong bus_clk_rate;
>> > +       struct gpio_desc cs_gpios[MAX_CS_COUNT]; };
>> > +
>> > +static int atmel_spi_claim_bus(struct udevice *dev) {
>> > +       struct udevice *bus = dev_get_parent(dev);
>> > +       struct atmel_spi_platdata *bus_plat = dev_get_platdata(bus);
>> > +       struct atmel_spi_priv *priv = dev_get_priv(bus);
>> > +       struct dm_spi_slave_platdata *slave_plat = dev_get_parent_platdata(dev);
>> > +       struct at91_spi *reg_base = bus_plat->regs;
>> > +       u32 cs = slave_plat->cs;
>> > +       u32 freq = priv->freq;
>> > +       u32 scbr, csrx, mode;
>> > +
>> > +       scbr = (priv->bus_clk_rate + freq - 1) / freq;
>> > +       if (scbr > ATMEL_SPI_CSRx_SCBR_MAX)
>> > +               return -EINVAL;
>> > +
>> > +       if (scbr < 1)
>> > +               scbr = 1;
>> > +
>> > +       csrx = ATMEL_SPI_CSRx_SCBR(scbr);
>> > +       csrx |= ATMEL_SPI_CSRx_BITS(ATMEL_SPI_BITS_8);
>> > +
>> > +       if (!(priv->mode & SPI_CPHA))
>> > +               csrx |= ATMEL_SPI_CSRx_NCPHA;
>> > +       if (priv->mode & SPI_CPOL)
>> > +               csrx |= ATMEL_SPI_CSRx_CPOL;
>> > +
>> > +       writel(csrx, &reg_base->csr[cs]);
>> > +
>> > +       mode = ATMEL_SPI_MR_MSTR |
>> > +              ATMEL_SPI_MR_MODFDIS |
>> > +              ATMEL_SPI_MR_WDRBT |
>> > +              ATMEL_SPI_MR_PCS(~(1 << cs));
>> > +
>> > +       writel(mode, &reg_base->mr);
>> > +
>>
>> claim_bus means have to enable spi not doing mode and freq stuff because this
>> claim is calling for every transactions from flash so, doing mode and freq stuff
>> every time little overhead. skip these and add mode stuff in atmel_spi_set_mode
>> freq in atmel_spi_set_speed.
>
> I totally agreed your opinions.
>
> But for Atmel SPI,  for different cs, the frequency is set by different the SPI_CSRx registers.
> That is to say, cs = 0, it is set by SPI_CSR0, cs =1 it is by SPI_CSR1, and so on.
> And ops callback (*set_speed)(struct udevice *bus, uint hz) doesn't provide the cs or slave device information.
>
> Similar reason for mode set.
>
> Maybe we should improve these callbacks. Do you agree?

Clear, yes we need to fix these callbacks.

thanks!
-- 
Jagan.