Re: [U-Boot] [PATCH v5] mmc: add generic mmc spi driver
On 04/28/2010 11:21 PM, Andy Fleming wrote:
+static int do_mmc_spi(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
+{
+ int dev_num = -1;
+ uint bus;
+ uint cs;
+ uint speed;
+ uint mode;
+ char *endp;
+ struct mmc *mmc;
+ struct mmc_spi_priv *priv;
+
+ do {
+ mmc = find_mmc_device(++dev_num);
+ } while (mmc strcmp(mmc-name, MMC_SPI));
+ if (!mmc) {
+ printf(Create MMC Device\n);
+ mmc = mmc_spi_init(CONFIG_MMC_SPI_BUS,
+ CONFIG_MMC_SPI_CS,
+ CONFIG_MMC_SPI_SPEED,
+ CONFIG_MMC_SPI_MODE);
+ if (!mmc) {
+ printf(Failed to create MMC Device\n);
+ return 1;
+ }
+ dev_num = mmc-block_dev.dev;
+ }
I'm not sure I understand the logic behind this code. The arguments
to the command should be used to either find the already-existing bus,
or to create a new one. Unless I'm misunderstanding, this searches
for the first MMC_SPI bus, and if it finds it, uses that, otherwise it
creates a new one with the values specified in the config file. Then
it parses the command and overwrites the old parameters for the bus
with new ones? Why? My instinct would be to create a separate
instance of an MMC_SPI bus for each bus and chip select. My SPI is
rusty, so maybe chip-select should be configurable on a use-by-use
basis.
Certainly the current code will only use at most one MMC_SPI bus even
if more are created, which seems wrong.
Hi Mike,
Could you please give me some suggestion on the mmc_spi subcommand?
1. In v5 patch, I assumed a single changeable mmc_spi device is enough.
2. Andy suggested to create a new mmc_spi device for each bus and cs.
Either way is fine to me. Which one do you prefer?
Best regards,
Thomas
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v5] mmc: add generic mmc spi driver
On Thursday 29 April 2010 10:51:03 Thomas Chou wrote: 2. Andy suggested to create a new mmc_spi device for each bus and cs. this is what i was suggesting from the start ... sorry if i wasnt clear -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v5] mmc: add generic mmc spi driver
This patch supports mmc/sd card with spi interface. It is based on
the generic mmc framework. It works with SDHC and supports write.
The crc7 lib func is merged from linux and used to compute mmc
command checksum.
There is a subcomamnd mmc_spi to setup spi bus and cs at run time.
Signed-off-by: Thomas Chou tho...@wytron.com.tw
---
v5: remove dev_num limit to search.
v4: change mmc_spi subcommand to search and create new mmc dev.
v3: add mmc_spi_init() proto to mmc_spi.h.
v2: add crc7, use cmd58 to read ocr, add subcommand mmc_spi.
common/Makefile |1 +
common/cmd_mmc_spi.c | 114 ++
drivers/mmc/Makefile |1 +
drivers/mmc/mmc_spi.c | 318 +
include/linux/crc7.h | 14 ++
include/mmc_spi.h | 27
lib/Makefile |1 +
lib/crc7.c| 62 ++
8 files changed, 538 insertions(+), 0 deletions(-)
create mode 100644 common/cmd_mmc_spi.c
create mode 100644 drivers/mmc/mmc_spi.c
create mode 100644 include/linux/crc7.h
create mode 100644 include/mmc_spi.h
create mode 100644 lib/crc7.c
diff --git a/common/Makefile b/common/Makefile
index dbf7a05..ee23e2f 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -118,6 +118,7 @@ COBJS-$(CONFIG_CMD_MII) += miiphyutil.o
COBJS-$(CONFIG_CMD_MII) += cmd_mii.o
COBJS-$(CONFIG_CMD_MISC) += cmd_misc.o
COBJS-$(CONFIG_CMD_MMC) += cmd_mmc.o
+COBJS-$(CONFIG_CMD_MMC_SPI) += cmd_mmc_spi.o
COBJS-$(CONFIG_MP) += cmd_mp.o
COBJS-$(CONFIG_CMD_MTDPARTS) += cmd_mtdparts.o
COBJS-$(CONFIG_CMD_NAND) += cmd_nand.o
diff --git a/common/cmd_mmc_spi.c b/common/cmd_mmc_spi.c
new file mode 100644
index 000..0cbb449
--- /dev/null
+++ b/common/cmd_mmc_spi.c
@@ -0,0 +1,114 @@
+/*
+ * Command for mmc_spi setup.
+ *
+ * Copyright (C) 2010 Thomas Chou tho...@wytron.com.tw
+ * Licensed under the GPL-2 or later.
+ */
+
+#include common.h
+#include malloc.h
+#include mmc.h
+#include spi.h
+#include mmc_spi.h
+
+#ifndef CONFIG_MMC_SPI_BUS
+# define CONFIG_MMC_SPI_BUS 0
+#endif
+#ifndef CONFIG_MMC_SPI_CS
+# define CONFIG_MMC_SPI_CS 1
+#endif
+#ifndef CONFIG_MMC_SPI_SPEED
+# define CONFIG_MMC_SPI_SPEED 3000
+#endif
+#ifndef CONFIG_MMC_SPI_MODE
+# define CONFIG_MMC_SPI_MODE SPI_MODE_3
+#endif
+
+static int do_mmc_spi(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
+{
+ int dev_num = -1;
+ uint bus;
+ uint cs;
+ uint speed;
+ uint mode;
+ char *endp;
+ struct mmc *mmc;
+ struct mmc_spi_priv *priv;
+
+ do {
+ mmc = find_mmc_device(++dev_num);
+ } while (mmc strcmp(mmc-name, MMC_SPI));
+ if (!mmc) {
+ printf(Create MMC Device\n);
+ mmc = mmc_spi_init(CONFIG_MMC_SPI_BUS,
+ CONFIG_MMC_SPI_CS,
+ CONFIG_MMC_SPI_SPEED,
+ CONFIG_MMC_SPI_MODE);
+ if (!mmc) {
+ printf(Failed to create MMC Device\n);
+ return 1;
+ }
+ dev_num = mmc-block_dev.dev;
+ }
+
+ priv = mmc-priv;
+ bus = priv-bus;
+ cs = priv-cs;
+ speed = priv-speed;
+ mode = priv-mode;
+
+ if (argc 2)
+ goto info;
+ cs = simple_strtoul(argv[1], endp, 0);
+ if (*argv[1] == 0 || (*endp != 0 *endp != ':'))
+ goto usage;
+ if (*endp == ':') {
+ if (endp[1] == 0)
+ goto usage;
+ bus = cs;
+ cs = simple_strtoul(endp + 1, endp, 0);
+ if (*endp != 0)
+ goto usage;
+ }
+ if (argc = 3) {
+ speed = simple_strtoul(argv[2], endp, 0);
+ if (*argv[2] == 0 || *endp != 0)
+ goto usage;
+ }
+ if (argc = 4) {
+ mode = simple_strtoul(argv[3], endp, 16);
+ if (*argv[3] == 0 || *endp != 0)
+ goto usage;
+ }
+ if (!spi_cs_is_valid(bus, cs)) {
+ printf(Invalid SPI bus %u cs %u\n, bus, cs);
+ return 1;
+ }
+
+ if (bus != priv-bus || cs != priv-cs ||
+ speed != priv-speed || mode != priv-mode) {
+ priv-bus = bus;
+ priv-cs = cs;
+ priv-speed = speed;
+ priv-mode = mode;
+ if (priv-slave) {
+ spi_free_slave(priv-slave);
+ priv-slave = NULL;
+ }
+ }
+info:
+ printf(%s: %d at %u:%u %u %u\n, mmc-name, dev_num,
+ bus, cs, speed, mode);
+ return 0;
+
+usage:
+ cmd_usage(cmdtp);
+ return 1;
+}
+
+U_BOOT_CMD(
+ mmc_spi,4, 0, do_mmc_spi,
+ mmc_spi setup,
+ [bus:][cs] [hz] [mode] - setup mmc_spi device on given\n
+ SPI bus and chip select\n
+);
diff --git
Re: [U-Boot] [PATCH v5] mmc: add generic mmc spi driver
On Wed, Apr 28, 2010 at 1:00 AM, Thomas Chou tho...@wytron.com.tw wrote:
This patch supports mmc/sd card with spi interface. It is based on
the generic mmc framework. It works with SDHC and supports write.
The crc7 lib func is merged from linux and used to compute mmc
command checksum.
This should probably be a separate patch.
There is a subcomamnd mmc_spi to setup spi bus and cs at run time.
Signed-off-by: Thomas Chou tho...@wytron.com.tw
+
+static int do_mmc_spi(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
+{
+ int dev_num = -1;
+ uint bus;
+ uint cs;
+ uint speed;
+ uint mode;
+ char *endp;
+ struct mmc *mmc;
+ struct mmc_spi_priv *priv;
+
+ do {
+ mmc = find_mmc_device(++dev_num);
+ } while (mmc strcmp(mmc-name, MMC_SPI));
+ if (!mmc) {
+ printf(Create MMC Device\n);
+ mmc = mmc_spi_init(CONFIG_MMC_SPI_BUS,
+ CONFIG_MMC_SPI_CS,
+ CONFIG_MMC_SPI_SPEED,
+ CONFIG_MMC_SPI_MODE);
+ if (!mmc) {
+ printf(Failed to create MMC Device\n);
+ return 1;
+ }
+ dev_num = mmc-block_dev.dev;
+ }
I'm not sure I understand the logic behind this code. The arguments
to the command should be used to either find the already-existing bus,
or to create a new one. Unless I'm misunderstanding, this searches
for the first MMC_SPI bus, and if it finds it, uses that, otherwise it
creates a new one with the values specified in the config file. Then
it parses the command and overwrites the old parameters for the bus
with new ones? Why? My instinct would be to create a separate
instance of an MMC_SPI bus for each bus and chip select. My SPI is
rusty, so maybe chip-select should be configurable on a use-by-use
basis.
Certainly the current code will only use at most one MMC_SPI bus even
if more are created, which seems wrong.
+
+ priv = mmc-priv;
+ bus = priv-bus;
+ cs = priv-cs;
+ speed = priv-speed;
+ mode = priv-mode;
+
+ if (argc 2)
+ goto info;
+ cs = simple_strtoul(argv[1], endp, 0);
+ if (*argv[1] == 0 || (*endp != 0 *endp != ':'))
+ goto usage;
+ if (*endp == ':') {
+ if (endp[1] == 0)
+ goto usage;
+ bus = cs;
+ cs = simple_strtoul(endp + 1, endp, 0);
+ if (*endp != 0)
+ goto usage;
+ }
+ if (argc = 3) {
+ speed = simple_strtoul(argv[2], endp, 0);
+ if (*argv[2] == 0 || *endp != 0)
+ goto usage;
+ }
+ if (argc = 4) {
+ mode = simple_strtoul(argv[3], endp, 16);
+ if (*argv[3] == 0 || *endp != 0)
+ goto usage;
+ }
+ if (!spi_cs_is_valid(bus, cs)) {
+ printf(Invalid SPI bus %u cs %u\n, bus, cs);
+ return 1;
+ }
+
+ if (bus != priv-bus || cs != priv-cs ||
+ speed != priv-speed || mode != priv-mode) {
+ priv-bus = bus;
+ priv-cs = cs;
+ priv-speed = speed;
+ priv-mode = mode;
+ if (priv-slave) {
+ spi_free_slave(priv-slave);
+ priv-slave = NULL;
+ }
+ }
diff --git a/drivers/mmc/mmc_spi.c b/drivers/mmc/mmc_spi.c
new file mode 100644
index 000..dedcef7
--- /dev/null
+++ b/drivers/mmc/mmc_spi.c
@@ -0,0 +1,318 @@
+/*
+ * generic mmc spi driver
+ *
+static uint mmc_spi_sendcmd(struct mmc *mmc, u8 cmdidx, u32 cmdarg)
+{
+ struct mmc_spi_priv *priv = mmc-priv;
+ u8 cmdo[7];
+ u8 r1;
+ int i;
+ cmdo[0] = 0xff;
+ cmdo[1] = 0x40 + cmdidx;
Use a #define'd constant for 0x40. Maybe do this:
/* MMC SPI commands start with a start bit 0 and a transmit bit 1 */
#define MMC_SPI_CMD(x) (0x40 | (x 0x3f))
cmdo[1] = MMC_SPI_CMD(cmdidx);
+ cmdo[2] = cmdarg 24;
+ cmdo[3] = cmdarg 16;
+ cmdo[4] = cmdarg 8;
+ cmdo[5] = cmdarg;
+ cmdo[6] = (crc7(0, cmdo[1], 5) 1) | 0x01;
+ spi_xfer(priv-slave, 7 * 8, cmdo, NULL, SPI_XFER_BEGIN);
+ for (i = 0; i CTOUT; i++) {
+ spi_xfer(priv-slave, 1 * 8, NULL, r1, 0);
+ if ((r1 0x80) == 0)
define a constant
+ break;
+ }
+ debug(%s:cmd%d resp%d %x\n, __func__, cmdidx, i, r1);
+ return r1;
+}
+
+static uint mmc_spi_readdata(struct mmc *mmc, char *buf,
+ u32 bcnt, u32 bsize)
+{
+ struct mmc_spi_priv *priv = mmc-priv;
+ u8 r1;
+ u8 crc[2];
+ int i;
+ while (bcnt--) {
+
Re: [U-Boot] [PATCH v5] mmc: add generic mmc spi driver
Hi Andy,
Thanks for you review.
On 04/28/2010 11:21 PM, Andy Fleming wrote:
The crc7 lib func is merged from linux and used to compute mmc
command checksum.
This should probably be a separate patch.
OK.
+
+ do {
+ mmc = find_mmc_device(++dev_num);
+ } while (mmc strcmp(mmc-name, MMC_SPI));
+ if (!mmc) {
+ printf(Create MMC Device\n);
+ mmc = mmc_spi_init(CONFIG_MMC_SPI_BUS,
+ CONFIG_MMC_SPI_CS,
+ CONFIG_MMC_SPI_SPEED,
+ CONFIG_MMC_SPI_MODE);
+ if (!mmc) {
+ printf(Failed to create MMC Device\n);
+ return 1;
+ }
+ dev_num = mmc-block_dev.dev;
+ }
I'm not sure I understand the logic behind this code. The arguments
to the command should be used to either find the already-existing bus,
or to create a new one. Unless I'm misunderstanding, this searches
for the first MMC_SPI bus, and if it finds it, uses that, otherwise it
creates a new one with the values specified in the config file. Then
it parses the command and overwrites the old parameters for the bus
with new ones? Why? My instinct would be to create a separate
instance of an MMC_SPI bus for each bus and chip select. My SPI is
rusty, so maybe chip-select should be configurable on a use-by-use
basis.
Certainly the current code will only use at most one MMC_SPI bus even
if more are created, which seems wrong.
Though more than one MMC_SPI device can be created for specific bus and
cs by calling mmc_spi_init() within board_mmc_init() or cpu_mmc_init().
This command is used to change the spi bus and chip select on a
use-by-use basis at run time, so one changeable mmc_spi device (the
first one if we found) should be enough.
+ cmdo[1] = 0x40 + cmdidx;
Use a #define'd constant for 0x40. Maybe do this:
/* MMC SPI commands start with a start bit 0 and a transmit bit 1 */
#define MMC_SPI_CMD(x) (0x40 | (x 0x3f))
cmdo[1] = MMC_SPI_CMD(cmdidx);
OK. Will define macros for all the cmd, token and response.
Why not check the CRC to make sure your data is correct?
OK. Will check CRC on data packet.
+
+static inline uint rswab(u8 *d)
Did you mean swap, here?
It should be swap.
Hmmm I'm not entirely sure this is the way to go. If I'm reading
this correctly, this function is converting between the standard SD
protocol, and the SPI version of the protocol. It might be better to
make mmc.c aware of which protocol it is using, so it a) doesn't issue
commands that the SPI card doesn't understand, and b) can properly
interpret responses.
I have avoided to touch the core mmc.c, so that I won't break other SD
hosts by accident. Only minor translation of initialization commands and
status mapping is enough to support SPI protocol.
+ }
+ if (data) {
+ debug(%s:data %x %x %x\n, __func__,
+ data-flags, data-blocks, data-blocksize);
+ if (data-flags == MMC_DATA_READ) {
+ r1 = mmc_spi_readdata(mmc, data-dest,
+ data-blocks, data-blocksize);
+ } else if (data-flags == MMC_DATA_WRITE) {
+ if (cmdidx == MMC_CMD_WRITE_MULTIPLE_BLOCK)
+ r1 = mmc_spi_writeblock(mmc, data-src,
+ data-blocks, data-blocksize);
+ else
+ r1 = mmc_spi_writedata(mmc, data-src,
+ data-blocks, data-blocksize);
+ }
Why not check r1 to make sure your writes actually succeeded?
OK. Will check data response.
+
+ mmc-voltages = MMC_VDD_32_33 | MMC_VDD_33_34;
Is this part of the SPI spec? If so, this is fine, otherwise, we need
to get the voltage information from the SPI bus, somehow.
There is no voltage capability from SPI bus. This assumes 3.3V
interface. Should I include other voltage?
+ mmc-f_max = speed;
+ mmc-f_min = mmc-f_max 9;
What's the logic behind f_min being f_max/512?
It yields f_min lower than 400KHz to meet the requirement for MMC.
Best regards,
Thomas
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
