| Message ID | 20190414202009.31268-2-digetx@gmail.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | memory: tegra: Introduce Tegra30 EMC driver | expand |
Quoting Dmitry Osipenko (2019-04-14 13:20:06) > diff --git a/drivers/clk/tegra/clk-tegra20-emc.c b/drivers/clk/tegra/clk-tegra20-emc.c > new file mode 100644 > index 000000000000..35b67a9989c8 > --- /dev/null > +++ b/drivers/clk/tegra/clk-tegra20-emc.c > @@ -0,0 +1,307 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#include <linux/bits.h> > +#include <linux/clk/tegra.h> Include clk-provider.h as this is a clk provider driver. > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/slab.h> > + > +#include "clk.h" > + > +#define CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK GENMASK(7, 0) > +#define CLK_SOURCE_EMC_2X_CLK_SRC_MASK GENMASK(31, 30) [...] > + > +static const struct clk_ops tegra_clk_emc_ops = { > + .recalc_rate = emc_recalc_rate, > + .get_parent = emc_get_parent, > + .set_parent = emc_set_parent, > + .set_rate = emc_set_rate, > + .set_rate_and_parent = emc_set_rate_and_parent, > + .determine_rate = emc_determine_rate, > +}; > + > +void tegra20_clk_set_emc_round_callback(void *round_cb, void *arg_cb) Why can't we have type safety on these function pointers? > +{ > + struct clk *clk = __clk_lookup("emc"); > + struct tegra_clk_emc *emc; > + struct clk_hw *hw; > + > + if (clk) { > + hw = __clk_get_hw(clk); > + emc = to_tegra_clk_emc(hw); > + > + emc->round_cb = round_cb; > + emc->arg_cb = arg_cb; > + } > +} > + > +bool tegra20_clk_emc_driver_available(void) > +{ > + struct clk *clk = __clk_lookup("emc"); Can we avoid using __clk_lookup()? Maybe by having the clk_hw pointer in this driver somehow? > + struct tegra_clk_emc *emc; > + struct clk_hw *hw; > + > + if (clk) { > + hw = __clk_get_hw(clk); This gets further to the point, we don't prefer to see drivers use __clk_get_hw() unless they absolutely need to. Maybe I don't understand the driver structure, but it seems like maybe the driver that's providing the callbacks could be the same driver that's registering these clks, and thus everything could be inside one file so that we don't have to pass around callbacks and clk_hw pointers? Commit text says "this is how it's been" but that's not a reason to keep doing it. > + emc = to_tegra_clk_emc(hw); > +
25.04.2019 22:07, Stephen Boyd пишет: > Quoting Dmitry Osipenko (2019-04-14 13:20:06) >> diff --git a/drivers/clk/tegra/clk-tegra20-emc.c b/drivers/clk/tegra/clk-tegra20-emc.c >> new file mode 100644 >> index 000000000000..35b67a9989c8 >> --- /dev/null >> +++ b/drivers/clk/tegra/clk-tegra20-emc.c >> @@ -0,0 +1,307 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> + >> +#include <linux/bits.h> >> +#include <linux/clk/tegra.h> > > Include clk-provider.h as this is a clk provider driver. Okay, although clk-provider.h is also included by clk.h below. >> +#include <linux/err.h> >> +#include <linux/io.h> >> +#include <linux/kernel.h> >> +#include <linux/slab.h> >> + >> +#include "clk.h" >> + >> +#define CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK GENMASK(7, 0) >> +#define CLK_SOURCE_EMC_2X_CLK_SRC_MASK GENMASK(31, 30) > [...] >> + >> +static const struct clk_ops tegra_clk_emc_ops = { >> + .recalc_rate = emc_recalc_rate, >> + .get_parent = emc_get_parent, >> + .set_parent = emc_set_parent, >> + .set_rate = emc_set_rate, >> + .set_rate_and_parent = emc_set_rate_and_parent, >> + .determine_rate = emc_determine_rate, >> +}; >> + >> +void tegra20_clk_set_emc_round_callback(void *round_cb, void *arg_cb) > > Why can't we have type safety on these function pointers? It is probably not really necessary since it's a platform-specific API. But I'll add an explicit type in v3 for consistency, thanks. >> +{ >> + struct clk *clk = __clk_lookup("emc"); >> + struct tegra_clk_emc *emc; >> + struct clk_hw *hw; >> + >> + if (clk) { >> + hw = __clk_get_hw(clk); >> + emc = to_tegra_clk_emc(hw); >> + >> + emc->round_cb = round_cb; >> + emc->arg_cb = arg_cb; >> + } >> +} >> + >> +bool tegra20_clk_emc_driver_available(void) >> +{ >> + struct clk *clk = __clk_lookup("emc"); > > Can we avoid using __clk_lookup()? Maybe by having the clk_hw pointer in > this driver somehow? tegra20_clk_emc_driver_available() is a private function of the Tegra's clk-core. Please note that prototype of this function is added to the local "drivers/clk/tegra/clk.h" file. It is indeed possible to use clk pointer instead of __clk_lookup() and I'll change that in v3 since it's causing some confusion. >> + struct tegra_clk_emc *emc; >> + struct clk_hw *hw; >> + >> + if (clk) { >> + hw = __clk_get_hw(clk); > > This gets further to the point, we don't prefer to see drivers use > __clk_get_hw() unless they absolutely need to. Maybe I don't understand > the driver structure, but it seems like maybe the driver that's > providing the callbacks could be the same driver that's registering > these clks, and thus everything could be inside one file so that we > don't have to pass around callbacks and clk_hw pointers? Commit text > says "this is how it's been" but that's not a reason to keep doing it. Again, tegra20_clk_emc_driver_available() is for the Terga's clk-core and not for the EMC (External Memory Controller) driver. This function is used to determine whether EMC driver is ready to handle clock-rate changes (PRE/POST rate-change notifications), clk users can't get EMC clock until driver is ready (i.e. the "round" callback is registered by the driver). Please see tegra20_clk_src_onecell_get() changes in this patch and drivers/memory/tegra/tegra20-emc.c for clarity. It is not possible to register clk from the EMC driver without having some custom integration with the clk-core because CLK and EMC are different hardware units and hence EMC driver doesn't have direct access to the CLK registers. I think it is better to keep CLK and memory-timing programming logically separated simply for consistency, although I'm open to suggestions.
Quoting Dmitry Osipenko (2019-04-25 14:42:19) > 25.04.2019 22:07, Stephen Boyd пишет: > > Quoting Dmitry Osipenko (2019-04-14 13:20:06) > >> diff --git a/drivers/clk/tegra/clk-tegra20-emc.c b/drivers/clk/tegra/clk-tegra20-emc.c > >> new file mode 100644 > >> index 000000000000..35b67a9989c8 > >> --- /dev/null > >> +++ b/drivers/clk/tegra/clk-tegra20-emc.c > >> @@ -0,0 +1,307 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> + > >> +#include <linux/bits.h> > >> +#include <linux/clk/tegra.h> > > > > Include clk-provider.h as this is a clk provider driver. > > Okay, although clk-provider.h is also included by clk.h below. We prefer explicit includes instead of implicit ones. Failing to do that leads to changes like the one I'm making now to push out linux/io.h to all the users who are relying on clk-provider.h to provide it for them. > > >> +{ > >> + struct clk *clk = __clk_lookup("emc"); > >> + struct tegra_clk_emc *emc; > >> + struct clk_hw *hw; > >> + > >> + if (clk) { > >> + hw = __clk_get_hw(clk); > >> + emc = to_tegra_clk_emc(hw); > >> + > >> + emc->round_cb = round_cb; > >> + emc->arg_cb = arg_cb; > >> + } > >> +} > >> + > >> +bool tegra20_clk_emc_driver_available(void) > >> +{ > >> + struct clk *clk = __clk_lookup("emc"); > > > > Can we avoid using __clk_lookup()? Maybe by having the clk_hw pointer in > > this driver somehow? > > tegra20_clk_emc_driver_available() is a private function of the Tegra's > clk-core. Please note that prototype of this function is added to the > local "drivers/clk/tegra/clk.h" file. > > It is indeed possible to use clk pointer instead of __clk_lookup() and > I'll change that in v3 since it's causing some confusion. Can you use a clk_hw pointer directly? I'd like to see clk provider drivers only use struct clk_hw, and clk consumers only use struct clk. > > >> + struct tegra_clk_emc *emc; > >> + struct clk_hw *hw; > >> + > >> + if (clk) { > >> + hw = __clk_get_hw(clk); > > > > This gets further to the point, we don't prefer to see drivers use > > __clk_get_hw() unless they absolutely need to. Maybe I don't understand > > the driver structure, but it seems like maybe the driver that's > > providing the callbacks could be the same driver that's registering > > these clks, and thus everything could be inside one file so that we > > don't have to pass around callbacks and clk_hw pointers? Commit text > > says "this is how it's been" but that's not a reason to keep doing it. > > Again, tegra20_clk_emc_driver_available() is for the Terga's clk-core > and not for the EMC (External Memory Controller) driver. This function > is used to determine whether EMC driver is ready to handle clock-rate > changes (PRE/POST rate-change notifications), clk users can't get EMC > clock until driver is ready (i.e. the "round" callback is registered by > the driver). Who are the other users of the EMC clk? Why does the EMC driver and the clk driver need to be in sync in such a way that requires us to check if some code in the EMC driver is ready before we can hand out clks from this clk provider? It looks like some tight coupling between these two pieces of code on the clk_get() path which is setting off alarms for me. > > Please see tegra20_clk_src_onecell_get() changes in this patch and > drivers/memory/tegra/tegra20-emc.c for clarity. > > It is not possible to register clk from the EMC driver without having > some custom integration with the clk-core because CLK and EMC are > different hardware units and hence EMC driver doesn't have direct access > to the CLK registers. I think it is better to keep CLK and memory-timing > programming logically separated simply for consistency, although I'm > open to suggestions. Sorry, I'm really confused and it's probably because I'm not taking the time to read all the Tegra code right now. I'm trying to understand the overall memory controller frequency logic. I understand that there's a memory controller, and a clk controller, and they're different hardware blocks. Presumably the clk controller can't change frequencies without informing the memory controller that it's going to change something so the memory controller can fix timings in the EMC register space. Is there some other entity that's needing to be deferred while the EMC and clk drivers tell each other that they're probed and ready so that these notifiers all work? Is that entity using clks directly instead of going through the EMC to change frequencies? If so, why can't we funnel that logic through EMC which then goes to the clk driver? That way it's clearly "thing that changes frequency" -> EMC -> clk controller call chain. It might even allow us to get rid of the rate notifiers?
26.04.2019 1:25, Stephen Boyd пишет: > Quoting Dmitry Osipenko (2019-04-25 14:42:19) >> 25.04.2019 22:07, Stephen Boyd пишет: >>> Quoting Dmitry Osipenko (2019-04-14 13:20:06) >>>> diff --git a/drivers/clk/tegra/clk-tegra20-emc.c b/drivers/clk/tegra/clk-tegra20-emc.c >>>> new file mode 100644 >>>> index 000000000000..35b67a9989c8 >>>> --- /dev/null >>>> +++ b/drivers/clk/tegra/clk-tegra20-emc.c >>>> @@ -0,0 +1,307 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> + >>>> +#include <linux/bits.h> >>>> +#include <linux/clk/tegra.h> >>> >>> Include clk-provider.h as this is a clk provider driver. >> >> Okay, although clk-provider.h is also included by clk.h below. > > We prefer explicit includes instead of implicit ones. Failing to do that > leads to changes like the one I'm making now to push out linux/io.h to > all the users who are relying on clk-provider.h to provide it for them. Ok. >> >>>> +{ >>>> + struct clk *clk = __clk_lookup("emc"); >>>> + struct tegra_clk_emc *emc; >>>> + struct clk_hw *hw; >>>> + >>>> + if (clk) { >>>> + hw = __clk_get_hw(clk); >>>> + emc = to_tegra_clk_emc(hw); >>>> + >>>> + emc->round_cb = round_cb; >>>> + emc->arg_cb = arg_cb; >>>> + } >>>> +} >>>> + >>>> +bool tegra20_clk_emc_driver_available(void) >>>> +{ >>>> + struct clk *clk = __clk_lookup("emc"); >>> >>> Can we avoid using __clk_lookup()? Maybe by having the clk_hw pointer in >>> this driver somehow? >> >> tegra20_clk_emc_driver_available() is a private function of the Tegra's >> clk-core. Please note that prototype of this function is added to the >> local "drivers/clk/tegra/clk.h" file. >> >> It is indeed possible to use clk pointer instead of __clk_lookup() and >> I'll change that in v3 since it's causing some confusion. > > Can you use a clk_hw pointer directly? I'd like to see clk provider > drivers only use struct clk_hw, and clk consumers only use struct clk. The clk_hw pointers are not stored directly anywhere in the Tegra's clk-driver, it will be a quite massive change to make the clk driver's code to use clk_hw pointers instead of struct clk. I can add a global variable for the tegra_clk_emc pointer, but it is not really necessary since there is __clk_get_hw. Could you please explain what's the problem with __clk_get_hw? >> >>>> + struct tegra_clk_emc *emc; >>>> + struct clk_hw *hw; >>>> + >>>> + if (clk) { >>>> + hw = __clk_get_hw(clk); >>> >>> This gets further to the point, we don't prefer to see drivers use >>> __clk_get_hw() unless they absolutely need to. Maybe I don't understand >>> the driver structure, but it seems like maybe the driver that's >>> providing the callbacks could be the same driver that's registering >>> these clks, and thus everything could be inside one file so that we >>> don't have to pass around callbacks and clk_hw pointers? Commit text >>> says "this is how it's been" but that's not a reason to keep doing it. >> >> Again, tegra20_clk_emc_driver_available() is for the Terga's clk-core >> and not for the EMC (External Memory Controller) driver. This function >> is used to determine whether EMC driver is ready to handle clock-rate >> changes (PRE/POST rate-change notifications), clk users can't get EMC >> clock until driver is ready (i.e. the "round" callback is registered by >> the driver). > > Who are the other users of the EMC clk? Why does the EMC driver and the > clk driver need to be in sync in such a way that requires us to check if > some code in the EMC driver is ready before we can hand out clks from > this clk provider? It looks like some tight coupling between these two > pieces of code on the clk_get() path which is setting off alarms for me. The other EMC clk user is the devfreq driver. We don't want the devfreq driver to get and use the EMC clock if memory timing can't be programmed. >> >> Please see tegra20_clk_src_onecell_get() changes in this patch and >> drivers/memory/tegra/tegra20-emc.c for clarity. >> >> It is not possible to register clk from the EMC driver without having >> some custom integration with the clk-core because CLK and EMC are >> different hardware units and hence EMC driver doesn't have direct access >> to the CLK registers. I think it is better to keep CLK and memory-timing >> programming logically separated simply for consistency, although I'm >> open to suggestions. > > Sorry, I'm really confused and it's probably because I'm not taking the > time to read all the Tegra code right now. I'm trying to understand the > overall memory controller frequency logic. No problems, sorry for the confusion. > I understand that there's a memory controller, and a clk controller, and > they're different hardware blocks. Presumably the clk controller can't > change frequencies without informing the memory controller that it's > going to change something so the memory controller can fix timings in > the EMC register space. That is correct. > Is there some other entity that's needing to be deferred while the EMC > and clk drivers tell each other that they're probed and ready so that > these notifiers all work? Is that entity using clks directly instead of > going through the EMC to change frequencies? If so, why can't we funnel > that logic through EMC which then goes to the clk driver? That way it's > clearly "thing that changes frequency" -> EMC -> clk controller call > chain. It might even allow us to get rid of the rate notifiers? > That entity is the devfreq driver which uses EMC clk directly right now. https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra-devfreq.c#n484 I think we can funnel all the rate-changing logic through the EMC drivers using the PM memory bandwidth QoS API, which is currently unsupported by the drivers. And then indeed the rate notifiers won't be needed. https://github.com/grate-driver/linux/commit/2aa2390dd0cb8bca98e4cea6a29d7bda64a51bb3 Actually it will be the next step to wire up the PM QoS support to all relevant drivers that I'm going to implement after the current CLK/EMC/DEVFREQ patch sets will land, it's just not realistic to do everything in a single hop.
Quoting Dmitry Osipenko (2019-04-25 17:03:11) > 26.04.2019 1:25, Stephen Boyd пишет: > > Quoting Dmitry Osipenko (2019-04-25 14:42:19) > >> 25.04.2019 22:07, Stephen Boyd пишет: > >>> Quoting Dmitry Osipenko (2019-04-14 13:20:06) > >>>> +{ > >>>> + struct clk *clk = __clk_lookup("emc"); > >>>> + struct tegra_clk_emc *emc; > >>>> + struct clk_hw *hw; > >>>> + > >>>> + if (clk) { > >>>> + hw = __clk_get_hw(clk); > >>>> + emc = to_tegra_clk_emc(hw); > >>>> + > >>>> + emc->round_cb = round_cb; > >>>> + emc->arg_cb = arg_cb; > >>>> + } > >>>> +} > >>>> + > >>>> +bool tegra20_clk_emc_driver_available(void) > >>>> +{ > >>>> + struct clk *clk = __clk_lookup("emc"); > >>> > >>> Can we avoid using __clk_lookup()? Maybe by having the clk_hw pointer in > >>> this driver somehow? > >> > >> tegra20_clk_emc_driver_available() is a private function of the Tegra's > >> clk-core. Please note that prototype of this function is added to the > >> local "drivers/clk/tegra/clk.h" file. > >> > >> It is indeed possible to use clk pointer instead of __clk_lookup() and > >> I'll change that in v3 since it's causing some confusion. > > > > Can you use a clk_hw pointer directly? I'd like to see clk provider > > drivers only use struct clk_hw, and clk consumers only use struct clk. > > The clk_hw pointers are not stored directly anywhere in the Tegra's > clk-driver, it will be a quite massive change to make the clk driver's > code to use clk_hw pointers instead of struct clk. Well they must be stored in the clk driver somewhere because that's how the whole clk framework works. > > I can add a global variable for the tegra_clk_emc pointer, but it is > not really necessary since there is __clk_get_hw. > > Could you please explain what's the problem with __clk_get_hw? If it's a large invasive change then no worries. The thin that's wrong with __clk_get_hw() is that it's using a struct clk, meaing that most likely this is a clk consumer driver doing something weird. > > >> > >>>> + struct tegra_clk_emc *emc; + struct clk_hw *hw; + + > >>>> if (clk) { + hw = __clk_get_hw(clk); > >>> > >>> This gets further to the point, we don't prefer to see drivers use > >>> __clk_get_hw() unless they absolutely need to. Maybe I don't > >>> understand the driver structure, but it seems like maybe the > >>> driver that's providing the callbacks could be the same driver > >>> that's registering these clks, and thus everything could be inside > >>> one file so that we don't have to pass around callbacks and clk_hw > >>> pointers? Commit text says "this is how it's been" but that's not > >>> a reason to keep doing it. > >> > >> Again, tegra20_clk_emc_driver_available() is for the Terga's > >> clk-core and not for the EMC (External Memory Controller) driver. > >> This function is used to determine whether EMC driver is ready to > >> handle clock-rate changes (PRE/POST rate-change notifications), clk > >> users can't get EMC clock until driver is ready (i.e. the "round" > >> callback is registered by the driver). > > > > Who are the other users of the EMC clk? Why does the EMC driver and > > the clk driver need to be in sync in such a way that requires us to > > check if some code in the EMC driver is ready before we can hand out > > clks from this clk provider? It looks like some tight coupling > > between these two pieces of code on the clk_get() path which is > > setting off alarms for me. > > The other EMC clk user is the devfreq driver. We don't want the > devfreq driver to get and use the EMC clock if memory timing can't be > programmed. Alright, got it! > That is correct. > > > Is there some other entity that's needing to be deferred while the > > EMC and clk drivers tell each other that they're probed and ready so > > that these notifiers all work? Is that entity using clks directly > > instead of going through the EMC to change frequencies? If so, why > > can't we funnel that logic through EMC which then goes to the clk > > driver? That way it's clearly "thing that changes frequency" -> EMC > > -> clk controller call chain. It might even allow us to get rid of > > the rate notifiers? > > > > That entity is the devfreq driver which uses EMC clk directly right > now. > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra-devfreq.c#n484 > > I think we can funnel all the rate-changing logic through the EMC > drivers using the PM memory bandwidth QoS API, which is currently > unsupported by the drivers. And then indeed the rate notifiers won't > be needed. Why does PM QoS matter here? I'm mostly asking for the chain to be stacked and the devfreq driver to be the same as the EMC driver and then have the EMC/devfreq combo driver use clk APIs to set rates and do things before and after. > > https://github.com/grate-driver/linux/commit/2aa2390dd0cb8bca98e4cea6a29d7bda64a51bb3 > > Actually it will be the next step to wire up the PM QoS support to all > relevant drivers that I'm going to implement after the current > CLK/EMC/DEVFREQ patch sets will land, it's just not realistic to do > everything in a single hop. Ok. It doesn't make me feel great but I suppose if you don't disappear after this code is merged things will work out in the end.
26.04.2019 3:41, Stephen Boyd пишет: > Quoting Dmitry Osipenko (2019-04-25 17:03:11) >> 26.04.2019 1:25, Stephen Boyd пишет: >>> Quoting Dmitry Osipenko (2019-04-25 14:42:19) >>>> 25.04.2019 22:07, Stephen Boyd пишет: >>>>> Quoting Dmitry Osipenko (2019-04-14 13:20:06) >>>>>> +{ >>>>>> + struct clk *clk = __clk_lookup("emc"); >>>>>> + struct tegra_clk_emc *emc; >>>>>> + struct clk_hw *hw; >>>>>> + >>>>>> + if (clk) { >>>>>> + hw = __clk_get_hw(clk); >>>>>> + emc = to_tegra_clk_emc(hw); >>>>>> + >>>>>> + emc->round_cb = round_cb; >>>>>> + emc->arg_cb = arg_cb; >>>>>> + } >>>>>> +} >>>>>> + >>>>>> +bool tegra20_clk_emc_driver_available(void) >>>>>> +{ >>>>>> + struct clk *clk = __clk_lookup("emc"); >>>>> >>>>> Can we avoid using __clk_lookup()? Maybe by having the clk_hw pointer in >>>>> this driver somehow? >>>> >>>> tegra20_clk_emc_driver_available() is a private function of the Tegra's >>>> clk-core. Please note that prototype of this function is added to the >>>> local "drivers/clk/tegra/clk.h" file. >>>> >>>> It is indeed possible to use clk pointer instead of __clk_lookup() and >>>> I'll change that in v3 since it's causing some confusion. >>> >>> Can you use a clk_hw pointer directly? I'd like to see clk provider >>> drivers only use struct clk_hw, and clk consumers only use struct clk. >> >> The clk_hw pointers are not stored directly anywhere in the Tegra's >> clk-driver, it will be a quite massive change to make the clk driver's >> code to use clk_hw pointers instead of struct clk. > > Well they must be stored in the clk driver somewhere because that's how > the whole clk framework works. AFAIK, Tegra stores them only in a form of clk->core->hw. >> >> I can add a global variable for the tegra_clk_emc pointer, but it is >> not really necessary since there is __clk_get_hw. >> >> Could you please explain what's the problem with __clk_get_hw? > > If it's a large invasive change then no worries. The thin that's wrong > with __clk_get_hw() is that it's using a struct clk, meaing that most > likely this is a clk consumer driver doing something weird. > >> >>>> >>>>>> + struct tegra_clk_emc *emc; + struct clk_hw *hw; + + >>>>>> if (clk) { + hw = __clk_get_hw(clk); >>>>> >>>>> This gets further to the point, we don't prefer to see drivers use >>>>> __clk_get_hw() unless they absolutely need to. Maybe I don't >>>>> understand the driver structure, but it seems like maybe the >>>>> driver that's providing the callbacks could be the same driver >>>>> that's registering these clks, and thus everything could be inside >>>>> one file so that we don't have to pass around callbacks and clk_hw >>>>> pointers? Commit text says "this is how it's been" but that's not >>>>> a reason to keep doing it. >>>> >>>> Again, tegra20_clk_emc_driver_available() is for the Terga's >>>> clk-core and not for the EMC (External Memory Controller) driver. >>>> This function is used to determine whether EMC driver is ready to >>>> handle clock-rate changes (PRE/POST rate-change notifications), clk >>>> users can't get EMC clock until driver is ready (i.e. the "round" >>>> callback is registered by the driver). >>> >>> Who are the other users of the EMC clk? Why does the EMC driver and >>> the clk driver need to be in sync in such a way that requires us to >>> check if some code in the EMC driver is ready before we can hand out >>> clks from this clk provider? It looks like some tight coupling >>> between these two pieces of code on the clk_get() path which is >>> setting off alarms for me. >> >> The other EMC clk user is the devfreq driver. We don't want the >> devfreq driver to get and use the EMC clock if memory timing can't be >> programmed. > > Alright, got it! > >> That is correct. >> >>> Is there some other entity that's needing to be deferred while the >>> EMC and clk drivers tell each other that they're probed and ready so >>> that these notifiers all work? Is that entity using clks directly >>> instead of going through the EMC to change frequencies? If so, why >>> can't we funnel that logic through EMC which then goes to the clk >>> driver? That way it's clearly "thing that changes frequency" -> EMC >>> -> clk controller call chain. It might even allow us to get rid of >>> the rate notifiers? >>> >> >> That entity is the devfreq driver which uses EMC clk directly right >> now. >> >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra-devfreq.c#n484 >> >> I think we can funnel all the rate-changing logic through the EMC >> drivers using the PM memory bandwidth QoS API, which is currently >> unsupported by the drivers. And then indeed the rate notifiers won't >> be needed. > > Why does PM QoS matter here? I'm mostly asking for the chain to be > stacked and the devfreq driver to be the same as the EMC driver and then > have the EMC/devfreq combo driver use clk APIs to set rates and do > things before and after. It could be implemented either way, the result will be the same. The final decision probably will be made after wiring up the PM QoS support and doing some more testing. Also note that EMC and devfreq are actually separate hardware blocks that are integrated. So it all could look like this (current variant that in works): +-------+ PM QoS +-+ |DEVICE1+----------->+E| CLK API +-------+ PM QoS |M+<----------+ +-+ +------->+C| +--->C| | +-+ |L| +-------+ | +-->K| |DEVICE2+---+ +-------+ CLK API | +-+ +-------+ |DEVFREQ+<---------+ +-------+ And like this: +-------+ PM QoS +-+ |DEVICE1+---------+ | +-------+ | | +-+ +-------+ PM QoS |E| CLK |C| |DEVICE2+-------->+M+<----->+L| +-------+ |C| API |K| +-------+ PM QoS | | +++ |DEVFREQ+---------+ | | +---+---+ +-+ | ^ | +------------------------+ CLK API Or like this: +-------+ PM QoS +-+ |DEVICE1+---------+ | +-------+ | | +-+ +-------+ PM QoS |E| CLK |C| |DEVICE2+-------->+M+<----->+L| +-------+ |C| API |K| +-------+ PM QoS | | +-+ |DEVFREQ+---------+ | +---+---+ +++ ^ | +--------------+ PM QoS As you can see there will be other drivers that will demand memory bandwidth too. For example the display controller performs isochronous memory transfers and hence may require higher bandwidth than devfreq suggests because devfreq judges the bandwidth demand by taking average of bandwidth consumption over a period of time. The other thing is that drivers should be abstracted from the knowledge about the EMC because: a) memory/device hardware units do not know about each other b) it is common to express memory bandwidth in Mbs and not in a clock rate It's only the devfreq driver that is a bit special because devfreq hardware is actually integrated with the EMC. >> >> https://github.com/grate-driver/linux/commit/2aa2390dd0cb8bca98e4cea6a29d7bda64a51bb3 >> >> Actually it will be the next step to wire up the PM QoS support to all >> relevant drivers that I'm going to implement after the current >> CLK/EMC/DEVFREQ patch sets will land, it's just not realistic to do >> everything in a single hop. > > Ok. It doesn't make me feel great but I suppose if you don't disappear > after this code is merged things will work out in the end. > Correct.
diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile index 4812e45c2214..df966ca06788 100644 --- a/drivers/clk/tegra/Makefile +++ b/drivers/clk/tegra/Makefile @@ -17,7 +17,9 @@ obj-y += clk-tegra-fixed.o obj-y += clk-tegra-super-gen4.o obj-$(CONFIG_TEGRA_CLK_EMC) += clk-emc.o obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += clk-tegra20.o +obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += clk-tegra20-emc.o obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += clk-tegra30.o +obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += clk-tegra20-emc.o obj-$(CONFIG_ARCH_TEGRA_114_SOC) += clk-tegra114.o obj-$(CONFIG_ARCH_TEGRA_124_SOC) += clk-tegra124.o obj-$(CONFIG_TEGRA_CLK_DFLL) += clk-tegra124-dfll-fcpu.o diff --git a/drivers/clk/tegra/clk-tegra20-emc.c b/drivers/clk/tegra/clk-tegra20-emc.c new file mode 100644 index 000000000000..35b67a9989c8 --- /dev/null +++ b/drivers/clk/tegra/clk-tegra20-emc.c @@ -0,0 +1,307 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/bits.h> +#include <linux/clk/tegra.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/slab.h> + +#include "clk.h" + +#define CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK GENMASK(7, 0) +#define CLK_SOURCE_EMC_2X_CLK_SRC_MASK GENMASK(31, 30) +#define CLK_SOURCE_EMC_2X_CLK_SRC_SHIFT 30 + +#define MC_EMC_SAME_FREQ BIT(16) +#define USE_PLLM_UD BIT(29) + +#define EMC_SRC_PLL_M 0 +#define EMC_SRC_PLL_C 1 +#define EMC_SRC_PLL_P 2 +#define EMC_SRC_CLK_M 3 + +static const char * const emc_parent_clk_names[] = { + "pll_m", "pll_c", "pll_p", "clk_m", +}; + +struct tegra_clk_emc { + struct clk_hw hw; + void __iomem *reg; + bool mc_same_freq; + bool want_low_jitter; + + long (*round_cb)(ulong rate, ulong rate_min, ulong rate_max, void *arg); + void *arg_cb; +}; + +static inline struct tegra_clk_emc *to_tegra_clk_emc(struct clk_hw *hw) +{ + return container_of(hw, struct tegra_clk_emc, hw); +} + +static unsigned long emc_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + struct tegra_clk_emc *emc = to_tegra_clk_emc(hw); + u32 val, div; + + val = readl_relaxed(emc->reg); + div = val & CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK; + + return DIV_ROUND_UP(parent_rate * 2, div + 2); +} + +static u8 emc_get_parent(struct clk_hw *hw) +{ + struct tegra_clk_emc *emc = to_tegra_clk_emc(hw); + + return readl_relaxed(emc->reg) >> CLK_SOURCE_EMC_2X_CLK_SRC_SHIFT; +} + +static int emc_set_parent(struct clk_hw *hw, u8 index) +{ + struct tegra_clk_emc *emc = to_tegra_clk_emc(hw); + u32 val, div; + + val = readl_relaxed(emc->reg); + val &= ~CLK_SOURCE_EMC_2X_CLK_SRC_MASK; + val |= index << CLK_SOURCE_EMC_2X_CLK_SRC_SHIFT; + + div = val & CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK; + + if (index == EMC_SRC_PLL_M && div == 0 && emc->want_low_jitter) + val |= USE_PLLM_UD; + else + val &= ~USE_PLLM_UD; + + if (emc->mc_same_freq) + val |= MC_EMC_SAME_FREQ; + else + val &= ~MC_EMC_SAME_FREQ; + + writel_relaxed(val, emc->reg); + + return 0; +} + +static int emc_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + struct tegra_clk_emc *emc = to_tegra_clk_emc(hw); + unsigned int index; + u32 val, div; + + div = div_frac_get(rate, parent_rate, 8, 1, 0); + + val = readl_relaxed(emc->reg); + val &= ~CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK; + val |= div; + + index = val >> CLK_SOURCE_EMC_2X_CLK_SRC_SHIFT; + + if (index == EMC_SRC_PLL_M && div == 0 && emc->want_low_jitter) + val |= USE_PLLM_UD; + else + val &= ~USE_PLLM_UD; + + if (emc->mc_same_freq) + val |= MC_EMC_SAME_FREQ; + else + val &= ~MC_EMC_SAME_FREQ; + + writel_relaxed(val, emc->reg); + + return 0; +} + +static int emc_set_rate_and_parent(struct clk_hw *hw, + unsigned long rate, + unsigned long parent_rate, + u8 index) +{ + struct tegra_clk_emc *emc = to_tegra_clk_emc(hw); + u32 val, div; + + div = div_frac_get(rate, parent_rate, 8, 1, 0); + + val = readl_relaxed(emc->reg); + + val &= ~CLK_SOURCE_EMC_2X_CLK_SRC_MASK; + val |= index << CLK_SOURCE_EMC_2X_CLK_SRC_SHIFT; + + val &= ~CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK; + val |= div; + + if (index == EMC_SRC_PLL_M && div == 0 && emc->want_low_jitter) + val |= USE_PLLM_UD; + else + val &= ~USE_PLLM_UD; + + if (emc->mc_same_freq) + val |= MC_EMC_SAME_FREQ; + else + val &= ~MC_EMC_SAME_FREQ; + + writel_relaxed(val, emc->reg); + + return 0; +} + +static int emc_determine_rate(struct clk_hw *hw, struct clk_rate_request *req) +{ + struct tegra_clk_emc *emc = to_tegra_clk_emc(hw); + struct clk_hw *parent_hw; + unsigned long divided_rate; + unsigned long parent_rate; + unsigned int i; + long emc_rate; + int div; + + emc_rate = emc->round_cb(req->rate, req->min_rate, req->max_rate, + emc->arg_cb); + if (emc_rate < 0) + return emc_rate; + + for (i = 0; i < ARRAY_SIZE(emc_parent_clk_names); i++) { + parent_hw = clk_hw_get_parent_by_index(hw, i); + + if (req->best_parent_hw == parent_hw) + parent_rate = req->best_parent_rate; + else + parent_rate = clk_hw_get_rate(parent_hw); + + if (emc_rate > parent_rate) + continue; + + div = div_frac_get(emc_rate, parent_rate, 8, 1, 0); + divided_rate = DIV_ROUND_UP(parent_rate * 2, div + 2); + + if (divided_rate != emc_rate) + continue; + + req->best_parent_rate = parent_rate; + req->best_parent_hw = parent_hw; + req->rate = emc_rate; + break; + } + + if (i == ARRAY_SIZE(emc_parent_clk_names)) { + pr_err_once("%s: can't find parent for rate %lu emc_rate %lu\n", + __func__, req->rate, emc_rate); + return -EINVAL; + } + + return 0; +} + +static const struct clk_ops tegra_clk_emc_ops = { + .recalc_rate = emc_recalc_rate, + .get_parent = emc_get_parent, + .set_parent = emc_set_parent, + .set_rate = emc_set_rate, + .set_rate_and_parent = emc_set_rate_and_parent, + .determine_rate = emc_determine_rate, +}; + +void tegra20_clk_set_emc_round_callback(void *round_cb, void *arg_cb) +{ + struct clk *clk = __clk_lookup("emc"); + struct tegra_clk_emc *emc; + struct clk_hw *hw; + + if (clk) { + hw = __clk_get_hw(clk); + emc = to_tegra_clk_emc(hw); + + emc->round_cb = round_cb; + emc->arg_cb = arg_cb; + } +} + +bool tegra20_clk_emc_driver_available(void) +{ + struct clk *clk = __clk_lookup("emc"); + struct tegra_clk_emc *emc; + struct clk_hw *hw; + + if (clk) { + hw = __clk_get_hw(clk); + emc = to_tegra_clk_emc(hw); + + return !!emc->round_cb; + } + + return false; +} + +struct clk *tegra20_clk_register_emc(void __iomem *ioaddr) +{ + struct tegra_clk_emc *emc; + struct clk_init_data init; + struct clk *clk; + + emc = kzalloc(sizeof(*emc), GFP_KERNEL); + if (!emc) + return NULL; + + init.name = "emc"; + init.ops = &tegra_clk_emc_ops; + init.flags = CLK_IS_CRITICAL; + init.parent_names = emc_parent_clk_names; + init.num_parents = ARRAY_SIZE(emc_parent_clk_names); + + emc->reg = ioaddr; + emc->hw.init = &init; + + clk = clk_register(NULL, &emc->hw); + if (IS_ERR(clk)) { + kfree(emc); + return NULL; + } + + return clk; +} + +void tegra30_clk_set_emc_round_callback(void *round_cb, void *arg_cb) +{ + tegra20_clk_set_emc_round_callback(round_cb, arg_cb); +} + +bool tegra30_clk_emc_driver_available(void) +{ + return tegra20_clk_emc_driver_available(); +} + +struct clk *tegra30_clk_register_emc(void __iomem *ioaddr) +{ + struct tegra_clk_emc *emc; + struct clk_hw *hw; + struct clk *clk; + + clk = tegra20_clk_register_emc(ioaddr); + if (!clk) + return NULL; + + hw = __clk_get_hw(clk); + emc = to_tegra_clk_emc(hw); + emc->want_low_jitter = true; + + return clk; +} + +int tegra30_clk_prepare_emc_mc_same_freq(struct clk *emc_clk, bool same) +{ + struct tegra_clk_emc *emc; + struct clk_hw *hw; + + if (emc_clk) { + hw = __clk_get_hw(emc_clk); + emc = to_tegra_clk_emc(hw); + emc->mc_same_freq = same; + + return 0; + } + + return -EINVAL; +} diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c index c71b61162a32..45472e43ab50 100644 --- a/drivers/clk/tegra/clk-tegra20.c +++ b/drivers/clk/tegra/clk-tegra20.c @@ -141,8 +141,6 @@ static struct cpu_clk_suspend_context { static void __iomem *clk_base; static void __iomem *pmc_base; -static DEFINE_SPINLOCK(emc_lock); - #define TEGRA_INIT_DATA_MUX(_name, _parents, _offset, \ _clk_num, _gate_flags, _clk_id) \ TEGRA_INIT_DATA(_name, NULL, NULL, _parents, _offset, \ @@ -771,7 +769,6 @@ static const char *pwm_parents[] = { "pll_p", "pll_c", "audio", "clk_m", static const char *mux_pllpcm_clkm[] = { "pll_p", "pll_c", "pll_m", "clk_m" }; static const char *mux_pllpdc_clkm[] = { "pll_p", "pll_d_out0", "pll_c", "clk_m" }; -static const char *mux_pllmcp_clkm[] = { "pll_m", "pll_c", "pll_p", "clk_m" }; static struct tegra_periph_init_data tegra_periph_clk_list[] = { TEGRA_INIT_DATA_MUX("i2s1", i2s1_parents, CLK_SOURCE_I2S1, 11, TEGRA_PERIPH_ON_APB, TEGRA20_CLK_I2S1), @@ -798,41 +795,6 @@ static struct tegra_periph_init_data tegra_periph_nodiv_clk_list[] = { TEGRA_INIT_DATA_NODIV("disp2", mux_pllpdc_clkm, CLK_SOURCE_DISP2, 30, 2, 26, 0, TEGRA20_CLK_DISP2), }; -static void __init tegra20_emc_clk_init(void) -{ - const u32 use_pllm_ud = BIT(29); - struct clk *clk; - u32 emc_reg; - - clk = clk_register_mux(NULL, "emc_mux", mux_pllmcp_clkm, - ARRAY_SIZE(mux_pllmcp_clkm), - CLK_SET_RATE_NO_REPARENT, - clk_base + CLK_SOURCE_EMC, - 30, 2, 0, &emc_lock); - - clk = tegra_clk_register_mc("mc", "emc_mux", clk_base + CLK_SOURCE_EMC, - &emc_lock); - clks[TEGRA20_CLK_MC] = clk; - - /* un-divided pll_m_out0 is currently unsupported */ - emc_reg = readl_relaxed(clk_base + CLK_SOURCE_EMC); - if (emc_reg & use_pllm_ud) { - pr_err("%s: un-divided PllM_out0 used as clock source\n", - __func__); - return; - } - - /* - * Note that 'emc_mux' source and 'emc' rate shouldn't be changed at - * the same time due to a HW bug, this won't happen because we're - * defining 'emc_mux' and 'emc' as distinct clocks. - */ - clk = tegra_clk_register_divider("emc", "emc_mux", - clk_base + CLK_SOURCE_EMC, CLK_IS_CRITICAL, - TEGRA_DIVIDER_INT, 0, 8, 1, &emc_lock); - clks[TEGRA20_CLK_EMC] = clk; -} - static void __init tegra20_periph_clk_init(void) { struct tegra_periph_init_data *data; @@ -846,7 +808,13 @@ static void __init tegra20_periph_clk_init(void) clks[TEGRA20_CLK_AC97] = clk; /* emc */ - tegra20_emc_clk_init(); + clk = tegra20_clk_register_emc(clk_base + CLK_SOURCE_EMC); + + clks[TEGRA20_CLK_EMC] = clk; + + clk = tegra_clk_register_mc("mc", "emc", clk_base + CLK_SOURCE_EMC, + NULL); + clks[TEGRA20_CLK_MC] = clk; /* dsi */ clk = tegra_clk_register_periph_gate("dsi", "pll_d", 0, clk_base, 0, @@ -1142,6 +1110,11 @@ static struct clk *tegra20_clk_src_onecell_get(struct of_phandle_args *clkspec, return ERR_PTR(-EPROBE_DEFER); } + if (clkspec->args[0] == TEGRA20_CLK_EMC) { + if (!tegra20_clk_emc_driver_available()) + return ERR_PTR(-EPROBE_DEFER); + } + return clk; } diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c index fa8d573ac626..9b5ffa0a6ed8 100644 --- a/drivers/clk/tegra/clk-tegra30.c +++ b/drivers/clk/tegra/clk-tegra30.c @@ -162,7 +162,6 @@ static unsigned long input_freq; static DEFINE_SPINLOCK(cml_lock); static DEFINE_SPINLOCK(pll_d_lock); -static DEFINE_SPINLOCK(emc_lock); #define TEGRA_INIT_DATA_MUX(_name, _parents, _offset, \ _clk_num, _gate_flags, _clk_id) \ @@ -819,7 +818,7 @@ static struct tegra_clk tegra30_clks[tegra_clk_max] __initdata = { [tegra_clk_pll_a] = { .dt_id = TEGRA30_CLK_PLL_A, .present = true }, [tegra_clk_pll_a_out0] = { .dt_id = TEGRA30_CLK_PLL_A_OUT0, .present = true }, [tegra_clk_cec] = { .dt_id = TEGRA30_CLK_CEC, .present = true }, - [tegra_clk_emc] = { .dt_id = TEGRA30_CLK_EMC, .present = true }, + [tegra_clk_emc] = { .dt_id = TEGRA30_CLK_EMC, .present = false }, }; static const char *pll_e_parents[] = { "pll_ref", "pll_p" }; @@ -1006,7 +1005,6 @@ static void __init tegra30_super_clk_init(void) static const char *mux_pllacp_clkm[] = { "pll_a_out0", "unused", "pll_p", "clk_m" }; static const char *mux_pllpcm_clkm[] = { "pll_p", "pll_c", "pll_m", "clk_m" }; -static const char *mux_pllmcp_clkm[] = { "pll_m", "pll_c", "pll_p", "clk_m" }; static const char *spdif_out_parents[] = { "pll_a_out0", "spdif_2x", "pll_p", "clk_m" }; static const char *mux_pllmcpa[] = { "pll_m", "pll_c", "pll_p", "pll_a_out0" }; @@ -1055,14 +1053,12 @@ static void __init tegra30_periph_clk_init(void) clks[TEGRA30_CLK_AFI] = clk; /* emc */ - clk = clk_register_mux(NULL, "emc_mux", mux_pllmcp_clkm, - ARRAY_SIZE(mux_pllmcp_clkm), - CLK_SET_RATE_NO_REPARENT, - clk_base + CLK_SOURCE_EMC, - 30, 2, 0, &emc_lock); + clk = tegra30_clk_register_emc(clk_base + CLK_SOURCE_EMC); + + clks[TEGRA30_CLK_EMC] = clk; - clk = tegra_clk_register_mc("mc", "emc_mux", clk_base + CLK_SOURCE_EMC, - &emc_lock); + clk = tegra_clk_register_mc("mc", "emc", clk_base + CLK_SOURCE_EMC, + NULL); clks[TEGRA30_CLK_MC] = clk; /* cml0 */ @@ -1313,6 +1309,23 @@ static struct tegra_audio_clk_info tegra30_audio_plls[] = { { "pll_a", &pll_a_params, tegra_clk_pll_a, "pll_p_out1" }, }; +static struct clk *tegra30_clk_src_onecell_get(struct of_phandle_args *clkspec, + void *data) +{ + struct clk *clk; + + clk = of_clk_src_onecell_get(clkspec, data); + if (IS_ERR(clk)) + return clk; + + if (clkspec->args[0] == TEGRA30_CLK_EMC) { + if (!tegra30_clk_emc_driver_available()) + return ERR_PTR(-EPROBE_DEFER); + } + + return clk; +} + static void __init tegra30_clock_init(struct device_node *np) { struct device_node *node; @@ -1356,7 +1369,7 @@ static void __init tegra30_clock_init(struct device_node *np) tegra_init_dup_clks(tegra_clk_duplicates, clks, TEGRA30_CLK_CLK_MAX); - tegra_add_of_provider(np, of_clk_src_onecell_get); + tegra_add_of_provider(np, tegra30_clk_src_onecell_get); tegra_register_devclks(devclks, ARRAY_SIZE(devclks)); tegra_clk_apply_init_table = tegra30_clock_apply_init_table; diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h index 09bccbb9640c..197cc1122e20 100644 --- a/drivers/clk/tegra/clk.h +++ b/drivers/clk/tegra/clk.h @@ -849,4 +849,10 @@ int div_frac_get(unsigned long rate, unsigned parent_rate, u8 width, udelay(delay); \ } while (0) +bool tegra20_clk_emc_driver_available(void); +struct clk *tegra20_clk_register_emc(void __iomem *ioaddr); + +bool tegra30_clk_emc_driver_available(void); +struct clk *tegra30_clk_register_emc(void __iomem *ioaddr); + #endif /* TEGRA_CLK_H */ diff --git a/include/linux/clk/tegra.h b/include/linux/clk/tegra.h index afb9edfa5d58..8e86951b2759 100644 --- a/include/linux/clk/tegra.h +++ b/include/linux/clk/tegra.h @@ -130,4 +130,10 @@ extern void tegra210_put_utmipll_in_iddq(void); extern void tegra210_put_utmipll_out_iddq(void); extern int tegra210_clk_handle_mbist_war(unsigned int id); +struct clk; + +void tegra20_clk_set_emc_round_callback(void *round_cb, void *arg_cb); +void tegra30_clk_set_emc_round_callback(void *round_cb, void *arg_cb); +int tegra30_clk_prepare_emc_mc_same_freq(struct clk *emc_clk, bool same); + #endif /* __LINUX_CLK_TEGRA_H_ */
A proper External Memory Controller clock rounding and parent selection functionality is required by the EMC drivers. It is not available using the generic clock implementation, hence add a custom one. The clock rate rounding shall be done by the EMC drivers because they have information about available memory timings, so the drivers will have to register a callback that will round the requested rate. EMC clock users won't be able to request EMC clock by getting -EPROBE_DEFER until EMC driver is probed and the callback is set up. The functionality is somewhat similar to the clk-emc.c which serves Tegra124+ SoC's, the later HW generations support more parent clock sources and the HW configuration and integration with the EMC drivers differs a tad from the older gens, hence it's not really worth to try to squash everything into a single source file. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/clk/tegra/Makefile | 2 + drivers/clk/tegra/clk-tegra20-emc.c | 307 ++++++++++++++++++++++++++++ drivers/clk/tegra/clk-tegra20.c | 51 ++--- drivers/clk/tegra/clk-tegra30.c | 35 +++- drivers/clk/tegra/clk.h | 6 + include/linux/clk/tegra.h | 6 + 6 files changed, 357 insertions(+), 50 deletions(-) create mode 100644 drivers/clk/tegra/clk-tegra20-emc.c