* [PATCH] MMC/SD host driver for Ricoh Bay1Controllers
@ 2008-04-27 14:17 Sascha Sommer
2008-05-01 23:39 ` Andrew Morton
0 siblings, 1 reply; 8+ messages in thread
From: Sascha Sommer @ 2008-04-27 14:17 UTC (permalink / raw)
To: LKML; +Cc: sdricohcs-devel
[-- Attachment #1: Type: text/plain, Size: 1742 bytes --]
Hi,
attached you can find a driver for MMC/SD card controllers that have been
integrated into some Notebooks with Ricoh Co Ltd RL5c476 II Cardbus bridges.
This includes the Samsung P35 notebook, the Dell X300 and some notebooks from
the Asus M6N series.
Whenever a MMC or SD card is insterted in the cardslot of one of these
notebooks the cardbus bridge will announce a
virtual Ricoh Bay1Controller PCMCIA card.
Since I submitted the first version of this driver to lkml last year many
changes have been made. The driver can now read and write to MMC and SD cards
and the reading speed for SD cards should be close to the windows driver now.
Many thanks to all the people that tested the driver and helped me to fix some
of the remaining bugs. I think it is now time to find out what needs to be
fixed before the driver can go into kernel.
While the driver is working fine for me there are still some not so nice parts
in it.
- the registers are on the cardbus bridge that is already claimed by yenta
socket therefore the driver has to use pci_get_device
- there is a lot of register polling because the reader does not support irqs
- some mmc commands require extra hacks. One of those commands is the
SD_APP_SEND_SCR command (a user reported that it is required for his SDHC
card). Like the read and write commands this command
requires a block read to read the scr but this does not seem to work.
I do not have any documentation for this device and the windows driver does
not support such commands so I cannot find the
needed info in its logfiles.
...
I'm thankfull for any kind of feedback.
Signed-off-by: Sascha Sommer <saschasommer@freenet.de>
Regards
Sascha
P.S. Please CC me as I'm not subscribed.
[-- Attachment #2: sdricoh_cs.patch --]
[-- Type: text/x-diff, Size: 18525 bytes --]
--- /dev/null 2007-09-21 23:50:58.000000000 +0200
+++ drivers/mmc/host/sdricoh_cs.c 2008-04-27 14:38:42.000000000 +0200
@@ -0,0 +1,647 @@
+/*
+ * sdricoh_cs.c - driver for Ricoh Secure Digital Card Readers that can be
+ * found on some Ricoh RL5c476 II cardbus bridge
+ *
+ * Copyright (C) 2006 - 2008 Sascha Sommer <saschasommer@freenet.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/highmem.h>
+#include <linux/pci.h>
+#include <linux/ioport.h>
+#include <linux/version.h>
+
+#include <pcmcia/cs_types.h>
+#include <pcmcia/cs.h>
+#include <pcmcia/cistpl.h>
+#include <pcmcia/ds.h>
+#include <asm/io.h>
+
+#include <linux/mmc/host.h>
+#include <linux/mmc/mmc.h>
+#include <linux/mmc/sd.h>
+#include <linux/mmc/sdio.h>
+
+#define DRIVER_NAME "sdricoh_cs"
+#define DRIVER_VERSION "0.1.4"
+
+static unsigned int debug = 0;
+static unsigned int switchlocked = 0;
+
+/* #define DEBUG */
+
+/* debug macros */
+
+#ifdef DEBUG
+#define REGDBG(fmt, arg...) do {\
+ if (debug > 1) \
+ printk(KERN_INFO "sdricoh_cs: "fmt, \
+ ##arg); } while (0)
+#else
+#define REGDBG(fmt, arg...)
+#endif
+
+#define DBG(fmt, arg...) do {\
+ if (debug > 0) \
+ printk(KERN_INFO DRIVER_NAME ": "fmt, \
+ ##arg); } while (0)
+
+#define ERR(fmt, arg...) do {\
+ printk(KERN_INFO DRIVER_NAME ": "fmt, \
+ ##arg); } while (0)
+
+#define INFO(fmt, arg...) do {\
+ printk(KERN_INFO DRIVER_NAME ": "fmt, \
+ ##arg); } while (0)
+/* i/o region */
+#define SDRICOH_PCI_REGION 0
+#define SDRICOH_PCI_REGION_SIZE 0x1000
+
+/* registers */
+#define R104_VERSION 0x104
+#define R200_CMD 0x200
+#define R204_CMD_ARG 0x204
+#define R208_DATAIO 0x208
+#define R20C_RESP 0x20c
+#define R21C_STATUS 0x21c
+#define R2E0_INIT 0x2e0
+#define R2E4_STATUS_RESP 0x2e4
+#define R2F0_RESET 0x2f0
+#define R224_MODE 0x224
+#define R228_POWER 0x228
+#define R230_DATA 0x230
+
+/* flags for the R21C_STATUS register */
+#define STATUS_CMD_FINISHED 0x00000001
+#define STATUS_TRANSFER_FINISHED 0x00000004
+#define STATUS_CARD_INSERTED 0x00000020
+#define STATUS_CARD_LOCKED 0x00000080
+#define STATUS_CMD_TIMEOUT 0x00400000
+#define STATUS_READY_TO_READ 0x01000000
+#define STATUS_READY_TO_WRITE 0x02000000
+#define STATUS_BUSY 0x40000000
+
+/* timeouts */
+#define INIT_TIMEOUT 100
+#define CMD_TIMEOUT 100000
+#define TRANSFER_TIMEOUT 100000
+#define BUSY_TIMEOUT 32767
+
+#define MODE_MMC 0
+#define MODE_SD 1
+#define MODE_SDHC 2
+
+/* list of supported pcmcia devices */
+static struct pcmcia_device_id pcmcia_ids[] = {
+ /* vendor and device strings followed by their crc32 hashes */
+ PCMCIA_DEVICE_PROD_ID12("RICOH", "Bay1Controller", 0xd9f522ed,
+ 0xc3901202),
+ PCMCIA_DEVICE_NULL,
+};
+
+MODULE_DEVICE_TABLE(pcmcia, pcmcia_ids);
+
+/* mmc privdata */
+struct sdricoh_host {
+ struct mmc_host *mmc; /* MMC structure */
+ unsigned char __iomem *iobase;
+ struct pci_dev *pci_dev;
+ int mode;
+};
+
+/***************** register i/o helper functions *****************************/
+
+static inline unsigned int sdricoh_readl(struct sdricoh_host *host,
+ unsigned int reg)
+{
+ unsigned int value = readl(host->iobase + reg);
+ REGDBG("rl %x 0x%x\n", reg, value);
+ return value;
+}
+
+static inline void sdricoh_writel(struct sdricoh_host *host, unsigned int reg,
+ unsigned int value)
+{
+ writel(value, host->iobase + reg);
+ REGDBG("wl %x 0x%x\n", reg, value);
+
+}
+
+static inline unsigned int sdricoh_readw(struct sdricoh_host *host,
+ unsigned int reg)
+{
+ unsigned int value = readw(host->iobase + reg);
+ REGDBG("rb %x 0x%x\n", reg, value);
+ return value;
+}
+
+static inline unsigned int sdricoh_readb(struct sdricoh_host *host,
+ unsigned int reg)
+{
+ unsigned int value = readb(host->iobase + reg);
+ REGDBG("rb %x 0x%x\n", reg, value);
+ return value;
+}
+
+
+static int sdricoh_query_status(struct sdricoh_host *host,unsigned int wanted,
+ unsigned int timeout){
+ unsigned int loop;
+ unsigned int status = 0;
+ for (loop = 0; loop < timeout; loop++) {
+ status = sdricoh_readl(host, R21C_STATUS);
+ sdricoh_writel(host, R2E4_STATUS_RESP, status);
+ if (status & wanted)
+ break;
+ }
+
+ if (loop == timeout) {
+ ERR("query_status: timeout waiting for data\n");
+ return -ETIMEDOUT;
+ }
+
+ /* do not do this check in the loop as some commands fail otherwise */
+ if(status & 0x7F0000){
+ ERR("waiting for status bit %x failed\n",wanted);
+ return -EINVAL;
+ }
+ return 0;
+
+}
+
+
+
+
+
+static int sdricoh_mmc_cmd(struct sdricoh_host *host, unsigned char opcode,
+ unsigned int arg)
+{
+ unsigned int status;
+ int result = 0;
+ unsigned int loop = 0;
+ /* reset status reg? */
+ sdricoh_writel(host, R21C_STATUS, 0x18);
+ /* fill parameters */
+ sdricoh_writel(host, R204_CMD_ARG, arg);
+ sdricoh_writel(host, R200_CMD, (0x10000 << 8) | opcode);
+ /* wait for command completion */
+ if (opcode) {
+ for (loop = 0; loop < CMD_TIMEOUT; loop++) {
+ status = sdricoh_readl(host, R21C_STATUS);
+ sdricoh_writel(host, R2E4_STATUS_RESP, status);
+ if (status & STATUS_CMD_FINISHED)
+ break;
+ }
+ if (loop == CMD_TIMEOUT || status & STATUS_CMD_TIMEOUT)
+ result = -ETIMEDOUT;
+
+ }
+ DBG("mmc_cmd opcode=%i arg=0x%x => %i (queries=%i)\n",
+ opcode, arg, result, loop);
+
+ if(result == 0){
+ /* EXT_CSD are filtered so this should be save */
+ if(opcode == SD_SEND_IF_COND){
+ if(host->mode != MODE_SDHC){
+ INFO("switching to SDHC mode\n");
+ host->mode = MODE_SDHC;
+ }
+ }
+
+ /* switch to SD mode if APP_CMDs are supported */
+ if(opcode == MMC_APP_CMD){
+ if(host->mode == MODE_MMC){
+ INFO("switching to SD mode\n");
+ host->mode = MODE_SD;
+ }
+ }
+ }
+
+ return result;
+
+}
+
+static int sdricoh_reset(struct sdricoh_host *host)
+{
+ DBG("reset\n");
+ sdricoh_writel(host, R2F0_RESET, 0x10001);
+ sdricoh_writel(host, R2E0_INIT, 0x10000);
+ if (sdricoh_readl(host, R2E0_INIT) != 0x10000)
+ return -EIO;
+ sdricoh_writel(host, R2E0_INIT, 0x10007);
+
+ sdricoh_writel(host, R224_MODE, 0x2000000);
+ sdricoh_writel(host, R228_POWER, 0xe0);
+
+
+ /* status register ? */
+ sdricoh_writel(host, R21C_STATUS, 0x18);
+
+ return 0;
+}
+
+static int sdricoh_blockio(struct sdricoh_host *host, int read,
+ unsigned int* buf)
+{
+ int i;
+ /* wait until the data is available */
+ if(read){
+ if(sdricoh_query_status(host,STATUS_READY_TO_READ,
+ TRANSFER_TIMEOUT))
+ return 0;
+ sdricoh_writel(host, R21C_STATUS, 0x18);
+ /* read data */
+ for (i = 0; i < 512 / 4; i++) {
+ buf[i] = sdricoh_readl(host, R230_DATA);
+ }
+ }else{
+ if(sdricoh_query_status(host,STATUS_READY_TO_WRITE,
+ TRANSFER_TIMEOUT))
+ return 0;
+ sdricoh_writel(host, R21C_STATUS, 0x18);
+ /* write data */
+ for (i = 0; i < 512 / 4; i++) {
+ sdricoh_writel(host, R230_DATA, buf[i]);
+ }
+ }
+
+ return 512;
+}
+
+static int sdricoh_busy(struct sdricoh_host* host){
+ unsigned int status;
+ int i;
+ /* wait until the tranfer is finished */
+ for (i = 0; i < BUSY_TIMEOUT; i++) {
+ status = sdricoh_readl(host, R21C_STATUS);
+ sdricoh_writel(host, R2E4_STATUS_RESP, status);
+ if (!(status & STATUS_BUSY))
+ break;
+ }
+ if(status & 0x7F0000)
+ return -EINVAL;
+ if(i == BUSY_TIMEOUT)
+ return -ETIMEDOUT;
+ return 0;
+}
+
+
+static void sdricoh_request(struct mmc_host *mmc, struct mmc_request *mrq)
+{
+ struct sdricoh_host *host = mmc_priv(mmc);
+ struct mmc_command *cmd = mrq->cmd;
+ struct mmc_data *data = cmd->data;
+ int i;
+
+ DBG("=============================\n");
+ DBG("sdricoh_request opcode=%i\n", cmd->opcode);
+
+ sdricoh_writel(host, R21C_STATUS, 0x18);
+
+ /* we cannot handle all commands that require a block transfer
+ therefore do some ugly special handling here
+ */
+ if(cmd->data){
+ switch(cmd->opcode){
+ /* working commands */
+ case MMC_READ_SINGLE_BLOCK:
+ case MMC_READ_MULTIPLE_BLOCK:
+ case MMC_WRITE_BLOCK:
+ break;
+ case SD_APP_SEND_SCR: /* required for SDHC */
+ cmd->error = sdricoh_mmc_cmd(host,
+ cmd->opcode,cmd->arg);
+ mmc_request_done(mmc, mrq);
+ return;
+ default:
+ DBG("unsupported command %i\n",cmd->opcode);
+ cmd->error = -EINVAL;
+ mmc_request_done(mmc, mrq);
+ return;
+ }
+ }
+
+
+ /* read/write commands seem to require this */
+ if (data) {
+ if((cmd->error = sdricoh_busy(host)))
+ ERR("sdricoh_request: unable to prepare transfer %x\n",
+ cmd->error);
+ sdricoh_writel(host, R208_DATAIO, 0);
+ }
+
+
+ cmd->error = sdricoh_mmc_cmd(host, cmd->opcode, cmd->arg);
+
+ /* read response buffer */
+ if (cmd->flags & MMC_RSP_PRESENT) {
+ if (cmd->flags & MMC_RSP_136) {
+ /* CRC is stripped so we need to do some shifting. */
+ for (i = 0; i < 4; i++) {
+ cmd->resp[i] =
+ sdricoh_readl(host,
+ R20C_RESP + (3 - i) * 4) << 8;
+ if (i != 3)
+ cmd->resp[i] |=
+ sdricoh_readb(host, R20C_RESP +
+ (3 - i) *4 - 1);
+ }
+ DBG("resp[0]=0x%x\n", cmd->resp[0]);
+ DBG("resp[1]=0x%x\n", cmd->resp[1]);
+ DBG("resp[2]=0x%x\n", cmd->resp[2]);
+ DBG("resp[2]=0x%x\n", cmd->resp[3]);
+ } else {
+ cmd->resp[0] = sdricoh_readl(host, R20C_RESP);
+ DBG("resp[0]=0x%x\n", cmd->resp[0]);
+ }
+ }
+
+ /* yet another workaround */
+ /* without the extra command SD cards do not work at all */
+ if (cmd->opcode == MMC_SELECT_CARD) {
+ if(host->mode != MODE_MMC){
+ sdricoh_mmc_cmd(host, MMC_APP_CMD, cmd->arg);
+ sdricoh_mmc_cmd(host, 0x46, 0x02);
+ }else{
+ sdricoh_writel(host, R228_POWER, 0xc0e0);
+ sdricoh_writel(host, R224_MODE, 0x2000301);
+ }
+ }
+
+ /* transfer data */
+ if (data && cmd->error == 0) {
+ DBG("transfer: blksz %i blocks %i sg_len %i sg length %i\n",
+ data->blksz, data->blocks, data->sg_len, data->sg->length);
+
+ /* enter data reading mode */
+ sdricoh_writel(host, R21C_STATUS, 0x837f031e);
+ for (i = 0; i < data->blocks; i++) {
+ unsigned int *buf;
+ struct page* page;
+ size_t xfered;
+ page = sg_page(data->sg);
+
+ buf = kmap(page) + data->sg->offset + (512 * i);
+ xfered =
+ sdricoh_blockio(host,data->flags & MMC_DATA_READ,buf);
+ kunmap(page);
+ if(!xfered){
+ ERR("sdricoh_request: block transfer failed\n");
+ cmd->error = -EINVAL;
+ break;
+ }else
+ data->bytes_xfered += xfered;
+ }
+
+ sdricoh_writel(host, R208_DATAIO, 1);
+
+ if(sdricoh_query_status(host,STATUS_TRANSFER_FINISHED,
+ TRANSFER_TIMEOUT)){
+ ERR("sdricoh_request: transfer end error\n");
+ cmd->error = -EINVAL;
+ }
+
+ if(!cmd->error && (cmd->error = sdricoh_busy(host)))
+ ERR("sdricoh_request: transfer not finished %x\n",
+ cmd->error);
+
+ }
+
+ mmc_request_done(mmc, mrq);
+ DBG("=============================\n");
+}
+
+static void sdricoh_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+ struct sdricoh_host *host = mmc_priv(mmc);
+ DBG("set_ios\n");
+
+ if (ios->power_mode == MMC_POWER_ON) {
+ sdricoh_writel(host, R228_POWER, 0xc0e0);
+
+ if(host->mode != MODE_MMC){
+ sdricoh_writel(host, R224_MODE, 0x2000300);
+ sdricoh_writel(host, R228_POWER, 0x40e0);
+ }else{
+ sdricoh_writel(host, R224_MODE, 0x2000340);
+ }
+ }else if(ios->power_mode == MMC_POWER_UP) {
+ sdricoh_writel(host, R224_MODE, 0x2000320);
+ sdricoh_writel(host, R228_POWER, 0xe0);
+ }
+}
+
+static int sdricoh_get_ro(struct mmc_host *mmc)
+{
+ struct sdricoh_host *host = mmc_priv(mmc);
+ unsigned int status;
+
+ status = sdricoh_readl(host, R21C_STATUS);
+ sdricoh_writel(host, R2E4_STATUS_RESP, status);
+
+ /* some notebooks seem to have the locked flag switched */
+ if(switchlocked)
+ return !(status & STATUS_CARD_LOCKED);
+
+ return (status & STATUS_CARD_LOCKED);
+}
+
+static struct mmc_host_ops sdricoh_ops = {
+ .request = sdricoh_request,
+ .set_ios = sdricoh_set_ios,
+ .get_ro = sdricoh_get_ro,
+};
+
+/* initialize the control and register it to the mmc framework */
+static int sdricoh_init_mmc(struct pci_dev *pci_dev,
+ struct pcmcia_device *pcmcia_dev)
+{
+ int result = 0;
+ void __iomem *iobase = NULL;
+ struct mmc_host *mmc = NULL;
+ struct sdricoh_host *host = NULL;
+ /* map iomem */
+ if (pci_resource_len(pci_dev, SDRICOH_PCI_REGION) !=
+ SDRICOH_PCI_REGION_SIZE) {
+ DBG("unexpected pci resource len\n");
+ return -ENODEV;
+ }
+ iobase =
+ pci_iomap(pci_dev, SDRICOH_PCI_REGION, SDRICOH_PCI_REGION_SIZE);
+ if (!iobase) {
+ ERR("unable to map iobase\n");
+ return -ENODEV;
+ }
+ /* check version? */
+ if (readl(iobase + R104_VERSION) != 0x4000) {
+ DBG("no supported mmc controller found\n");
+ result = -ENODEV;
+ goto err;
+ }
+ /* allocate privdata */
+ mmc = pcmcia_dev->priv =
+ mmc_alloc_host(sizeof(struct sdricoh_host), &pcmcia_dev->dev);
+ if (!mmc) {
+ ERR("mmc_alloc_host failed\n");
+ result = -ENOMEM;
+ goto err;
+ }
+ host = mmc_priv(mmc);
+
+ host->iobase = iobase;
+
+ mmc->ops = &sdricoh_ops;
+
+ /* FIXME: frequency and voltage handling is done by the controller
+ */
+ mmc->f_min = 450000;
+ mmc->f_max = 24000000;
+ mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
+
+ mmc->max_seg_size = 1024 * 512;
+
+ /* reset the controler */
+ if (sdricoh_reset(host)) {
+ DBG("could not reset\n");
+ result = -EIO;
+ goto err;
+
+ }
+
+ result = mmc_add_host(mmc);
+
+ if (!result) {
+ DBG("mmc host registered\n");
+ return 0;
+ }
+
+ err:
+ if (iobase)
+ iounmap(iobase);
+ if (mmc)
+ mmc_free_host(mmc);
+
+ return result;
+}
+
+/* search for supported mmc controllers */
+static int sdricoh_pcmcia_probe(struct pcmcia_device *pcmcia_dev)
+{
+ struct pci_dev *pci_dev = NULL;
+
+ INFO("Searching MMC controller for pcmcia device %s %s ...\n",
+ pcmcia_dev->prod_id[0],pcmcia_dev->prod_id[1] );
+
+ /* search pci cardbus bridge that contains the mmc controler */
+ /* the io region is already claimed by yenta_socket... */
+ while ((pci_dev =
+ pci_get_device(PCI_VENDOR_ID_RICOH, PCI_DEVICE_ID_RICOH_RL5C476,
+ pci_dev))) {
+ /* try to init the device */
+ if (!sdricoh_init_mmc(pci_dev, pcmcia_dev)){
+ INFO("MMC controller found\n");
+ return 0;
+ }
+
+ }
+ ERR("No MMC controller was found.\n");
+ return -ENODEV;
+}
+
+static void sdricoh_pcmcia_detach(struct pcmcia_device *link)
+{
+ struct mmc_host *mmc = link->priv;
+
+ DBG("detach\n");
+
+ flush_scheduled_work();
+
+ /* remove mmc host */
+ if (mmc) {
+ struct sdricoh_host *host = mmc_priv(mmc);
+ mmc_remove_host(mmc);
+ pci_iounmap(host->pci_dev, host->iobase);
+ pci_dev_put(host->pci_dev);
+ mmc_free_host(mmc);
+ }
+ pcmcia_disable_device(link);
+
+}
+
+static int sdricoh_pcmcia_suspend(struct pcmcia_device *link)
+{
+ struct mmc_host *mmc = link->priv;
+ DBG("suspend\n");
+ mmc_suspend_host(mmc, PMSG_SUSPEND);
+ return 0;
+}
+
+static int sdricoh_pcmcia_resume(struct pcmcia_device *link)
+{
+ struct mmc_host *mmc = link->priv;
+ DBG("resume\n");
+ sdricoh_reset(mmc_priv(mmc));
+ mmc_resume_host(mmc);
+ return 0;
+}
+
+static struct pcmcia_driver sdricoh_driver = {
+ .drv = {
+ .name = DRIVER_NAME,
+ },
+ .probe = sdricoh_pcmcia_probe,
+ .remove = sdricoh_pcmcia_detach,
+ .id_table = pcmcia_ids,
+ .suspend = sdricoh_pcmcia_suspend,
+ .resume = sdricoh_pcmcia_resume,
+};
+
+/*****************************************************************************\
+ * *
+ * Driver init/exit *
+ * *
+\*****************************************************************************/
+
+static int __init sdricoh_drv_init(void)
+{
+ DBG("Ricoh PCMCIA Secure Digital Interface driver\n");
+ DBG("Copyright(c) 2006 - 2008 Sascha Sommer\n");
+ return pcmcia_register_driver(&sdricoh_driver);
+}
+
+static void __exit sdricoh_drv_exit(void)
+{
+ DBG("exiting\n");
+ pcmcia_unregister_driver(&sdricoh_driver);
+
+}
+
+module_init(sdricoh_drv_init);
+module_exit(sdricoh_drv_exit);
+
+module_param(debug, uint, 0444);
+module_param(switchlocked, uint, 0444);
+
+MODULE_AUTHOR("Sascha Sommer <saschasommer@freenet.de>");
+MODULE_DESCRIPTION("Ricoh PCMCIA Secure Digital Interface driver");
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL");
+
+MODULE_PARM_DESC(debug, "Enable debugging (default 0)");
+MODULE_PARM_DESC(switchlocked, "Switch the cards locked status."
+ "Use this when unlocked cards are shown readonly (default 0)");
--- drivers/mmc/host.org/Kconfig 2008-04-27 14:35:31.000000000 +0200
+++ drivers/mmc/host/Kconfig 2008-04-27 15:15:29.000000000 +0200
@@ -130,3 +130,13 @@
If unsure, or if your system has no SPI master driver, say N.
+config MMC_SDRICOH_CS
+ tristate "MMC/SD driver for Ricoh Bay1Controllers (EXPERIMENTAL)"
+ depends on EXPERIMENTAL && MMC && PCI && PCMCIA && YENTA
+ help
+ Say Y here if your Notebook reports a Ricoh Bay1Controller PCMCIA
+ card whenever you insert a MMC or SD card into the card slot.
+
+ To compile this driver as a module, choose M here: the
+ module will be called sdricoh_cs.
+
--- drivers/mmc/host.org/Makefile 2008-04-27 14:35:31.000000000 +0200
+++ drivers/mmc/host/Makefile 2008-04-27 14:37:13.000000000 +0200
@@ -17,4 +17,5 @@
obj-$(CONFIG_MMC_AT91) += at91_mci.o
obj-$(CONFIG_MMC_TIFM_SD) += tifm_sd.o
obj-$(CONFIG_MMC_SPI) += mmc_spi.o
+obj-$(CONFIG_MMC_SDRICOH_CS) += sdricoh_cs.o
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MMC/SD host driver for Ricoh Bay1Controllers
2008-04-27 14:17 [PATCH] MMC/SD host driver for Ricoh Bay1Controllers Sascha Sommer
@ 2008-05-01 23:39 ` Andrew Morton
2008-05-11 8:50 ` Sascha Sommer
0 siblings, 1 reply; 8+ messages in thread
From: Andrew Morton @ 2008-05-01 23:39 UTC (permalink / raw)
To: Sascha Sommer; +Cc: linux-kernel, sdricohcs-devel, Pierre Ossman
On Sun, 27 Apr 2008 16:17:53 +0200
Sascha Sommer <saschasommer@freenet.de> wrote:
> Hi,
>
> attached you can find a driver for MMC/SD card controllers that have been
> integrated into some Notebooks with Ricoh Co Ltd RL5c476 II Cardbus bridges.
Thanks.
Please do copy Pierre on mmc patches.
> This includes the Samsung P35 notebook, the Dell X300 and some notebooks from
> the Asus M6N series.
> Whenever a MMC or SD card is insterted in the cardslot of one of these
> notebooks the cardbus bridge will announce a
> virtual Ricoh Bay1Controller PCMCIA card.
>
> Since I submitted the first version of this driver to lkml last year many
> changes have been made. The driver can now read and write to MMC and SD cards
> and the reading speed for SD cards should be close to the windows driver now.
> Many thanks to all the people that tested the driver and helped me to fix some
> of the remaining bugs. I think it is now time to find out what needs to be
> fixed before the driver can go into kernel.
>
> While the driver is working fine for me there are still some not so nice parts
> in it.
> - the registers are on the cardbus bridge that is already claimed by yenta
> socket therefore the driver has to use pci_get_device
> - there is a lot of register polling because the reader does not support irqs
> - some mmc commands require extra hacks. One of those commands is the
> SD_APP_SEND_SCR command (a user reported that it is required for his SDHC
> card). Like the read and write commands this command
> requires a block read to read the scr but this does not seem to work.
> I do not have any documentation for this device and the windows driver does
> not support such commands so I cannot find the
> needed info in its logfiles.
> ...
>
> I'm thankfull for any kind of feedback.
>
> Signed-off-by: Sascha Sommer <saschasommer@freenet.de>
The patch has a large number of trivial layout problems, most of which we
wold ordinarily prefer be fixed. Please pass the diff through
scripts/checkpatch.pl and consider the output?
>
> ...
>
> +#define DRIVER_NAME "sdricoh_cs"
> +#define DRIVER_VERSION "0.1.4"
Please consider removing the driver version. It becomes useless once the
code is merge into the kernel - it cannot be used to determine exactly
which version of the driver your users are running. Distros may patch the
driver, other kernel develoeprs may patch it and forget to increment the
version number, etc.
To regenerate a user's driver source the only reliable approach is to find
out their kernel version and then go find the source to that kernel.
> +static unsigned int debug = 0;
> +static unsigned int switchlocked = 0;
> +
> +/* #define DEBUG */
> +
> +/* debug macros */
> +
> +#ifdef DEBUG
> +#define REGDBG(fmt, arg...) do {\
> + if (debug > 1) \
> + printk(KERN_INFO "sdricoh_cs: "fmt, \
Use DRIVER_NAME here
> + ##arg); } while (0)
> +#else
> +#define REGDBG(fmt, arg...)
> +#endif
> +
> +#define DBG(fmt, arg...) do {\
> + if (debug > 0) \
> + printk(KERN_INFO DRIVER_NAME ": "fmt, \
> + ##arg); } while (0)
> +
> +#define ERR(fmt, arg...) do {\
> + printk(KERN_INFO DRIVER_NAME ": "fmt, \
> + ##arg); } while (0)
> +
> +#define INFO(fmt, arg...) do {\
> + printk(KERN_INFO DRIVER_NAME ": "fmt, \
> + ##arg); } while (0)
It'd be nice to use common debug macros rather than home-made ones.
include/linux/kernel.h has one. But don't bust a gut over it - thousands
of drivers do this :(
>
> ...
>
> +static int sdricoh_query_status(struct sdricoh_host *host,unsigned int wanted,
> + unsigned int timeout){
> + unsigned int loop;
> + unsigned int status = 0;
> + for (loop = 0; loop < timeout; loop++) {
> + status = sdricoh_readl(host, R21C_STATUS);
> + sdricoh_writel(host, R2E4_STATUS_RESP, status);
> + if (status & wanted)
> + break;
> + }
> +
> + if (loop == timeout) {
> + ERR("query_status: timeout waiting for data\n");
> + return -ETIMEDOUT;
> + }
Something went wrong with the indenting there, although I expect checkpatch
will notice it.
> + /* do not do this check in the loop as some commands fail otherwise */
> + if(status & 0x7F0000){
> + ERR("waiting for status bit %x failed\n",wanted);
> + return -EINVAL;
> + }
> + return 0;
> +
> +}
> +
> +
> +
> +
> +
?
> +static int sdricoh_mmc_cmd(struct sdricoh_host *host, unsigned char opcode,
> + unsigned int arg)
> +{
> + unsigned int status;
> + int result = 0;
> + unsigned int loop = 0;
> + /* reset status reg? */
> + sdricoh_writel(host, R21C_STATUS, 0x18);
> + /* fill parameters */
> + sdricoh_writel(host, R204_CMD_ARG, arg);
> + sdricoh_writel(host, R200_CMD, (0x10000 << 8) | opcode);
> + /* wait for command completion */
> + if (opcode) {
> + for (loop = 0; loop < CMD_TIMEOUT; loop++) {
> + status = sdricoh_readl(host, R21C_STATUS);
> + sdricoh_writel(host, R2E4_STATUS_RESP, status);
> + if (status & STATUS_CMD_FINISHED)
> + break;
> + }
> + if (loop == CMD_TIMEOUT || status & STATUS_CMD_TIMEOUT)
The test of STATUS_CMD_TIMEOUT here is redundant.
Perhaps you meant to break out of the loop if STATUS_CMD_TIMEOUT becomes
set.
> + result = -ETIMEDOUT;
> +
> + }
> + DBG("mmc_cmd opcode=%i arg=0x%x => %i (queries=%i)\n",
> + opcode, arg, result, loop);
> +
> + if(result == 0){
> + /* EXT_CSD are filtered so this should be save */
> + if(opcode == SD_SEND_IF_COND){
> + if(host->mode != MODE_SDHC){
> + INFO("switching to SDHC mode\n");
> + host->mode = MODE_SDHC;
> + }
> + }
> +
> + /* switch to SD mode if APP_CMDs are supported */
> + if(opcode == MMC_APP_CMD){
> + if(host->mode == MODE_MMC){
> + INFO("switching to SD mode\n");
> + host->mode = MODE_SD;
> + }
> + }
> + }
> +
> + return result;
> +
> +}
>
> ...
>
> +static int sdricoh_blockio(struct sdricoh_host *host, int read,
> + unsigned int* buf)
> +{
> + int i;
> + /* wait until the data is available */
> + if(read){
> + if(sdricoh_query_status(host,STATUS_READY_TO_READ,
> + TRANSFER_TIMEOUT))
> + return 0;
> + sdricoh_writel(host, R21C_STATUS, 0x18);
> + /* read data */
> + for (i = 0; i < 512 / 4; i++) {
> + buf[i] = sdricoh_readl(host, R230_DATA);
> + }
Will this code work correctly on big-endian machines?
> + }else{
> + if(sdricoh_query_status(host,STATUS_READY_TO_WRITE,
> + TRANSFER_TIMEOUT))
> + return 0;
> + sdricoh_writel(host, R21C_STATUS, 0x18);
> + /* write data */
> + for (i = 0; i < 512 / 4; i++) {
> + sdricoh_writel(host, R230_DATA, buf[i]);
> + }
> + }
> +
> + return 512;
> +}
> +
> +static int sdricoh_busy(struct sdricoh_host* host){
the brace goes on the next line, please.
> + unsigned int status;
> + int i;
> + /* wait until the tranfer is finished */
> + for (i = 0; i < BUSY_TIMEOUT; i++) {
> + status = sdricoh_readl(host, R21C_STATUS);
> + sdricoh_writel(host, R2E4_STATUS_RESP, status);
> + if (!(status & STATUS_BUSY))
> + break;
missing tab
> + }
> + if(status & 0x7F0000)
> + return -EINVAL;
> + if(i == BUSY_TIMEOUT)
> + return -ETIMEDOUT;
> + return 0;
> +}
> +
> +
> +static void sdricoh_request(struct mmc_host *mmc, struct mmc_request *mrq)
> +{
> + struct sdricoh_host *host = mmc_priv(mmc);
> + struct mmc_command *cmd = mrq->cmd;
> + struct mmc_data *data = cmd->data;
> + int i;
> +
> + DBG("=============================\n");
> + DBG("sdricoh_request opcode=%i\n", cmd->opcode);
> +
> + sdricoh_writel(host, R21C_STATUS, 0x18);
> +
> + /* we cannot handle all commands that require a block transfer
> + therefore do some ugly special handling here
> + */
> + if(cmd->data){
> + switch(cmd->opcode){
> + /* working commands */
> + case MMC_READ_SINGLE_BLOCK:
> + case MMC_READ_MULTIPLE_BLOCK:
> + case MMC_WRITE_BLOCK:
> + break;
> + case SD_APP_SEND_SCR: /* required for SDHC */
> + cmd->error = sdricoh_mmc_cmd(host,
> + cmd->opcode,cmd->arg);
> + mmc_request_done(mmc, mrq);
> + return;
> + default:
> + DBG("unsupported command %i\n",cmd->opcode);
> + cmd->error = -EINVAL;
> + mmc_request_done(mmc, mrq);
> + return;
> + }
we normally will indent the body of the switch statement one tabstop less
than this.
> + }
> +
> +
> + /* read/write commands seem to require this */
> + if (data) {
> + if((cmd->error = sdricoh_busy(host)))
> + ERR("sdricoh_request: unable to prepare transfer %x\n",
> + cmd->error);
> + sdricoh_writel(host, R208_DATAIO, 0);
> + }
> +
> +
> + cmd->error = sdricoh_mmc_cmd(host, cmd->opcode, cmd->arg);
> +
> + /* read response buffer */
> + if (cmd->flags & MMC_RSP_PRESENT) {
> + if (cmd->flags & MMC_RSP_136) {
> + /* CRC is stripped so we need to do some shifting. */
> + for (i = 0; i < 4; i++) {
> + cmd->resp[i] =
> + sdricoh_readl(host,
> + R20C_RESP + (3 - i) * 4) << 8;
> + if (i != 3)
> + cmd->resp[i] |=
> + sdricoh_readb(host, R20C_RESP +
> + (3 - i) *4 - 1);
> + }
> + DBG("resp[0]=0x%x\n", cmd->resp[0]);
> + DBG("resp[1]=0x%x\n", cmd->resp[1]);
> + DBG("resp[2]=0x%x\n", cmd->resp[2]);
> + DBG("resp[2]=0x%x\n", cmd->resp[3]);
> + } else {
> + cmd->resp[0] = sdricoh_readl(host, R20C_RESP);
> + DBG("resp[0]=0x%x\n", cmd->resp[0]);
> + }
> + }
> +
> + /* yet another workaround */
> + /* without the extra command SD cards do not work at all */
> + if (cmd->opcode == MMC_SELECT_CARD) {
> + if(host->mode != MODE_MMC){
> + sdricoh_mmc_cmd(host, MMC_APP_CMD, cmd->arg);
> + sdricoh_mmc_cmd(host, 0x46, 0x02);
> + }else{
> + sdricoh_writel(host, R228_POWER, 0xc0e0);
> + sdricoh_writel(host, R224_MODE, 0x2000301);
> + }
> + }
> +
> + /* transfer data */
> + if (data && cmd->error == 0) {
> + DBG("transfer: blksz %i blocks %i sg_len %i sg length %i\n",
> + data->blksz, data->blocks, data->sg_len, data->sg->length);
> +
> + /* enter data reading mode */
> + sdricoh_writel(host, R21C_STATUS, 0x837f031e);
> + for (i = 0; i < data->blocks; i++) {
> + unsigned int *buf;
> + struct page* page;
> + size_t xfered;
> + page = sg_page(data->sg);
> +
> + buf = kmap(page) + data->sg->offset + (512 * i);
> + xfered =
> + sdricoh_blockio(host,data->flags & MMC_DATA_READ,buf);
> + kunmap(page);
hm. I don't really know how the kernel gets down into here, but I wonder
if this driver (and, I bet, lots of similar ones) should be doing
flush_dcache_page() after modifying the page. If this page can be file
pagecache or user memory then "yes". Unless it's done elsewhere for us.
> + if(!xfered){
> + ERR("sdricoh_request: block transfer failed\n");
> + cmd->error = -EINVAL;
> + break;
> + }else
> + data->bytes_xfered += xfered;
> + }
> +
> + sdricoh_writel(host, R208_DATAIO, 1);
> +
> + if(sdricoh_query_status(host,STATUS_TRANSFER_FINISHED,
> + TRANSFER_TIMEOUT)){
> + ERR("sdricoh_request: transfer end error\n");
> + cmd->error = -EINVAL;
> + }
> +
> + if(!cmd->error && (cmd->error = sdricoh_busy(host)))
> + ERR("sdricoh_request: transfer not finished %x\n",
> + cmd->error);
> +
> + }
> +
> + mmc_request_done(mmc, mrq);
> + DBG("=============================\n");
> +}
>
> ...
>
> +/* initialize the control and register it to the mmc framework */
> +static int sdricoh_init_mmc(struct pci_dev *pci_dev,
> + struct pcmcia_device *pcmcia_dev)
> +{
> + int result = 0;
> + void __iomem *iobase = NULL;
> + struct mmc_host *mmc = NULL;
> + struct sdricoh_host *host = NULL;
> + /* map iomem */
> + if (pci_resource_len(pci_dev, SDRICOH_PCI_REGION) !=
> + SDRICOH_PCI_REGION_SIZE) {
> + DBG("unexpected pci resource len\n");
> + return -ENODEV;
> + }
> + iobase =
> + pci_iomap(pci_dev, SDRICOH_PCI_REGION, SDRICOH_PCI_REGION_SIZE);
> + if (!iobase) {
> + ERR("unable to map iobase\n");
> + return -ENODEV;
> + }
> + /* check version? */
> + if (readl(iobase + R104_VERSION) != 0x4000) {
> + DBG("no supported mmc controller found\n");
> + result = -ENODEV;
> + goto err;
> + }
> + /* allocate privdata */
> + mmc = pcmcia_dev->priv =
> + mmc_alloc_host(sizeof(struct sdricoh_host), &pcmcia_dev->dev);
> + if (!mmc) {
> + ERR("mmc_alloc_host failed\n");
> + result = -ENOMEM;
> + goto err;
> + }
> + host = mmc_priv(mmc);
> +
> + host->iobase = iobase;
> +
> + mmc->ops = &sdricoh_ops;
> +
> + /* FIXME: frequency and voltage handling is done by the controller
> + */
> + mmc->f_min = 450000;
> + mmc->f_max = 24000000;
> + mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> +
> + mmc->max_seg_size = 1024 * 512;
> +
> + /* reset the controler */
> + if (sdricoh_reset(host)) {
> + DBG("could not reset\n");
> + result = -EIO;
> + goto err;
forgot to free pcmcia_dev->priv here?
> + }
> +
> + result = mmc_add_host(mmc);
> +
> + if (!result) {
> + DBG("mmc host registered\n");
> + return 0;
> + }
> +
> + err:
> + if (iobase)
> + iounmap(iobase);
> + if (mmc)
> + mmc_free_host(mmc);
> +
> + return result;
> +}
>
> ...
>
> +static void sdricoh_pcmcia_detach(struct pcmcia_device *link)
> +{
> + struct mmc_host *mmc = link->priv;
> +
> + DBG("detach\n");
> +
> + flush_scheduled_work();
what work are we flushing here?
> + /* remove mmc host */
> + if (mmc) {
> + struct sdricoh_host *host = mmc_priv(mmc);
> + mmc_remove_host(mmc);
> + pci_iounmap(host->pci_dev, host->iobase);
> + pci_dev_put(host->pci_dev);
> + mmc_free_host(mmc);
> + }
> + pcmcia_disable_device(link);
> +
> +}
>
> ...
>
> --- drivers/mmc/host.org/Kconfig 2008-04-27 14:35:31.000000000 +0200
> +++ drivers/mmc/host/Kconfig 2008-04-27 15:15:29.000000000 +0200
> @@ -130,3 +130,13 @@
>
> If unsure, or if your system has no SPI master driver, say N.
>
> +config MMC_SDRICOH_CS
> + tristate "MMC/SD driver for Ricoh Bay1Controllers (EXPERIMENTAL)"
> + depends on EXPERIMENTAL && MMC && PCI && PCMCIA && YENTA
Is the dependency on YENTA correct?
> + help
> + Say Y here if your Notebook reports a Ricoh Bay1Controller PCMCIA
> + card whenever you insert a MMC or SD card into the card slot.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called sdricoh_cs.
> +
>
> ...
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MMC/SD host driver for Ricoh Bay1Controllers
2008-05-01 23:39 ` Andrew Morton
@ 2008-05-11 8:50 ` Sascha Sommer
2008-05-12 17:54 ` Pierre Ossman
0 siblings, 1 reply; 8+ messages in thread
From: Sascha Sommer @ 2008-05-11 8:50 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, sdricohcs-devel, Pierre Ossman
[-- Attachment #1: Type: text/plain, Size: 15846 bytes --]
Hi,
Thanks for the review. I attached a new patch.
On Freitag, 2. Mai 2008, Andrew Morton wrote:
> On Sun, 27 Apr 2008 16:17:53 +0200
>
> Sascha Sommer <saschasommer@freenet.de> wrote:
> > Hi,
> >
> > attached you can find a driver for MMC/SD card controllers that have been
> > integrated into some Notebooks with Ricoh Co Ltd RL5c476 II Cardbus
> > bridges.
>
> Thanks.
>
> Please do copy Pierre on mmc patches.
>
> > This includes the Samsung P35 notebook, the Dell X300 and some notebooks
> > from the Asus M6N series.
> > Whenever a MMC or SD card is insterted in the cardslot of one of these
> > notebooks the cardbus bridge will announce a
> > virtual Ricoh Bay1Controller PCMCIA card.
> >
> > Since I submitted the first version of this driver to lkml last year many
> > changes have been made. The driver can now read and write to MMC and SD
> > cards and the reading speed for SD cards should be close to the windows
> > driver now. Many thanks to all the people that tested the driver and
> > helped me to fix some of the remaining bugs. I think it is now time to
> > find out what needs to be fixed before the driver can go into kernel.
> >
> > While the driver is working fine for me there are still some not so nice
> > parts in it.
> > - the registers are on the cardbus bridge that is already claimed by
> > yenta socket therefore the driver has to use pci_get_device
> > - there is a lot of register polling because the reader does not support
> > irqs - some mmc commands require extra hacks. One of those commands is
> > the SD_APP_SEND_SCR command (a user reported that it is required for his
> > SDHC card). Like the read and write commands this command
> > requires a block read to read the scr but this does not seem to work.
> > I do not have any documentation for this device and the windows driver
> > does not support such commands so I cannot find the
> > needed info in its logfiles.
> > ...
> >
> > I'm thankfull for any kind of feedback.
> >
> > Signed-off-by: Sascha Sommer <saschasommer@freenet.de>
>
> The patch has a large number of trivial layout problems, most of which we
> wold ordinarily prefer be fixed. Please pass the diff through
> scripts/checkpatch.pl and consider the output?
>
Done.
> > ...
> >
> > +#define DRIVER_NAME "sdricoh_cs"
> > +#define DRIVER_VERSION "0.1.4"
>
> Please consider removing the driver version. It becomes useless once the
> code is merge into the kernel - it cannot be used to determine exactly
> which version of the driver your users are running. Distros may patch the
> driver, other kernel develoeprs may patch it and forget to increment the
> version number, etc.
>
> To regenerate a user's driver source the only reliable approach is to find
> out their kernel version and then go find the source to that kernel.
Removed.
>
> > +static unsigned int debug = 0;
> > +static unsigned int switchlocked = 0;
> > +
> > +/* #define DEBUG */
> > +
> > +/* debug macros */
> > +
> > +#ifdef DEBUG
> > +#define REGDBG(fmt, arg...) do {\
> > + if (debug > 1) \
> > + printk(KERN_INFO "sdricoh_cs: "fmt, \
>
> Use DRIVER_NAME here
>
> > + ##arg); } while (0)
> > +#else
> > +#define REGDBG(fmt, arg...)
> > +#endif
> > +
> > +#define DBG(fmt, arg...) do {\
> > + if (debug > 0) \
> > + printk(KERN_INFO DRIVER_NAME ": "fmt, \
> > + ##arg); } while (0)
> > +
> > +#define ERR(fmt, arg...) do {\
> > + printk(KERN_INFO DRIVER_NAME ": "fmt, \
> > + ##arg); } while (0)
> > +
> > +#define INFO(fmt, arg...) do {\
> > + printk(KERN_INFO DRIVER_NAME ": "fmt, \
> > + ##arg); } while (0)
>
> It'd be nice to use common debug macros rather than home-made ones.
> include/linux/kernel.h has one. But don't bust a gut over it - thousands
> of drivers do this :(
>
Changed.
> > ...
> >
> > +static int sdricoh_query_status(struct sdricoh_host *host,unsigned int
> > wanted, + unsigned int timeout){
> > + unsigned int loop;
> > + unsigned int status = 0;
> > + for (loop = 0; loop < timeout; loop++) {
> > + status = sdricoh_readl(host, R21C_STATUS);
> > + sdricoh_writel(host, R2E4_STATUS_RESP, status);
> > + if (status & wanted)
> > + break;
> > + }
> > +
> > + if (loop == timeout) {
> > + ERR("query_status: timeout waiting for data\n");
> > + return -ETIMEDOUT;
> > + }
>
> Something went wrong with the indenting there, although I expect checkpatch
> will notice it.
>
Fixed.
> > + /* do not do this check in the loop as some commands fail otherwise */
> > + if(status & 0x7F0000){
> > + ERR("waiting for status bit %x failed\n",wanted);
> > + return -EINVAL;
> > + }
> > + return 0;
> > +
> > +}
> > +
> > +
> > +
> > +
> > +
>
> ?
>
Fixed.
> > +static int sdricoh_mmc_cmd(struct sdricoh_host *host, unsigned char
> > opcode, + unsigned int arg)
> > +{
> > + unsigned int status;
> > + int result = 0;
> > + unsigned int loop = 0;
> > + /* reset status reg? */
> > + sdricoh_writel(host, R21C_STATUS, 0x18);
> > + /* fill parameters */
> > + sdricoh_writel(host, R204_CMD_ARG, arg);
> > + sdricoh_writel(host, R200_CMD, (0x10000 << 8) | opcode);
> > + /* wait for command completion */
> > + if (opcode) {
> > + for (loop = 0; loop < CMD_TIMEOUT; loop++) {
> > + status = sdricoh_readl(host, R21C_STATUS);
> > + sdricoh_writel(host, R2E4_STATUS_RESP, status);
> > + if (status & STATUS_CMD_FINISHED)
> > + break;
> > + }
> > + if (loop == CMD_TIMEOUT || status & STATUS_CMD_TIMEOUT)
>
> The test of STATUS_CMD_TIMEOUT here is redundant.
>
> Perhaps you meant to break out of the loop if STATUS_CMD_TIMEOUT becomes
> set.
>
It is a bit strange. The bits sometimes do not get reset so doing the check
inside the loop leads to wrong negatives. I added a comment.
> > + result = -ETIMEDOUT;
> > +
> > + }
> > + DBG("mmc_cmd opcode=%i arg=0x%x => %i (queries=%i)\n",
> > + opcode, arg, result, loop);
> > +
> > + if(result == 0){
> > + /* EXT_CSD are filtered so this should be save */
> > + if(opcode == SD_SEND_IF_COND){
> > + if(host->mode != MODE_SDHC){
> > + INFO("switching to SDHC mode\n");
> > + host->mode = MODE_SDHC;
> > + }
> > + }
> > +
> > + /* switch to SD mode if APP_CMDs are supported */
> > + if(opcode == MMC_APP_CMD){
> > + if(host->mode == MODE_MMC){
> > + INFO("switching to SD mode\n");
> > + host->mode = MODE_SD;
> > + }
> > + }
> > + }
> > +
> > + return result;
> > +
> > +}
> >
> > ...
> >
> > +static int sdricoh_blockio(struct sdricoh_host *host, int read,
> > + unsigned int* buf)
> > +{
> > + int i;
> > + /* wait until the data is available */
> > + if(read){
> > + if(sdricoh_query_status(host,STATUS_READY_TO_READ,
> > + TRANSFER_TIMEOUT))
> > + return 0;
> > + sdricoh_writel(host, R21C_STATUS, 0x18);
> > + /* read data */
> > + for (i = 0; i < 512 / 4; i++) {
> > + buf[i] = sdricoh_readl(host, R230_DATA);
> > + }
>
> Will this code work correctly on big-endian machines?
>
Most likely not and there might be other big-endian related problems in the
driver. I don't know of a case where this hardware can be found on big
endian.
> > + }else{
> > + if(sdricoh_query_status(host,STATUS_READY_TO_WRITE,
> > + TRANSFER_TIMEOUT))
> > + return 0;
> > + sdricoh_writel(host, R21C_STATUS, 0x18);
> > + /* write data */
> > + for (i = 0; i < 512 / 4; i++) {
> > + sdricoh_writel(host, R230_DATA, buf[i]);
> > + }
> > + }
> > +
> > + return 512;
> > +}
> > +
> > +static int sdricoh_busy(struct sdricoh_host* host){
>
> the brace goes on the next line, please.
>
Fixed.
> > + unsigned int status;
> > + int i;
> > + /* wait until the tranfer is finished */
> > + for (i = 0; i < BUSY_TIMEOUT; i++) {
> > + status = sdricoh_readl(host, R21C_STATUS);
> > + sdricoh_writel(host, R2E4_STATUS_RESP, status);
> > + if (!(status & STATUS_BUSY))
> > + break;
>
> missing tab
>
Fixed.
> > + }
> > + if(status & 0x7F0000)
> > + return -EINVAL;
> > + if(i == BUSY_TIMEOUT)
> > + return -ETIMEDOUT;
> > + return 0;
> > +}
> > +
> > +
> > +static void sdricoh_request(struct mmc_host *mmc, struct mmc_request
> > *mrq) +{
> > + struct sdricoh_host *host = mmc_priv(mmc);
> > + struct mmc_command *cmd = mrq->cmd;
> > + struct mmc_data *data = cmd->data;
> > + int i;
> > +
> > + DBG("=============================\n");
> > + DBG("sdricoh_request opcode=%i\n", cmd->opcode);
> > +
> > + sdricoh_writel(host, R21C_STATUS, 0x18);
> > +
> > + /* we cannot handle all commands that require a block transfer
> > + therefore do some ugly special handling here
> > + */
> > + if(cmd->data){
> > + switch(cmd->opcode){
> > + /* working commands */
> > + case MMC_READ_SINGLE_BLOCK:
> > + case MMC_READ_MULTIPLE_BLOCK:
> > + case MMC_WRITE_BLOCK:
> > + break;
> > + case SD_APP_SEND_SCR: /* required for SDHC */
> > + cmd->error = sdricoh_mmc_cmd(host,
> > + cmd->opcode,cmd->arg);
> > + mmc_request_done(mmc, mrq);
> > + return;
> > + default:
> > + DBG("unsupported command %i\n",cmd->opcode);
> > + cmd->error = -EINVAL;
> > + mmc_request_done(mmc, mrq);
> > + return;
> > + }
>
> we normally will indent the body of the switch statement one tabstop less
> than this.
>
Fixed.
> > + }
> > +
> > +
> > + /* read/write commands seem to require this */
> > + if (data) {
> > + if((cmd->error = sdricoh_busy(host)))
> > + ERR("sdricoh_request: unable to prepare transfer %x\n",
> > + cmd->error);
> > + sdricoh_writel(host, R208_DATAIO, 0);
> > + }
> > +
> > +
> > + cmd->error = sdricoh_mmc_cmd(host, cmd->opcode, cmd->arg);
> > +
> > + /* read response buffer */
> > + if (cmd->flags & MMC_RSP_PRESENT) {
> > + if (cmd->flags & MMC_RSP_136) {
> > + /* CRC is stripped so we need to do some shifting. */
> > + for (i = 0; i < 4; i++) {
> > + cmd->resp[i] =
> > + sdricoh_readl(host,
> > + R20C_RESP + (3 - i) * 4) << 8;
> > + if (i != 3)
> > + cmd->resp[i] |=
> > + sdricoh_readb(host, R20C_RESP +
> > + (3 - i) *4 - 1);
> > + }
> > + DBG("resp[0]=0x%x\n", cmd->resp[0]);
> > + DBG("resp[1]=0x%x\n", cmd->resp[1]);
> > + DBG("resp[2]=0x%x\n", cmd->resp[2]);
> > + DBG("resp[2]=0x%x\n", cmd->resp[3]);
> > + } else {
> > + cmd->resp[0] = sdricoh_readl(host, R20C_RESP);
> > + DBG("resp[0]=0x%x\n", cmd->resp[0]);
> > + }
> > + }
> > +
> > + /* yet another workaround */
> > + /* without the extra command SD cards do not work at all */
> > + if (cmd->opcode == MMC_SELECT_CARD) {
> > + if(host->mode != MODE_MMC){
> > + sdricoh_mmc_cmd(host, MMC_APP_CMD, cmd->arg);
> > + sdricoh_mmc_cmd(host, 0x46, 0x02);
> > + }else{
> > + sdricoh_writel(host, R228_POWER, 0xc0e0);
> > + sdricoh_writel(host, R224_MODE, 0x2000301);
> > + }
> > + }
> > +
> > + /* transfer data */
> > + if (data && cmd->error == 0) {
> > + DBG("transfer: blksz %i blocks %i sg_len %i sg length %i\n",
> > + data->blksz, data->blocks, data->sg_len,
> > data->sg->length); +
> > + /* enter data reading mode */
> > + sdricoh_writel(host, R21C_STATUS, 0x837f031e);
> > + for (i = 0; i < data->blocks; i++) {
> > + unsigned int *buf;
> > + struct page* page;
> > + size_t xfered;
> > + page = sg_page(data->sg);
> > +
> > + buf = kmap(page) + data->sg->offset + (512 * i);
> > + xfered =
> > + sdricoh_blockio(host,data->flags & MMC_DATA_READ,buf);
> > + kunmap(page);
>
> hm. I don't really know how the kernel gets down into here, but I wonder
> if this driver (and, I bet, lots of similar ones) should be doing
> flush_dcache_page() after modifying the page. If this page can be file
> pagecache or user memory then "yes". Unless it's done elsewhere for us.
>
Some other mmc host drivers seem to do it so I added it, too.
> > + if(!xfered){
> > + ERR("sdricoh_request: block transfer failed\n");
> > + cmd->error = -EINVAL;
> > + break;
> > + }else
> > + data->bytes_xfered += xfered;
> > + }
> > +
> > + sdricoh_writel(host, R208_DATAIO, 1);
> > +
> > + if(sdricoh_query_status(host,STATUS_TRANSFER_FINISHED,
> > + TRANSFER_TIMEOUT)){
> > + ERR("sdricoh_request: transfer end error\n");
> > + cmd->error = -EINVAL;
> > + }
> > +
> > + if(!cmd->error && (cmd->error = sdricoh_busy(host)))
> > + ERR("sdricoh_request: transfer not finished %x\n",
> > + cmd->error);
> > +
> > + }
> > +
> > + mmc_request_done(mmc, mrq);
> > + DBG("=============================\n");
> > +}
> >
> > ...
> >
> > +/* initialize the control and register it to the mmc framework */
> > +static int sdricoh_init_mmc(struct pci_dev *pci_dev,
> > + struct pcmcia_device *pcmcia_dev)
> > +{
> > + int result = 0;
> > + void __iomem *iobase = NULL;
> > + struct mmc_host *mmc = NULL;
> > + struct sdricoh_host *host = NULL;
> > + /* map iomem */
> > + if (pci_resource_len(pci_dev, SDRICOH_PCI_REGION) !=
> > + SDRICOH_PCI_REGION_SIZE) {
> > + DBG("unexpected pci resource len\n");
> > + return -ENODEV;
> > + }
> > + iobase =
> > + pci_iomap(pci_dev, SDRICOH_PCI_REGION, SDRICOH_PCI_REGION_SIZE);
> > + if (!iobase) {
> > + ERR("unable to map iobase\n");
> > + return -ENODEV;
> > + }
> > + /* check version? */
> > + if (readl(iobase + R104_VERSION) != 0x4000) {
> > + DBG("no supported mmc controller found\n");
> > + result = -ENODEV;
> > + goto err;
> > + }
> > + /* allocate privdata */
> > + mmc = pcmcia_dev->priv =
> > + mmc_alloc_host(sizeof(struct sdricoh_host), &pcmcia_dev->dev);
> > + if (!mmc) {
> > + ERR("mmc_alloc_host failed\n");
> > + result = -ENOMEM;
> > + goto err;
> > + }
> > + host = mmc_priv(mmc);
> > +
> > + host->iobase = iobase;
> > +
> > + mmc->ops = &sdricoh_ops;
> > +
> > + /* FIXME: frequency and voltage handling is done by the controller
> > + */
> > + mmc->f_min = 450000;
> > + mmc->f_max = 24000000;
> > + mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> > +
> > + mmc->max_seg_size = 1024 * 512;
> > +
> > + /* reset the controler */
> > + if (sdricoh_reset(host)) {
> > + DBG("could not reset\n");
> > + result = -EIO;
> > + goto err;
>
> forgot to free pcmcia_dev->priv here?
>
No. It is the same as mmc and that gets freed later.
> > + }
> > +
> > + result = mmc_add_host(mmc);
> > +
> > + if (!result) {
> > + DBG("mmc host registered\n");
> > + return 0;
> > + }
> > +
> > + err:
> > + if (iobase)
> > + iounmap(iobase);
> > + if (mmc)
> > + mmc_free_host(mmc);
> > +
> > + return result;
> > +}
> >
> > ...
> >
> > +static void sdricoh_pcmcia_detach(struct pcmcia_device *link)
> > +{
> > + struct mmc_host *mmc = link->priv;
> > +
> > + DBG("detach\n");
> > +
> > + flush_scheduled_work();
>
> what work are we flushing here?
>
Removed.
> > + /* remove mmc host */
> > + if (mmc) {
> > + struct sdricoh_host *host = mmc_priv(mmc);
> > + mmc_remove_host(mmc);
> > + pci_iounmap(host->pci_dev, host->iobase);
> > + pci_dev_put(host->pci_dev);
> > + mmc_free_host(mmc);
> > + }
> > + pcmcia_disable_device(link);
> > +
> > +}
> >
> > ...
> >
> > --- drivers/mmc/host.org/Kconfig 2008-04-27 14:35:31.000000000 +0200
> > +++ drivers/mmc/host/Kconfig 2008-04-27 15:15:29.000000000 +0200
> > @@ -130,3 +130,13 @@
> >
> > If unsure, or if your system has no SPI master driver, say N.
> >
> > +config MMC_SDRICOH_CS
> > + tristate "MMC/SD driver for Ricoh Bay1Controllers (EXPERIMENTAL)"
> > + depends on EXPERIMENTAL && MMC && PCI && PCMCIA && YENTA
>
> Is the dependency on YENTA correct?
>
Yes. At least noone who wants to enable this driver will forget the yenta
driver then.
Regards
Sascha
[-- Attachment #2: sdricoh_cs_try2.patch --]
[-- Type: text/x-diff, Size: 18364 bytes --]
MMC/SD host driver for Ricoh Bay1Controllers
Signed-off-by: Sascha Sommer <saschasommer@freenet.de>
--- /dev/null 2007-09-21 23:50:58.000000000 +0200
+++ drivers/mmc/host/sdricoh_cs.c 2008-05-11 10:07:36.000000000 +0200
@@ -0,0 +1,632 @@
+/*
+ * sdricoh_cs.c - driver for Ricoh Secure Digital Card Readers that can be
+ * found on some Ricoh RL5c476 II cardbus bridge
+ *
+ * Copyright (C) 2006 - 2008 Sascha Sommer <saschasommer@freenet.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+
+/*
+#define DEBUG
+#define VERBOSE_DEBUG
+*/
+#include <linux/delay.h>
+#include <linux/highmem.h>
+#include <linux/pci.h>
+#include <linux/ioport.h>
+#include <linux/version.h>
+
+#include <pcmcia/cs_types.h>
+#include <pcmcia/cs.h>
+#include <pcmcia/cistpl.h>
+#include <pcmcia/ds.h>
+#include <linux/io.h>
+
+#include <linux/mmc/host.h>
+#include <linux/mmc/mmc.h>
+#include <linux/mmc/sd.h>
+#include <linux/mmc/sdio.h>
+
+#define DRIVER_NAME "sdricoh_cs"
+
+static unsigned int switchlocked;
+
+/* i/o region */
+#define SDRICOH_PCI_REGION 0
+#define SDRICOH_PCI_REGION_SIZE 0x1000
+
+/* registers */
+#define R104_VERSION 0x104
+#define R200_CMD 0x200
+#define R204_CMD_ARG 0x204
+#define R208_DATAIO 0x208
+#define R20C_RESP 0x20c
+#define R21C_STATUS 0x21c
+#define R2E0_INIT 0x2e0
+#define R2E4_STATUS_RESP 0x2e4
+#define R2F0_RESET 0x2f0
+#define R224_MODE 0x224
+#define R228_POWER 0x228
+#define R230_DATA 0x230
+
+/* flags for the R21C_STATUS register */
+#define STATUS_CMD_FINISHED 0x00000001
+#define STATUS_TRANSFER_FINISHED 0x00000004
+#define STATUS_CARD_INSERTED 0x00000020
+#define STATUS_CARD_LOCKED 0x00000080
+#define STATUS_CMD_TIMEOUT 0x00400000
+#define STATUS_READY_TO_READ 0x01000000
+#define STATUS_READY_TO_WRITE 0x02000000
+#define STATUS_BUSY 0x40000000
+
+/* timeouts */
+#define INIT_TIMEOUT 100
+#define CMD_TIMEOUT 100000
+#define TRANSFER_TIMEOUT 100000
+#define BUSY_TIMEOUT 32767
+
+#define MODE_MMC 0
+#define MODE_SD 1
+#define MODE_SDHC 2
+
+/* list of supported pcmcia devices */
+static struct pcmcia_device_id pcmcia_ids[] = {
+ /* vendor and device strings followed by their crc32 hashes */
+ PCMCIA_DEVICE_PROD_ID12("RICOH", "Bay1Controller", 0xd9f522ed,
+ 0xc3901202),
+ PCMCIA_DEVICE_NULL,
+};
+
+MODULE_DEVICE_TABLE(pcmcia, pcmcia_ids);
+
+/* mmc privdata */
+struct sdricoh_host {
+ struct device *dev;
+ struct mmc_host *mmc; /* MMC structure */
+ unsigned char __iomem *iobase;
+ struct pci_dev *pci_dev;
+ int mode;
+};
+
+/***************** register i/o helper functions *****************************/
+
+static inline unsigned int sdricoh_readl(struct sdricoh_host *host,
+ unsigned int reg)
+{
+ unsigned int value = readl(host->iobase + reg);
+ dev_vdbg(host->dev, "rl %x 0x%x\n", reg, value);
+ return value;
+}
+
+static inline void sdricoh_writel(struct sdricoh_host *host, unsigned int reg,
+ unsigned int value)
+{
+ writel(value, host->iobase + reg);
+ dev_vdbg(host->dev, "wl %x 0x%x\n", reg, value);
+
+}
+
+static inline unsigned int sdricoh_readw(struct sdricoh_host *host,
+ unsigned int reg)
+{
+ unsigned int value = readw(host->iobase + reg);
+ dev_vdbg(host->dev, "rb %x 0x%x\n", reg, value);
+ return value;
+}
+
+static inline unsigned int sdricoh_readb(struct sdricoh_host *host,
+ unsigned int reg)
+{
+ unsigned int value = readb(host->iobase + reg);
+ dev_vdbg(host->dev, "rb %x 0x%x\n", reg, value);
+ return value;
+}
+
+static int sdricoh_query_status(struct sdricoh_host *host, unsigned int wanted,
+ unsigned int timeout){
+ unsigned int loop;
+ unsigned int status = 0;
+ struct device *dev = host->dev;
+ for (loop = 0; loop < timeout; loop++) {
+ status = sdricoh_readl(host, R21C_STATUS);
+ sdricoh_writel(host, R2E4_STATUS_RESP, status);
+ if (status & wanted)
+ break;
+ }
+
+ if (loop == timeout) {
+ dev_err(dev, "query_status: timeout waiting for data\n");
+ return -ETIMEDOUT;
+ }
+
+ /* do not do this check in the loop as some commands fail otherwise */
+ if (status & 0x7F0000) {
+ dev_err(dev, "waiting for status bit %x failed\n", wanted);
+ return -EINVAL;
+ }
+ return 0;
+
+}
+
+static int sdricoh_mmc_cmd(struct sdricoh_host *host, unsigned char opcode,
+ unsigned int arg)
+{
+ unsigned int status;
+ int result = 0;
+ unsigned int loop = 0;
+ struct device *dev = host->dev;
+ /* reset status reg? */
+ sdricoh_writel(host, R21C_STATUS, 0x18);
+ /* fill parameters */
+ sdricoh_writel(host, R204_CMD_ARG, arg);
+ sdricoh_writel(host, R200_CMD, (0x10000 << 8) | opcode);
+ /* wait for command completion */
+ if (opcode) {
+ for (loop = 0; loop < CMD_TIMEOUT; loop++) {
+ status = sdricoh_readl(host, R21C_STATUS);
+ sdricoh_writel(host, R2E4_STATUS_RESP, status);
+ if (status & STATUS_CMD_FINISHED)
+ break;
+ }
+ /* don't check for timeout in the loop it is not always
+ reset correctly
+ */
+ if (loop == CMD_TIMEOUT || status & STATUS_CMD_TIMEOUT)
+ result = -ETIMEDOUT;
+
+ }
+ dev_dbg(dev, "mmc_cmd opcode=%i arg=0x%x => %i (queries=%i)\n",
+ opcode, arg, result, loop);
+
+ if (result == 0) {
+ /* EXT_CSD are filtered so this should be save */
+ if (opcode == SD_SEND_IF_COND) {
+ if (host->mode != MODE_SDHC) {
+ dev_info(dev, "switching to SDHC mode\n");
+ host->mode = MODE_SDHC;
+ }
+ }
+
+ /* switch to SD mode if APP_CMDs are supported */
+ if (opcode == MMC_APP_CMD) {
+ if (host->mode == MODE_MMC) {
+ dev_info(dev, "switching to SD mode\n");
+ host->mode = MODE_SD;
+ }
+ }
+ }
+
+ return result;
+
+}
+
+static int sdricoh_reset(struct sdricoh_host *host)
+{
+ dev_dbg(host->dev, "reset\n");
+ sdricoh_writel(host, R2F0_RESET, 0x10001);
+ sdricoh_writel(host, R2E0_INIT, 0x10000);
+ if (sdricoh_readl(host, R2E0_INIT) != 0x10000)
+ return -EIO;
+ sdricoh_writel(host, R2E0_INIT, 0x10007);
+
+ sdricoh_writel(host, R224_MODE, 0x2000000);
+ sdricoh_writel(host, R228_POWER, 0xe0);
+
+
+ /* status register ? */
+ sdricoh_writel(host, R21C_STATUS, 0x18);
+
+ return 0;
+}
+
+static int sdricoh_blockio(struct sdricoh_host *host, int read,
+ unsigned int *buf)
+{
+ int i;
+ /* wait until the data is available */
+ if (read) {
+ if (sdricoh_query_status(host, STATUS_READY_TO_READ,
+ TRANSFER_TIMEOUT))
+ return 0;
+ sdricoh_writel(host, R21C_STATUS, 0x18);
+ /* read data */
+ for (i = 0; i < 512 / 4; i++)
+ buf[i] = sdricoh_readl(host, R230_DATA);
+ } else {
+ if (sdricoh_query_status(host, STATUS_READY_TO_WRITE,
+ TRANSFER_TIMEOUT))
+ return 0;
+ sdricoh_writel(host, R21C_STATUS, 0x18);
+ /* write data */
+ for (i = 0; i < 512 / 4; i++)
+ sdricoh_writel(host, R230_DATA, buf[i]);
+ }
+
+ return 512;
+}
+
+static int sdricoh_busy(struct sdricoh_host *host)
+{
+ unsigned int status;
+ int i;
+ /* wait until the tranfer is finished */
+ for (i = 0; i < BUSY_TIMEOUT; i++) {
+ status = sdricoh_readl(host, R21C_STATUS);
+ sdricoh_writel(host, R2E4_STATUS_RESP, status);
+ if (!(status & STATUS_BUSY))
+ break;
+ }
+ if (status & 0x7F0000)
+ return -EINVAL;
+ if (i == BUSY_TIMEOUT)
+ return -ETIMEDOUT;
+ return 0;
+}
+
+
+static void sdricoh_request(struct mmc_host *mmc, struct mmc_request *mrq)
+{
+ struct sdricoh_host *host = mmc_priv(mmc);
+ struct mmc_command *cmd = mrq->cmd;
+ struct mmc_data *data = cmd->data;
+ struct device *dev = host->dev;
+ int i;
+
+ dev_dbg(dev, "=============================\n");
+ dev_dbg(dev, "sdricoh_request opcode=%i\n", cmd->opcode);
+
+ sdricoh_writel(host, R21C_STATUS, 0x18);
+
+ /* we cannot handle all commands that require a block transfer
+ therefore do some ugly special handling here
+ */
+ if (cmd->data) {
+ switch (cmd->opcode) {
+ /* working commands */
+ case MMC_READ_SINGLE_BLOCK:
+ case MMC_READ_MULTIPLE_BLOCK:
+ case MMC_WRITE_BLOCK:
+ case MMC_WRITE_MULTIPLE_BLOCK:
+ break;
+ case SD_APP_SEND_SCR: /* required for SDHC */
+ cmd->error = sdricoh_mmc_cmd(host, cmd->opcode,
+ cmd->arg);
+ mmc_request_done(mmc, mrq);
+ return;
+ default:
+ dev_dbg(dev, "unsupported command %i\n", cmd->opcode);
+ cmd->error = -EINVAL;
+ mmc_request_done(mmc, mrq);
+ return;
+ }
+ }
+
+
+ /* read/write commands seem to require this */
+ if (data) {
+ cmd->error = sdricoh_busy(host);
+ if (cmd->error)
+ dev_err(dev, "sdricoh_request: unable to prepare "
+ "transfer %x\n", cmd->error);
+ sdricoh_writel(host, R208_DATAIO, 0);
+ }
+
+
+ cmd->error = sdricoh_mmc_cmd(host, cmd->opcode, cmd->arg);
+
+ /* read response buffer */
+ if (cmd->flags & MMC_RSP_PRESENT) {
+ if (cmd->flags & MMC_RSP_136) {
+ /* CRC is stripped so we need to do some shifting. */
+ for (i = 0; i < 4; i++) {
+ cmd->resp[i] =
+ sdricoh_readl(host,
+ R20C_RESP + (3 - i) * 4) << 8;
+ if (i != 3)
+ cmd->resp[i] |=
+ sdricoh_readb(host, R20C_RESP +
+ (3 - i) * 4 - 1);
+ }
+ dev_dbg(dev, "resp[0]=0x%x\n", cmd->resp[0]);
+ dev_dbg(dev, "resp[1]=0x%x\n", cmd->resp[1]);
+ dev_dbg(dev, "resp[2]=0x%x\n", cmd->resp[2]);
+ dev_dbg(dev, "resp[2]=0x%x\n", cmd->resp[3]);
+ } else {
+ cmd->resp[0] = sdricoh_readl(host, R20C_RESP);
+ dev_dbg(dev, "resp[0]=0x%x\n", cmd->resp[0]);
+ }
+ }
+
+ /* yet another workaround */
+ /* without the extra command SD cards do not work at all */
+ if (cmd->opcode == MMC_SELECT_CARD) {
+ if (host->mode != MODE_MMC) {
+ sdricoh_mmc_cmd(host, MMC_APP_CMD, cmd->arg);
+ sdricoh_mmc_cmd(host, 0x46, 0x02);
+ } else {
+ sdricoh_writel(host, R228_POWER, 0xc0e0);
+ sdricoh_writel(host, R224_MODE, 0x2000301);
+ }
+ }
+
+ /* transfer data */
+ if (data && cmd->error == 0) {
+ dev_dbg(dev, "transfer: blksz %i blocks %i sg_len %i "
+ "sg length %i\n", data->blksz, data->blocks,
+ data->sg_len, data->sg->length);
+
+ /* enter data reading mode */
+ sdricoh_writel(host, R21C_STATUS, 0x837f031e);
+ for (i = 0; i < data->blocks; i++) {
+ unsigned int *buf;
+ struct page *page;
+ size_t xfered;
+ page = sg_page(data->sg);
+
+ buf = kmap(page) + data->sg->offset + (512 * i);
+ xfered =
+ sdricoh_blockio(host,
+ data->flags & MMC_DATA_READ, buf);
+ kunmap(page);
+ flush_dcache_page(page);
+ if (!xfered) {
+ dev_err(dev, "sdricoh_request: "
+ "block transfer failed\n");
+ cmd->error = -EINVAL;
+ break;
+ } else
+ data->bytes_xfered += xfered;
+ }
+
+ sdricoh_writel(host, R208_DATAIO, 1);
+
+ if (sdricoh_query_status(host, STATUS_TRANSFER_FINISHED,
+ TRANSFER_TIMEOUT)) {
+ dev_err(dev, "sdricoh_request: transfer end error\n");
+ cmd->error = -EINVAL;
+ }
+
+ if (!cmd->error) {
+ cmd->error = sdricoh_busy(host);
+ if (cmd->error)
+ dev_err(dev, "sdricoh_request: transfer not "
+ "finished %x\n", cmd->error);
+ }
+
+ }
+
+ mmc_request_done(mmc, mrq);
+ dev_dbg(dev, "=============================\n");
+}
+
+static void sdricoh_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+ struct sdricoh_host *host = mmc_priv(mmc);
+ dev_dbg(host->dev, "set_ios\n");
+
+ if (ios->power_mode == MMC_POWER_ON) {
+ sdricoh_writel(host, R228_POWER, 0xc0e0);
+
+ if (host->mode != MODE_MMC) {
+ sdricoh_writel(host, R224_MODE, 0x2000300);
+ sdricoh_writel(host, R228_POWER, 0x40e0);
+ } else {
+ sdricoh_writel(host, R224_MODE, 0x2000340);
+ }
+ } else if (ios->power_mode == MMC_POWER_UP) {
+ sdricoh_writel(host, R224_MODE, 0x2000320);
+ sdricoh_writel(host, R228_POWER, 0xe0);
+ }
+}
+
+static int sdricoh_get_ro(struct mmc_host *mmc)
+{
+ struct sdricoh_host *host = mmc_priv(mmc);
+ unsigned int status;
+
+ status = sdricoh_readl(host, R21C_STATUS);
+ sdricoh_writel(host, R2E4_STATUS_RESP, status);
+
+ /* some notebooks seem to have the locked flag switched */
+ if (switchlocked)
+ return !(status & STATUS_CARD_LOCKED);
+
+ return (status & STATUS_CARD_LOCKED);
+}
+
+static struct mmc_host_ops sdricoh_ops = {
+ .request = sdricoh_request,
+ .set_ios = sdricoh_set_ios,
+ .get_ro = sdricoh_get_ro,
+};
+
+/* initialize the control and register it to the mmc framework */
+static int sdricoh_init_mmc(struct pci_dev *pci_dev,
+ struct pcmcia_device *pcmcia_dev)
+{
+ int result = 0;
+ void __iomem *iobase = NULL;
+ struct mmc_host *mmc = NULL;
+ struct sdricoh_host *host = NULL;
+ struct device *dev = &pcmcia_dev->dev;
+ /* map iomem */
+ if (pci_resource_len(pci_dev, SDRICOH_PCI_REGION) !=
+ SDRICOH_PCI_REGION_SIZE) {
+ dev_dbg(dev, "unexpected pci resource len\n");
+ return -ENODEV;
+ }
+ iobase =
+ pci_iomap(pci_dev, SDRICOH_PCI_REGION, SDRICOH_PCI_REGION_SIZE);
+ if (!iobase) {
+ dev_err(dev, "unable to map iobase\n");
+ return -ENODEV;
+ }
+ /* check version? */
+ if (readl(iobase + R104_VERSION) != 0x4000) {
+ dev_dbg(dev, "no supported mmc controller found\n");
+ result = -ENODEV;
+ goto err;
+ }
+ /* allocate privdata */
+ mmc = pcmcia_dev->priv =
+ mmc_alloc_host(sizeof(struct sdricoh_host), &pcmcia_dev->dev);
+ if (!mmc) {
+ dev_err(dev, "mmc_alloc_host failed\n");
+ result = -ENOMEM;
+ goto err;
+ }
+ host = mmc_priv(mmc);
+
+ host->iobase = iobase;
+ host->dev = dev;
+ host->pci_dev = pci_dev;
+
+ mmc->ops = &sdricoh_ops;
+
+ /* FIXME: frequency and voltage handling is done by the controller
+ */
+ mmc->f_min = 450000;
+ mmc->f_max = 24000000;
+ mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
+
+ mmc->max_seg_size = 1024 * 512;
+
+ /* reset the controler */
+ if (sdricoh_reset(host)) {
+ dev_dbg(dev, "could not reset\n");
+ result = -EIO;
+ goto err;
+
+ }
+
+ result = mmc_add_host(mmc);
+
+ if (!result) {
+ dev_dbg(dev, "mmc host registered\n");
+ return 0;
+ }
+
+err:
+ if (iobase)
+ iounmap(iobase);
+ if (mmc)
+ mmc_free_host(mmc);
+
+ return result;
+}
+
+/* search for supported mmc controllers */
+static int sdricoh_pcmcia_probe(struct pcmcia_device *pcmcia_dev)
+{
+ struct pci_dev *pci_dev = NULL;
+
+ dev_info(&pcmcia_dev->dev, "Searching MMC controller for pcmcia device"
+ " %s %s ...\n", pcmcia_dev->prod_id[0], pcmcia_dev->prod_id[1]);
+
+ /* search pci cardbus bridge that contains the mmc controler */
+ /* the io region is already claimed by yenta_socket... */
+ while ((pci_dev =
+ pci_get_device(PCI_VENDOR_ID_RICOH, PCI_DEVICE_ID_RICOH_RL5C476,
+ pci_dev))) {
+ /* try to init the device */
+ if (!sdricoh_init_mmc(pci_dev, pcmcia_dev)) {
+ dev_info(&pcmcia_dev->dev, "MMC controller found\n");
+ return 0;
+ }
+
+ }
+ dev_err(&pcmcia_dev->dev, "No MMC controller was found.\n");
+ return -ENODEV;
+}
+
+static void sdricoh_pcmcia_detach(struct pcmcia_device *link)
+{
+ struct mmc_host *mmc = link->priv;
+
+ dev_dbg(&link->dev, "detach\n");
+
+ /* remove mmc host */
+ if (mmc) {
+ struct sdricoh_host *host = mmc_priv(mmc);
+ mmc_remove_host(mmc);
+ pci_iounmap(host->pci_dev, host->iobase);
+ pci_dev_put(host->pci_dev);
+ mmc_free_host(mmc);
+ }
+ pcmcia_disable_device(link);
+
+}
+
+static int sdricoh_pcmcia_suspend(struct pcmcia_device *link)
+{
+ struct mmc_host *mmc = link->priv;
+ dev_dbg(&link->dev, "suspend\n");
+ mmc_suspend_host(mmc, PMSG_SUSPEND);
+ return 0;
+}
+
+static int sdricoh_pcmcia_resume(struct pcmcia_device *link)
+{
+ struct mmc_host *mmc = link->priv;
+ dev_dbg(&link->dev, "resume\n");
+ sdricoh_reset(mmc_priv(mmc));
+ mmc_resume_host(mmc);
+ return 0;
+}
+
+static struct pcmcia_driver sdricoh_driver = {
+ .drv = {
+ .name = DRIVER_NAME,
+ },
+ .probe = sdricoh_pcmcia_probe,
+ .remove = sdricoh_pcmcia_detach,
+ .id_table = pcmcia_ids,
+ .suspend = sdricoh_pcmcia_suspend,
+ .resume = sdricoh_pcmcia_resume,
+};
+
+/*****************************************************************************\
+ * *
+ * Driver init/exit *
+ * *
+\*****************************************************************************/
+
+static int __init sdricoh_drv_init(void)
+{
+ pr_debug("Ricoh PCMCIA Secure Digital Interface driver\n");
+ pr_debug("Copyright(c) 2006 - 2008 Sascha Sommer\n");
+ return pcmcia_register_driver(&sdricoh_driver);
+}
+
+static void __exit sdricoh_drv_exit(void)
+{
+ pr_debug("exiting\n");
+ pcmcia_unregister_driver(&sdricoh_driver);
+
+}
+
+module_init(sdricoh_drv_init);
+module_exit(sdricoh_drv_exit);
+
+module_param(switchlocked, uint, 0444);
+
+MODULE_AUTHOR("Sascha Sommer <saschasommer@freenet.de>");
+MODULE_DESCRIPTION("Ricoh PCMCIA Secure Digital Interface driver");
+MODULE_LICENSE("GPL");
+
+MODULE_PARM_DESC(switchlocked, "Switch the cards locked status."
+ "Use this when unlocked cards are shown readonly (default 0)");
--- drivers/mmc/host.org/Makefile 2008-04-27 14:35:31.000000000 +0200
+++ drivers/mmc/host/Makefile 2008-04-27 14:37:13.000000000 +0200
@@ -17,4 +17,5 @@
obj-$(CONFIG_MMC_AT91) += at91_mci.o
obj-$(CONFIG_MMC_TIFM_SD) += tifm_sd.o
obj-$(CONFIG_MMC_SPI) += mmc_spi.o
+obj-$(CONFIG_MMC_SDRICOH_CS) += sdricoh_cs.o
--- drivers/mmc/host.org/Kconfig 2008-04-27 14:35:31.000000000 +0200
+++ drivers/mmc/host/Kconfig 2008-04-27 15:15:29.000000000 +0200
@@ -130,3 +130,13 @@
If unsure, or if your system has no SPI master driver, say N.
+config MMC_SDRICOH_CS
+ tristate "MMC/SD driver for Ricoh Bay1Controllers (EXPERIMENTAL)"
+ depends on EXPERIMENTAL && MMC && PCI && PCMCIA && YENTA
+ help
+ Say Y here if your Notebook reports a Ricoh Bay1Controller PCMCIA
+ card whenever you insert a MMC or SD card into the card slot.
+
+ To compile this driver as a module, choose M here: the
+ module will be called sdricoh_cs.
+
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MMC/SD host driver for Ricoh Bay1Controllers
2008-05-11 8:50 ` Sascha Sommer
@ 2008-05-12 17:54 ` Pierre Ossman
2008-05-18 12:05 ` Sascha Sommer
0 siblings, 1 reply; 8+ messages in thread
From: Pierre Ossman @ 2008-05-12 17:54 UTC (permalink / raw)
To: Sascha Sommer; +Cc: Andrew Morton, linux-kernel, sdricohcs-devel
Big thanks to Andrew for helping out with the basic review here. :)
On Sun, 11 May 2008 10:50:55 +0200
Sascha Sommer <saschasommer@freenet.de> wrote:
> On Freitag, 2. Mai 2008, Andrew Morton wrote:
> >
> > Will this code work correctly on big-endian machines?
> >
>
> Most likely not and there might be other big-endian related problems in the
> driver. I don't know of a case where this hardware can be found on big
> endian.
>
That's not a really good reason to just ignore the problem though.
Endian safe code is generally more bug free and more easily understood
because it doesn't use arch specific shortcuts. I won't NAK the patch
because of this, but I do prefer endian safe code even if there is no
hardware built that needs it right now. It's always good to practice
writing such code anyway. :)
> >
> > hm. I don't really know how the kernel gets down into here, but I wonder
> > if this driver (and, I bet, lots of similar ones) should be doing
> > flush_dcache_page() after modifying the page. If this page can be file
> > pagecache or user memory then "yes". Unless it's done elsewhere for us.
> >
Andrew, could you elaborate a bit on this (couldn't find anything in
the docs)? AFAIK dcache is a VFS thing, so I don't see how low level
devices should have to deal with it. The VFS should be well aware of
what pages have been submitted for data transfers.
> >
> > Is the dependency on YENTA correct?
> >
>
> Yes. At least noone who wants to enable this driver will forget the yenta
> driver then.
>
But is there a technical dependency? We shouldn't be introducing
artificial dependencies just because of how Ricoh is currently bundling
things.
The actual review:
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/sd.h>
> +#include <linux/mmc/sdio.h>
Including these three in a host driver is always an error. The driver
should be operating at a lower level than the stuff these describe.
> + /* wait for command completion */
> + if (opcode) {
> + for (loop = 0; loop < CMD_TIMEOUT; loop++) {
> + status = sdricoh_readl(host, R21C_STATUS);
> + sdricoh_writel(host, R2E4_STATUS_RESP, status);
> + if (status & STATUS_CMD_FINISHED)
> + break;
> + }
You might want to have a look at making this a bit more compliant with
the spec. A MMC command can take 64 clock cycles plus the transfer time
for the command and response. So you need to figure out how long a
readl takes (as that is your source of delays) and adjust CMD_TIMEOUT
depending on current clock frequency. You can submit a patch for this
later though.
> + dev_dbg(dev, "mmc_cmd opcode=%i arg=0x%x => %i (queries=%i)\n",
> + opcode, arg, result, loop);
The core should be giving you the same output.
> + if (result == 0) {
> + /* EXT_CSD are filtered so this should be save */
> + if (opcode == SD_SEND_IF_COND) {
> + if (host->mode != MODE_SDHC) {
> + dev_info(dev, "switching to SDHC mode\n");
> + host->mode = MODE_SDHC;
> + }
> + }
> +
> + /* switch to SD mode if APP_CMDs are supported */
> + if (opcode == MMC_APP_CMD) {
> + if (host->mode == MODE_MMC) {
> + dev_info(dev, "switching to SD mode\n");
> + host->mode = MODE_SD;
> + }
> + }
> + }
What's the point of this? Looking at the rest of the code, it seems to
be mostly that you haven't properly mapped up the 4-bit bus control
(which is used on MMC cards as well these days).
> +static int sdricoh_blockio(struct sdricoh_host *host, int read,
> + unsigned int *buf)
> +{
> + int i;
> + /* wait until the data is available */
> + if (read) {
> + if (sdricoh_query_status(host, STATUS_READY_TO_READ,
> + TRANSFER_TIMEOUT))
> + return 0;
> + sdricoh_writel(host, R21C_STATUS, 0x18);
> + /* read data */
> + for (i = 0; i < 512 / 4; i++)
> + buf[i] = sdricoh_readl(host, R230_DATA);
Is the controller only capable of 512 byte transfers? I seriously hope
not, but if that's the case then you still need to make sure you fail
transfers that aren't a multiple of 512.
> + /* wait until the tranfer is finished */
> + for (i = 0; i < BUSY_TIMEOUT; i++) {
> + status = sdricoh_readl(host, R21C_STATUS);
> + sdricoh_writel(host, R2E4_STATUS_RESP, status);
> + if (!(status & STATUS_BUSY))
> + break;
> + }
The card can be busy for minutes with some operations, so this is
probably wrong.
> + dev_dbg(dev, "=============================\n");
> + dev_dbg(dev, "sdricoh_request opcode=%i\n", cmd->opcode);
More redundant debug output.
> + /* we cannot handle all commands that require a block transfer
> + therefore do some ugly special handling here
> + */
What kind of analysis have you done here? It's likely that the
controller just needs some special handling, as there is so far only a
single known controller (wbsd) that looks at opcodes.
> + if (cmd->data) {
> + switch (cmd->opcode) {
> + /* working commands */
> + case MMC_READ_SINGLE_BLOCK:
> + case MMC_READ_MULTIPLE_BLOCK:
> + case MMC_WRITE_BLOCK:
> + case MMC_WRITE_MULTIPLE_BLOCK:
> + break;
In any case, you should do what the hardware does and put a list of the
actual integers the hardware looks at. Using the defines implies that
it is those specific opcodes that are a problem (which likely isn't the
case as opcodes are context dependent).
> + case SD_APP_SEND_SCR: /* required for SDHC */
> + cmd->error = sdricoh_mmc_cmd(host, cmd->opcode,
> + cmd->arg);
> + mmc_request_done(mmc, mrq);
> + return;
What are you doing here?
> +
> + /* read/write commands seem to require this */
> + if (data) {
> + cmd->error = sdricoh_busy(host);
> + if (cmd->error)
mmc_block should make sure this isn't a problem, so if you need this
then we have a bug somewhere. Still, you should be waiting for the busy
signal to go away at the end of commands, not compensating for it in
the subsequent one.
> + }
> + dev_dbg(dev, "resp[0]=0x%x\n", cmd->resp[0]);
> + dev_dbg(dev, "resp[1]=0x%x\n", cmd->resp[1]);
> + dev_dbg(dev, "resp[2]=0x%x\n", cmd->resp[2]);
> + dev_dbg(dev, "resp[2]=0x%x\n", cmd->resp[3]);
> + } else {
> + cmd->resp[0] = sdricoh_readl(host, R20C_RESP);
> + dev_dbg(dev, "resp[0]=0x%x\n", cmd->resp[0]);
> + }
More redundant output. There's a few others, but you get the picture...
> + /* yet another workaround */
> + /* without the extra command SD cards do not work at all */
> + if (cmd->opcode == MMC_SELECT_CARD) {
> + if (host->mode != MODE_MMC) {
> + sdricoh_mmc_cmd(host, MMC_APP_CMD, cmd->arg);
> + sdricoh_mmc_cmd(host, 0x46, 0x02);
> + } else {
> + sdricoh_writel(host, R228_POWER, 0xc0e0);
> + sdricoh_writel(host, R224_MODE, 0x2000301);
> + }
> + }
> +
This is probably caused by the fact that you're messing with the SCR
earlier. You're sending a faked ACMD6 (SET_BUS_WIDTH) to the card.
> + buf = kmap(page) + data->sg->offset + (512 * i);
You cannot just assume that the block size is 512 bytes.
> +MODULE_PARM_DESC(switchlocked, "Switch the cards locked status."
> + "Use this when unlocked cards are shown readonly (default 0)");
This doesn't seem to be used anywhere in the code.
You've done a great job figuring out how this controller works, but
there is still a few more steps that need to be taken. The opcode
specific workarounds need to go, so a bit more testing and probing is
necessary. For example bit 0x40 of the mode register looks like a prime
candidate for bus width control.
Rgds
--
-- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org
rdesktop, core developer http://www.rdesktop.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MMC/SD host driver for Ricoh Bay1Controllers
2008-05-12 17:54 ` Pierre Ossman
@ 2008-05-18 12:05 ` Sascha Sommer
2008-05-25 10:46 ` Pierre Ossman
0 siblings, 1 reply; 8+ messages in thread
From: Sascha Sommer @ 2008-05-18 12:05 UTC (permalink / raw)
To: Pierre Ossman; +Cc: Andrew Morton, linux-kernel, sdricohcs-devel
Hi,
thanks for the review. See my comments below.
On Montag, 12. Mai 2008, Pierre Ossman wrote:
> Big thanks to Andrew for helping out with the basic review here. :)
>
> On Sun, 11 May 2008 10:50:55 +0200
>
> Sascha Sommer <saschasommer@freenet.de> wrote:
> > On Freitag, 2. Mai 2008, Andrew Morton wrote:
> > > Will this code work correctly on big-endian machines?
> >
> > Most likely not and there might be other big-endian related problems in
> > the driver. I don't know of a case where this hardware can be found on
> > big endian.
>
> That's not a really good reason to just ignore the problem though.
> Endian safe code is generally more bug free and more easily understood
> because it doesn't use arch specific shortcuts. I won't NAK the patch
> because of this, but I do prefer endian safe code even if there is no
> hardware built that needs it right now. It's always good to practice
> writing such code anyway. :)
>
I will fix that then.
> > > hm. I don't really know how the kernel gets down into here, but I
> > > wonder if this driver (and, I bet, lots of similar ones) should be
> > > doing flush_dcache_page() after modifying the page. If this page can
> > > be file pagecache or user memory then "yes". Unless it's done
> > > elsewhere for us.
>
> Andrew, could you elaborate a bit on this (couldn't find anything in
> the docs)? AFAIK dcache is a VFS thing, so I don't see how low level
> devices should have to deal with it. The VFS should be well aware of
> what pages have been submitted for data transfers.
>
> > > Is the dependency on YENTA correct?
> >
> > Yes. At least noone who wants to enable this driver will forget the yenta
> > driver then.
>
> But is there a technical dependency? We shouldn't be introducing
> artificial dependencies just because of how Ricoh is currently bundling
> things.
>
The driver should compile without it and it should not crash without it.
However it will never get activated without a driver for the yenta bridge.
> The actual review:
> > +#include <linux/mmc/mmc.h>
> > +#include <linux/mmc/sd.h>
> > +#include <linux/mmc/sdio.h>
>
> Including these three in a host driver is always an error. The driver
> should be operating at a lower level than the stuff these describe.
>
That is the point where I'm not sure about. While I agree that the driver
theoretically shouldn't care about the commands it currently cannot handle
all commands that read or write a data block. In this context the commands
itself are the problem and not some integer values.
> > + /* wait for command completion */
> > + if (opcode) {
> > + for (loop = 0; loop < CMD_TIMEOUT; loop++) {
> > + status = sdricoh_readl(host, R21C_STATUS);
> > + sdricoh_writel(host, R2E4_STATUS_RESP, status);
> > + if (status & STATUS_CMD_FINISHED)
> > + break;
> > + }
>
> You might want to have a look at making this a bit more compliant with
> the spec. A MMC command can take 64 clock cycles plus the transfer time
> for the command and response. So you need to figure out how long a
> readl takes (as that is your source of delays) and adjust CMD_TIMEOUT
> depending on current clock frequency. You can submit a patch for this
> later though.
>
I will try to fix that with a later patch.
> > + dev_dbg(dev, "mmc_cmd opcode=%i arg=0x%x => %i (queries=%i)\n",
> > + opcode, arg, result, loop);
>
> The core should be giving you the same output.
>
> > + if (result == 0) {
> > + /* EXT_CSD are filtered so this should be save */
> > + if (opcode == SD_SEND_IF_COND) {
> > + if (host->mode != MODE_SDHC) {
> > + dev_info(dev, "switching to SDHC mode\n");
> > + host->mode = MODE_SDHC;
> > + }
> > + }
> > +
> > + /* switch to SD mode if APP_CMDs are supported */
> > + if (opcode == MMC_APP_CMD) {
> > + if (host->mode == MODE_MMC) {
> > + dev_info(dev, "switching to SD mode\n");
> > + host->mode = MODE_SD;
> > + }
> > + }
> > + }
>
> What's the point of this? Looking at the rest of the code, it seems to
> be mostly that you haven't properly mapped up the 4-bit bus control
> (which is used on MMC cards as well these days).
>
Hm it seems like you are right here. I was confused by the opcode |= 64 and
the fact that the scr command never really worked so linux would never
activate the 4-bit bus mode. Windows does this unconditionally for SD cards.
It also looks like all ACMDS have this |= 64
> > +static int sdricoh_blockio(struct sdricoh_host *host, int read,
> > + unsigned int *buf)
> > +{
> > + int i;
> > + /* wait until the data is available */
> > + if (read) {
> > + if (sdricoh_query_status(host, STATUS_READY_TO_READ,
> > + TRANSFER_TIMEOUT))
> > + return 0;
> > + sdricoh_writel(host, R21C_STATUS, 0x18);
> > + /* read data */
> > + for (i = 0; i < 512 / 4; i++)
> > + buf[i] = sdricoh_readl(host, R230_DATA);
>
> Is the controller only capable of 512 byte transfers? I seriously hope
> not, but if that's the case then you still need to make sure you fail
> transfers that aren't a multiple of 512.
>
I think I found the register now where one can adjust the transfer size but
this requires a lot more testing.
> > + /* wait until the tranfer is finished */
> > + for (i = 0; i < BUSY_TIMEOUT; i++) {
> > + status = sdricoh_readl(host, R21C_STATUS);
> > + sdricoh_writel(host, R2E4_STATUS_RESP, status);
> > + if (!(status & STATUS_BUSY))
> > + break;
> > + }
>
> The card can be busy for minutes with some operations, so this is
> probably wrong.
>
What do you propose instead? Moving this code into a seperate thread?
> > + dev_dbg(dev, "=============================\n");
> > + dev_dbg(dev, "sdricoh_request opcode=%i\n", cmd->opcode);
>
> More redundant debug output.
>
> > + /* we cannot handle all commands that require a block transfer
> > + therefore do some ugly special handling here
> > + */
>
> What kind of analysis have you done here? It's likely that the
> controller just needs some special handling, as there is so far only a
> single known controller (wbsd) that looks at opcodes.
>
See below.
> > + if (cmd->data) {
> > + switch (cmd->opcode) {
> > + /* working commands */
> > + case MMC_READ_SINGLE_BLOCK:
> > + case MMC_READ_MULTIPLE_BLOCK:
> > + case MMC_WRITE_BLOCK:
> > + case MMC_WRITE_MULTIPLE_BLOCK:
> > + break;
>
> In any case, you should do what the hardware does and put a list of the
> actual integers the hardware looks at. Using the defines implies that
> it is those specific opcodes that are a problem (which likely isn't the
> case as opcodes are context dependent).
>From my point of view this is such an unlikely case. From what I currently
know all opcodes that are required to read a data block and that are not
listed above very likely do not work. When I for example let the EXT_CSD
command through this filter the command itself will succeed but the bit that
notifies us if the data is available for read does not get set. The status
doesn't change. If I leave out the checks and just read from the data
register only zeros are returned.
Note that the windows driver does not use any of these commands.
Do you know of a case where a broken card caused such a behaviour?
Unfortunatelly I do not have another card reader or other MMC cards to test.
>
> > + case SD_APP_SEND_SCR: /* required for SDHC */
> > + cmd->error = sdricoh_mmc_cmd(host, cmd->opcode,
> > + cmd->arg);
> > + mmc_request_done(mmc, mrq);
> > + return;
>
> What are you doing here?
>
Well that is one of these extra special handling hacks. While leaving out the
SEND_SCR command for my SD card makes no difference a user reported that his
SDHC card won't work without it. In any case the MMC layer requires that this
command succeeds but reading the data block doesn't work. When I do a |= 64
like for the BUS_WIDTH command I can read something but I'm not sure yet if
this is the SCR or some random values. When I need to do the |= 64 I also
would need to know for every command if this they are an ACMD or not. I think
it would be nice if the mmc layer could give a hint here.
> > +
> > + /* read/write commands seem to require this */
> > + if (data) {
> > + cmd->error = sdricoh_busy(host);
> > + if (cmd->error)
>
> mmc_block should make sure this isn't a problem, so if you need this
> then we have a bug somewhere. Still, you should be waiting for the busy
> signal to go away at the end of commands, not compensating for it in
> the subsequent one.
>
This is some cruft from the early days.
> > + }
> > + dev_dbg(dev, "resp[0]=0x%x\n", cmd->resp[0]);
> > + dev_dbg(dev, "resp[1]=0x%x\n", cmd->resp[1]);
> > + dev_dbg(dev, "resp[2]=0x%x\n", cmd->resp[2]);
> > + dev_dbg(dev, "resp[2]=0x%x\n", cmd->resp[3]);
> > + } else {
> > + cmd->resp[0] = sdricoh_readl(host, R20C_RESP);
> > + dev_dbg(dev, "resp[0]=0x%x\n", cmd->resp[0]);
> > + }
>
> More redundant output. There's a few others, but you get the picture...
>
What would you say if I want to keep them ;)? For testing and debugging I find
it a lot easier to just play with some local debug statements instead of
checking back with the mmc framework when what values gets printed etc.
> > + /* yet another workaround */
> > + /* without the extra command SD cards do not work at all */
> > + if (cmd->opcode == MMC_SELECT_CARD) {
> > + if (host->mode != MODE_MMC) {
> > + sdricoh_mmc_cmd(host, MMC_APP_CMD, cmd->arg);
> > + sdricoh_mmc_cmd(host, 0x46, 0x02);
> > + } else {
> > + sdricoh_writel(host, R228_POWER, 0xc0e0);
> > + sdricoh_writel(host, R224_MODE, 0x2000301);
> > + }
> > + }
> > +
>
> This is probably caused by the fact that you're messing with the SCR
> earlier. You're sending a faked ACMD6 (SET_BUS_WIDTH) to the card.
>
I think this can be avoided now.
> > + buf = kmap(page) + data->sg->offset + (512 * i);
>
> You cannot just assume that the block size is 512 bytes.
>
> > +MODULE_PARM_DESC(switchlocked, "Switch the cards locked status."
> > + "Use this when unlocked cards are shown readonly (default 0)");
>
> This doesn't seem to be used anywhere in the code.
>
It is. See sdricoh_get_ro(). There also seem to be different versions of the
window driver for different notebooks that compensate for this behaviour...
Regards
Sascha
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MMC/SD host driver for Ricoh Bay1Controllers
2008-05-18 12:05 ` Sascha Sommer
@ 2008-05-25 10:46 ` Pierre Ossman
2008-05-25 10:54 ` Pierre Ossman
0 siblings, 1 reply; 8+ messages in thread
From: Pierre Ossman @ 2008-05-25 10:46 UTC (permalink / raw)
To: Sascha Sommer; +Cc: Andrew Morton, linux-kernel, sdricohcs-devel
On Sun, 18 May 2008 14:05:01 +0200
Sascha Sommer <saschasommer@freenet.de> wrote:
>
> The driver should compile without it and it should not crash without it.
> However it will never get activated without a driver for the yenta bridge.
>
But does it have to be a yenta bridge? Would not the driver work fine
if Ricoh added it to something with another cardbus interface?
>
> > The actual review:
> > > +#include <linux/mmc/mmc.h>
> > > +#include <linux/mmc/sd.h>
> > > +#include <linux/mmc/sdio.h>
> >
> > Including these three in a host driver is always an error. The driver
> > should be operating at a lower level than the stuff these describe.
> >
>
> That is the point where I'm not sure about. While I agree that the driver
> theoretically shouldn't care about the commands it currently cannot handle
> all commands that read or write a data block. In this context the commands
> itself are the problem and not some integer values.
>
That isn't really possible as the hardware would have to be extremely
complex to figure out which command is being issued (it would have to
emulate the card state machine for every possible card type).
More realistically, the hardware looks at one of two things; the opcode
integer (rare), or some property of the command (e.g. "long reply" or
"busy signal"). Neither require the above headers.
> >
> > What's the point of this? Looking at the rest of the code, it seems to
> > be mostly that you haven't properly mapped up the 4-bit bus control
> > (which is used on MMC cards as well these days).
> >
>
> Hm it seems like you are right here. I was confused by the opcode |= 64 and
> the fact that the scr command never really worked so linux would never
> activate the 4-bit bus mode. Windows does this unconditionally for SD cards.
> It also looks like all ACMDS have this |= 64
>
Windows drivers tend to make lots and lots of assumptions, which makes
them a bit annoying to trace. :/
> >
> > Is the controller only capable of 512 byte transfers? I seriously hope
> > not, but if that's the case then you still need to make sure you fail
> > transfers that aren't a multiple of 512.
> >
>
> I think I found the register now where one can adjust the transfer size but
> this requires a lot more testing.
>
This can also be added later if you like. Just as long as you add some
safe guards to make sure it doesn't foul up the system on other sizes.
> > > + /* wait until the tranfer is finished */
> > > + for (i = 0; i < BUSY_TIMEOUT; i++) {
> > > + status = sdricoh_readl(host, R21C_STATUS);
> > > + sdricoh_writel(host, R2E4_STATUS_RESP, status);
> > > + if (!(status & STATUS_BUSY))
> > > + break;
> > > + }
> >
> > The card can be busy for minutes with some operations, so this is
> > probably wrong.
> >
>
> What do you propose instead? Moving this code into a seperate thread?
>
I didn't really check where this was called, but if you have process
context then you could just sleep. Or have a workqueue that you keep
rescheduling.
For now, you can just return success instead of timeout (many other
drivers forget to wait for busy) and make a note to fix this later. The
upper layers are forgiving about leaving the card in a busy state.
(It is a bug to do so though, so it should be fixed eventually)
> > > + if (cmd->data) {
> > > + switch (cmd->opcode) {
> > > + /* working commands */
> > > + case MMC_READ_SINGLE_BLOCK:
> > > + case MMC_READ_MULTIPLE_BLOCK:
> > > + case MMC_WRITE_BLOCK:
> > > + case MMC_WRITE_MULTIPLE_BLOCK:
> > > + break;
> >
> > In any case, you should do what the hardware does and put a list of the
> > actual integers the hardware looks at. Using the defines implies that
> > it is those specific opcodes that are a problem (which likely isn't the
> > case as opcodes are context dependent).
>
> From my point of view this is such an unlikely case. From what I currently
> know all opcodes that are required to read a data block and that are not
> listed above very likely do not work. When I for example let the EXT_CSD
> command through this filter the command itself will succeed but the bit that
> notifies us if the data is available for read does not get set. The status
> doesn't change. If I leave out the checks and just read from the data
> register only zeros are returned.
My [somewhat unclear] point is that the problem is not with the
commands themselves, but with some property specific to the failing
commands. So you need to compare the details of successful commands
with failing ones and figure out which of the differences is causing
the problem. Then you start coding logic around that property, not the
command.
> Note that the windows driver does not use any of these commands.
I guess it doesn't properly support MMC in that case. Not that unusual.
> Do you know of a case where a broken card caused such a behaviour?
Not that I can remember, no.
> > > + case SD_APP_SEND_SCR: /* required for SDHC */
> > > + cmd->error = sdricoh_mmc_cmd(host, cmd->opcode,
> > > + cmd->arg);
> > > + mmc_request_done(mmc, mrq);
> > > + return;
> >
> > What are you doing here?
> >
>
> Well that is one of these extra special handling hacks. While leaving out the
> SEND_SCR command for my SD card makes no difference a user reported that his
> SDHC card won't work without it. In any case the MMC layer requires that this
> command succeeds but reading the data block doesn't work. When I do a |= 64
> like for the BUS_WIDTH command I can read something but I'm not sure yet if
> this is the SCR or some random values. When I need to do the |= 64 I also
> would need to know for every command if this they are an ACMD or not. I think
> it would be nice if the mmc layer could give a hint here.
>
There is no difference between a ACMD and a CMD from a hardware point
of view, so the core isn't exporting that information. In other words,
that can't be the problem.
The SCR is read while the card is still in 1-bit mode, which is the
most likely source of problems. I suggest looking at what the Windows
driver does for MMC cards (who were 1-bit only until 4.0).
> >
> > More redundant output. There's a few others, but you get the picture...
> >
>
> What would you say if I want to keep them ;)? For testing and debugging I find
> it a lot easier to just play with some local debug statements instead of
> checking back with the mmc framework when what values gets printed etc.
>
I won't NAK it, if that's what you're asking. But you should really
consider making the most of the core stuff. Your debug statements will
be unfamiliar to other mmc developers, so people will not be able to
help you out as easily when you (or your users) post dumps.
> > > +MODULE_PARM_DESC(switchlocked, "Switch the cards locked status."
> > > + "Use this when unlocked cards are shown readonly (default 0)");
> >
> > This doesn't seem to be used anywhere in the code.
> >
>
> It is. See sdricoh_get_ro(). There also seem to be different versions of the
> window driver for different notebooks that compensate for this behaviour...
>
Ah. I somehow missed that. Never mind then.
Rgds
--
-- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org
rdesktop, core developer http://www.rdesktop.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MMC/SD host driver for Ricoh Bay1Controllers
2008-05-25 10:46 ` Pierre Ossman
@ 2008-05-25 10:54 ` Pierre Ossman
2008-05-25 12:33 ` Sascha Sommer
0 siblings, 1 reply; 8+ messages in thread
From: Pierre Ossman @ 2008-05-25 10:54 UTC (permalink / raw)
To: Sascha Sommer; +Cc: Andrew Morton, linux-kernel, sdricohcs-devel
Just to be clear, the only thing holding this patch back is the
reliance on opcodes. Once that can be fixed then I'll merge the driver.
(And by fix I don't mean "make those opcodes work", I mean "figure out
the command properties instead of lists of opcodes")
Rgds
--
-- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org
rdesktop, core developer http://www.rdesktop.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MMC/SD host driver for Ricoh Bay1Controllers
2008-05-25 10:54 ` Pierre Ossman
@ 2008-05-25 12:33 ` Sascha Sommer
0 siblings, 0 replies; 8+ messages in thread
From: Sascha Sommer @ 2008-05-25 12:33 UTC (permalink / raw)
To: Pierre Ossman; +Cc: Andrew Morton, linux-kernel, sdricohcs-devel
Hi,
On Sonntag, 25. Mai 2008, Pierre Ossman wrote:
> Just to be clear, the only thing holding this patch back is the
> reliance on opcodes. Once that can be fixed then I'll merge the driver.
>
> (And by fix I don't mean "make those opcodes work", I mean "figure out
> the command properties instead of lists of opcodes")
>
I think I was able to fix most of the problems you pointed out in the other
mail. There still is some |=64 for ACMDs. The SEND_SCR cmd doesn't work with
out it. I asked some users to test the new version and will send a new patch
after Linuxtag. Right now the code can be found at
http://sdricohcs.svn.sourceforge.net/viewvc/sdricohcs/sdricoh_cs/sdricoh_cs.c?revision=71&view=markup
The busy signal handling is not fixed yet and I think something similar will
also be required for the test that checks when the data has been read from
the card so that the driver does not waste to many cpu cycles.
Regards
Sascha
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2008-05-25 12:33 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-04-27 14:17 [PATCH] MMC/SD host driver for Ricoh Bay1Controllers Sascha Sommer
2008-05-01 23:39 ` Andrew Morton
2008-05-11 8:50 ` Sascha Sommer
2008-05-12 17:54 ` Pierre Ossman
2008-05-18 12:05 ` Sascha Sommer
2008-05-25 10:46 ` Pierre Ossman
2008-05-25 10:54 ` Pierre Ossman
2008-05-25 12:33 ` Sascha Sommer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome