| Message ID | 20180307132020.30951-1-kishon@ti.com |
|---|---|
| Headers | show |
| Series | mmc: sdhci-omap: Add UHS/HS200 mode support | expand |
* Kishon Vijay Abraham I <kishon@ti.com> [180307 13:21]: > Changes from v2: > *) Used soc_device_match() instead of pdata-quirks as per Tony's > suggestions. Looks good to me now thanks. For the whole series: Acked-by: Tony Lindgren <tony@atomide.com>
On 7 March 2018 at 14:20, Kishon Vijay Abraham I <kishon@ti.com> wrote: > Add UHS/HS200 mode support in sdhci-omap. The programming sequence > for voltage switching, tuning is followed from AM572x TRM > http://www.ti.com/lit/ug/spruhz6i/spruhz6i.pdf > (Similar to all AM57x/DRA7x SoCs). The patch series also implements > workaround for errata published in > http://www.ti.com/lit/er/sprz429k/sprz429k.pdf > > patches are created on top of > git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git next > > Patches in v2 already applied to mmc -next are dropped from the > series. > > Changes from v2: > *) Changed SW timeout logic as per Adrians's suggestion > *) Validated SDIO with the current patches (added a couple of fixes > found while adding SDIO support). > *) Used soc_device_match() instead of pdata-quirks as per Tony's > suggestions. > > Changes from v1: > *) Only poll on DAT0 and DATI for card_busy status > *) Cleanup iodelay patch as suggested by Tony. > *) Added quirk to disable HW timeout > *) Use the existing data timer but program a relatively accurate > SW timeout value (Impacts all platforms) > *) Fix a bug in sdhci which was using data_timer for non data line > commands > > Kishon Vijay Abraham I (11): > mmc: sdhci-omap: Fix when capabilities are obtained from > SDHCI_CAPABILITIES reg > mmc: sdhci-omap: Remove setting ADMA capability in driver > mmc: sdhci-omap: Workaround for Errata i843 > mmc: sdhci-omap: Invoke sdhci_get_of_property to read sdhci dt > properties > mmc: sdhci: Disable HS200 mode if controller can't support 1.8v > mmc: sdhci: Add quirk to disable HW timeout > mmc: sdhci: Program a relatively accurate SW timeout value > mmc: sdhci-omap: Workaround for Errata i834 > dt-bindings: sdhci-omap: Add K2G specific binding > mmc: sdhci-omap: Add support for MMC/SD controller in k2g SoC > mmc: sdhci-omap: Add sdhci_omap specific ops for enable_sdio_irq > > .../devicetree/bindings/mmc/sdhci-omap.txt | 2 + > drivers/mmc/host/sdhci-omap.c | 78 +++++++++++++++++++--- > drivers/mmc/host/sdhci.c | 75 +++++++++++++++++---- > drivers/mmc/host/sdhci.h | 15 +++++ > 4 files changed, 148 insertions(+), 22 deletions(-) > > -- > 2.11.0 > Overall this looks good to me, however it seems like we need to give Adrian a little more time to comment. Kind regards Uffe
On 15/03/18 10:47, Ulf Hansson wrote: > On 7 March 2018 at 14:20, Kishon Vijay Abraham I <kishon@ti.com> wrote: >> Add UHS/HS200 mode support in sdhci-omap. The programming sequence >> for voltage switching, tuning is followed from AM572x TRM >> http://www.ti.com/lit/ug/spruhz6i/spruhz6i.pdf >> (Similar to all AM57x/DRA7x SoCs). The patch series also implements >> workaround for errata published in >> http://www.ti.com/lit/er/sprz429k/sprz429k.pdf >> >> patches are created on top of >> git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git next >> >> Patches in v2 already applied to mmc -next are dropped from the >> series. >> >> Changes from v2: >> *) Changed SW timeout logic as per Adrians's suggestion >> *) Validated SDIO with the current patches (added a couple of fixes >> found while adding SDIO support). >> *) Used soc_device_match() instead of pdata-quirks as per Tony's >> suggestions. >> >> Changes from v1: >> *) Only poll on DAT0 and DATI for card_busy status >> *) Cleanup iodelay patch as suggested by Tony. >> *) Added quirk to disable HW timeout >> *) Use the existing data timer but program a relatively accurate >> SW timeout value (Impacts all platforms) >> *) Fix a bug in sdhci which was using data_timer for non data line >> commands >> >> Kishon Vijay Abraham I (11): >> mmc: sdhci-omap: Fix when capabilities are obtained from >> SDHCI_CAPABILITIES reg >> mmc: sdhci-omap: Remove setting ADMA capability in driver >> mmc: sdhci-omap: Workaround for Errata i843 >> mmc: sdhci-omap: Invoke sdhci_get_of_property to read sdhci dt >> properties >> mmc: sdhci: Disable HS200 mode if controller can't support 1.8v >> mmc: sdhci: Add quirk to disable HW timeout >> mmc: sdhci: Program a relatively accurate SW timeout value >> mmc: sdhci-omap: Workaround for Errata i834 >> dt-bindings: sdhci-omap: Add K2G specific binding >> mmc: sdhci-omap: Add support for MMC/SD controller in k2g SoC >> mmc: sdhci-omap: Add sdhci_omap specific ops for enable_sdio_irq >> >> .../devicetree/bindings/mmc/sdhci-omap.txt | 2 + >> drivers/mmc/host/sdhci-omap.c | 78 +++++++++++++++++++--- >> drivers/mmc/host/sdhci.c | 75 +++++++++++++++++---- >> drivers/mmc/host/sdhci.h | 15 +++++ >> 4 files changed, 148 insertions(+), 22 deletions(-) >> >> -- >> 2.11.0 >> > > Overall this looks good to me, however it seems like we need to give > Adrian a little more time to comment. > I will try to look at it today.
On Thursday 15 March 2018 02:22 PM, Adrian Hunter wrote: > On 15/03/18 10:47, Ulf Hansson wrote: >> On 7 March 2018 at 14:20, Kishon Vijay Abraham I <kishon@ti.com> wrote: >>> Add UHS/HS200 mode support in sdhci-omap. The programming sequence >>> for voltage switching, tuning is followed from AM572x TRM >>> http://www.ti.com/lit/ug/spruhz6i/spruhz6i.pdf >>> (Similar to all AM57x/DRA7x SoCs). The patch series also implements >>> workaround for errata published in >>> http://www.ti.com/lit/er/sprz429k/sprz429k.pdf >>> >>> patches are created on top of >>> git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git next >>> >>> Patches in v2 already applied to mmc -next are dropped from the >>> series. >>> >>> Changes from v2: >>> *) Changed SW timeout logic as per Adrians's suggestion >>> *) Validated SDIO with the current patches (added a couple of fixes >>> found while adding SDIO support). >>> *) Used soc_device_match() instead of pdata-quirks as per Tony's >>> suggestions. >>> >>> Changes from v1: >>> *) Only poll on DAT0 and DATI for card_busy status >>> *) Cleanup iodelay patch as suggested by Tony. >>> *) Added quirk to disable HW timeout >>> *) Use the existing data timer but program a relatively accurate >>> SW timeout value (Impacts all platforms) >>> *) Fix a bug in sdhci which was using data_timer for non data line >>> commands >>> >>> Kishon Vijay Abraham I (11): >>> mmc: sdhci-omap: Fix when capabilities are obtained from >>> SDHCI_CAPABILITIES reg >>> mmc: sdhci-omap: Remove setting ADMA capability in driver >>> mmc: sdhci-omap: Workaround for Errata i843 >>> mmc: sdhci-omap: Invoke sdhci_get_of_property to read sdhci dt >>> properties >>> mmc: sdhci: Disable HS200 mode if controller can't support 1.8v >>> mmc: sdhci: Add quirk to disable HW timeout >>> mmc: sdhci: Program a relatively accurate SW timeout value >>> mmc: sdhci-omap: Workaround for Errata i834 >>> dt-bindings: sdhci-omap: Add K2G specific binding >>> mmc: sdhci-omap: Add support for MMC/SD controller in k2g SoC >>> mmc: sdhci-omap: Add sdhci_omap specific ops for enable_sdio_irq >>> >>> .../devicetree/bindings/mmc/sdhci-omap.txt | 2 + >>> drivers/mmc/host/sdhci-omap.c | 78 +++++++++++++++++++--- >>> drivers/mmc/host/sdhci.c | 75 +++++++++++++++++---- >>> drivers/mmc/host/sdhci.h | 15 +++++ >>> 4 files changed, 148 insertions(+), 22 deletions(-) >>> >>> -- >>> 2.11.0 >>> >> >> Overall this looks good to me, however it seems like we need to give >> Adrian a little more time to comment. >> > > I will try to look at it today. sure, thanks! -Kishon
On 07/03/18 15:20, Kishon Vijay Abraham I wrote: > Though MMC controller can indicate HS200 mode capability (by > using "mmc-hs200-1_8v" dt property), if the IO lines in the board > is connected to 3.3v supply, HS200 mode cannot be supported. > Such boards have "no-1-8-v" property in their dts file. Disable HS200 > mode for boards which have "no-1-8-v" set. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/mmc/host/sdhci.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 2ededa7f43df..aa8b5ca0d1b0 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -3672,6 +3672,7 @@ int sdhci_setup_host(struct sdhci_host *host) > if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) { > host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 | > SDHCI_SUPPORT_DDR50); > + mmc->caps2 &= ~MMC_CAP2_HS200; Pedantically, shouldn't that be: ~(MMC_CAP2_HS200_1_8V_SDR | MMC_CAP2_HS400_1_8V) We have MMC_CAP2_HSX00_1_2V so maybe we should add: #define MMC_CAP2_HSX00_1_8V (MMC_CAP2_HS200_1_8V_SDR | MMC_CAP2_HS400_1_8V) And then mmc->caps2 &= ~MMC_CAP2_HSX00_1_8V; > } > > /* Any UHS-I mode in caps implies SDR12 and SDR25 support. */ >
On 07/03/18 15:20, Kishon Vijay Abraham I wrote: > sdhci has a 10 second timeout to catch devices that stop responding. > Instead of programming 10 second arbitrary value, calculate the total time > it would take for the entire transfer to happen and program the timeout > value accordingly. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/mmc/host/sdhci.c | 47 ++++++++++++++++++++++++++++++++++++++++------- > drivers/mmc/host/sdhci.h | 10 ++++++++++ > 2 files changed, 50 insertions(+), 7 deletions(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 1dd117cbeb6e..baab67bfa39b 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host) > return sg_dma_address(host->data->sg); > } > > +static void sdhci_calc_sw_timeout(struct sdhci_host *host, > + struct mmc_command *cmd, > + unsigned int target_timeout) > +{ > + struct mmc_data *data = cmd->data; > + struct mmc_host *mmc = host->mmc; > + u64 transfer_time; > + struct mmc_ios *ios = &mmc->ios; > + unsigned char bus_width = 1 << ios->bus_width; > + unsigned int blksz; > + unsigned int freq; > + > + if (data) { > + blksz = data->blksz; > + freq = host->mmc->actual_clock ? : host->clock; > + transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width); > + do_div(transfer_time, freq); > + /* multiply by '2' to account for any unknowns */ > + transfer_time = transfer_time * 2; > + /* calculate timeout for the entire data */ > + host->data_timeout = (data->blocks * ((target_timeout * > + NSEC_PER_USEC) + > + transfer_time)); (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow for timeouts greater than about 4 seconds. > + } else { > + host->data_timeout = (u64)target_timeout * NSEC_PER_USEC; > + } > + > + host->data_timeout += MMC_CMD_TRANSFER_TIME; Need to allow for target_timeout == 0 so: if (host->data_timeout) host->data_timeout += MMC_CMD_TRANSFER_TIME; > +} > + > static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) > { > u8 count; > @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) > if (count >= 0xF) > break; > } > + sdhci_calc_sw_timeout(host, cmd, target_timeout); If you make the changes I suggest for patch 6, then this would move sdhci_calc_sw_timeout() into sdhci_set_timeout(). I suggest you factor out the target_timeout calculation e.g. static unsigned int sdhci_target_timeout(struct sdhci_host *host, struct mmc_command *cmd, struct mmc_data *data) { unsigned int target_timeout; /* timeout in us */ if (!data) target_timeout = cmd->busy_timeout * 1000; else { target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000); if (host->clock && data->timeout_clks) { unsigned long long val; /* * data->timeout_clks is in units of clock cycles. * host->clock is in Hz. target_timeout is in us. * Hence, us = 1000000 * cycles / Hz. Round up. */ val = 1000000ULL * data->timeout_clks; if (do_div(val, host->clock)) target_timeout++; target_timeout += val; } } return target_timeout; } And call it from sdhci_calc_sw_timeout() > > return count; > } > @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) > mdelay(1); > } > > - timeout = jiffies; > - if (!cmd->data && cmd->busy_timeout > 9000) > - timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ; > - else > - timeout += 10 * HZ; > - sdhci_mod_timer(host, cmd->mrq, timeout); > - > host->cmd = cmd; > if (sdhci_data_line_cmd(cmd)) { > WARN_ON(host->data_cmd); > @@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) > cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200) > flags |= SDHCI_CMD_DATA; > > + timeout = jiffies; > + if (host->data_timeout > 0) { This can be just: if (host->data_timeout) { > + timeout += nsecs_to_jiffies(host->data_timeout); > + host->data_timeout = 0; It would be better to initialize host->data_timeout = 0 at the top of sdhci_prepare_data(). Also still need: else if (!cmd->data && cmd->busy_timeout > 9000) { timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ; > + } else { > + timeout += 10 * HZ; > + } > + sdhci_mod_timer(host, cmd->mrq, timeout); > + > sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND); > } > EXPORT_SYMBOL_GPL(sdhci_send_command); > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h > index ff283ee08854..29b242fd17de 100644 > --- a/drivers/mmc/host/sdhci.h > +++ b/drivers/mmc/host/sdhci.h > @@ -332,6 +332,14 @@ struct sdhci_adma2_64_desc { > /* Allow for a a command request and a data request at the same time */ > #define SDHCI_MAX_MRQS 2 > > +/* > + * 48bit command and 136 bit response in 100KHz clock could take upto 2.48ms. > + * However since the start time of the command, the time between > + * command and response, and the time between response and start of data is > + * not known, set the command transfer time to 10ms. > + */ > +#define MMC_CMD_TRANSFER_TIME (10 * NSEC_PER_MSEC) /* max 10 ms */ > + > enum sdhci_cookie { > COOKIE_UNMAPPED, > COOKIE_PRE_MAPPED, /* mapped by sdhci_pre_req() */ > @@ -555,6 +563,8 @@ struct sdhci_host { > /* Host SDMA buffer boundary. */ > u32 sdma_boundary; > > + u64 data_timeout; > + > unsigned long private[0] ____cacheline_aligned; > }; > >
On 07/03/18 15:20, Kishon Vijay Abraham I wrote: > sdhci_omap_config_iodelay_pinctrl_state() requires caps and caps2 to be > initialized (speed mode capabilities like UHS/HS200) before it is > invoked. While mmc_of_parse() initializes caps/caps2 if capabilities is > populated in device tree, it will remain uninitialized for capabilities > obtained from SDHCI_CAPABILITIES register. > Fix sdhci_omap_config_iodelay_pinctrl_state() to be used even while > getting the capabilities from SDHCI_CAPABILITIES register by invoking > sdhci_setup_host() before sdhci_omap_config_iodelay_pinctrl_state(). > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/mmc/host/sdhci-omap.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c > index 1456abd5eeb9..3cce30584d2f 100644 > --- a/drivers/mmc/host/sdhci-omap.c > +++ b/drivers/mmc/host/sdhci-omap.c > @@ -916,10 +916,6 @@ static int sdhci_omap_probe(struct platform_device *pdev) > goto err_put_sync; > } > > - ret = sdhci_omap_config_iodelay_pinctrl_state(omap_host); > - if (ret) > - goto err_put_sync; > - > host->mmc_host_ops.get_ro = mmc_gpio_get_ro; > host->mmc_host_ops.start_signal_voltage_switch = > sdhci_omap_start_signal_voltage_switch; > @@ -930,7 +926,15 @@ static int sdhci_omap_probe(struct platform_device *pdev) > sdhci_read_caps(host); > host->caps |= SDHCI_CAN_DO_ADMA2; > > - ret = sdhci_add_host(host); > + ret = sdhci_setup_host(host); > + if (ret) > + goto err_put_sync; > + > + ret = sdhci_omap_config_iodelay_pinctrl_state(omap_host); > + if (ret) > + goto err_put_sync; > + > + ret = __sdhci_add_host(host); > if (ret) > goto err_put_sync; if __sdhci_add_host() fails, then sdhci_cleanup_host() needs to be called to free resources allocated by sdhci_setup_host().
On 07/03/18 15:20, Kishon Vijay Abraham I wrote: > sdhci can directly get ADMA capability from MMCHS_CAPA register. > Remove explicitly setting ADMA here as some instances might not have > ADMA enabled. (sdhci_read_caps() is also removed from here since > sdhci_setup_host() invokes it). > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/host/sdhci-omap.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c > index 3cce30584d2f..0c40b13fb67d 100644 > --- a/drivers/mmc/host/sdhci-omap.c > +++ b/drivers/mmc/host/sdhci-omap.c > @@ -923,9 +923,6 @@ static int sdhci_omap_probe(struct platform_device *pdev) > host->mmc_host_ops.card_busy = sdhci_omap_card_busy; > host->mmc_host_ops.execute_tuning = sdhci_omap_execute_tuning; > > - sdhci_read_caps(host); > - host->caps |= SDHCI_CAN_DO_ADMA2; > - > ret = sdhci_setup_host(host); > if (ret) > goto err_put_sync; >
On 07/03/18 15:20, Kishon Vijay Abraham I wrote: > Invoke sdhci_get_of_property defined in sdhci-pltfm.c to read > sdhci specific properties from dt node. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/host/sdhci-omap.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c > index fbc20a4fbb23..314dbe4d7412 100644 > --- a/drivers/mmc/host/sdhci-omap.c > +++ b/drivers/mmc/host/sdhci-omap.c > @@ -893,6 +893,7 @@ static int sdhci_omap_probe(struct platform_device *pdev) > host->ioaddr += offset; > > mmc = host->mmc; > + sdhci_get_of_property(pdev); > ret = mmc_of_parse(mmc); > if (ret) > goto err_pltfm_free; >
On 07/03/18 15:20, Kishon Vijay Abraham I wrote: > Add sdhci_omap_enable_sdio_irq to set CTPL and CLKEXTFREE bits in > MMCHS_CON register required to detect asynchronous card interrupt > on DAT[1]. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/host/sdhci-omap.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c > index 14dd51b51b41..8ceb3956b211 100644 > --- a/drivers/mmc/host/sdhci-omap.c > +++ b/drivers/mmc/host/sdhci-omap.c > @@ -36,6 +36,7 @@ > #define CON_DDR BIT(19) > #define CON_CLKEXTFREE BIT(16) > #define CON_PADEN BIT(15) > +#define CON_CTPL BIT(11) > #define CON_INIT BIT(1) > #define CON_OD BIT(0) > > @@ -226,6 +227,23 @@ static void sdhci_omap_conf_bus_power(struct sdhci_omap_host *omap_host, > } > } > > +static void sdhci_omap_enable_sdio_irq(struct mmc_host *mmc, int enable) > +{ > + struct sdhci_host *host = mmc_priv(mmc); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host); > + u32 reg; > + > + reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_CON); > + if (enable) > + reg |= (CON_CTPL | CON_CLKEXTFREE); > + else > + reg &= ~(CON_CTPL | CON_CLKEXTFREE); > + sdhci_omap_writel(omap_host, SDHCI_OMAP_CON, reg); > + > + sdhci_enable_sdio_irq(mmc, enable); > +} > + > static inline void sdhci_omap_set_dll(struct sdhci_omap_host *omap_host, > int count) > { > @@ -962,6 +980,7 @@ static int sdhci_omap_probe(struct platform_device *pdev) > host->mmc_host_ops.set_ios = sdhci_omap_set_ios; > host->mmc_host_ops.card_busy = sdhci_omap_card_busy; > host->mmc_host_ops.execute_tuning = sdhci_omap_execute_tuning; > + host->mmc_host_ops.enable_sdio_irq = sdhci_omap_enable_sdio_irq; > > ret = sdhci_setup_host(host); > if (ret) >
Hi, On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote: > On 07/03/18 15:20, Kishon Vijay Abraham I wrote: >> sdhci has a 10 second timeout to catch devices that stop responding. >> Instead of programming 10 second arbitrary value, calculate the total time >> it would take for the entire transfer to happen and program the timeout >> value accordingly. >> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> --- >> drivers/mmc/host/sdhci.c | 47 ++++++++++++++++++++++++++++++++++++++++------- >> drivers/mmc/host/sdhci.h | 10 ++++++++++ >> 2 files changed, 50 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >> index 1dd117cbeb6e..baab67bfa39b 100644 >> --- a/drivers/mmc/host/sdhci.c >> +++ b/drivers/mmc/host/sdhci.c >> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host) >> return sg_dma_address(host->data->sg); >> } >> >> +static void sdhci_calc_sw_timeout(struct sdhci_host *host, >> + struct mmc_command *cmd, >> + unsigned int target_timeout) >> +{ >> + struct mmc_data *data = cmd->data; >> + struct mmc_host *mmc = host->mmc; >> + u64 transfer_time; >> + struct mmc_ios *ios = &mmc->ios; >> + unsigned char bus_width = 1 << ios->bus_width; >> + unsigned int blksz; >> + unsigned int freq; >> + >> + if (data) { >> + blksz = data->blksz; >> + freq = host->mmc->actual_clock ? : host->clock; >> + transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width); >> + do_div(transfer_time, freq); >> + /* multiply by '2' to account for any unknowns */ >> + transfer_time = transfer_time * 2; >> + /* calculate timeout for the entire data */ >> + host->data_timeout = (data->blocks * ((target_timeout * >> + NSEC_PER_USEC) + >> + transfer_time)); > > (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow > for timeouts greater than about 4 seconds. > >> + } else { >> + host->data_timeout = (u64)target_timeout * NSEC_PER_USEC; >> + } >> + >> + host->data_timeout += MMC_CMD_TRANSFER_TIME; > > Need to allow for target_timeout == 0 so: > > if (host->data_timeout) > host->data_timeout += MMC_CMD_TRANSFER_TIME; > >> +} >> + >> static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) >> { >> u8 count; >> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) >> if (count >= 0xF) >> break; >> } >> + sdhci_calc_sw_timeout(host, cmd, target_timeout); > > If you make the changes I suggest for patch 6, then this would > move sdhci_calc_sw_timeout() into sdhci_set_timeout(). > > I suggest you factor out the target_timeout calculation e.g. > > static unsigned int sdhci_target_timeout(struct sdhci_host *host, > struct mmc_command *cmd, > struct mmc_data *data) > { > unsigned int target_timeout; > > /* timeout in us */ > if (!data) > target_timeout = cmd->busy_timeout * 1000; > else { > target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000); > if (host->clock && data->timeout_clks) { > unsigned long long val; > > /* > * data->timeout_clks is in units of clock cycles. > * host->clock is in Hz. target_timeout is in us. > * Hence, us = 1000000 * cycles / Hz. Round up. > */ > val = 1000000ULL * data->timeout_clks; > if (do_div(val, host->clock)) > target_timeout++; > target_timeout += val; > } > } > > return target_timeout; > } > > And call it from sdhci_calc_sw_timeout() > >> >> return count; >> } >> @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) >> mdelay(1); >> } >> >> - timeout = jiffies; >> - if (!cmd->data && cmd->busy_timeout > 9000) >> - timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ; >> - else >> - timeout += 10 * HZ; >> - sdhci_mod_timer(host, cmd->mrq, timeout); >> - >> host->cmd = cmd; >> if (sdhci_data_line_cmd(cmd)) { >> WARN_ON(host->data_cmd); >> @@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) >> cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200) >> flags |= SDHCI_CMD_DATA; >> >> + timeout = jiffies; >> + if (host->data_timeout > 0) { > > This can be just: > > if (host->data_timeout) { > >> + timeout += nsecs_to_jiffies(host->data_timeout); >> + host->data_timeout = 0; > > It would be better to initialize host->data_timeout = 0 at the top of > sdhci_prepare_data(). > > Also still need: > > else if (!cmd->data && cmd->busy_timeout > 9000) { > timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ; sdhci_calc_sw_timeout should have calculated the timeout for this case too no? Thanks Kishon
On 16/03/18 08:29, Kishon Vijay Abraham I wrote: > Hi, > > On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote: >> On 07/03/18 15:20, Kishon Vijay Abraham I wrote: >>> sdhci has a 10 second timeout to catch devices that stop responding. >>> Instead of programming 10 second arbitrary value, calculate the total time >>> it would take for the entire transfer to happen and program the timeout >>> value accordingly. >>> >>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>> --- >>> drivers/mmc/host/sdhci.c | 47 ++++++++++++++++++++++++++++++++++++++++------- >>> drivers/mmc/host/sdhci.h | 10 ++++++++++ >>> 2 files changed, 50 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>> index 1dd117cbeb6e..baab67bfa39b 100644 >>> --- a/drivers/mmc/host/sdhci.c >>> +++ b/drivers/mmc/host/sdhci.c >>> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host) >>> return sg_dma_address(host->data->sg); >>> } >>> >>> +static void sdhci_calc_sw_timeout(struct sdhci_host *host, >>> + struct mmc_command *cmd, >>> + unsigned int target_timeout) >>> +{ >>> + struct mmc_data *data = cmd->data; >>> + struct mmc_host *mmc = host->mmc; >>> + u64 transfer_time; >>> + struct mmc_ios *ios = &mmc->ios; >>> + unsigned char bus_width = 1 << ios->bus_width; >>> + unsigned int blksz; >>> + unsigned int freq; >>> + >>> + if (data) { >>> + blksz = data->blksz; >>> + freq = host->mmc->actual_clock ? : host->clock; >>> + transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width); >>> + do_div(transfer_time, freq); >>> + /* multiply by '2' to account for any unknowns */ >>> + transfer_time = transfer_time * 2; >>> + /* calculate timeout for the entire data */ >>> + host->data_timeout = (data->blocks * ((target_timeout * >>> + NSEC_PER_USEC) + >>> + transfer_time)); >> >> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow >> for timeouts greater than about 4 seconds. >> >>> + } else { >>> + host->data_timeout = (u64)target_timeout * NSEC_PER_USEC; >>> + } >>> + >>> + host->data_timeout += MMC_CMD_TRANSFER_TIME; >> >> Need to allow for target_timeout == 0 so: >> >> if (host->data_timeout) >> host->data_timeout += MMC_CMD_TRANSFER_TIME; >> >>> +} >>> + >>> static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) >>> { >>> u8 count; >>> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) >>> if (count >= 0xF) >>> break; >>> } >>> + sdhci_calc_sw_timeout(host, cmd, target_timeout); >> >> If you make the changes I suggest for patch 6, then this would >> move sdhci_calc_sw_timeout() into sdhci_set_timeout(). >> >> I suggest you factor out the target_timeout calculation e.g. >> >> static unsigned int sdhci_target_timeout(struct sdhci_host *host, >> struct mmc_command *cmd, >> struct mmc_data *data) >> { >> unsigned int target_timeout; >> >> /* timeout in us */ >> if (!data) >> target_timeout = cmd->busy_timeout * 1000; >> else { >> target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000); >> if (host->clock && data->timeout_clks) { >> unsigned long long val; >> >> /* >> * data->timeout_clks is in units of clock cycles. >> * host->clock is in Hz. target_timeout is in us. >> * Hence, us = 1000000 * cycles / Hz. Round up. >> */ >> val = 1000000ULL * data->timeout_clks; >> if (do_div(val, host->clock)) >> target_timeout++; >> target_timeout += val; >> } >> } >> >> return target_timeout; >> } >> >> And call it from sdhci_calc_sw_timeout() >> >>> >>> return count; >>> } >>> @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) >>> mdelay(1); >>> } >>> >>> - timeout = jiffies; >>> - if (!cmd->data && cmd->busy_timeout > 9000) >>> - timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ; >>> - else >>> - timeout += 10 * HZ; >>> - sdhci_mod_timer(host, cmd->mrq, timeout); >>> - >>> host->cmd = cmd; >>> if (sdhci_data_line_cmd(cmd)) { >>> WARN_ON(host->data_cmd); >>> @@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) >>> cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200) >>> flags |= SDHCI_CMD_DATA; >>> >>> + timeout = jiffies; >>> + if (host->data_timeout > 0) { >> >> This can be just: >> >> if (host->data_timeout) { >> >>> + timeout += nsecs_to_jiffies(host->data_timeout); >>> + host->data_timeout = 0; >> >> It would be better to initialize host->data_timeout = 0 at the top of >> sdhci_prepare_data(). >> >> Also still need: >> >> else if (!cmd->data && cmd->busy_timeout > 9000) { >> timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ; > > sdhci_calc_sw_timeout should have calculated the timeout for this case too no? Yes, but I was thinking you would only calculate when it was needed.
Hi Adrian, On Friday 16 March 2018 07:51 PM, Adrian Hunter wrote: > On 16/03/18 08:29, Kishon Vijay Abraham I wrote: >> Hi, >> >> On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote: >>> On 07/03/18 15:20, Kishon Vijay Abraham I wrote: >>>> sdhci has a 10 second timeout to catch devices that stop responding. >>>> Instead of programming 10 second arbitrary value, calculate the total time >>>> it would take for the entire transfer to happen and program the timeout >>>> value accordingly. >>>> >>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>> --- >>>> drivers/mmc/host/sdhci.c | 47 ++++++++++++++++++++++++++++++++++++++++------- >>>> drivers/mmc/host/sdhci.h | 10 ++++++++++ >>>> 2 files changed, 50 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>>> index 1dd117cbeb6e..baab67bfa39b 100644 >>>> --- a/drivers/mmc/host/sdhci.c >>>> +++ b/drivers/mmc/host/sdhci.c >>>> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host) >>>> return sg_dma_address(host->data->sg); >>>> } >>>> >>>> +static void sdhci_calc_sw_timeout(struct sdhci_host *host, >>>> + struct mmc_command *cmd, >>>> + unsigned int target_timeout) >>>> +{ >>>> + struct mmc_data *data = cmd->data; >>>> + struct mmc_host *mmc = host->mmc; >>>> + u64 transfer_time; >>>> + struct mmc_ios *ios = &mmc->ios; >>>> + unsigned char bus_width = 1 << ios->bus_width; >>>> + unsigned int blksz; >>>> + unsigned int freq; >>>> + >>>> + if (data) { >>>> + blksz = data->blksz; >>>> + freq = host->mmc->actual_clock ? : host->clock; >>>> + transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width); >>>> + do_div(transfer_time, freq); >>>> + /* multiply by '2' to account for any unknowns */ >>>> + transfer_time = transfer_time * 2; >>>> + /* calculate timeout for the entire data */ >>>> + host->data_timeout = (data->blocks * ((target_timeout * >>>> + NSEC_PER_USEC) + >>>> + transfer_time)); >>> >>> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow >>> for timeouts greater than about 4 seconds. >>> >>>> + } else { >>>> + host->data_timeout = (u64)target_timeout * NSEC_PER_USEC; >>>> + } >>>> + >>>> + host->data_timeout += MMC_CMD_TRANSFER_TIME; >>> >>> Need to allow for target_timeout == 0 so: >>> >>> if (host->data_timeout) >>> host->data_timeout += MMC_CMD_TRANSFER_TIME; >>> >>>> +} >>>> + >>>> static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) >>>> { >>>> u8 count; >>>> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) >>>> if (count >= 0xF) >>>> break; >>>> } >>>> + sdhci_calc_sw_timeout(host, cmd, target_timeout); >>> >>> If you make the changes I suggest for patch 6, then this would >>> move sdhci_calc_sw_timeout() into sdhci_set_timeout(). >>> >>> I suggest you factor out the target_timeout calculation e.g. >>> >>> static unsigned int sdhci_target_timeout(struct sdhci_host *host, >>> struct mmc_command *cmd, >>> struct mmc_data *data) >>> { >>> unsigned int target_timeout; >>> >>> /* timeout in us */ >>> if (!data) >>> target_timeout = cmd->busy_timeout * 1000; >>> else { >>> target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000); >>> if (host->clock && data->timeout_clks) { >>> unsigned long long val; >>> >>> /* >>> * data->timeout_clks is in units of clock cycles. >>> * host->clock is in Hz. target_timeout is in us. >>> * Hence, us = 1000000 * cycles / Hz. Round up. >>> */ >>> val = 1000000ULL * data->timeout_clks; >>> if (do_div(val, host->clock)) >>> target_timeout++; >>> target_timeout += val; >>> } >>> } >>> >>> return target_timeout; >>> } >>> >>> And call it from sdhci_calc_sw_timeout() >>> >>>> >>>> return count; >>>> } >>>> @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) >>>> mdelay(1); >>>> } >>>> >>>> - timeout = jiffies; >>>> - if (!cmd->data && cmd->busy_timeout > 9000) >>>> - timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ; >>>> - else >>>> - timeout += 10 * HZ; >>>> - sdhci_mod_timer(host, cmd->mrq, timeout); >>>> - >>>> host->cmd = cmd; >>>> if (sdhci_data_line_cmd(cmd)) { >>>> WARN_ON(host->data_cmd); >>>> @@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) >>>> cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200) >>>> flags |= SDHCI_CMD_DATA; >>>> >>>> + timeout = jiffies; >>>> + if (host->data_timeout > 0) { >>> >>> This can be just: >>> >>> if (host->data_timeout) { >>> >>>> + timeout += nsecs_to_jiffies(host->data_timeout); >>>> + host->data_timeout = 0; >>> >>> It would be better to initialize host->data_timeout = 0 at the top of >>> sdhci_prepare_data(). >>> >>> Also still need: >>> >>> else if (!cmd->data && cmd->busy_timeout > 9000) { >>> timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ; >> >> sdhci_calc_sw_timeout should have calculated the timeout for this case too no? > > Yes, but I was thinking you would only calculate when it was needed. I feel since we would have anyways calculated data_timeout, we should use that instead unless you see a problem with that. Thanks Kishon
On 19/03/18 11:20, Kishon Vijay Abraham I wrote: > Hi Adrian, > > On Friday 16 March 2018 07:51 PM, Adrian Hunter wrote: >> On 16/03/18 08:29, Kishon Vijay Abraham I wrote: >>> Hi, >>> >>> On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote: >>>> On 07/03/18 15:20, Kishon Vijay Abraham I wrote: >>>>> sdhci has a 10 second timeout to catch devices that stop responding. >>>>> Instead of programming 10 second arbitrary value, calculate the total time >>>>> it would take for the entire transfer to happen and program the timeout >>>>> value accordingly. >>>>> >>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>>> --- >>>>> drivers/mmc/host/sdhci.c | 47 ++++++++++++++++++++++++++++++++++++++++------- >>>>> drivers/mmc/host/sdhci.h | 10 ++++++++++ >>>>> 2 files changed, 50 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>>>> index 1dd117cbeb6e..baab67bfa39b 100644 >>>>> --- a/drivers/mmc/host/sdhci.c >>>>> +++ b/drivers/mmc/host/sdhci.c >>>>> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host) >>>>> return sg_dma_address(host->data->sg); >>>>> } >>>>> >>>>> +static void sdhci_calc_sw_timeout(struct sdhci_host *host, >>>>> + struct mmc_command *cmd, >>>>> + unsigned int target_timeout) >>>>> +{ >>>>> + struct mmc_data *data = cmd->data; >>>>> + struct mmc_host *mmc = host->mmc; >>>>> + u64 transfer_time; >>>>> + struct mmc_ios *ios = &mmc->ios; >>>>> + unsigned char bus_width = 1 << ios->bus_width; >>>>> + unsigned int blksz; >>>>> + unsigned int freq; >>>>> + >>>>> + if (data) { >>>>> + blksz = data->blksz; >>>>> + freq = host->mmc->actual_clock ? : host->clock; >>>>> + transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width); >>>>> + do_div(transfer_time, freq); >>>>> + /* multiply by '2' to account for any unknowns */ >>>>> + transfer_time = transfer_time * 2; >>>>> + /* calculate timeout for the entire data */ >>>>> + host->data_timeout = (data->blocks * ((target_timeout * >>>>> + NSEC_PER_USEC) + >>>>> + transfer_time)); >>>> >>>> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow >>>> for timeouts greater than about 4 seconds. >>>> >>>>> + } else { >>>>> + host->data_timeout = (u64)target_timeout * NSEC_PER_USEC; >>>>> + } >>>>> + >>>>> + host->data_timeout += MMC_CMD_TRANSFER_TIME; >>>> >>>> Need to allow for target_timeout == 0 so: >>>> >>>> if (host->data_timeout) >>>> host->data_timeout += MMC_CMD_TRANSFER_TIME; >>>> >>>>> +} >>>>> + >>>>> static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) >>>>> { >>>>> u8 count; >>>>> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) >>>>> if (count >= 0xF) >>>>> break; >>>>> } >>>>> + sdhci_calc_sw_timeout(host, cmd, target_timeout); >>>> >>>> If you make the changes I suggest for patch 6, then this would >>>> move sdhci_calc_sw_timeout() into sdhci_set_timeout(). >>>> >>>> I suggest you factor out the target_timeout calculation e.g. >>>> >>>> static unsigned int sdhci_target_timeout(struct sdhci_host *host, >>>> struct mmc_command *cmd, >>>> struct mmc_data *data) >>>> { >>>> unsigned int target_timeout; >>>> >>>> /* timeout in us */ >>>> if (!data) >>>> target_timeout = cmd->busy_timeout * 1000; >>>> else { >>>> target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000); >>>> if (host->clock && data->timeout_clks) { >>>> unsigned long long val; >>>> >>>> /* >>>> * data->timeout_clks is in units of clock cycles. >>>> * host->clock is in Hz. target_timeout is in us. >>>> * Hence, us = 1000000 * cycles / Hz. Round up. >>>> */ >>>> val = 1000000ULL * data->timeout_clks; >>>> if (do_div(val, host->clock)) >>>> target_timeout++; >>>> target_timeout += val; >>>> } >>>> } >>>> >>>> return target_timeout; >>>> } >>>> >>>> And call it from sdhci_calc_sw_timeout() >>>> >>>>> >>>>> return count; >>>>> } >>>>> @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) >>>>> mdelay(1); >>>>> } >>>>> >>>>> - timeout = jiffies; >>>>> - if (!cmd->data && cmd->busy_timeout > 9000) >>>>> - timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ; >>>>> - else >>>>> - timeout += 10 * HZ; >>>>> - sdhci_mod_timer(host, cmd->mrq, timeout); >>>>> - >>>>> host->cmd = cmd; >>>>> if (sdhci_data_line_cmd(cmd)) { >>>>> WARN_ON(host->data_cmd); >>>>> @@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) >>>>> cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200) >>>>> flags |= SDHCI_CMD_DATA; >>>>> >>>>> + timeout = jiffies; >>>>> + if (host->data_timeout > 0) { >>>> >>>> This can be just: >>>> >>>> if (host->data_timeout) { >>>> >>>>> + timeout += nsecs_to_jiffies(host->data_timeout); >>>>> + host->data_timeout = 0; >>>> >>>> It would be better to initialize host->data_timeout = 0 at the top of >>>> sdhci_prepare_data(). >>>> >>>> Also still need: >>>> >>>> else if (!cmd->data && cmd->busy_timeout > 9000) { >>>> timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ; >>> >>> sdhci_calc_sw_timeout should have calculated the timeout for this case too no? >> >> Yes, but I was thinking you would only calculate when it was needed. > > I feel since we would have anyways calculated data_timeout, we should use that > instead unless you see a problem with that. I would prefer not to calculate data_timeout when a hardware timeout is being used.
Hi Adrian, On Monday 19 March 2018 03:30 PM, Adrian Hunter wrote: > On 19/03/18 11:20, Kishon Vijay Abraham I wrote: >> Hi Adrian, >> >> On Friday 16 March 2018 07:51 PM, Adrian Hunter wrote: >>> On 16/03/18 08:29, Kishon Vijay Abraham I wrote: >>>> Hi, >>>> >>>> On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote: >>>>> On 07/03/18 15:20, Kishon Vijay Abraham I wrote: >>>>>> sdhci has a 10 second timeout to catch devices that stop responding. >>>>>> Instead of programming 10 second arbitrary value, calculate the total time >>>>>> it would take for the entire transfer to happen and program the timeout >>>>>> value accordingly. >>>>>> >>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>>>> --- >>>>>> drivers/mmc/host/sdhci.c | 47 ++++++++++++++++++++++++++++++++++++++++------- >>>>>> drivers/mmc/host/sdhci.h | 10 ++++++++++ >>>>>> 2 files changed, 50 insertions(+), 7 deletions(-) >>>>>> >>>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>>>>> index 1dd117cbeb6e..baab67bfa39b 100644 >>>>>> --- a/drivers/mmc/host/sdhci.c >>>>>> +++ b/drivers/mmc/host/sdhci.c >>>>>> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host) >>>>>> return sg_dma_address(host->data->sg); >>>>>> } >>>>>> >>>>>> +static void sdhci_calc_sw_timeout(struct sdhci_host *host, >>>>>> + struct mmc_command *cmd, >>>>>> + unsigned int target_timeout) >>>>>> +{ >>>>>> + struct mmc_data *data = cmd->data; >>>>>> + struct mmc_host *mmc = host->mmc; >>>>>> + u64 transfer_time; >>>>>> + struct mmc_ios *ios = &mmc->ios; >>>>>> + unsigned char bus_width = 1 << ios->bus_width; >>>>>> + unsigned int blksz; >>>>>> + unsigned int freq; >>>>>> + >>>>>> + if (data) { >>>>>> + blksz = data->blksz; >>>>>> + freq = host->mmc->actual_clock ? : host->clock; >>>>>> + transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width); >>>>>> + do_div(transfer_time, freq); >>>>>> + /* multiply by '2' to account for any unknowns */ >>>>>> + transfer_time = transfer_time * 2; >>>>>> + /* calculate timeout for the entire data */ >>>>>> + host->data_timeout = (data->blocks * ((target_timeout * >>>>>> + NSEC_PER_USEC) + >>>>>> + transfer_time)); >>>>> >>>>> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow >>>>> for timeouts greater than about 4 seconds. >>>>> >>>>>> + } else { >>>>>> + host->data_timeout = (u64)target_timeout * NSEC_PER_USEC; >>>>>> + } >>>>>> + >>>>>> + host->data_timeout += MMC_CMD_TRANSFER_TIME; >>>>> >>>>> Need to allow for target_timeout == 0 so: >>>>> >>>>> if (host->data_timeout) >>>>> host->data_timeout += MMC_CMD_TRANSFER_TIME; >>>>> >>>>>> +} >>>>>> + >>>>>> static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) >>>>>> { >>>>>> u8 count; >>>>>> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) >>>>>> if (count >= 0xF) >>>>>> break; >>>>>> } >>>>>> + sdhci_calc_sw_timeout(host, cmd, target_timeout); >>>>> >>>>> If you make the changes I suggest for patch 6, then this would >>>>> move sdhci_calc_sw_timeout() into sdhci_set_timeout(). >>>>> >>>>> I suggest you factor out the target_timeout calculation e.g. >>>>> >>>>> static unsigned int sdhci_target_timeout(struct sdhci_host *host, >>>>> struct mmc_command *cmd, >>>>> struct mmc_data *data) >>>>> { >>>>> unsigned int target_timeout; >>>>> >>>>> /* timeout in us */ >>>>> if (!data) >>>>> target_timeout = cmd->busy_timeout * 1000; >>>>> else { >>>>> target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000); >>>>> if (host->clock && data->timeout_clks) { >>>>> unsigned long long val; >>>>> >>>>> /* >>>>> * data->timeout_clks is in units of clock cycles. >>>>> * host->clock is in Hz. target_timeout is in us. >>>>> * Hence, us = 1000000 * cycles / Hz. Round up. >>>>> */ >>>>> val = 1000000ULL * data->timeout_clks; >>>>> if (do_div(val, host->clock)) >>>>> target_timeout++; >>>>> target_timeout += val; >>>>> } >>>>> } >>>>> >>>>> return target_timeout; >>>>> } >>>>> >>>>> And call it from sdhci_calc_sw_timeout() >>>>> >>>>>> >>>>>> return count; >>>>>> } >>>>>> @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) >>>>>> mdelay(1); >>>>>> } >>>>>> >>>>>> - timeout = jiffies; >>>>>> - if (!cmd->data && cmd->busy_timeout > 9000) >>>>>> - timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ; >>>>>> - else >>>>>> - timeout += 10 * HZ; >>>>>> - sdhci_mod_timer(host, cmd->mrq, timeout); >>>>>> - >>>>>> host->cmd = cmd; >>>>>> if (sdhci_data_line_cmd(cmd)) { >>>>>> WARN_ON(host->data_cmd); >>>>>> @@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) >>>>>> cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200) >>>>>> flags |= SDHCI_CMD_DATA; >>>>>> >>>>>> + timeout = jiffies; >>>>>> + if (host->data_timeout > 0) { >>>>> >>>>> This can be just: >>>>> >>>>> if (host->data_timeout) { >>>>> >>>>>> + timeout += nsecs_to_jiffies(host->data_timeout); >>>>>> + host->data_timeout = 0; >>>>> >>>>> It would be better to initialize host->data_timeout = 0 at the top of >>>>> sdhci_prepare_data(). >>>>> >>>>> Also still need: >>>>> >>>>> else if (!cmd->data && cmd->busy_timeout > 9000) { >>>>> timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ; >>>> >>>> sdhci_calc_sw_timeout should have calculated the timeout for this case too no? >>> >>> Yes, but I was thinking you would only calculate when it was needed. >> >> I feel since we would have anyways calculated data_timeout, we should use that >> instead unless you see a problem with that. > > I would prefer not to calculate data_timeout when a hardware timeout is > being used. > That differs from what I had thought. This patch tries to program a relatively accurate SW timeout value (for data_timer) irrespective of whether hardware timeout is used or not. This only tries to change the 10 Sec SW timeout value programmed for all data transfer commands. Thanks Kishon
Hi Adrian, On Monday 19 March 2018 03:49 PM, Kishon Vijay Abraham I wrote: > Hi Adrian, > > On Monday 19 March 2018 03:30 PM, Adrian Hunter wrote: >> On 19/03/18 11:20, Kishon Vijay Abraham I wrote: >>> Hi Adrian, >>> >>> On Friday 16 March 2018 07:51 PM, Adrian Hunter wrote: >>>> On 16/03/18 08:29, Kishon Vijay Abraham I wrote: >>>>> Hi, >>>>> >>>>> On Thursday 15 March 2018 06:43 PM, Adrian Hunter wrote: >>>>>> On 07/03/18 15:20, Kishon Vijay Abraham I wrote: >>>>>>> sdhci has a 10 second timeout to catch devices that stop responding. >>>>>>> Instead of programming 10 second arbitrary value, calculate the total time >>>>>>> it would take for the entire transfer to happen and program the timeout >>>>>>> value accordingly. >>>>>>> >>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>>>>> --- >>>>>>> drivers/mmc/host/sdhci.c | 47 ++++++++++++++++++++++++++++++++++++++++------- >>>>>>> drivers/mmc/host/sdhci.h | 10 ++++++++++ >>>>>>> 2 files changed, 50 insertions(+), 7 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>>>>>> index 1dd117cbeb6e..baab67bfa39b 100644 >>>>>>> --- a/drivers/mmc/host/sdhci.c >>>>>>> +++ b/drivers/mmc/host/sdhci.c >>>>>>> @@ -709,6 +709,36 @@ static u32 sdhci_sdma_address(struct sdhci_host *host) >>>>>>> return sg_dma_address(host->data->sg); >>>>>>> } >>>>>>> >>>>>>> +static void sdhci_calc_sw_timeout(struct sdhci_host *host, >>>>>>> + struct mmc_command *cmd, >>>>>>> + unsigned int target_timeout) >>>>>>> +{ >>>>>>> + struct mmc_data *data = cmd->data; >>>>>>> + struct mmc_host *mmc = host->mmc; >>>>>>> + u64 transfer_time; >>>>>>> + struct mmc_ios *ios = &mmc->ios; >>>>>>> + unsigned char bus_width = 1 << ios->bus_width; >>>>>>> + unsigned int blksz; >>>>>>> + unsigned int freq; >>>>>>> + >>>>>>> + if (data) { >>>>>>> + blksz = data->blksz; >>>>>>> + freq = host->mmc->actual_clock ? : host->clock; >>>>>>> + transfer_time = (u64)blksz * NSEC_PER_SEC * (8 / bus_width); >>>>>>> + do_div(transfer_time, freq); >>>>>>> + /* multiply by '2' to account for any unknowns */ >>>>>>> + transfer_time = transfer_time * 2; >>>>>>> + /* calculate timeout for the entire data */ >>>>>>> + host->data_timeout = (data->blocks * ((target_timeout * >>>>>>> + NSEC_PER_USEC) + >>>>>>> + transfer_time)); >>>>>> >>>>>> (target_timeout * NSEC_PER_USEC) might be 32-bit and therefore overflow >>>>>> for timeouts greater than about 4 seconds. >>>>>> >>>>>>> + } else { >>>>>>> + host->data_timeout = (u64)target_timeout * NSEC_PER_USEC; >>>>>>> + } >>>>>>> + >>>>>>> + host->data_timeout += MMC_CMD_TRANSFER_TIME; >>>>>> >>>>>> Need to allow for target_timeout == 0 so: >>>>>> >>>>>> if (host->data_timeout) >>>>>> host->data_timeout += MMC_CMD_TRANSFER_TIME; >>>>>> >>>>>>> +} >>>>>>> + >>>>>>> static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) >>>>>>> { >>>>>>> u8 count; >>>>>>> @@ -766,6 +796,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd) >>>>>>> if (count >= 0xF) >>>>>>> break; >>>>>>> } >>>>>>> + sdhci_calc_sw_timeout(host, cmd, target_timeout); >>>>>> >>>>>> If you make the changes I suggest for patch 6, then this would >>>>>> move sdhci_calc_sw_timeout() into sdhci_set_timeout(). >>>>>> >>>>>> I suggest you factor out the target_timeout calculation e.g. >>>>>> >>>>>> static unsigned int sdhci_target_timeout(struct sdhci_host *host, >>>>>> struct mmc_command *cmd, >>>>>> struct mmc_data *data) >>>>>> { >>>>>> unsigned int target_timeout; >>>>>> >>>>>> /* timeout in us */ >>>>>> if (!data) >>>>>> target_timeout = cmd->busy_timeout * 1000; >>>>>> else { >>>>>> target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000); >>>>>> if (host->clock && data->timeout_clks) { >>>>>> unsigned long long val; >>>>>> >>>>>> /* >>>>>> * data->timeout_clks is in units of clock cycles. >>>>>> * host->clock is in Hz. target_timeout is in us. >>>>>> * Hence, us = 1000000 * cycles / Hz. Round up. >>>>>> */ >>>>>> val = 1000000ULL * data->timeout_clks; >>>>>> if (do_div(val, host->clock)) >>>>>> target_timeout++; >>>>>> target_timeout += val; >>>>>> } >>>>>> } >>>>>> >>>>>> return target_timeout; >>>>>> } >>>>>> >>>>>> And call it from sdhci_calc_sw_timeout() >>>>>> >>>>>>> >>>>>>> return count; >>>>>>> } >>>>>>> @@ -1175,13 +1206,6 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) >>>>>>> mdelay(1); >>>>>>> } >>>>>>> >>>>>>> - timeout = jiffies; >>>>>>> - if (!cmd->data && cmd->busy_timeout > 9000) >>>>>>> - timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ; >>>>>>> - else >>>>>>> - timeout += 10 * HZ; >>>>>>> - sdhci_mod_timer(host, cmd->mrq, timeout); >>>>>>> - >>>>>>> host->cmd = cmd; >>>>>>> if (sdhci_data_line_cmd(cmd)) { >>>>>>> WARN_ON(host->data_cmd); >>>>>>> @@ -1221,6 +1245,15 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) >>>>>>> cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200) >>>>>>> flags |= SDHCI_CMD_DATA; >>>>>>> >>>>>>> + timeout = jiffies; >>>>>>> + if (host->data_timeout > 0) { >>>>>> >>>>>> This can be just: >>>>>> >>>>>> if (host->data_timeout) { >>>>>> >>>>>>> + timeout += nsecs_to_jiffies(host->data_timeout); >>>>>>> + host->data_timeout = 0; >>>>>> >>>>>> It would be better to initialize host->data_timeout = 0 at the top of >>>>>> sdhci_prepare_data(). >>>>>> >>>>>> Also still need: >>>>>> >>>>>> else if (!cmd->data && cmd->busy_timeout > 9000) { >>>>>> timeout += DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ; >>>>> >>>>> sdhci_calc_sw_timeout should have calculated the timeout for this case too no? >>>> >>>> Yes, but I was thinking you would only calculate when it was needed. >>> >>> I feel since we would have anyways calculated data_timeout, we should use that >>> instead unless you see a problem with that. >> >> I would prefer not to calculate data_timeout when a hardware timeout is >> being used. >> > > That differs from what I had thought. This patch tries to program a relatively > accurate SW timeout value (for data_timer) irrespective of whether hardware > timeout is used or not. This only tries to change the 10 Sec SW timeout value > programmed for all data transfer commands. IMHO since we calculate the worst case timeout value we should be using that for all cases where we are able to calculate the timeout value so that we don't give a too high or too low timeout value. Let me know If this sounds okay to you. Thanks Kishon
Add UHS/HS200 mode support in sdhci-omap. The programming sequence for voltage switching, tuning is followed from AM572x TRM http://www.ti.com/lit/ug/spruhz6i/spruhz6i.pdf (Similar to all AM57x/DRA7x SoCs). The patch series also implements workaround for errata published in http://www.ti.com/lit/er/sprz429k/sprz429k.pdf patches are created on top of git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git next Patches in v2 already applied to mmc -next are dropped from the series. Changes from v2: *) Changed SW timeout logic as per Adrians's suggestion *) Validated SDIO with the current patches (added a couple of fixes found while adding SDIO support). *) Used soc_device_match() instead of pdata-quirks as per Tony's suggestions. Changes from v1: *) Only poll on DAT0 and DATI for card_busy status *) Cleanup iodelay patch as suggested by Tony. *) Added quirk to disable HW timeout *) Use the existing data timer but program a relatively accurate SW timeout value (Impacts all platforms) *) Fix a bug in sdhci which was using data_timer for non data line commands Kishon Vijay Abraham I (11): mmc: sdhci-omap: Fix when capabilities are obtained from SDHCI_CAPABILITIES reg mmc: sdhci-omap: Remove setting ADMA capability in driver mmc: sdhci-omap: Workaround for Errata i843 mmc: sdhci-omap: Invoke sdhci_get_of_property to read sdhci dt properties mmc: sdhci: Disable HS200 mode if controller can't support 1.8v mmc: sdhci: Add quirk to disable HW timeout mmc: sdhci: Program a relatively accurate SW timeout value mmc: sdhci-omap: Workaround for Errata i834 dt-bindings: sdhci-omap: Add K2G specific binding mmc: sdhci-omap: Add support for MMC/SD controller in k2g SoC mmc: sdhci-omap: Add sdhci_omap specific ops for enable_sdio_irq .../devicetree/bindings/mmc/sdhci-omap.txt | 2 + drivers/mmc/host/sdhci-omap.c | 78 +++++++++++++++++++--- drivers/mmc/host/sdhci.c | 75 +++++++++++++++++---- drivers/mmc/host/sdhci.h | 15 +++++ 4 files changed, 148 insertions(+), 22 deletions(-)