| Message ID | 20170714064302.20383-6-wens@csie.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 14 July 2017 at 08:42, Chen-Yu Tsai <wens@csie.org> wrote: > On the SoCs that introduced the new timing mode for MMC controllers, > both the old (where the clock delays are set in the CCU) and new > (where the clock delays are set in the MMC controller) timing modes > are available, and we have to support them both. However there are > two bits that control which mode is active. One is in the CCU, the > other is in the MMC controller. The settings on both sides must be > the same, or nothing will work. > > The CCU's get/set_phase callbacks return -ENOTSUPP when the new > timing mode is active. This provides a way to know which mode is > active on that side, and we can set the bit on the MMC controller > side accordingly. > > Signed-off-by: Chen-Yu Tsai <wens@csie.org> > --- > drivers/mmc/host/sunxi-mmc.c | 34 ++++++++++++++++++++++++++++++---- > 1 file changed, 30 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c > index 0fb4e4c119e1..56e45c65b52d 100644 > --- a/drivers/mmc/host/sunxi-mmc.c > +++ b/drivers/mmc/host/sunxi-mmc.c > @@ -22,6 +22,7 @@ > #include <linux/err.h> > > #include <linux/clk.h> > +#include <linux/clk/sunxi-ng.h> I don't like this. This looks like an SoC specific hack. > #include <linux/gpio.h> > #include <linux/platform_device.h> > #include <linux/spinlock.h> > @@ -259,7 +260,7 @@ struct sunxi_mmc_cfg { > /* Does DATA0 needs to be masked while the clock is updated */ > bool mask_data0; > > - bool needs_new_timings; > + bool has_new_timings; > }; > > struct sunxi_mmc_host { > @@ -293,6 +294,9 @@ struct sunxi_mmc_host { > > /* vqmmc */ > bool vqmmc_enabled; > + > + /* timings */ > + bool use_new_timings; > }; > > static int sunxi_mmc_reset_host(struct sunxi_mmc_host *host) > @@ -714,7 +718,7 @@ static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host, > { > int index; > > - if (!host->cfg->clk_delays) > + if (host->use_new_timings) > return 0; > > /* determine delays */ > @@ -765,6 +769,15 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host, > ios->bus_width == MMC_BUS_WIDTH_8) > clock <<= 1; > > + if (host->use_new_timings) { > + ret = sunxi_ccu_set_mmc_timing_mode(host->clk_mmc, true); Can't this be solved through some other generic API/interface? > + if (ret) { > + dev_err(mmc_dev(mmc), > + "error setting new timing mode\n"); > + return ret; > + } > + } > + [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 14, 2017 at 5:26 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 14 July 2017 at 08:42, Chen-Yu Tsai <wens@csie.org> wrote: >> On the SoCs that introduced the new timing mode for MMC controllers, >> both the old (where the clock delays are set in the CCU) and new >> (where the clock delays are set in the MMC controller) timing modes >> are available, and we have to support them both. However there are >> two bits that control which mode is active. One is in the CCU, the >> other is in the MMC controller. The settings on both sides must be >> the same, or nothing will work. >> >> The CCU's get/set_phase callbacks return -ENOTSUPP when the new >> timing mode is active. This provides a way to know which mode is >> active on that side, and we can set the bit on the MMC controller >> side accordingly. Argh... I forgot to update the commit log... :( >> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >> --- >> drivers/mmc/host/sunxi-mmc.c | 34 ++++++++++++++++++++++++++++++---- >> 1 file changed, 30 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c >> index 0fb4e4c119e1..56e45c65b52d 100644 >> --- a/drivers/mmc/host/sunxi-mmc.c >> +++ b/drivers/mmc/host/sunxi-mmc.c >> @@ -22,6 +22,7 @@ >> #include <linux/err.h> >> >> #include <linux/clk.h> >> +#include <linux/clk/sunxi-ng.h> > > I don't like this. This looks like an SoC specific hack. > >> #include <linux/gpio.h> >> #include <linux/platform_device.h> >> #include <linux/spinlock.h> >> @@ -259,7 +260,7 @@ struct sunxi_mmc_cfg { >> /* Does DATA0 needs to be masked while the clock is updated */ >> bool mask_data0; >> >> - bool needs_new_timings; >> + bool has_new_timings; >> }; >> >> struct sunxi_mmc_host { >> @@ -293,6 +294,9 @@ struct sunxi_mmc_host { >> >> /* vqmmc */ >> bool vqmmc_enabled; >> + >> + /* timings */ >> + bool use_new_timings; >> }; >> >> static int sunxi_mmc_reset_host(struct sunxi_mmc_host *host) >> @@ -714,7 +718,7 @@ static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host, >> { >> int index; >> >> - if (!host->cfg->clk_delays) >> + if (host->use_new_timings) >> return 0; >> >> /* determine delays */ >> @@ -765,6 +769,15 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host, >> ios->bus_width == MMC_BUS_WIDTH_8) >> clock <<= 1; >> >> + if (host->use_new_timings) { >> + ret = sunxi_ccu_set_mmc_timing_mode(host->clk_mmc, true); > > Can't this be solved through some other generic API/interface? The old discussion is here: https://lkml.org/lkml/2017/5/5/77 It is possible to piggy back on existing API, but as Maxime mentioned back in the discussion, it is confusing. IIRC Mike said (via Maxime) an SoC specific call was the easy way to handle this. I don't think there's anything generic about this. Even if you could have a _set_mode callback for the clks, the modes would be SoC specific anyway. ChenYu > >> + if (ret) { >> + dev_err(mmc_dev(mmc), >> + "error setting new timing mode\n"); >> + return ret; >> + } >> + } >> + > > [...] > > Kind regards > Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14 July 2017 at 11:40, Chen-Yu Tsai <wens@csie.org> wrote: > On Fri, Jul 14, 2017 at 5:26 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 14 July 2017 at 08:42, Chen-Yu Tsai <wens@csie.org> wrote: >>> On the SoCs that introduced the new timing mode for MMC controllers, >>> both the old (where the clock delays are set in the CCU) and new >>> (where the clock delays are set in the MMC controller) timing modes >>> are available, and we have to support them both. However there are >>> two bits that control which mode is active. One is in the CCU, the >>> other is in the MMC controller. The settings on both sides must be >>> the same, or nothing will work. >>> >>> The CCU's get/set_phase callbacks return -ENOTSUPP when the new >>> timing mode is active. This provides a way to know which mode is >>> active on that side, and we can set the bit on the MMC controller >>> side accordingly. > > Argh... I forgot to update the commit log... :( > >>> >>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>> --- >>> drivers/mmc/host/sunxi-mmc.c | 34 ++++++++++++++++++++++++++++++---- >>> 1 file changed, 30 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c >>> index 0fb4e4c119e1..56e45c65b52d 100644 >>> --- a/drivers/mmc/host/sunxi-mmc.c >>> +++ b/drivers/mmc/host/sunxi-mmc.c >>> @@ -22,6 +22,7 @@ >>> #include <linux/err.h> >>> >>> #include <linux/clk.h> >>> +#include <linux/clk/sunxi-ng.h> >> >> I don't like this. This looks like an SoC specific hack. >> >>> #include <linux/gpio.h> >>> #include <linux/platform_device.h> >>> #include <linux/spinlock.h> >>> @@ -259,7 +260,7 @@ struct sunxi_mmc_cfg { >>> /* Does DATA0 needs to be masked while the clock is updated */ >>> bool mask_data0; >>> >>> - bool needs_new_timings; >>> + bool has_new_timings; >>> }; >>> >>> struct sunxi_mmc_host { >>> @@ -293,6 +294,9 @@ struct sunxi_mmc_host { >>> >>> /* vqmmc */ >>> bool vqmmc_enabled; >>> + >>> + /* timings */ >>> + bool use_new_timings; >>> }; >>> >>> static int sunxi_mmc_reset_host(struct sunxi_mmc_host *host) >>> @@ -714,7 +718,7 @@ static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host, >>> { >>> int index; >>> >>> - if (!host->cfg->clk_delays) >>> + if (host->use_new_timings) >>> return 0; >>> >>> /* determine delays */ >>> @@ -765,6 +769,15 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host, >>> ios->bus_width == MMC_BUS_WIDTH_8) >>> clock <<= 1; >>> >>> + if (host->use_new_timings) { >>> + ret = sunxi_ccu_set_mmc_timing_mode(host->clk_mmc, true); >> >> Can't this be solved through some other generic API/interface? > > The old discussion is here: https://lkml.org/lkml/2017/5/5/77 > > It is possible to piggy back on existing API, but as Maxime mentioned > back in the discussion, it is confusing. > > IIRC Mike said (via Maxime) an SoC specific call was the easy way > to handle this. I don't think there's anything generic about this. > Even if you could have a _set_mode callback for the clks, the modes > would be SoC specific anyway. Right. But it would benefit that we can keep drivers generic, as they are using generic APIs/interfaces. I prefer that. Anyway, let me try to dig up the earlier discussion. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Fri, Jul 14, 2017 at 02:42:56PM +0800, Chen-Yu Tsai wrote: > On the SoCs that introduced the new timing mode for MMC controllers, > both the old (where the clock delays are set in the CCU) and new > (where the clock delays are set in the MMC controller) timing modes > are available, and we have to support them both. However there are > two bits that control which mode is active. One is in the CCU, the > other is in the MMC controller. The settings on both sides must be > the same, or nothing will work. > > The CCU's get/set_phase callbacks return -ENOTSUPP when the new > timing mode is active. This provides a way to know which mode is > active on that side, and we can set the bit on the MMC controller > side accordingly. > > Signed-off-by: Chen-Yu Tsai <wens@csie.org> > --- > drivers/mmc/host/sunxi-mmc.c | 34 ++++++++++++++++++++++++++++++---- > 1 file changed, 30 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c > index 0fb4e4c119e1..56e45c65b52d 100644 > --- a/drivers/mmc/host/sunxi-mmc.c > +++ b/drivers/mmc/host/sunxi-mmc.c > @@ -22,6 +22,7 @@ > #include <linux/err.h> > > #include <linux/clk.h> > +#include <linux/clk/sunxi-ng.h> > #include <linux/gpio.h> > #include <linux/platform_device.h> > #include <linux/spinlock.h> > @@ -259,7 +260,7 @@ struct sunxi_mmc_cfg { > /* Does DATA0 needs to be masked while the clock is updated */ > bool mask_data0; > > - bool needs_new_timings; > + bool has_new_timings; I think we should have both, it's a bit different. Newer SoCs like the A64 can only operate using new timings, while the older ones can operate in both modes. In one case, we're forced to use it, in the other one it's a policy. We should differentiate both cases. Looks good otherwise, thanks! Maxime
On Fri, Jul 14, 2017 at 11:57:35AM +0200, Ulf Hansson wrote: > >>> + if (host->use_new_timings) { > >>> + ret = sunxi_ccu_set_mmc_timing_mode(host->clk_mmc, true); > >> > >> Can't this be solved through some other generic API/interface? > > > > The old discussion is here: https://lkml.org/lkml/2017/5/5/77 > > > > It is possible to piggy back on existing API, but as Maxime mentioned > > back in the discussion, it is confusing. > > > > IIRC Mike said (via Maxime) an SoC specific call was the easy way > > to handle this. I don't think there's anything generic about this. > > Even if you could have a _set_mode callback for the clks, the modes > > would be SoC specific anyway. > > Right. But it would benefit that we can keep drivers generic, as they > are using generic APIs/interfaces. I prefer that. > > Anyway, let me try to dig up the earlier discussion. There's really not any generic way to support that. Even if we reuse some other function (clk_set_phase/clk_get_phase was suggested), and use error codes and / or values to differentiate between two modes, this will be very much implementation-specific as well, and any other SoC that in theory would be using that will very likely to not implement the same behaviour for its clocks. And this driver is only used on one SoC family, so it's not really a big deal anyway. Maxime
Hi Chen-Yu,
[auto build test ERROR on linus/master]
[also build test ERROR on v4.13-rc1]
[cannot apply to robh/for-next clk/clk-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Chen-Yu-Tsai/ARM-sun8i-a83t-Add-support-for-MMC-controllers/20170715-071008
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64
All errors (new ones prefixed by >>):
>> ERROR: "sunxi_ccu_set_mmc_timing_mode" [drivers/mmc/host/sunxi-mmc.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Mon, Jul 17, 2017 at 5:17 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Hi, > > On Fri, Jul 14, 2017 at 02:42:56PM +0800, Chen-Yu Tsai wrote: >> On the SoCs that introduced the new timing mode for MMC controllers, >> both the old (where the clock delays are set in the CCU) and new >> (where the clock delays are set in the MMC controller) timing modes >> are available, and we have to support them both. However there are >> two bits that control which mode is active. One is in the CCU, the >> other is in the MMC controller. The settings on both sides must be >> the same, or nothing will work. >> >> The CCU's get/set_phase callbacks return -ENOTSUPP when the new >> timing mode is active. This provides a way to know which mode is >> active on that side, and we can set the bit on the MMC controller >> side accordingly. >> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >> --- >> drivers/mmc/host/sunxi-mmc.c | 34 ++++++++++++++++++++++++++++++---- >> 1 file changed, 30 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c >> index 0fb4e4c119e1..56e45c65b52d 100644 >> --- a/drivers/mmc/host/sunxi-mmc.c >> +++ b/drivers/mmc/host/sunxi-mmc.c >> @@ -22,6 +22,7 @@ >> #include <linux/err.h> >> >> #include <linux/clk.h> >> +#include <linux/clk/sunxi-ng.h> >> #include <linux/gpio.h> >> #include <linux/platform_device.h> >> #include <linux/spinlock.h> >> @@ -259,7 +260,7 @@ struct sunxi_mmc_cfg { >> /* Does DATA0 needs to be masked while the clock is updated */ >> bool mask_data0; >> >> - bool needs_new_timings; >> + bool has_new_timings; > > I think we should have both, it's a bit different. Newer SoCs like the > A64 can only operate using new timings, while the older ones can > operate in both modes. > > In one case, we're forced to use it, in the other one it's a > policy. We should differentiate both cases. For the A64's case, the limit is implied by not having any clk_delays. But yes, I'll keep "needs_new_timings", and rename the new option to "has_timing_switch" to make things clearer. ChenYu > > Looks good otherwise, thanks! > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 19, 2017 at 04:59:23PM +0800, Chen-Yu Tsai wrote: > On Mon, Jul 17, 2017 at 5:17 PM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > Hi, > > > > On Fri, Jul 14, 2017 at 02:42:56PM +0800, Chen-Yu Tsai wrote: > >> On the SoCs that introduced the new timing mode for MMC controllers, > >> both the old (where the clock delays are set in the CCU) and new > >> (where the clock delays are set in the MMC controller) timing modes > >> are available, and we have to support them both. However there are > >> two bits that control which mode is active. One is in the CCU, the > >> other is in the MMC controller. The settings on both sides must be > >> the same, or nothing will work. > >> > >> The CCU's get/set_phase callbacks return -ENOTSUPP when the new > >> timing mode is active. This provides a way to know which mode is > >> active on that side, and we can set the bit on the MMC controller > >> side accordingly. > >> > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> > >> --- > >> drivers/mmc/host/sunxi-mmc.c | 34 ++++++++++++++++++++++++++++++---- > >> 1 file changed, 30 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c > >> index 0fb4e4c119e1..56e45c65b52d 100644 > >> --- a/drivers/mmc/host/sunxi-mmc.c > >> +++ b/drivers/mmc/host/sunxi-mmc.c > >> @@ -22,6 +22,7 @@ > >> #include <linux/err.h> > >> > >> #include <linux/clk.h> > >> +#include <linux/clk/sunxi-ng.h> > >> #include <linux/gpio.h> > >> #include <linux/platform_device.h> > >> #include <linux/spinlock.h> > >> @@ -259,7 +260,7 @@ struct sunxi_mmc_cfg { > >> /* Does DATA0 needs to be masked while the clock is updated */ > >> bool mask_data0; > >> > >> - bool needs_new_timings; > >> + bool has_new_timings; > > > > I think we should have both, it's a bit different. Newer SoCs like the > > A64 can only operate using new timings, while the older ones can > > operate in both modes. > > > > In one case, we're forced to use it, in the other one it's a > > policy. We should differentiate both cases. > > For the A64's case, the limit is implied by not having any clk_delays. FWIW, I'm really not a big fan of that either :) Explicit is better than implicit.© > But yes, I'll keep "needs_new_timings", and rename the new option to > "has_timing_switch" to make things clearer. Great, thanks! Maxime
diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c index 0fb4e4c119e1..56e45c65b52d 100644 --- a/drivers/mmc/host/sunxi-mmc.c +++ b/drivers/mmc/host/sunxi-mmc.c @@ -22,6 +22,7 @@ #include <linux/err.h> #include <linux/clk.h> +#include <linux/clk/sunxi-ng.h> #include <linux/gpio.h> #include <linux/platform_device.h> #include <linux/spinlock.h> @@ -259,7 +260,7 @@ struct sunxi_mmc_cfg { /* Does DATA0 needs to be masked while the clock is updated */ bool mask_data0; - bool needs_new_timings; + bool has_new_timings; }; struct sunxi_mmc_host { @@ -293,6 +294,9 @@ struct sunxi_mmc_host { /* vqmmc */ bool vqmmc_enabled; + + /* timings */ + bool use_new_timings; }; static int sunxi_mmc_reset_host(struct sunxi_mmc_host *host) @@ -714,7 +718,7 @@ static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host, { int index; - if (!host->cfg->clk_delays) + if (host->use_new_timings) return 0; /* determine delays */ @@ -765,6 +769,15 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host, ios->bus_width == MMC_BUS_WIDTH_8) clock <<= 1; + if (host->use_new_timings) { + ret = sunxi_ccu_set_mmc_timing_mode(host->clk_mmc, true); + if (ret) { + dev_err(mmc_dev(mmc), + "error setting new timing mode\n"); + return ret; + } + } + rate = clk_round_rate(host->clk_mmc, clock); if (rate < 0) { dev_err(mmc_dev(mmc), "error rounding clk to %d: %ld\n", @@ -793,7 +806,7 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host, } mmc_writel(host, REG_CLKCR, rval); - if (host->cfg->needs_new_timings) { + if (host->use_new_timings) { /* Don't touch the delay bits */ rval = mmc_readl(host, REG_SD_NTSR); rval |= SDXC_2X_TIMING_MODE; @@ -1105,7 +1118,7 @@ static const struct sunxi_mmc_cfg sun50i_a64_cfg = { .clk_delays = NULL, .can_calibrate = true, .mask_data0 = true, - .needs_new_timings = true, + .has_new_timings = true, }; static const struct sunxi_mmc_cfg sun50i_a64_emmc_cfg = { @@ -1262,6 +1275,19 @@ static int sunxi_mmc_probe(struct platform_device *pdev) goto error_free_host; } + if (host->cfg->clk_delays && host->cfg->has_new_timings) { + /* + * Supports both old and new timing modes. + * Try setting the clk to new timing mode. + */ + ret = sunxi_ccu_set_mmc_timing_mode(host->clk_mmc, true); + if (!ret) + host->use_new_timings = true; + } else if (host->cfg->has_new_timings) { + /* Supports new timing mode only */ + host->use_new_timings = true; + } + mmc->ops = &sunxi_mmc_ops; mmc->max_blk_count = 8192; mmc->max_blk_size = 4096;
On the SoCs that introduced the new timing mode for MMC controllers, both the old (where the clock delays are set in the CCU) and new (where the clock delays are set in the MMC controller) timing modes are available, and we have to support them both. However there are two bits that control which mode is active. One is in the CCU, the other is in the MMC controller. The settings on both sides must be the same, or nothing will work. The CCU's get/set_phase callbacks return -ENOTSUPP when the new timing mode is active. This provides a way to know which mode is active on that side, and we can set the bit on the MMC controller side accordingly. Signed-off-by: Chen-Yu Tsai <wens@csie.org> --- drivers/mmc/host/sunxi-mmc.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-)