| Message ID | 1455913003-6140-5-git-send-email-wsa@the-dreams.de (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On 19 February 2016 at 21:16, Wolfram Sang <wsa@the-dreams.de> wrote: > From: Ben Hutchings <ben.hutchings@codethink.co.uk> > > Based on work by Shinobu Uehara and Ben Dooks. This adds the voltage > switch operation needed for all UHS-I modes, but not the tuning needed > for SDR-104 which will come later. > > The card_busy implementation is a bit of a guess, but works for me on > an R8A7790 chip. > > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/mmc/host/tmio_mmc.h | 3 +++ > drivers/mmc/host/tmio_mmc_pio.c | 31 +++++++++++++++++++++++++++++++ > 2 files changed, 34 insertions(+) > > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > index b44b5890290622..aabd36955e73fb 100644 > --- a/drivers/mmc/host/tmio_mmc.h > +++ b/drivers/mmc/host/tmio_mmc.h > @@ -101,6 +101,9 @@ struct tmio_mmc_host { > void (*clk_disable)(struct tmio_mmc_host *host); > int (*multi_io_quirk)(struct mmc_card *card, > unsigned int direction, int blk_size); > + > + int (*start_signal_voltage_switch)(struct tmio_mmc_host *host, > + unsigned char signal_voltage); Do you really need to add a new tmio specific callback for this? Why can't you instead have the tmio variant driver assign the ->start_signal_voltage_switch() callback into the struct mmc_host_ops instead? > }; > > 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 ae81b34f17a5a5..081a3def9305c1 100644 > --- a/drivers/mmc/host/tmio_mmc_pio.c > +++ b/drivers/mmc/host/tmio_mmc_pio.c > @@ -1012,12 +1012,43 @@ static int tmio_multi_io_quirk(struct mmc_card *card, > return blk_size; > } > > +static int tmio_mmc_start_signal_voltage_switch(struct mmc_host *mmc, > + struct mmc_ios *ios) > +{ > + struct tmio_mmc_host *host = mmc_priv(mmc); > + > + if (!host->start_signal_voltage_switch) > + return 0; > + > + return host->start_signal_voltage_switch(host, ios->signal_voltage); > +} > + > +static int tmio_mmc_card_busy(struct mmc_host *mmc) > +{ > + struct tmio_mmc_host *host = mmc_priv(mmc); > + u32 status; > + > + pm_runtime_get_sync(mmc_dev(mmc)); This isn't needed as the mmc core already deal with runtime PM of the host device via mmc_claim|release_host(). > + status = sd_ctrl_read32(host, CTL_STATUS); > + pm_runtime_mark_last_busy(mmc_dev(mmc)); > + pm_runtime_put_autosuspend(mmc_dev(mmc)); Ditto. > + > + /* > + * Card signals busy by pulling down all of DAT[3:0]. This status > + * flag appears to reflect DAT3. > + */ > + return !(status & TMIO_STAT_SIGSTATE_A); > +} > + > static const struct mmc_host_ops tmio_mmc_ops = { > .request = tmio_mmc_request, > .set_ios = tmio_mmc_set_ios, > .get_ro = tmio_mmc_get_ro, > .get_cd = mmc_gpio_get_cd, > .enable_sdio_irq = tmio_mmc_enable_sdio_irq, > + .start_signal_voltage_switch > + = tmio_mmc_start_signal_voltage_switch, > + .card_busy = tmio_mmc_card_busy, > .multi_io_quirk = tmio_multi_io_quirk, > }; > Kind regards Uffe
On Thu, Mar 03, 2016 at 03:41:13PM +0100, Ulf Hansson wrote: > On 19 February 2016 at 21:16, Wolfram Sang <wsa@the-dreams.de> wrote: > > From: Ben Hutchings <ben.hutchings@codethink.co.uk> > > > > Based on work by Shinobu Uehara and Ben Dooks. This adds the voltage > > switch operation needed for all UHS-I modes, but not the tuning needed > > for SDR-104 which will come later. > > > > The card_busy implementation is a bit of a guess, but works for me on > > an R8A7790 chip. > > > > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > --- > > drivers/mmc/host/tmio_mmc.h | 3 +++ > > drivers/mmc/host/tmio_mmc_pio.c | 31 +++++++++++++++++++++++++++++++ > > 2 files changed, 34 insertions(+) > > > > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > > index b44b5890290622..aabd36955e73fb 100644 > > --- a/drivers/mmc/host/tmio_mmc.h > > +++ b/drivers/mmc/host/tmio_mmc.h > > @@ -101,6 +101,9 @@ struct tmio_mmc_host { > > void (*clk_disable)(struct tmio_mmc_host *host); > > int (*multi_io_quirk)(struct mmc_card *card, > > unsigned int direction, int blk_size); > > + > > + int (*start_signal_voltage_switch)(struct tmio_mmc_host *host, > > + unsigned char signal_voltage); > > Do you really need to add a new tmio specific callback for this? > > Why can't you instead have the tmio variant driver assign the > ->start_signal_voltage_switch() callback into the struct mmc_host_ops > instead? Can do if you prefer that. I agree that it will save a bit of code; but it also hides this feature, because all the other config is in struct tmio_mmc_host. I like the better visibility a tad more.
> > + > > + int (*start_signal_voltage_switch)(struct tmio_mmc_host *host, > > + unsigned char signal_voltage); > > Do you really need to add a new tmio specific callback for this? > > Why can't you instead have the tmio variant driver assign the > ->start_signal_voltage_switch() callback into the struct mmc_host_ops > instead? Just to see how it looks, I tried your suggestion. However, mmc_ops is consted all the way into the core (rightfully, I'd say), so this makes this approach quite clumsy. I kept the above callback now, just changed the second paramater to pass the whole ios so I can use it with mmc_regulator_set_vqmmc(). > > +static int tmio_mmc_card_busy(struct mmc_host *mmc) > > +{ > > + struct tmio_mmc_host *host = mmc_priv(mmc); > > + u32 status; > > + > > + pm_runtime_get_sync(mmc_dev(mmc)); > > This isn't needed as the mmc core already deal with runtime PM of the > host device via mmc_claim|release_host(). sdhci.c does it, too - missing cleanup?
On 9 March 2016 at 17:17, Wolfram Sang <wsa@the-dreams.de> wrote: > >> > + >> > + int (*start_signal_voltage_switch)(struct tmio_mmc_host *host, >> > + unsigned char signal_voltage); >> >> Do you really need to add a new tmio specific callback for this? >> >> Why can't you instead have the tmio variant driver assign the >> ->start_signal_voltage_switch() callback into the struct mmc_host_ops >> instead? > > Just to see how it looks, I tried your suggestion. However, mmc_ops is > consted all the way into the core (rightfully, I'd say), so this makes > this approach quite clumsy. I kept the above callback now, just changed > the second paramater to pass the whole ios so I can use it with > mmc_regulator_set_vqmmc(). Okay, let me elaborate a bit more on what I really meant. In tmio_mmc_of_parse(), the mmc->ops gets assigned to the &tmio_mmc_ops. Before doing that, you can conditionally assign ->start_signal_voltage_switch() callback within the &tmio_mmc_ops, right!? Why am I suggesting this? I want to avoid us evolving the code in the wrong direction, similar what has happened to SDHCI. *One* way to address this, is to minimize unnecessary variant specific callbacks. > >> > +static int tmio_mmc_card_busy(struct mmc_host *mmc) >> > +{ >> > + struct tmio_mmc_host *host = mmc_priv(mmc); >> > + u32 status; >> > + >> > + pm_runtime_get_sync(mmc_dev(mmc)); >> >> This isn't needed as the mmc core already deal with runtime PM of the >> host device via mmc_claim|release_host(). > > sdhci.c does it, too - missing cleanup? We quite recently added this into the mmc core, before it was the controller driver itself who dealt with pm_runtime_get|put(). So, yes several drivers can be cleaned up. Kind regards Uffe
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index b44b5890290622..aabd36955e73fb 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -101,6 +101,9 @@ struct tmio_mmc_host { void (*clk_disable)(struct tmio_mmc_host *host); int (*multi_io_quirk)(struct mmc_card *card, unsigned int direction, int blk_size); + + int (*start_signal_voltage_switch)(struct tmio_mmc_host *host, + unsigned char signal_voltage); }; 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 ae81b34f17a5a5..081a3def9305c1 100644 --- a/drivers/mmc/host/tmio_mmc_pio.c +++ b/drivers/mmc/host/tmio_mmc_pio.c @@ -1012,12 +1012,43 @@ static int tmio_multi_io_quirk(struct mmc_card *card, return blk_size; } +static int tmio_mmc_start_signal_voltage_switch(struct mmc_host *mmc, + struct mmc_ios *ios) +{ + struct tmio_mmc_host *host = mmc_priv(mmc); + + if (!host->start_signal_voltage_switch) + return 0; + + return host->start_signal_voltage_switch(host, ios->signal_voltage); +} + +static int tmio_mmc_card_busy(struct mmc_host *mmc) +{ + struct tmio_mmc_host *host = mmc_priv(mmc); + u32 status; + + pm_runtime_get_sync(mmc_dev(mmc)); + status = sd_ctrl_read32(host, CTL_STATUS); + pm_runtime_mark_last_busy(mmc_dev(mmc)); + pm_runtime_put_autosuspend(mmc_dev(mmc)); + + /* + * Card signals busy by pulling down all of DAT[3:0]. This status + * flag appears to reflect DAT3. + */ + return !(status & TMIO_STAT_SIGSTATE_A); +} + static const struct mmc_host_ops tmio_mmc_ops = { .request = tmio_mmc_request, .set_ios = tmio_mmc_set_ios, .get_ro = tmio_mmc_get_ro, .get_cd = mmc_gpio_get_cd, .enable_sdio_irq = tmio_mmc_enable_sdio_irq, + .start_signal_voltage_switch + = tmio_mmc_start_signal_voltage_switch, + .card_busy = tmio_mmc_card_busy, .multi_io_quirk = tmio_multi_io_quirk, };