| Message ID | 20160727212457.20038-8-swarren@wwwdotorg.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Warren |
| Headers | show |
Hi Stephen, On 07/28/2016 06:24 AM, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> > > Tegra186 supports the new standard clock and reset APIs. Older Tegra SoCs > still use custom APIs. Enhance the Tegra MMC driver so that it can operate > with either set of APIs. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > Cc: Pantelis Antoniou <panto@antoniou-consulting.com> > --- > arch/arm/include/asm/arch-tegra/tegra_mmc.h | 8 ++++- > drivers/mmc/tegra_mmc.c | 55 ++++++++++++++++++++++++----- > 2 files changed, 53 insertions(+), 10 deletions(-) > > diff --git a/arch/arm/include/asm/arch-tegra/tegra_mmc.h b/arch/arm/include/asm/arch-tegra/tegra_mmc.h > index 75e56c4ea786..07ef4c04c858 100644 > --- a/arch/arm/include/asm/arch-tegra/tegra_mmc.h > +++ b/arch/arm/include/asm/arch-tegra/tegra_mmc.h > @@ -9,6 +9,9 @@ > #ifndef __TEGRA_MMC_H_ > #define __TEGRA_MMC_H_ > > +#include <common.h> > +#include <clk.h> > +#include <reset.h> > #include <fdtdec.h> > #include <asm/gpio.h> > > @@ -134,7 +137,10 @@ struct mmc_host { > int id; /* device id/number, 0-3 */ > int enabled; /* 1 to enable, 0 to disable */ > int width; /* Bus Width, 1, 4 or 8 */ > -#ifndef CONFIG_TEGRA186 > +#ifdef CONFIG_TEGRA186 > + struct reset_ctl reset_ctl; > + struct clk clk; > +#else > enum periph_id mmc_id; /* Peripheral ID: PERIPH_ID_... */ > #endif > struct gpio_desc cd_gpio; /* Change Detect GPIO */ > diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c > index c9d9432e5e87..b27ca635ac50 100644 > --- a/drivers/mmc/tegra_mmc.c > +++ b/drivers/mmc/tegra_mmc.c > @@ -9,6 +9,7 @@ > > #include <bouncebuf.h> > #include <common.h> > +#include <dm/device.h> > #include <asm/gpio.h> > #include <asm/io.h> > #ifndef CONFIG_TEGRA186 > @@ -359,11 +360,14 @@ static void mmc_change_clock(struct mmc_host *host, uint clock) > */ > if (clock == 0) > goto out; > -#ifndef CONFIG_TEGRA186 > +#ifdef CONFIG_TEGRA186 > + { > + ulong rate = clk_set_rate(&host->clk, clock); > + div = (rate + clock - 1) / clock; > + } It seems that doesn't need to add the bracket. > +#else > clock_adjust_periph_pll_div(host->mmc_id, CLOCK_ID_PERIPH, clock, > &div); > -#else > - div = (20000000 + clock - 1) / clock; > #endif > debug("div = %d\n", div); > > @@ -538,6 +542,9 @@ static int do_mmc_init(int dev_index, bool removable) > { > struct mmc_host *host; > struct mmc *mmc; > +#ifdef CONFIG_TEGRA186 > + int ret; > +#endif > > /* DT should have been read & host config filled in */ > host = &mmc_host[dev_index]; > @@ -549,7 +556,21 @@ static int do_mmc_init(int dev_index, bool removable) > gpio_get_number(&host->cd_gpio)); > > host->clock = 0; > -#ifndef CONFIG_TEGRA186 > + > +#ifdef CONFIG_TEGRA186 > + ret = reset_assert(&host->reset_ctl); > + if (ret) > + return ret; > + ret = clk_enable(&host->clk); > + if (ret) > + return ret; > + ret = clk_set_rate(&host->clk, 20000000); > + if (IS_ERR_VALUE(ret)) > + return ret; > + ret = reset_deassert(&host->reset_ctl); > + if (ret) > + return ret; > +#else > clock_start_periph_pll(host->mmc_id, CLOCK_ID_PERIPH, 20000000); > #endif > > @@ -576,11 +597,7 @@ static int do_mmc_init(int dev_index, bool removable) > * (actually 52MHz) > */ > host->cfg.f_min = 375000; > -#ifndef CONFIG_TEGRA186 > host->cfg.f_max = 48000000; > -#else > - host->cfg.f_max = 375000; > -#endif > > host->cfg.b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT; > > @@ -612,7 +629,27 @@ static int mmc_get_config(const void *blob, int node, struct mmc_host *host, > return -FDT_ERR_NOTFOUND; > } > > -#ifndef CONFIG_TEGRA186 > +#ifdef CONFIG_TEGRA186 > + { > + /* > + * FIXME: This variable should go away when the MMC device > + * actually is a udevice. > + */ > + struct udevice dev; > + int ret; > + dev.of_offset = node; > + ret = reset_get_by_name(&dev, "sdmmc", &host->reset_ctl); > + if (ret) { > + debug("reset_get_by_index() failed: %d\n", ret); > + return ret; > + } > + ret = clk_get_by_name(&dev, "sdmmc", &host->clk); > + if (ret) { > + debug("clk_get_by_index() failed: %d\n", ret); > + return ret; > + } > + } Ditto. Best Regards, Jaehoon Chung > +#else > host->mmc_id = clock_decode_periph_id(blob, node); > if (host->mmc_id == PERIPH_ID_NONE) { > debug("%s: could not decode periph id\n", __func__); >
On 07/27/2016 07:09 PM, Jaehoon Chung wrote: > Hi Stephen, > > On 07/28/2016 06:24 AM, Stephen Warren wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> Tegra186 supports the new standard clock and reset APIs. Older Tegra SoCs >> still use custom APIs. Enhance the Tegra MMC driver so that it can operate >> with either set of APIs. >> diff --git a/arch/arm/include/asm/arch-tegra/tegra_mmc.h b/arch/arm/include/asm/arch-tegra/tegra_mmc.h >> +#ifdef CONFIG_TEGRA186 >> + { >> + ulong rate = clk_set_rate(&host->clk, clock); >> + div = (rate + clock - 1) / clock; >> + } > > It seems that doesn't need to add the bracket. There's a variable declaration at the start of the block. Without the brackets, the compiler can/will complain about variable declarations being in the middle of a block.
Hi Stephen, On 27 July 2016 at 15:24, Stephen Warren <swarren@wwwdotorg.org> wrote: > From: Stephen Warren <swarren@nvidia.com> > > Tegra186 supports the new standard clock and reset APIs. Older Tegra SoCs > still use custom APIs. Enhance the Tegra MMC driver so that it can operate > with either set of APIs. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > Cc: Pantelis Antoniou <panto@antoniou-consulting.com> > --- > arch/arm/include/asm/arch-tegra/tegra_mmc.h | 8 ++++- > drivers/mmc/tegra_mmc.c | 55 ++++++++++++++++++++++++----- > 2 files changed, 53 insertions(+), 10 deletions(-) Shouldn't we fix up the code to all use the new APIs? - Simon
Hi Stephen, On 1 August 2016 at 09:50, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 07/31/2016 08:20 PM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 27 July 2016 at 15:24, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> >>> From: Stephen Warren <swarren@nvidia.com> >>> >>> Tegra186 supports the new standard clock and reset APIs. Older Tegra SoCs >>> still use custom APIs. Enhance the Tegra MMC driver so that it can >>> operate >>> with either set of APIs. >>> >>> Signed-off-by: Stephen Warren <swarren@nvidia.com> >>> Cc: Pantelis Antoniou <panto@antoniou-consulting.com> >>> --- >>> arch/arm/include/asm/arch-tegra/tegra_mmc.h | 8 ++++- >>> drivers/mmc/tegra_mmc.c | 55 >>> ++++++++++++++++++++++++----- >>> 2 files changed, 53 insertions(+), 10 deletions(-) >> >> >> Shouldn't we fix up the code to all use the new APIs? > > > Eventually yes. However, that's something that will take a lot of work. When > similar common APIs were introduced into Linux, there was a transition > period of 1-2 years where new code was immediately written to the new APIs, > and old code (e.g. legacy clock API implementation, and its callers) was > slowly converted. I would expect the same thing in U-Boot; any other > approach means preventing new work until the conversions are complete, which > would be rather stagnating. I still don't like the #ifdefs? Does Linux have #ifdefs in the mmc driver? Also the work to convert to CONFIG_BLK, CONFIG_DM_MMC_OPS is not a lot of work. Regards, Simon
On 08/03/2016 07:16 PM, Simon Glass wrote: > Hi Stephen, > > On 1 August 2016 at 09:50, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 07/31/2016 08:20 PM, Simon Glass wrote: >>> >>> Hi Stephen, >>> >>> On 27 July 2016 at 15:24, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> >>>> From: Stephen Warren <swarren@nvidia.com> >>>> >>>> Tegra186 supports the new standard clock and reset APIs. Older Tegra SoCs >>>> still use custom APIs. Enhance the Tegra MMC driver so that it can >>>> operate >>>> with either set of APIs. >>>> >>>> Signed-off-by: Stephen Warren <swarren@nvidia.com> >>>> Cc: Pantelis Antoniou <panto@antoniou-consulting.com> >>>> --- >>>> arch/arm/include/asm/arch-tegra/tegra_mmc.h | 8 ++++- >>>> drivers/mmc/tegra_mmc.c | 55 >>>> ++++++++++++++++++++++++----- >>>> 2 files changed, 53 insertions(+), 10 deletions(-) >>> >>> >>> Shouldn't we fix up the code to all use the new APIs? >> >> >> Eventually yes. However, that's something that will take a lot of work. When >> similar common APIs were introduced into Linux, there was a transition >> period of 1-2 years where new code was immediately written to the new APIs, >> and old code (e.g. legacy clock API implementation, and its callers) was >> slowly converted. I would expect the same thing in U-Boot; any other >> approach means preventing new work until the conversions are complete, which >> would be rather stagnating. > > I still don't like the #ifdefs? Does Linux have #ifdefs in the mmc driver? Linux is fully converted already. See my other response for more details. > Also the work to convert to CONFIG_BLK, CONFIG_DM_MMC_OPS is not a lot of work. Sure, but that's a separate API conversion. I really don't want to dump too many conversions, especially unrelated conversions, into a single patch or series. Besides, I could have sworn that either you or TomW had started work on that or agreed to do it?
diff --git a/arch/arm/include/asm/arch-tegra/tegra_mmc.h b/arch/arm/include/asm/arch-tegra/tegra_mmc.h index 75e56c4ea786..07ef4c04c858 100644 --- a/arch/arm/include/asm/arch-tegra/tegra_mmc.h +++ b/arch/arm/include/asm/arch-tegra/tegra_mmc.h @@ -9,6 +9,9 @@ #ifndef __TEGRA_MMC_H_ #define __TEGRA_MMC_H_ +#include <common.h> +#include <clk.h> +#include <reset.h> #include <fdtdec.h> #include <asm/gpio.h> @@ -134,7 +137,10 @@ struct mmc_host { int id; /* device id/number, 0-3 */ int enabled; /* 1 to enable, 0 to disable */ int width; /* Bus Width, 1, 4 or 8 */ -#ifndef CONFIG_TEGRA186 +#ifdef CONFIG_TEGRA186 + struct reset_ctl reset_ctl; + struct clk clk; +#else enum periph_id mmc_id; /* Peripheral ID: PERIPH_ID_... */ #endif struct gpio_desc cd_gpio; /* Change Detect GPIO */ diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c index c9d9432e5e87..b27ca635ac50 100644 --- a/drivers/mmc/tegra_mmc.c +++ b/drivers/mmc/tegra_mmc.c @@ -9,6 +9,7 @@ #include <bouncebuf.h> #include <common.h> +#include <dm/device.h> #include <asm/gpio.h> #include <asm/io.h> #ifndef CONFIG_TEGRA186 @@ -359,11 +360,14 @@ static void mmc_change_clock(struct mmc_host *host, uint clock) */ if (clock == 0) goto out; -#ifndef CONFIG_TEGRA186 +#ifdef CONFIG_TEGRA186 + { + ulong rate = clk_set_rate(&host->clk, clock); + div = (rate + clock - 1) / clock; + } +#else clock_adjust_periph_pll_div(host->mmc_id, CLOCK_ID_PERIPH, clock, &div); -#else - div = (20000000 + clock - 1) / clock; #endif debug("div = %d\n", div); @@ -538,6 +542,9 @@ static int do_mmc_init(int dev_index, bool removable) { struct mmc_host *host; struct mmc *mmc; +#ifdef CONFIG_TEGRA186 + int ret; +#endif /* DT should have been read & host config filled in */ host = &mmc_host[dev_index]; @@ -549,7 +556,21 @@ static int do_mmc_init(int dev_index, bool removable) gpio_get_number(&host->cd_gpio)); host->clock = 0; -#ifndef CONFIG_TEGRA186 + +#ifdef CONFIG_TEGRA186 + ret = reset_assert(&host->reset_ctl); + if (ret) + return ret; + ret = clk_enable(&host->clk); + if (ret) + return ret; + ret = clk_set_rate(&host->clk, 20000000); + if (IS_ERR_VALUE(ret)) + return ret; + ret = reset_deassert(&host->reset_ctl); + if (ret) + return ret; +#else clock_start_periph_pll(host->mmc_id, CLOCK_ID_PERIPH, 20000000); #endif @@ -576,11 +597,7 @@ static int do_mmc_init(int dev_index, bool removable) * (actually 52MHz) */ host->cfg.f_min = 375000; -#ifndef CONFIG_TEGRA186 host->cfg.f_max = 48000000; -#else - host->cfg.f_max = 375000; -#endif host->cfg.b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT; @@ -612,7 +629,27 @@ static int mmc_get_config(const void *blob, int node, struct mmc_host *host, return -FDT_ERR_NOTFOUND; } -#ifndef CONFIG_TEGRA186 +#ifdef CONFIG_TEGRA186 + { + /* + * FIXME: This variable should go away when the MMC device + * actually is a udevice. + */ + struct udevice dev; + int ret; + dev.of_offset = node; + ret = reset_get_by_name(&dev, "sdmmc", &host->reset_ctl); + if (ret) { + debug("reset_get_by_index() failed: %d\n", ret); + return ret; + } + ret = clk_get_by_name(&dev, "sdmmc", &host->clk); + if (ret) { + debug("clk_get_by_index() failed: %d\n", ret); + return ret; + } + } +#else host->mmc_id = clock_decode_periph_id(blob, node); if (host->mmc_id == PERIPH_ID_NONE) { debug("%s: could not decode periph id\n", __func__);