| Message ID | 1463727743-7567-2-git-send-email-horms+renesas@verge.net.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Simon Thank you for your patch. Picky comment from me :) > From: Ai Kyuse <ai.kyuse.uw@renesas.com> > > Add tuning support for use with SDR104 mode > > Signed-off-by: Ai Kyuse <ai.kyuse.uw@renesas.com> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > --- (snip) > @@ -160,6 +161,11 @@ struct tmio_mmc_host { > unsigned int direction, int blk_size); > int (*start_signal_voltage_switch)(struct mmc_host *mmc, > struct mmc_ios *ios); > + unsigned int (*init_tuning)(struct tmio_mmc_host *host); > + int (*prepare_tuning)(struct tmio_mmc_host *host, unsigned long tap); > + int (*select_tuning)(struct tmio_mmc_host *host, bool *tap); > + bool (*retuning)(struct tmio_mmc_host *host); > + void (*hw_reset)(struct tmio_mmc_host *host); > }; There is no (*retuning) code ? > +static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode) > +{ > + struct tmio_mmc_host *host = mmc_priv(mmc); > + unsigned int num = 0; > + int i, ret = 0; > + bool *tap; > + > + if (host->init_tuning) > + num = host->init_tuning(host); > + if (!num) { > + /* Tuning is not supported */ > + ret = 0; > + goto out; > + } > + > + tap = kmalloc(num * 2, GFP_KERNEL); > + if (tap == NULL) { > + ret = -ENOMEM; > + goto out; > + } if (!tap) { ... } > + /* > + * Issue CMD19 twice for each tap > + */ > + for (i = 0; i < 2 * num; i++) { > + int err; > + > + if (host->prepare_tuning) > + host->prepare_tuning(host, i); you want to check return value here ? int (*prepare_tuning)(xxx); > + err = mmc_send_tuning(host->mmc, opcode, NULL); > + if (err && err != -EILSEQ) { > + ret = err; > + goto err_free; > + } > + tap[i] = (err == 0); > + > + mdelay(1); > + } > + > + if (host->select_tuning) > + ret = host->select_tuning(host, tap); Here, "tap" was alloc:ed array, but there is no array_size parameter. A littile bit strange for me..., but I'm not sure Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 23, 2016 at 12:53:59AM +0000, Kuninori Morimoto wrote: > > Hi Simon > > Thank you for your patch. > Picky comment from me :) Thanks for noticing these things and sorry for not having done so myself. > > From: Ai Kyuse <ai.kyuse.uw@renesas.com> > > > > Add tuning support for use with SDR104 mode > > > > Signed-off-by: Ai Kyuse <ai.kyuse.uw@renesas.com> > > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > > --- > (snip) > > @@ -160,6 +161,11 @@ struct tmio_mmc_host { > > unsigned int direction, int blk_size); > > int (*start_signal_voltage_switch)(struct mmc_host *mmc, > > struct mmc_ios *ios); > > + unsigned int (*init_tuning)(struct tmio_mmc_host *host); > > + int (*prepare_tuning)(struct tmio_mmc_host *host, unsigned long tap); > > + int (*select_tuning)(struct tmio_mmc_host *host, bool *tap); > > + bool (*retuning)(struct tmio_mmc_host *host); > > + void (*hw_reset)(struct tmio_mmc_host *host); > > }; > > There is no (*retuning) code ? Thanks, I think returning should simply be removed. > > +static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode) > > +{ > > + struct tmio_mmc_host *host = mmc_priv(mmc); > > + unsigned int num = 0; > > + int i, ret = 0; > > + bool *tap; > > + > > + if (host->init_tuning) > > + num = host->init_tuning(host); > > + if (!num) { > > + /* Tuning is not supported */ > > + ret = 0; > > + goto out; > > + } > > + > > + tap = kmalloc(num * 2, GFP_KERNEL); > > + if (tap == NULL) { > > + ret = -ENOMEM; > > + goto out; > > + } > > if (!tap) { > ... > } ok > > + /* > > + * Issue CMD19 twice for each tap > > + */ > > + for (i = 0; i < 2 * num; i++) { > > + int err; > > + > > + if (host->prepare_tuning) > > + host->prepare_tuning(host, i); > > you want to check return value here ? > int (*prepare_tuning)(xxx); The implementation of prepare_tuning doesn't seem to ever return an error. Perhaps it would be best to just change the type to: void (*prepare_tuning)(xxx); > > + err = mmc_send_tuning(host->mmc, opcode, NULL); > > + if (err && err != -EILSEQ) { > > + ret = err; > > + goto err_free; > > + } > > + tap[i] = (err == 0); > > + > > + mdelay(1); > > + } > > + > > + if (host->select_tuning) > > + ret = host->select_tuning(host, tap); > > Here, "tap" was alloc:ed array, but there is no array_size parameter. > A littile bit strange for me..., but I'm not sure Right. I think its safe. But its not very clean. I will add a size parameter. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index 1aac2ad8edf2..b2168370af22 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -150,6 +150,7 @@ struct tmio_mmc_host { struct mutex ios_lock; /* protect set_ios() context */ bool native_hotplug; bool sdio_irq_enabled; + u32 scc_tappos; int (*write16_hook)(struct tmio_mmc_host *host, int addr); int (*clk_enable)(struct tmio_mmc_host *host); @@ -160,6 +161,11 @@ struct tmio_mmc_host { unsigned int direction, int blk_size); int (*start_signal_voltage_switch)(struct mmc_host *mmc, struct mmc_ios *ios); + unsigned int (*init_tuning)(struct tmio_mmc_host *host); + int (*prepare_tuning)(struct tmio_mmc_host *host, unsigned long tap); + int (*select_tuning)(struct tmio_mmc_host *host, bool *tap); + bool (*retuning)(struct tmio_mmc_host *host); + void (*hw_reset)(struct tmio_mmc_host *host); }; struct tmio_mmc_host *tmio_mmc_host_alloc(struct platform_device *pdev); diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c index 12dc0440a487..e20f6522c9f9 100644 --- a/drivers/mmc/host/tmio_mmc_pio.c +++ b/drivers/mmc/host/tmio_mmc_pio.c @@ -36,6 +36,7 @@ #include <linux/io.h> #include <linux/irq.h> #include <linux/mfd/tmio.h> +#include <linux/mmc/card.h> #include <linux/mmc/host.h> #include <linux/mmc/mmc.h> #include <linux/mmc/slot-gpio.h> @@ -363,7 +364,8 @@ static int tmio_mmc_start_command(struct tmio_mmc_host *host, struct mmc_command * multiple block transfer */ if ((host->pdata->flags & TMIO_MMC_HAVE_CMD12_CTRL) && - (cmd->opcode == SD_IO_RW_EXTENDED)) + ((cmd->opcode == SD_IO_RW_EXTENDED) || + host->mrq->sbc)) c |= NO_CMD12_ISSUE; } if (data->flags & MMC_DATA_READ) @@ -756,6 +758,74 @@ static int tmio_mmc_start_data(struct tmio_mmc_host *host, return 0; } +static void tmio_mmc_hw_reset(struct mmc_host *mmc) +{ + struct tmio_mmc_host *host = mmc_priv(mmc); + + if (host->hw_reset) + host->hw_reset(host); + + mmc_retune_timer_stop(host->mmc); + mmc_retune_needed(host->mmc); +} + +static int tmio_mmc_execute_tuning(struct mmc_host *mmc, u32 opcode) +{ + struct tmio_mmc_host *host = mmc_priv(mmc); + unsigned int num = 0; + int i, ret = 0; + bool *tap; + + if (host->init_tuning) + num = host->init_tuning(host); + if (!num) { + /* Tuning is not supported */ + ret = 0; + goto out; + } + + tap = kmalloc(num * 2, GFP_KERNEL); + if (tap == NULL) { + ret = -ENOMEM; + goto out; + } + + /* + * Issue CMD19 twice for each tap + */ + for (i = 0; i < 2 * num; i++) { + int err; + + if (host->prepare_tuning) + host->prepare_tuning(host, i); + + err = mmc_send_tuning(host->mmc, opcode, NULL); + if (err && err != -EILSEQ) { + ret = err; + goto err_free; + } + tap[i] = (err == 0); + + mdelay(1); + } + + if (host->select_tuning) + ret = host->select_tuning(host, tap); + +err_free: + kfree(tap); +out: + if (ret < 0) { + dev_warn(&host->pdev->dev, "Tuning procedure failed\n"); + tmio_mmc_hw_reset(mmc); + } else { + host->mmc->retune_period = 0; + } + + return ret; + +} + /* Process requests from the MMC layer */ static void tmio_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) { @@ -781,6 +851,14 @@ static void tmio_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq) spin_unlock_irqrestore(&host->lock, flags); + if (mrq->sbc) { + ret = tmio_mmc_start_command(host, mrq->sbc); + if (ret) + goto fail; + host->last_req_ts = jiffies; + host->mrq = mrq; + } + if (mrq->data) { ret = tmio_mmc_start_data(host, mrq->data); if (ret) @@ -978,6 +1056,8 @@ static struct mmc_host_ops tmio_mmc_ops = { .enable_sdio_irq = tmio_mmc_enable_sdio_irq, .card_busy = tmio_mmc_card_busy, .multi_io_quirk = tmio_multi_io_quirk, + .execute_tuning = tmio_mmc_execute_tuning, + .hw_reset = tmio_mmc_hw_reset, }; static int tmio_mmc_init_ocr(struct tmio_mmc_host *host) @@ -1202,6 +1282,9 @@ int tmio_mmc_host_runtime_suspend(struct device *dev) struct mmc_host *mmc = dev_get_drvdata(dev); struct tmio_mmc_host *host = mmc_priv(mmc); + mmc_retune_timer_stop(host->mmc); + mmc_retune_needed(host->mmc); + tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL); if (host->clk_cache)