Re: [PATCH v2 1/4] clk: tegra20/30: Add custom EMC clock implementation

[Dmitry Osipenko <digetx@gmail.com>]

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.