[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