Message ID | 20200409175238.3586487-8-thierry.reding@gmail.com (mailing list archive) |
---|---|
State | Awaiting Upstream, archived |
Headers | show |
Series | Add EMC scaling support for Tegra210 | expand |
09.04.2020 20:52, Thierry Reding пишет: ... > +static long tegra210_clk_emc_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *prate) > +{ > + struct tegra210_clk_emc *emc = to_tegra210_clk_emc(hw); > + struct tegra210_clk_emc_provider *provider = emc->provider; > + unsigned int i; > + > + if (!provider || !provider->configs || provider->num_configs == 0) > + return clk_hw_get_rate(hw); This still looks wrong to me. Nobody should be able to get EMC clock until provider is registered. This is troublesome, especially given that you're allowing the EMC driver to be compiled as a loadable module. For example, this won't work with the current ACTMON driver because it builds OPP table based on the clk-rate rounding during the driver's probe, so it won't be able to do it properly if provider is "temporarily" missing. ... I think that in a longer run we should stop manually building the ACTMON's OPP table and instead define a proper OPP table (per-HW Speedo ID, with voltages) in a device-tree. But this is just a vague plans for the future for now.
09.04.2020 20:52, Thierry Reding пишет: ... > +int tegra210_clk_emc_attach(struct clk *clk, > + struct tegra210_clk_emc_provider *provider) > +{ > + struct clk_hw *hw = __clk_get_hw(clk); > + struct tegra210_clk_emc *emc = to_tegra210_clk_emc(hw); > + struct device *dev = provider->dev; > + unsigned int i; > + int err; > + > + if (!try_module_get(provider->owner)) > + return -ENODEV; Is the EMC driver module bumping its own refcount by itself? In the other patch I already suggested that the EMC module should be disallowed to be unloaded once it has been loaded, seems you're already doing it. Correct?
On Thu, Apr 09, 2020 at 09:24:31PM +0300, Dmitry Osipenko wrote: > 09.04.2020 20:52, Thierry Reding пишет: > ... > > +static long tegra210_clk_emc_round_rate(struct clk_hw *hw, unsigned long rate, > > + unsigned long *prate) > > +{ > > + struct tegra210_clk_emc *emc = to_tegra210_clk_emc(hw); > > + struct tegra210_clk_emc_provider *provider = emc->provider; > > + unsigned int i; > > + > > + if (!provider || !provider->configs || provider->num_configs == 0) > > + return clk_hw_get_rate(hw); > > This still looks wrong to me. Nobody should be able to get EMC clock > until provider is registered. The EMC clock is mostly orthogonal to the provider. The provider really only allows you to actually change the frequency. The clock will still remain even if the provider goes away, it just will loose the ability to change rate. > This is troublesome, especially given that you're allowing the EMC > driver to be compiled as a loadable module. For example, this won't work > with the current ACTMON driver because it builds OPP table based on the > clk-rate rounding during the driver's probe, so it won't be able to do > it properly if provider is "temporarily" missing. > > ... I think that in a longer run we should stop manually building the > ACTMON's OPP table and instead define a proper OPP table (per-HW Speedo > ID, with voltages) in a device-tree. But this is just a vague plans for > the future for now. This code only applies to Tegra210 and we don't currently support ACTMON on Tegra210. I'm also not sure we'll ever do because using interconnects to describe paths to system memory and then using ICC requests for each driver to submit memory bandwidth requests seems like a better way of dealing with this problem than using ACTMON to monitor activity because that only allows you to react, whereas we really want to be able to allocate memory bandwidth upfront. Thierry
On Fri, Apr 10, 2020 at 11:49:18PM +0300, Dmitry Osipenko wrote: > 09.04.2020 20:52, Thierry Reding пишет: > ... > > +int tegra210_clk_emc_attach(struct clk *clk, > > + struct tegra210_clk_emc_provider *provider) > > +{ > > + struct clk_hw *hw = __clk_get_hw(clk); > > + struct tegra210_clk_emc *emc = to_tegra210_clk_emc(hw); > > + struct device *dev = provider->dev; > > + unsigned int i; > > + int err; > > + > > + if (!try_module_get(provider->owner)) > > + return -ENODEV; > > Is the EMC driver module bumping its own refcount by itself? Hm... this doesn't really look useful in retrospect. Yes, the EMC is effectively grabbing a reference to itself. > In the other patch I already suggested that the EMC module should be > disallowed to be unloaded once it has been loaded, seems you're already > doing it. Correct? Interestingly, it all seems to work fine if I unload the module at runtime, not sure why I'm not prevented from unloading the module. Thierry
14.04.2020 17:34, Thierry Reding пишет: > On Thu, Apr 09, 2020 at 09:24:31PM +0300, Dmitry Osipenko wrote: >> 09.04.2020 20:52, Thierry Reding пишет: >> ... >>> +static long tegra210_clk_emc_round_rate(struct clk_hw *hw, unsigned long rate, >>> + unsigned long *prate) >>> +{ >>> + struct tegra210_clk_emc *emc = to_tegra210_clk_emc(hw); >>> + struct tegra210_clk_emc_provider *provider = emc->provider; >>> + unsigned int i; >>> + >>> + if (!provider || !provider->configs || provider->num_configs == 0) >>> + return clk_hw_get_rate(hw); >> >> This still looks wrong to me. Nobody should be able to get EMC clock >> until provider is registered. > > The EMC clock is mostly orthogonal to the provider. The provider really > only allows you to actually change the frequency. The clock will still > remain even if the provider goes away, it just will loose the ability to > change rate. It's not only about changing the clock rate, but also about rounding the rate and etc. Besides, you won't be able to change the rate until provider is registered, which might be a quite big problem by itself. >> This is troublesome, especially given that you're allowing the EMC >> driver to be compiled as a loadable module. For example, this won't work >> with the current ACTMON driver because it builds OPP table based on the >> clk-rate rounding during the driver's probe, so it won't be able to do >> it properly if provider is "temporarily" missing. >> >> ... I think that in a longer run we should stop manually building the >> ACTMON's OPP table and instead define a proper OPP table (per-HW Speedo >> ID, with voltages) in a device-tree. But this is just a vague plans for >> the future for now. > > This code only applies to Tegra210 and we don't currently support ACTMON > on Tegra210. I'm also not sure we'll ever do because using interconnects > to describe paths to system memory and then using ICC requests for each > driver to submit memory bandwidth requests seems like a better way of > dealing with this problem than using ACTMON to monitor activity because > that only allows you to react, whereas we really want to be able to > allocate memory bandwidth upfront. You absolutely have to have the ACTMON support if you want to provide a good user experience because interconnect won't take into account the dynamic CPU memory traffic. Without ACTMON support CPU will turn into a "turtle" if memory runs on a lowest freq, while CPU needs the highest. Secondly, the interconnect could underestimate the memory BW requirement because memory performance depends quite a lot on the memory-accessing patterns and it's not possible to predict it properly. Otherwise you may need to always overestimate the BW, which perhaps is not what anyone would really want to have. I'm not sure why you're resisting to do it all properly from the start, it looks to me that it will take you just a few lines of code (like in a case of the T20/30 EMC).
On Tue, Apr 14, 2020 at 06:18:29PM +0300, Dmitry Osipenko wrote: > 14.04.2020 17:34, Thierry Reding пишет: > > On Thu, Apr 09, 2020 at 09:24:31PM +0300, Dmitry Osipenko wrote: > >> 09.04.2020 20:52, Thierry Reding пишет: > >> ... > >>> +static long tegra210_clk_emc_round_rate(struct clk_hw *hw, unsigned long rate, > >>> + unsigned long *prate) > >>> +{ > >>> + struct tegra210_clk_emc *emc = to_tegra210_clk_emc(hw); > >>> + struct tegra210_clk_emc_provider *provider = emc->provider; > >>> + unsigned int i; > >>> + > >>> + if (!provider || !provider->configs || provider->num_configs == 0) > >>> + return clk_hw_get_rate(hw); > >> > >> This still looks wrong to me. Nobody should be able to get EMC clock > >> until provider is registered. > > > > The EMC clock is mostly orthogonal to the provider. The provider really > > only allows you to actually change the frequency. The clock will still > > remain even if the provider goes away, it just will loose the ability to > > change rate. > > It's not only about changing the clock rate, but also about rounding the > rate and etc. The code will currently just return the configured rate when no provider is available. It's going to always round to that one rate and it will refuse to set another one. The EMC clock is basically going to function as a fixed clock while no provider is attached. > Besides, you won't be able to change the rate until provider is > registered, which might be a quite big problem by itself. Until the provider is registered, there's just no way to change the rate. You always need to write MC and EMC registers in order to change the rate, so trying to change it when the MC/EMC drivers aren't available isn't going to work. > >> This is troublesome, especially given that you're allowing the EMC > >> driver to be compiled as a loadable module. For example, this won't work > >> with the current ACTMON driver because it builds OPP table based on the > >> clk-rate rounding during the driver's probe, so it won't be able to do > >> it properly if provider is "temporarily" missing. > >> > >> ... I think that in a longer run we should stop manually building the > >> ACTMON's OPP table and instead define a proper OPP table (per-HW Speedo > >> ID, with voltages) in a device-tree. But this is just a vague plans for > >> the future for now. > > > > This code only applies to Tegra210 and we don't currently support ACTMON > > on Tegra210. I'm also not sure we'll ever do because using interconnects > > to describe paths to system memory and then using ICC requests for each > > driver to submit memory bandwidth requests seems like a better way of > > dealing with this problem than using ACTMON to monitor activity because > > that only allows you to react, whereas we really want to be able to > > allocate memory bandwidth upfront. > > You absolutely have to have the ACTMON support if you want to provide a > good user experience because interconnect won't take into account the > dynamic CPU memory traffic. Without ACTMON support CPU will turn into a > "turtle" if memory runs on a lowest freq, while CPU needs the highest. Can we not guess a bandwidth based on the CPU frequency? Yes, that's perhaps going to be an overestimation if the CPU doesn't actually access memory, but that's better than nothing at all. Also, at this point I'm less worried about power consumption rather than making Tegra210 devices perform useful tasks. Yes, eventually we'll want to fine-tune power consumption, but it's going to take a bit of work to get there. In the meantime, giving people a way to set an EMC frequency other than that set on boot is going to make them very happy. > Secondly, the interconnect could underestimate the memory BW requirement > because memory performance depends quite a lot on the memory-accessing > patterns and it's not possible to predict it properly. Otherwise you may > need to always overestimate the BW, which perhaps is not what anyone > would really want to have. Overestimating might be a good starting point, though. At this point I'm mostly concerned about being able to change the memory frequency at all because many systems are mostly unusable at the boot EMC frequency. Like I said, if ACTMON really does prove to be useful I'm all for adding support on Tegra210, but I don't think trying to do everything all at once is a very good plan. So I'm trying to get there in incremental steps. > I'm not sure why you're resisting to do it all properly from the start, > it looks to me that it will take you just a few lines of code (like in a > case of the T20/30 EMC). I'm not trying to resist anything. I'm just saying that all of the issues that you're bringing up aren't an immediate concern. My main concerns right now are to: a) allow people to change the EMC frequency (and hopefully soon also allow the EMC frequency to be changed based on bandwidth demands by memory client drivers) and b) not bloat the kernel more than it has to (while my configuration isn't tweaked, it's pretty standard and the resulting image is roughly 20 MiB; adding the Tegra210 EMC driver adds another 64 KiB). And if we really do want to add ACTMON support later on, you already suggested a better way of moving forward, so it sounds to me like that would be a nice incremental improvement, certainly much better than bloating the kernel even further by requiring this to be built-in and preventing it from being unloaded. Thierry
14.04.2020 20:10, Thierry Reding пишет: > On Tue, Apr 14, 2020 at 06:18:29PM +0300, Dmitry Osipenko wrote: >> 14.04.2020 17:34, Thierry Reding пишет: >>> On Thu, Apr 09, 2020 at 09:24:31PM +0300, Dmitry Osipenko wrote: >>>> 09.04.2020 20:52, Thierry Reding пишет: >>>> ... >>>>> +static long tegra210_clk_emc_round_rate(struct clk_hw *hw, unsigned long rate, >>>>> + unsigned long *prate) >>>>> +{ >>>>> + struct tegra210_clk_emc *emc = to_tegra210_clk_emc(hw); >>>>> + struct tegra210_clk_emc_provider *provider = emc->provider; >>>>> + unsigned int i; >>>>> + >>>>> + if (!provider || !provider->configs || provider->num_configs == 0) >>>>> + return clk_hw_get_rate(hw); >>>> >>>> This still looks wrong to me. Nobody should be able to get EMC clock >>>> until provider is registered. >>> >>> The EMC clock is mostly orthogonal to the provider. The provider really >>> only allows you to actually change the frequency. The clock will still >>> remain even if the provider goes away, it just will loose the ability to >>> change rate. >> >> It's not only about changing the clock rate, but also about rounding the >> rate and etc. > > The code will currently just return the configured rate when no provider > is available. It's going to always round to that one rate and it will > refuse to set another one. The EMC clock is basically going to function > as a fixed clock while no provider is attached. Yes, I'm telling that this should be a wrong thing to do. >> Besides, you won't be able to change the rate until provider is >> registered, which might be a quite big problem by itself. > > Until the provider is registered, there's just no way to change the > rate. You always need to write MC and EMC registers in order to change > the rate, so trying to change it when the MC/EMC drivers aren't > available isn't going to work. Again, clk_get() should return -EPROBE_DEFER until provider is registered. >>>> This is troublesome, especially given that you're allowing the EMC >>>> driver to be compiled as a loadable module. For example, this won't work >>>> with the current ACTMON driver because it builds OPP table based on the >>>> clk-rate rounding during the driver's probe, so it won't be able to do >>>> it properly if provider is "temporarily" missing. >>>> >>>> ... I think that in a longer run we should stop manually building the >>>> ACTMON's OPP table and instead define a proper OPP table (per-HW Speedo >>>> ID, with voltages) in a device-tree. But this is just a vague plans for >>>> the future for now. >>> >>> This code only applies to Tegra210 and we don't currently support ACTMON >>> on Tegra210. I'm also not sure we'll ever do because using interconnects >>> to describe paths to system memory and then using ICC requests for each >>> driver to submit memory bandwidth requests seems like a better way of >>> dealing with this problem than using ACTMON to monitor activity because >>> that only allows you to react, whereas we really want to be able to >>> allocate memory bandwidth upfront. >> >> You absolutely have to have the ACTMON support if you want to provide a >> good user experience because interconnect won't take into account the >> dynamic CPU memory traffic. Without ACTMON support CPU will turn into a >> "turtle" if memory runs on a lowest freq, while CPU needs the highest. > > Can we not guess a bandwidth based on the CPU frequency? Yes, that's > perhaps going to be an overestimation if the CPU doesn't actually access > memory, but that's better than nothing at all. CPU load doesn't reflect the memory usage. 1. CPU could be 100% loaded while not making memory accesses at all (100% cache hit). 2. CPU governor settings could be changed by user and CPU != memory. ACTMON is specifically designated to memory scaling based on actual memory usage statistics. T210 ACTMON is similar to T124, it should be easy to support. > Also, at this point I'm less worried about power consumption rather than > making Tegra210 devices perform useful tasks. Yes, eventually we'll want > to fine-tune power consumption, but it's going to take a bit of work to > get there. In the meantime, giving people a way to set an EMC frequency > other than that set on boot is going to make them very happy. > >> Secondly, the interconnect could underestimate the memory BW requirement >> because memory performance depends quite a lot on the memory-accessing >> patterns and it's not possible to predict it properly. Otherwise you may >> need to always overestimate the BW, which perhaps is not what anyone >> would really want to have. > > Overestimating might be a good starting point, though. At this point I'm > mostly concerned about being able to change the memory frequency at all > because many systems are mostly unusable at the boot EMC frequency. > > Like I said, if ACTMON really does prove to be useful I'm all for adding > support on Tegra210, but I don't think trying to do everything all at > once is a very good plan. So I'm trying to get there in incremental > steps. > >> I'm not sure why you're resisting to do it all properly from the start, >> it looks to me that it will take you just a few lines of code (like in a >> case of the T20/30 EMC). > > I'm not trying to resist anything. I'm just saying that all of the > issues that you're bringing up aren't an immediate concern. > > My main concerns right now are to: a) allow people to change the EMC > frequency (and hopefully soon also allow the EMC frequency to be changed > based on bandwidth demands by memory client drivers) and b) not bloat > the kernel more than it has to (while my configuration isn't tweaked, > it's pretty standard and the resulting image is roughly 20 MiB; adding > the Tegra210 EMC driver adds another 64 KiB). > > And if we really do want to add ACTMON support later on, you already > suggested a better way of moving forward, so it sounds to me like that > would be a nice incremental improvement, certainly much better than > bloating the kernel even further by requiring this to be built-in and > preventing it from being unloaded. Up to you then :)
diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile index dec508ef2ada..fc369a5d21dc 100644 --- a/drivers/clk/tegra/Makefile +++ b/drivers/clk/tegra/Makefile @@ -25,5 +25,6 @@ obj-$(CONFIG_TEGRA_CLK_EMC) += clk-tegra124-emc.o obj-$(CONFIG_ARCH_TEGRA_132_SOC) += clk-tegra124.o obj-y += cvb.o obj-$(CONFIG_ARCH_TEGRA_210_SOC) += clk-tegra210.o +obj-$(CONFIG_ARCH_TEGRA_210_SOC) += clk-tegra210-emc.o obj-$(CONFIG_CLK_TEGRA_BPMP) += clk-bpmp.o obj-y += clk-utils.o diff --git a/drivers/clk/tegra/clk-tegra210-emc.c b/drivers/clk/tegra/clk-tegra210-emc.c new file mode 100644 index 000000000000..352a2c3fc374 --- /dev/null +++ b/drivers/clk/tegra/clk-tegra210-emc.c @@ -0,0 +1,369 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2015-2020, NVIDIA CORPORATION. All rights reserved. + */ + +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/clk/tegra.h> +#include <linux/device.h> +#include <linux/module.h> +#include <linux/io.h> +#include <linux/slab.h> + +#define CLK_SOURCE_EMC 0x19c +#define CLK_SOURCE_EMC_2X_CLK_SRC GENMASK(31, 29) +#define CLK_SOURCE_EMC_MC_EMC_SAME_FREQ BIT(16) +#define CLK_SOURCE_EMC_2X_CLK_DIVISOR GENMASK(7, 0) + +#define CLK_SRC_PLLM 0 +#define CLK_SRC_PLLC 1 +#define CLK_SRC_PLLP 2 +#define CLK_SRC_CLK_M 3 +#define CLK_SRC_PLLM_UD 4 +#define CLK_SRC_PLLMB_UD 5 +#define CLK_SRC_PLLMB 6 +#define CLK_SRC_PLLP_UD 7 + +struct tegra210_clk_emc { + struct clk_hw hw; + void __iomem *regs; + + struct tegra210_clk_emc_provider *provider; + + struct clk *parents[8]; +}; + +static inline struct tegra210_clk_emc * +to_tegra210_clk_emc(struct clk_hw *hw) +{ + return container_of(hw, struct tegra210_clk_emc, hw); +} + +static const char *tegra210_clk_emc_parents[] = { + "pll_m", "pll_c", "pll_p", "clk_m", "pll_m_ud", "pll_mb_ud", + "pll_mb", "pll_p_ud", +}; + +static u8 tegra210_clk_emc_get_parent(struct clk_hw *hw) +{ + struct tegra210_clk_emc *emc = to_tegra210_clk_emc(hw); + u32 value; + u8 src; + + value = readl_relaxed(emc->regs + CLK_SOURCE_EMC); + src = FIELD_GET(CLK_SOURCE_EMC_2X_CLK_SRC, value); + + return src; +} + +static unsigned long tegra210_clk_emc_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + struct tegra210_clk_emc *emc = to_tegra210_clk_emc(hw); + u32 value, div; + + /* + * CCF assumes that neither the parent nor its rate will change during + * ->set_rate(), so the parent rate passed in here was cached from the + * parent before the ->set_rate() call. + * + * This can lead to wrong results being reported for the EMC clock if + * the parent and/or parent rate have changed as part of the EMC rate + * change sequence. Fix this by overriding the parent clock with what + * we know to be the correct value after the rate change. + */ + parent_rate = clk_hw_get_rate(clk_hw_get_parent(hw)); + + value = readl_relaxed(emc->regs + CLK_SOURCE_EMC); + + div = FIELD_GET(CLK_SOURCE_EMC_2X_CLK_DIVISOR, value); + div += 2; + + return DIV_ROUND_UP(parent_rate * 2, div); +} + +static long tegra210_clk_emc_round_rate(struct clk_hw *hw, unsigned long rate, + unsigned long *prate) +{ + struct tegra210_clk_emc *emc = to_tegra210_clk_emc(hw); + struct tegra210_clk_emc_provider *provider = emc->provider; + unsigned int i; + + if (!provider || !provider->configs || provider->num_configs == 0) + return clk_hw_get_rate(hw); + + for (i = 0; i < provider->num_configs; i++) { + if (provider->configs[i].rate >= rate) + return provider->configs[i].rate; + } + + return provider->configs[i - 1].rate; +} + +static struct clk *tegra210_clk_emc_find_parent(struct tegra210_clk_emc *emc, + u8 index) +{ + struct clk_hw *parent = clk_hw_get_parent_by_index(&emc->hw, index); + const char *name = clk_hw_get_name(parent); + + /* XXX implement cache? */ + + return __clk_lookup(name); +} + +static int tegra210_clk_emc_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + struct tegra210_clk_emc *emc = to_tegra210_clk_emc(hw); + struct tegra210_clk_emc_provider *provider = emc->provider; + struct tegra210_clk_emc_config *config; + struct device *dev = provider->dev; + struct clk_hw *old, *new, *parent; + u8 old_idx, new_idx, index; + struct clk *clk; + unsigned int i; + int err; + + if (!provider || !provider->configs || provider->num_configs == 0) + return -EINVAL; + + for (i = 0; i < provider->num_configs; i++) { + if (provider->configs[i].rate >= rate) { + config = &provider->configs[i]; + break; + } + } + + if (i == provider->num_configs) + config = &provider->configs[i - 1]; + + old_idx = tegra210_clk_emc_get_parent(hw); + new_idx = FIELD_GET(CLK_SOURCE_EMC_2X_CLK_SRC, config->value); + + old = clk_hw_get_parent_by_index(hw, old_idx); + new = clk_hw_get_parent_by_index(hw, new_idx); + + /* if the rate has changed... */ + if (config->parent_rate != clk_hw_get_rate(old)) { + /* ... but the clock source remains the same ... */ + if (new_idx == old_idx) { + /* ... switch to the alternative clock source. */ + switch (new_idx) { + case CLK_SRC_PLLM: + new_idx = CLK_SRC_PLLMB; + break; + + case CLK_SRC_PLLM_UD: + new_idx = CLK_SRC_PLLMB_UD; + break; + + case CLK_SRC_PLLMB_UD: + new_idx = CLK_SRC_PLLM_UD; + break; + + case CLK_SRC_PLLMB: + new_idx = CLK_SRC_PLLM; + break; + } + + /* + * This should never happen because we can't deal with + * it. + */ + if (WARN_ON(new_idx == old_idx)) + return -EINVAL; + + new = clk_hw_get_parent_by_index(hw, new_idx); + } + + index = new_idx; + parent = new; + } else { + index = old_idx; + parent = old; + } + + clk = tegra210_clk_emc_find_parent(emc, index); + if (IS_ERR(clk)) { + err = PTR_ERR(clk); + dev_err(dev, "failed to get parent clock for index %u: %d\n", + index, err); + return err; + } + + /* set the new parent clock to the required rate */ + if (clk_get_rate(clk) != config->parent_rate) { + err = clk_set_rate(clk, config->parent_rate); + if (err < 0) { + dev_err(dev, "failed to set rate %lu Hz for %pC: %d\n", + config->parent_rate, clk, err); + return err; + } + } + + /* enable the new parent clock */ + if (parent != old) { + err = clk_prepare_enable(clk); + if (err < 0) { + dev_err(dev, "failed to enable parent clock %pC: %d\n", + clk, err); + return err; + } + } + + /* update the EMC source configuration to reflect the new parent */ + config->value &= ~CLK_SOURCE_EMC_2X_CLK_SRC; + config->value |= FIELD_PREP(CLK_SOURCE_EMC_2X_CLK_SRC, index); + + /* + * Finally, switch the EMC programming with both old and new parent + * clocks enabled. + */ + err = provider->set_rate(dev, config); + if (err < 0) { + dev_err(dev, "failed to set EMC rate to %lu Hz: %d\n", rate, + err); + + /* + * If we're unable to switch to the new EMC frequency, we no + * longer need the new parent to be enabled. + */ + if (parent != old) + clk_disable_unprepare(clk); + + return err; + } + + /* reparent to new parent clock and disable the old parent clock */ + if (parent != old) { + clk = tegra210_clk_emc_find_parent(emc, old_idx); + if (IS_ERR(clk)) { + err = PTR_ERR(clk); + dev_err(dev, + "failed to get parent clock for index %u: %d\n", + old_idx, err); + return err; + } + + clk_hw_reparent(hw, parent); + clk_disable_unprepare(clk); + } + + return err; +} + +static const struct clk_ops tegra210_clk_emc_ops = { + .get_parent = tegra210_clk_emc_get_parent, + .recalc_rate = tegra210_clk_emc_recalc_rate, + .round_rate = tegra210_clk_emc_round_rate, + .set_rate = tegra210_clk_emc_set_rate, +}; + +struct clk *tegra210_clk_register_emc(struct device_node *np, + void __iomem *regs) +{ + struct tegra210_clk_emc *emc; + struct clk_init_data init; + struct clk *clk; + + emc = kzalloc(sizeof(*emc), GFP_KERNEL); + if (!emc) + return ERR_PTR(-ENOMEM); + + emc->regs = regs; + + init.name = "emc"; + init.ops = &tegra210_clk_emc_ops; + init.flags = CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE; + init.parent_names = tegra210_clk_emc_parents; + init.num_parents = ARRAY_SIZE(tegra210_clk_emc_parents); + emc->hw.init = &init; + + clk = clk_register(NULL, &emc->hw); + if (IS_ERR(clk)) { + kfree(emc); + return clk; + } + + return clk; +} + +int tegra210_clk_emc_attach(struct clk *clk, + struct tegra210_clk_emc_provider *provider) +{ + struct clk_hw *hw = __clk_get_hw(clk); + struct tegra210_clk_emc *emc = to_tegra210_clk_emc(hw); + struct device *dev = provider->dev; + unsigned int i; + int err; + + if (!try_module_get(provider->owner)) + return -ENODEV; + + for (i = 0; i < provider->num_configs; i++) { + struct tegra210_clk_emc_config *config = &provider->configs[i]; + struct clk_hw *parent; + bool same_freq; + u8 div, src; + + div = FIELD_GET(CLK_SOURCE_EMC_2X_CLK_DIVISOR, config->value); + src = FIELD_GET(CLK_SOURCE_EMC_2X_CLK_SRC, config->value); + + /* do basic sanity checking on the EMC timings */ + if (div & 0x1) { + dev_err(dev, "invalid odd divider %u for rate %lu Hz\n", + div, config->rate); + err = -EINVAL; + goto put; + } + + same_freq = config->value & CLK_SOURCE_EMC_MC_EMC_SAME_FREQ; + + if (same_freq != config->same_freq) { + dev_err(dev, + "ambiguous EMC to MC ratio for rate %lu Hz\n", + config->rate); + err = -EINVAL; + goto put; + } + + parent = clk_hw_get_parent_by_index(hw, src); + config->parent = src; + + if (src == CLK_SRC_PLLM || src == CLK_SRC_PLLM_UD) { + config->parent_rate = config->rate * (1 + div / 2); + } else { + unsigned long rate = config->rate * (1 + div / 2); + + config->parent_rate = clk_hw_get_rate(parent); + + if (config->parent_rate != rate) { + dev_err(dev, + "rate %lu Hz does not match input\n", + config->rate); + err = -EINVAL; + goto put; + } + } + } + + emc->provider = provider; + + return 0; + +put: + module_put(provider->owner); + return err; +} +EXPORT_SYMBOL_GPL(tegra210_clk_emc_attach); + +void tegra210_clk_emc_detach(struct clk *clk) +{ + struct tegra210_clk_emc *emc = to_tegra210_clk_emc(__clk_get_hw(clk)); + + module_put(emc->provider->owner); + emc->provider = NULL; +} +EXPORT_SYMBOL_GPL(tegra210_clk_emc_detach); diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h index 2c9a68302e02..b87e90569314 100644 --- a/drivers/clk/tegra/clk.h +++ b/drivers/clk/tegra/clk.h @@ -907,4 +907,7 @@ void tegra_clk_periph_resume(void); bool tegra20_clk_emc_driver_available(struct clk_hw *emc_hw); struct clk *tegra20_clk_register_emc(void __iomem *ioaddr, bool low_jitter); +struct clk *tegra210_clk_register_emc(struct device_node *np, + void __iomem *regs); + #endif /* TEGRA_CLK_H */ diff --git a/include/linux/clk/tegra.h b/include/linux/clk/tegra.h index 5b0bdb413460..3f01d43f0598 100644 --- a/include/linux/clk/tegra.h +++ b/include/linux/clk/tegra.h @@ -146,4 +146,28 @@ void tegra20_clk_set_emc_round_callback(tegra20_clk_emc_round_cb *round_cb, void *cb_arg); int tegra20_clk_prepare_emc_mc_same_freq(struct clk *emc_clk, bool same); +struct tegra210_clk_emc_config { + unsigned long rate; + bool same_freq; + u32 value; + + unsigned long parent_rate; + u8 parent; +}; + +struct tegra210_clk_emc_provider { + struct module *owner; + struct device *dev; + + struct tegra210_clk_emc_config *configs; + unsigned int num_configs; + + int (*set_rate)(struct device *dev, + const struct tegra210_clk_emc_config *config); +}; + +int tegra210_clk_emc_attach(struct clk *clk, + struct tegra210_clk_emc_provider *provider); +void tegra210_clk_emc_detach(struct clk *clk); + #endif /* __LINUX_CLK_TEGRA_H_ */