+Tom for the coding style question
Hi,
On 4 March 2015 at 04:22, Haikun.Wang@freescale.com Haikun.Wang@freescale.com wrote:
-----Original Message----- From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass Sent: Tuesday, March 03, 2015 2:56 AM To: Wang Haikun-B53464 Cc: u-boot@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@freescale.com Haikun.Wang@freescale.com wrote:
Move the Freescale DSPI driver over to driver model.
Signed-off-by: Haikun Wang B53464@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@denx.de.
- Copyright (C) 2004-2009, 2015 Freescale Semiconductor, Inc.
- TsiChung Liew (Tsi-Chung.Liew@freescale.com)
- Chao Fu (B44548@freescale.com)
- Haikun Wang (B53464@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;elsemcr_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, mode0x%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);elsereturn -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++);elsedspi_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);elsedspi_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:
- 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@freescale.com)
- Chao Fu (B44548@freesacle.com)
- Haikun Wang (B53464@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