[U-Boot] [PATCH v1]dm : spi: Convert Freescale DSPI to driver model

Simon Glass sjg at chromium.org
Thu Mar 5 20:45:16 CET 2015


+Tom for the coding style question

Hi,

On 4 March 2015 at 04:22, Haikun.Wang at freescale.com
<Haikun.Wang at freescale.com> wrote:
>
>> -----Original Message-----
>> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
>> Sent: Tuesday, March 03, 2015 2:56 AM
>> To: Wang Haikun-B53464
>> Cc: u-boot at lists.denx.de; Jagan Teki
>> Subject: Re: [U-Boot] [PATCH v1]dm : spi: Convert Freescale DSPI to
>> driver model
>>
>> Hi,
>>
>> On 28 February 2015 at 03:54, Haikun.Wang at freescale.com
>> <Haikun.Wang at freescale.com> wrote:
>> > Move the Freescale DSPI driver over to driver model.
>> >
>> > Signed-off-by: Haikun Wang <B53464 at freescale.com>
>> > ---
>> >
>> > This patch adds two new files drivers/spi/fsl_dspi.c and
>> include/fsl_dspi.h.
>> > They will replace files drivers/spi/cf_spi.c and
>> > arch/m68k/include/asm/coldfire/dspi.h.
>> > I need submit patch to remove them later.
>> > Board dts files are also needed to make this change work.
>>
>> Apart from one thing (the chip selects) this all looks correct to me.
>> Some style comments below. Also the patch seems to be in Courier font!
>>
>> I'm interested in your thoughts on what is missing from the SPI uclass
>> too.
>>
>> >
>> > Changes in v1: None
>> >
>> > drivers/spi/Makefile   |   1 +
>> > drivers/spi/fsl_dspi.c | 461
>> > +++++++++++++++++++++++++++++++++++++++++++++++++
>> > include/fsl_dspi.h     | 156 +++++++++++++++++
>> > 3 files changed, 618 insertions(+)
>> > create mode 100644 drivers/spi/fsl_dspi.c create mode 100644
>> > include/fsl_dspi.h
>> >
>> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index
>> > edbd520..9c2b8de 100644
>> > --- a/drivers/spi/Makefile
>> > +++ b/drivers/spi/Makefile
>> > @@ -49,3 +49,4 @@ obj-$(CONFIG_TI_QSPI) += ti_qspi.o
>> > obj-$(CONFIG_XILINX_SPI) += xilinx_spi.o
>> > obj-$(CONFIG_ZYNQ_SPI) += zynq_spi.o
>> > obj-$(CONFIG_FSL_QSPI) += fsl_qspi.o
>> > +obj-$(CONFIG_FSL_DSPI) += fsl_dspi.o
>> > diff --git a/drivers/spi/fsl_dspi.c b/drivers/spi/fsl_dspi.c new file
>> > mode 100644 index 0000000..69c037b
>> > --- /dev/null
>> > +++ b/drivers/spi/fsl_dspi.c
>> > @@ -0,0 +1,461 @@
>> > +/*
>> > + * (C) Copyright 2000-2003
>> > + * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
>> > + *
>> > + * Copyright (C) 2004-2009, 2015 Freescale Semiconductor, Inc.
>> > + * TsiChung Liew (Tsi-Chung.Liew at freescale.com)
>> > + * Chao Fu (B44548 at freescale.com)
>> > + * Haikun Wang (B53464 at freescale.com)
>> > + *
>> > + * SPDX-License-Identifier:    GPL-2.0+
>> > + */
>> > +#include <dm.h>
>> > +#include <errno.h>
>> > +#include <common.h>
>> > +#include <spi.h>
>> > +#include <malloc.h>
>> > +#include <asm/io.h>
>> > +#include <fdtdec.h>
>> > +#include <asm/arch/clock.h>
>> > +#include <fsl_dspi.h>
>> > +
>> > +
>>
>> Remove extract blank line
>>
>> > +DECLARE_GLOBAL_DATA_PTR;
>> > +
>> > +/* fsl_dspi_platdata flag */
>> > +#define DSPI_FLAG_REGMAP_ENDIAN_BIG    (1 << 0)
>> > +
>> > +/* idle data value */
>> > +#define DSPI_IDLE_VAL                 (0x0)
>>
>> Please no brackets around simple constants (I understand if they are -ve,
>> but this serves no purpose).
> [] fine.
>>
>> > +
>> > +/* max chipselect signals number */
>> > +#define FSL_DSPI_MAX_CHIPSELECT        (6)
>> > +
>> > +/* CTAR register pre-configure value */
>> > +#define DSPI_CTAR_DEFAULT_VALUE        (DSPI_CTAR_TRSZ(7) | \
>> > +                                       DSPI_CTAR_PCSSCK_1CLK | \
>> > +                                       DSPI_CTAR_PASC(0) | \
>> > +                                       DSPI_CTAR_PDT(0) | \
>> > +                                       DSPI_CTAR_CSSCK(0) | \
>> > +                                       DSPI_CTAR_ASC(0) | \
>> > +                                       DSPI_CTAR_DT(0))
>> > +
>> > +/* CTAR register pre-configure mask */
>> > +#define DSPI_CTAR_SET_MODE_MASK        (DSPI_CTAR_TRSZ(15) | \
>> > +                                       DSPI_CTAR_PCSSCK(3) | \
>> > +                                       DSPI_CTAR_PASC(3) | \
>> > +                                       DSPI_CTAR_PDT(3) | \
>> > +                                       DSPI_CTAR_CSSCK(15) | \
>> > +                                       DSPI_CTAR_ASC(15) | \
>> > +                                       DSPI_CTAR_DT(15))
>> > +
>>
>> Please comment these things below:
> [] fine.
>>
>> /**
>>  * struct fsl_dspi_platdata - platform data for ...
>>  *
>>  * @flag:    some sort of flag and you can find the enum here...
>> ...
>>
>> > +struct fsl_dspi_platdata {
>> > +       uint flag;
>> > +       uint baudrate;
>> > +       uint num_chipselect;
>> > +       uint regs;
>> > +};
>> > +
>> > +struct fsl_dspi_priv {
>> > +       uint mode;
>> > +       uint mcr_val;
>> > +       uint bus_clk;
>> > +       uint baudrate;
>> > +       uint charbit;
>> > +       uint num_chipselect;
>> > +       uint ctar_val[FSL_DSPI_MAX_CHIPSELECT];
>> > +       uint regs;
>> > +       struct dm_spi_slave_platdata *cur_slave_plat;
>>
>> What is that for?
>>
>> > +};
>> > +
>> > +static uint dspi_read32(struct udevice *bus, uint offset) {
>> > +       struct fsl_dspi_platdata *plat = dev_get_platdata(bus);
>>
>> blank line between declarations and code (throughout)
>>
>> > +       return plat->flag & DSPI_FLAG_REGMAP_ENDIAN_BIG ?
>> > +               in_be32(plat->regs + offset) : in_le32(plat->regs +
>> > + offset);
>>
>> I think it is better to put regs in your private data rather than
>> platform data. Platform data should be used in your probe function mostly.
>> It's OK to use it sparing for uncommon things, but this feels like
>> something that should be in private data.
> [] Ok, I will move it.
>>
>> I have tended to put an fdt_addr_t in platform data and a struct
>> reg_definition * in private data.
>>
>> Also where is your C structure for the registers? We normally use that
>> sort of thing in U-Boot.
> [] There is no obvious difference between the two methods. And I think the code seems more clean in current way.

See the bottom of this page.

http://www.denx.de/wiki/U-Boot/CodingStyle

>>
>> > +}
>> > +
>> > +static void dspi_write32(struct udevice *bus, uint offset, uint val)
>> > +{
>> > +       struct fsl_dspi_platdata *plat = dev_get_platdata(bus);
>> > +       plat->flag & DSPI_FLAG_REGMAP_ENDIAN_BIG ?
>> > +               out_be32(plat->regs + offset, val) :
>> > +               out_le32(plat->regs + offset, val);
>> > +       return;
>> > +}
>> > +
>> > +static void dspi_halt(struct udevice *bus, u8 halt) {
>> > +       uint mcr_val;
>> > +
>> > +       mcr_val = dspi_read32(bus, DSPI_MCR);
>> > +
>> > +       if (halt)
>> > +               mcr_val |= DSPI_MCR_HALT;
>> > +       else
>> > +               mcr_val &= ~DSPI_MCR_HALT;
>> > +
>> > +       dspi_write32(bus, DSPI_MCR, mcr_val);
>> > +
>> > +       return;
>> > +}
>> > +
>> > +static int fsl_dspi_child_pre_probe(struct udevice *dev) {
>> > +       struct dm_spi_slave_platdata *slave_plat =
>> > dev_get_parent_platdata(dev);
>> > +       struct fsl_dspi_priv *priv = dev_get_priv(dev->parent);
>> > +
>> > +       if (slave_plat->cs >= priv->num_chipselect) {
>> > +               printf("DSPI invalid chipselect number %d(max %d)!\n",
>> > +                      slave_plat->cs, priv->num_chipselect - 1);
>>
>> Maybe you want debug() here, up to you.
> [] ok.
>>
>> > +               return -EINVAL;
>> > +       }
>> > +
>> > +       priv->ctar_val[slave_plat->cs] = DSPI_CTAR_DEFAULT_VALUE;
>> > +
>> > +       priv->cur_slave_plat = slave_plat;
>>
>> Not sure what you are doing here?
>>
>> > +
>> > +       debug("DSPI pre_probe slave device on CS %u, max_hz %u, mode
>> > 0x%x.\n",
>> > +             slave_plat->cs, slave_plat->max_hz, slave_plat->mode);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int fsl_dspi_child_post_remove(struct udevice *dev) {
>> > +       struct fsl_dspi_priv *priv = dev_get_priv(dev->parent);
>> > +       priv->cur_slave_plat = NULL;
>>
>> or here?
>>
>> > +       return 0;
>> > +}
>> > +
>> > +static int fsl_dspi_probe(struct udevice *bus) {
>> > +       struct fsl_dspi_platdata *plat = dev_get_platdata(bus);
>> > +       struct fsl_dspi_priv *priv = dev_get_priv(bus);
>> > +       struct dm_spi_bus *dm_spi_bus;
>> > +       uint mcr_val = 0;
>> > +
>> > +       dm_spi_bus = bus->uclass_priv;
>> > +
>> > +       /* get input clk frequency */
>> > +       priv->bus_clk = mxc_get_clock(MXC_DSPI_CLK);
>> > +       priv->regs = plat->regs;
>> > +       priv->num_chipselect = plat->num_chipselect;
>> > +       priv->baudrate = plat->baudrate;
>> > +       /* frame data length in bits, default 8bits */
>> > +       priv->charbit = 8;
>> > +
>> > +       dm_spi_bus->max_hz = plat->baudrate;
>> > +
>> > +       /* halt DSPI module */
>> > +       dspi_halt(bus, 1);
>> > +
>> > +       /* default: all CS inactive state is high */
>> > +       mcr_val |= DSPI_MCR_MSTR | DSPI_MCR_PCSIS_MASK |
>> > +               DSPI_MCR_CRXF | DSPI_MCR_CTXF;
>> > +       dspi_write32(bus, DSPI_MCR, mcr_val);
>> > +
>> > +       /* resume module */
>> > +       dspi_halt(bus, 0);
>> > +
>> > +       priv->mcr_val = mcr_val;
>> > +
>> > +       debug("%s probe done, bus-num %d.\n", bus->name, bus->seq);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int fsl_dspi_claim_bus(struct udevice *bus) {
>> > +       uint mcr_val;
>> > +
>> > +       dspi_halt(bus, 1);
>> > +       mcr_val = dspi_read32(bus, DSPI_MCR);
>> > +       /* flush RX and TX FIFO */
>> > +       mcr_val |= (DSPI_MCR_CTXF | DSPI_MCR_CRXF);
>> > +       dspi_write32(bus, DSPI_MCR, mcr_val);
>> > +
>> > +       dspi_halt(bus, 0);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int fsl_dspi_release_bus(struct udevice *bus) {
>> > +       /* halt module */
>> > +       dspi_halt(bus, 1);
>> > +       return 0;
>> > +}
>> > +
>> > +static void dspi_tx(struct udevice *bus, u32 ctrl, u16 data) {
>> > +       while (((dspi_read32(bus, DSPI_SR) & 0x0000F000) >> 12) >= 4)
>> > +               ;
>>
>> Can this ever timeout/hang? Also why are you using u16 for the data?
>> It doesn't seem necessary here or in the next function. You could use
>> u32 I think?
> [] fine, I will handle the timeout case. Max data size of every transfer is 16bits, so I use u16.
>>
>> > +
>> > +       dspi_write32(bus, DSPI_TFR, (ctrl | data));
>> > +
>> > +       return;
>> > +}
>> > +
>> > +static u16 dspi_rx(struct udevice *bus) {
>> > +       while ((dspi_read32(bus, DSPI_SR) & 0x000000F0) == 0)
>> > +               ;
>>
>> or this?
>>
>> > +
>> > +       return (u16)(dspi_read32(bus, DSPI_RFR) & 0xFFFF); }
>> > +
>> > +static int fsl_dspi_xfer(struct udevice *dev, unsigned int bitlen,
>> > +               const void *dout, void *din, unsigned long flags) {
>> > +       struct fsl_dspi_priv *priv;
>> > +       struct dm_spi_slave_platdata *slave_plat =
>> > dev_get_parent_platdata(dev);
>> > +       struct udevice *bus;
>> > +       u16 *spi_rd16 = NULL, *spi_wr16 = NULL;
>> > +       u8 *spi_rd = NULL, *spi_wr = NULL;
>> > +       static u32 ctrl;
>> > +       uint len = bitlen >> 3;
>> > +
>> > +       bus = dev->parent;
>> > +       if (bus)
>> > +               priv = dev_get_priv(bus);
>> > +       else
>> > +               return -ENODEV;
>>
>> This must have a parent. There is no need for this check. If someone
>> calls this with the root device they deserve trouble! If you like you
>> could have
>>
>> assert(bus);
> [] fine, I will remove.
>>
>> > +
>> > +       if (priv->charbit == 16) {
>> > +               bitlen >>= 1;
>> > +               spi_wr16 = (u16 *)dout;
>> > +               spi_rd16 = (u16 *)din;
>> > +       } else {
>> > +               spi_wr = (u8 *)dout;
>> > +               spi_rd = (u8 *)din;
>> > +       }
>> > +
>> > +       if ((flags & SPI_XFER_BEGIN) == SPI_XFER_BEGIN)
>> > +               ctrl |= DSPI_TFR_CONT;
>> > +
>> > +       ctrl = ctrl & DSPI_TFR_CONT;
>> > +       ctrl = ctrl | DSPI_TFR_CTAS(0) | DSPI_TFR_PCS(slave_plat->cs);
>> > +
>> > +       if (len > 1) {
>> > +               int tmp_len = len - 1;
>> > +               while (tmp_len--) {
>> > +                       if (dout != NULL) {
>> > +                               if (priv->charbit == 16)
>> > +                                       dspi_tx(bus, ctrl, *spi_wr16++);
>> > +                               else
>> > +                                       dspi_tx(bus, ctrl, *spi_wr++);
>> > +                               dspi_rx(bus);
>> > +                       }
>> > +
>> > +                       if (din != NULL) {
>> > +                               dspi_tx(bus, ctrl, DSPI_IDLE_VAL);
>> > +                               if (priv->charbit == 16)
>> > +                                       *spi_rd16++ = dspi_rx(bus);
>> > +                               else
>> > +                                       *spi_rd++ = dspi_rx(bus);
>> > +                       }
>> > +               }
>> > +
>> > +               len = 1;        /* remaining byte */
>> > +       }
>> > +
>> > +       if ((flags & SPI_XFER_END) == SPI_XFER_END)
>> > +               ctrl &= ~DSPI_TFR_CONT;
>> > +
>> > +       if (len) {
>> > +               if (dout != NULL) {
>> > +                       if (priv->charbit == 16)
>> > +                               dspi_tx(bus, ctrl, *spi_wr16);
>> > +                       else
>> > +                               dspi_tx(bus, ctrl, *spi_wr);
>> > +                       dspi_rx(bus);
>> > +               }
>> > +
>> > +               if (din != NULL) {
>> > +                       dspi_tx(bus, ctrl, DSPI_IDLE_VAL);
>> > +                       if (priv->charbit == 16)
>> > +                               *spi_rd16 = dspi_rx(bus);
>> > +                       else
>> > +                               *spi_rd = dspi_rx(bus);
>> > +               }
>> > +       } else {
>> > +               /* dummy read */
>> > +               dspi_tx(bus, ctrl, DSPI_IDLE_VAL);
>> > +               dspi_rx(bus);
>> > +       }
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int fsl_dspi_hz_to_spi_baud(int *pbr, int *br,
>> > +               int speed_hz, uint clkrate)
>>
>> Should document args here.
> [] fine, I will add.
>>
>> > +{
>> > +       /* Valid baud rate pre-scaler values */
>> > +       int pbr_tbl[4] = {2, 3, 5, 7};
>> > +       int brs[16] = {2, 4, 6, 8,
>> > +               16, 32, 64, 128,
>> > +               256, 512, 1024, 2048,
>> > +               4096, 8192, 16384, 32768};
>> > +       int temp, i = 0, j = 0;
>> > +
>> > +       temp = clkrate / speed_hz;
>> > +
>> > +       for (i = 0; i < ARRAY_SIZE(pbr_tbl); i++)
>> > +               for (j = 0; j < ARRAY_SIZE(brs); j++) {
>> > +                       if (pbr_tbl[i] * brs[j] >= temp) {
>> > +                               *pbr = i;
>> > +                               *br = j;
>> > +                               return 0;
>> > +                       }
>> > +               }
>> > +
>> > +       printf("Can not find valid baud rate,speed_hz is %d, ",
>> speed_hz);
>> > +       printf("clkrate is %d, we use the max prescaler value.\n",
>> > + clkrate);
>> > +
>> > +       *pbr = ARRAY_SIZE(pbr_tbl) - 1;
>> > +       *br =  ARRAY_SIZE(brs) - 1;
>> > +       return -EINVAL;
>> > +}
>> > +
>> > +static int fsl_dspi_set_speed(struct udevice *bus, uint speed) {
>> > +       u8 cs;
>> > +       int ret;
>> > +       uint bus_setup;
>> > +       int best_i, best_j, bus_clk;
>> > +       struct fsl_dspi_priv *priv = dev_get_priv(bus);
>> > +       struct dm_spi_slave_platdata *slave_plat =
>> > +priv->cur_slave_plat;
>> > +
>> > +       cs = slave_plat->cs;
>>
>> You can't do this. Is the problem that you have a different speed setting
>> for each chip select? If so then we should either fix the uclass
>> interface to support this, or you could store the speed and adjust it
>> just before the transfer. But in this function you don't know the chip
>> select and should not try to guess it.
> []fine, CS is really not needed in this function, I will improve.
>>
>> > +
>> > +       bus_clk = priv->bus_clk;
>> > +
>> > +       debug("DSPI set_speed: expected SCK speed %u, bus_clk %u.\n",
>> > +             speed, bus_clk);
>> > +
>> > +       bus_setup = dspi_read32(bus, DSPI_CTAR(0));
>> > +       bus_setup &= ~(DSPI_CTAR_DBR | DSPI_CTAR_PBR(0x3) |
>> > DSPI_CTAR_BR(0xf));
>> > +       bus_setup |= priv->ctar_val[cs];
>> > +
>> > +       ret = fsl_dspi_hz_to_spi_baud(&best_i, &best_j, speed, bus_clk);
>> > +       if (ret) {
>> > +               speed = priv->baudrate;
>> > +               debug("DSPI set_speed use default SCK rate %u.\n",
>> speed);
>> > +               fsl_dspi_hz_to_spi_baud(&best_i, &best_j, speed,
>> bus_clk);
>> > +       }
>> > +
>> > +       bus_setup |= (DSPI_CTAR_PBR(best_i) | DSPI_CTAR_BR(best_j));
>> > +       dspi_write32(bus, DSPI_CTAR(0), bus_setup);
>> > +
>> > +       priv->baudrate = speed;
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int fsl_dspi_set_mode(struct udevice *bus, uint mode) {
>> > +       u8 cs;
>> > +       uint bus_setup;
>> > +       uint mcr_val;
>> > +       struct fsl_dspi_priv *priv = dev_get_priv(bus);
>> > +       struct dm_spi_slave_platdata *slave_plat =
>> > +priv->cur_slave_plat;
>> > +
>> > +       debug("DSPI set_mode: mode 0x%x.\n", mode);
>> > +
>> > +       cs = slave_plat->cs;
>>
>> Similar point here.
> [] Here is different from the above function, I need know the cs here, or I remove some configuration to other function?
> There are two reasons about this.
> A. We need configure transfer mode and clock attributes in CTAR register before transfer.
> There are some other attributes except CPOL, CPHA, LSBFE in CTAR register.
> And those  attributes value may be different between every chipselect device.
> I get CPOL, CPHA, LSBFE from SPI uclass.
> I can get the rest attributes' value by two methods, defining macros or reading from device tree.
> I adopt the first method in this version.
> Later I plan to add a attribute in flash device dts node.
> My current process flow:
> 1. define macro for CTAR pre-configure value, CTAR pre-configure values may be different between every chipselect device and
> I only define one in this version, need improve.
> 2. Save CTAR pre-configure value in function 'child_pre_probe' identify with chipselect number.
> 3. And then write the CTAR pre-register value to register in function 'set_mode' according corresponding chipselect number.
>
> B. when configure the state of PCSx active, I also need know the chipselect number because DSPI have more than one chipselect signal
> and they need be configured independently.

The current driver model SPI uclass only supports a single speed/mode
for the whole bus. It does not understand different modes.

One option is to stop the speed/mode in the set_mode/set_speed()
function and then action it the in the activate() method. By that
point, you know the chip select and can do the right thing.

If you do that, then fsl_dspi_set_mode() will only store the value in
priv, it will not touch the hardware.

>>
>> > +
>> > +       bus_setup = dspi_read32(bus, DSPI_CTAR(0));
>> > +       bus_setup &= ~DSPI_CTAR_SET_MODE_MASK;
>> > +       bus_setup |= priv->ctar_val[cs];
>> > +       bus_setup &= ~(DSPI_CTAR_CPOL | DSPI_CTAR_CPHA |
>> > + DSPI_CTAR_LSBFE);
>> > +
>> > +       if (mode & SPI_CPOL)
>> > +               bus_setup |= DSPI_CTAR_CPOL;
>> > +       if (mode & SPI_CPHA)
>> > +               bus_setup |= DSPI_CTAR_CPHA;
>> > +       if (mode & SPI_LSB_FIRST)
>> > +               bus_setup |= DSPI_CTAR_LSBFE;
>> > +
>> > +       dspi_write32(bus, DSPI_CTAR(0), bus_setup);
>> > +
>> > +       if (mode & SPI_CS_HIGH) {
>> > +               dspi_halt(bus, 1);
>> > +               mcr_val = dspi_read32(bus, DSPI_MCR);
>> > +               /* CSx inactive state is low */
>> > +               mcr_val &= ~DSPI_MCR_PCSIS(cs);
>> > +               dspi_write32(bus, DSPI_MCR, mcr_val);
>> > +               dspi_halt(bus, 0);
>> > +       }
>> > +
>> > +       priv->mode = mode;
>> > +       priv->charbit = ((dspi_read32(bus, DSPI_CTAR(0)) & 0x78000000)
>> > +                == 0x78000000) ? 16 : 8;
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int fsl_dspi_bind(struct udevice *bus) {
>> > +       debug("%s assigned req_seq %d.\n", bus->name, bus->req_seq);
>> > +       return 0;
>>
>> You can drop this unless you want it for debugging. If the latter, please
>> add a comment like /* This doesn't do anything except help with debugging
>> */
> [] fine, I will add the comment.
>>
>> > +}
>> > +
>> > +static int fsl_dspi_ofdata_to_platdata(struct udevice *bus) {
>> > +       uint addr;
>> > +       struct fsl_dspi_platdata *plat = bus->platdata;
>> > +       const void *blob = gd->fdt_blob;
>> > +       int node = bus->of_offset;
>> > +
>> > +       plat->flag = 0;
>>
>> It will already be zero. All data allocated by driver model is zeroed.
> [] fine, will remove.
>>
>> > +       if (fdtdec_get_bool(blob, node, "big-endian"))
>> > +               plat->flag |= DSPI_FLAG_REGMAP_ENDIAN_BIG;
>> > +
>> Extra black line here
>>
>> > +
>> > +       plat->num_chipselect =
>> > +               fdtdec_get_int(blob, node, "num-cs",
>> > FSL_DSPI_MAX_CHIPSELECT);
>> > +
>> > +       addr = fdtdec_get_addr(blob, node, "reg");
>>
>> You can use dev_get_addr(dev) if you like.
> [] any difference?
>>
>> > +       if (addr == FDT_ADDR_T_NONE) {
>> > +               printf("DSPI: Can't get base address or size\n");
>> > +               return -ENOMEM;
>> > +       }
>> > +       plat->regs = addr;
>> > +
>> > +       plat->baudrate =
>> > +               fdtdec_get_int(blob, node, "spi-max-frequency",
>> > + 10000000);
>>
>> The name 'baudrate' bothers me, but that if that is what you normally
>> call it on your hardware, then that is fine.
> [] fine, I will rename with "speed_hz", how about?

Sure.

>>
>> > +
>> > +       debug("DSPI: regs=0x%x, max-frequency=%d, endianess=%s,
>> > num-cs=%d\n",
>> > +             plat->regs, plat->baudrate,
>> > +             plat->flag & DSPI_FLAG_REGMAP_ENDIAN_BIG ? "be" : "le",
>> > +             plat->num_chipselect);
>> > +
>> > +       return 0;
>> > +}
>> > +static const struct dm_spi_ops fsl_dspi_ops = {
>> > +       .claim_bus      = fsl_dspi_claim_bus,
>> > +       .release_bus    = fsl_dspi_release_bus,
>> > +       .xfer           = fsl_dspi_xfer,
>> > +       .set_speed      = fsl_dspi_set_speed,
>> > +       .set_mode       = fsl_dspi_set_mode,
>> > +};
>> > +
>> > +static const struct udevice_id fsl_dspi_ids[] = {
>> > +       { .compatible = "fsl,vf610-dspi" },
>> > +       { }
>> > +};
>> > +
>> > +U_BOOT_DRIVER(fsl_dspi) = {
>> > +       .name   = "fsl_dspi",
>> > +       .id     = UCLASS_SPI,
>> > +       .of_match = fsl_dspi_ids,
>> > +       .ops    = &fsl_dspi_ops,
>> > +       .ofdata_to_platdata = fsl_dspi_ofdata_to_platdata,
>> > +       .platdata_auto_alloc_size = sizeof(struct fsl_dspi_platdata),
>> > +       .priv_auto_alloc_size = sizeof(struct fsl_dspi_priv),
>> > +       .probe  = fsl_dspi_probe,
>> > +       .child_pre_probe = fsl_dspi_child_pre_probe,
>> > +       .child_post_remove = fsl_dspi_child_post_remove,
>> > +       .bind = fsl_dspi_bind,
>> > +};
>> > diff --git a/include/fsl_dspi.h b/include/fsl_dspi.h new file mode
>> > 100644 index 0000000..8cf02d3
>> > --- /dev/null
>> > +++ b/include/fsl_dspi.h
>> > @@ -0,0 +1,156 @@
>> > +/*
>> > + * Freescale DSPI Module Defines
>> > + *
>> > + * Copyright (C) 2004-2007, 2015 Freescale Semiconductor, Inc.
>> > + * TsiChung Liew (Tsi-Chung.Liew at freescale.com)
>> > + * Chao Fu (B44548 at freesacle.com)
>> > + * Haikun Wang (B53464 at freescale.com)
>> > + *
>> > + * SPDX-License-Identifier:    GPL-2.0+
>> > + */
>> > +
>> > +#ifndef _FSL_DSPI_H_
>> > +#define _FSL_DSPI_H_
>> > +
>> > +/* DMA Serial Peripheral Interface (DSPI) */ struct dspi {
>> > +       u32 mcr;        /* 0x00 */
>> > +       u32 resv0;      /* 0x04 */
>> > +       u32 tcr;        /* 0x08 */
>> > +       u32 ctar[8];    /* 0x0C - 0x28 */
>> > +       u32 sr;         /* 0x2C */
>> > +       u32 irsr;       /* 0x30 */
>> > +       u32 tfr;        /* 0x34 - PUSHR */
>> > +       u32 rfr;        /* 0x38 - POPR */
>> > +#ifdef CONFIG_MCF547x_8x
>> > +       u32 tfdr[4];    /* 0x3C */
>> > +       u8 resv2[0x30]; /* 0x40 */
>> > +       u32 rfdr[4];    /* 0x7C */
>> > +#else
>> > +       u32 tfdr[16];   /* 0x3C */
>> > +       u32 rfdr[16];   /* 0x7C */
>> > +#endif
>>
>> Since you have this structure, can you not use it in your code for
>> register access?
> [] I copy it from the old driver. I will remove it if we keep the current registers access method.
>>

That seems to go against U-Boot style to me.

Regards,
Simon


More information about the U-Boot mailing list