* Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Brendan Higgins @ 2019-07-16 7:57 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, KERNEL
In-Reply-To: <20190715221554.8417320665@mail.kernel.org>
On Mon, Jul 15, 2019 at 3:15 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-07-12 01:17:30)
> > diff --git a/include/kunit/kunit-stream.h b/include/kunit/kunit-stream.h
> > new file mode 100644
> > index 0000000000000..a7b53eabf6be4
> > --- /dev/null
> > +++ b/include/kunit/kunit-stream.h
> > @@ -0,0 +1,81 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * C++ stream style string formatter and printer used in KUnit for outputting
> > + * KUnit messages.
> > + *
> > + * Copyright (C) 2019, Google LLC.
> > + * Author: Brendan Higgins <brendanhiggins@google.com>
> > + */
> > +
> > +#ifndef _KUNIT_KUNIT_STREAM_H
> > +#define _KUNIT_KUNIT_STREAM_H
> > +
> > +#include <linux/types.h>
> > +#include <kunit/string-stream.h>
> > +
> > +struct kunit;
> > +
> > +/**
> > + * struct kunit_stream - a std::stream style string builder.
> > + *
> > + * A std::stream style string builder. Allows messages to be built up and
> > + * printed all at once.
> > + */
> > +struct kunit_stream {
> > + /* private: internal use only. */
> > + struct kunit *test;
> > + const char *level;
>
> Is the level changed? See my comment below, but I wonder if this whole
> struct can go away and the wrappers can just operate on 'struct
> string_stream' instead.
I was inclined to agree with you when I first read your comment, but
then I thought about the case that someone wants to add in a debug
message (of which I currently have none). I think under most
circumstances a user of kunit_stream would likely want to pick a
default verbosity that maybe I should provide, but may still want
different verbosity levels.
The main reason I want to keep the types separate, string_stream vs.
kunit_stream, is that they are intended to be used differently.
string_stream is just a generic string builder. If you are using that,
you are expecting to see someone building the string at some point and
then doing something interesting with it. kunit_stream really tells
you specifically that KUnit is putting together a message to
communicate something to a user of KUnit. It is really used in a very
specific way, and I wouldn't want to generalize its usage beyond how
it is currently used. I think in order to preserve the author's
intention it adds clarity to keep the types separate regardless of how
similar they might be in reality.
> > + struct string_stream *internal_stream;
> > +};
> > diff --git a/kunit/kunit-stream.c b/kunit/kunit-stream.c
> > new file mode 100644
> > index 0000000000000..8bea1f22eafb5
> > --- /dev/null
> > +++ b/kunit/kunit-stream.c
> > @@ -0,0 +1,123 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * C++ stream style string formatter and printer used in KUnit for outputting
> > + * KUnit messages.
> > + *
> > + * Copyright (C) 2019, Google LLC.
> > + * Author: Brendan Higgins <brendanhiggins@google.com>
> > + */
> > +
> > +#include <kunit/test.h>
> > +#include <kunit/kunit-stream.h>
> > +#include <kunit/string-stream.h>
> > +
> > +void kunit_stream_add(struct kunit_stream *kstream, const char *fmt, ...)
> > +{
> > + va_list args;
> > + struct string_stream *stream = kstream->internal_stream;
> > +
> > + va_start(args, fmt);
> > +
> > + if (string_stream_vadd(stream, fmt, args) < 0)
> > + kunit_err(kstream->test,
> > + "Failed to allocate fragment: %s\n",
> > + fmt);
> > +
> > + va_end(args);
> > +}
> > +
> > +void kunit_stream_append(struct kunit_stream *kstream,
> > + struct kunit_stream *other)
> > +{
> > + struct string_stream *other_stream = other->internal_stream;
> > + const char *other_content;
> > +
> > + other_content = string_stream_get_string(other_stream);
> > +
> > + if (!other_content) {
> > + kunit_err(kstream->test,
> > + "Failed to get string from second argument for appending\n");
> > + return;
> > + }
> > +
> > + kunit_stream_add(kstream, other_content);
> > +}
>
> Why can't this function be implemented in the string_stream API? Seems
> valid to want to append one stream to another and that isn't
> kunit_stream specific.
Fair point. Will do.
> > +
> > +void kunit_stream_clear(struct kunit_stream *kstream)
> > +{
> > + string_stream_clear(kstream->internal_stream);
> > +}
> > +
> > +void kunit_stream_commit(struct kunit_stream *kstream)
> > +{
> > + struct string_stream *stream = kstream->internal_stream;
> > + struct string_stream_fragment *fragment;
> > + struct kunit *test = kstream->test;
> > + char *buf;
> > +
> > + buf = string_stream_get_string(stream);
> > + if (!buf) {
> > + kunit_err(test,
> > + "Could not allocate buffer, dumping stream:\n");
> > + list_for_each_entry(fragment, &stream->fragments, node) {
> > + kunit_err(test, fragment->fragment);
> > + }
> > + kunit_err(test, "\n");
> > + goto cleanup;
> > + }
> > +
> > + kunit_printk(kstream->level, test, buf);
> > + kfree(buf);
> > +
> > +cleanup:
>
> Drop the goto and use an 'else' please.
Will do.
> > + kunit_stream_clear(kstream);
> > +}
> > +
> > +static int kunit_stream_init(struct kunit_resource *res, void *context)
> > +{
> > + struct kunit *test = context;
> > + struct kunit_stream *stream;
> > +
> > + stream = kzalloc(sizeof(*stream), GFP_KERNEL);
> > + if (!stream)
> > + return -ENOMEM;
> > +
> > + res->allocation = stream;
> > + stream->test = test;
> > + stream->internal_stream = alloc_string_stream(test);
> > +
> > + if (!stream->internal_stream)
> > + return -ENOMEM;
> > +
> > + return 0;
> > +}
> > +
> > +static void kunit_stream_free(struct kunit_resource *res)
> > +{
> > + struct kunit_stream *stream = res->allocation;
> > +
> > + if (!string_stream_is_empty(stream->internal_stream)) {
> > + kunit_err(stream->test,
> > + "End of test case reached with uncommitted stream entries\n");
> > + kunit_stream_commit(stream);
> > + }
> > +}
> > +
>
> Nitpick: Drop this extra newline.
Oops, nice catch.
> > diff --git a/kunit/test.c b/kunit/test.c
> > index f165c9d8e10b0..29edf34a89a37 100644
> > --- a/kunit/test.c
> > +++ b/kunit/test.c
> > @@ -120,6 +120,12 @@ static void kunit_print_test_case_ok_not_ok(struct kunit_case *test_case,
> > test_case->name);
> > }
> >
> > +void kunit_fail(struct kunit *test, struct kunit_stream *stream)
>
> Why doesn't 'struct kunit' have a 'struct kunit_stream' inside of it? It
> seems that the two are highly related, to the point that it might just
> make sense to have
A `struct kunit_stream` is usually associated with a message that is
being built up over time like maybe an expectation; it is meant to
capture the idea that we might want to send some information out to
the user pertaining to some thing 'X', but we aren't sure that we
actually want to send it until 'X' is complete, but do to the nature
of 'X' it is easier to start constructing the message before 'X' is
complete.
Consider a complicated expectation, there might be multiple conditions
that satisfy it and multiple conditions which could make it fail. As
we start exploring the input to the expectation we gain information
that we might want to share back with the user if the expectation were
to fail and we might get that information before we are actually sure
that the expectation does indeed fail.
When we first step into the expectation we immediately know the
function name, file name, and line number where we are called and
would want to put that information into any message we would send to
the user about this expectation. Next, we might want to check a
property of the input, it may or may not be enough information on its
own for the expectation to fail, but we want to share the result of
the property check with the user regardless, BUT only if the
expectation as a whole fails.
Hence, we can have multiple `struct kunit_stream`s associated with a
`struct kunit` active at any given time.
> struct kunit {
> struct kunit_stream stream;
> ...
> };
>
> > +{
> > + kunit_set_failure(test);
> > + kunit_stream_commit(stream);
>
> And then this function can just take a test and the stream can be
> associated with the test directly. Use container_of() to get to the test
> when the only pointer in hand is for the stream too.
Unfortunately that wouldn't work. See my above explanation.
> > +}
> > +
> > void kunit_init_test(struct kunit *test, const char *name)
> > {
> > mutex_init(&test->lock);
Thanks!
^ permalink raw reply
* Re: [PATCH] ARM: dts: imx6ul-kontron-ul2: Add Exceet/Kontron iMX6-UL2 SoM
From: Krzysztof Kozlowski @ 2019-07-16 7:50 UTC (permalink / raw)
To: Schrempf Frieder
Cc: Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <5cbd8bb2-6ecb-7e55-1580-e580e2c340dd@kontron.de>
On Fri, 12 Jul 2019 at 17:21, Schrempf Frieder
<frieder.schrempf@kontron.de> wrote:
>
> Hi Krzysztof,
>
> On 12.07.19 16:12, Krzysztof Kozlowski wrote:
> > Add support for iMX6-UL2 modules from Kontron Electronics GmbH (before
> > acquisition: Exceet Electronics) and evalkit boards based on it:
> >
> > 1. i.MX6 UL System-on-Module, a 25x25 mm solderable module (LGA pads and
> > pin castellations) with 256 MB RAM, 1 MB NOR-Flash, 256 MB NAND and
> > other interfaces,
> > 1. UL2 evalkit, w/wo eMMC, without display,
> > 2. UL2 evalkit with 4.3" display,
> > 3. UL2 evalkit with 5.0" display.
> >
> > This includes device nodes for unsupported displays (Admatec
> > T043C004800272T2A and T070P133T0S301).
> >
> > The work is based on Exceet source code (GPLv2) with numerous changes:
> > 1. Reorganize files,
> > 2. Rename Exceet -> Kontron,
> > 3. Fix coding style errors,
> > 4. Fix DTC warnings,
> > 5. Extend compatibles so eval boards inherit the SoM compatible,
> > 6. Use defines instead of GPIO flag values,
> > 7. Adjust operating points of CPU0,
> > 8. Sort nodes alphabetically.
> >
> > In downstream BSP the Exceet name still appears in multiple places
> > therefore I left it in the model names.
>
> First, thanks for your work. I planned to upstream these boards myself
> after the FSL QSPI spi-mem driver was merged in 5.1, but didn't have
> time to finalize and send the patches.
>
> Meanwhile we came up with a new naming scheme for our boards, that
> hasn't been implemented yet. But I would like to take this chance to
> implement the new scheme.
Sure, I see no problem in using different names, matching downstream
kernel. Just point me to proper names.
> Also there are some more flavors of the SoM (with i.MX6ULL instead of
> i.MX6UL, with 512MiB instead of 256MiB flash/RAM), that I would like to
> add and for which common parts of the SoM dtsi would need to be factored
> out to a separate file.
I have only this one particular flavor so I would prefer to upstream
only this one. I do not know all the possible combinations or for
example the most interesting ones. I think after this patchset we can
refactor the DTS whenever its needed - split common parts, add new
files.
> I would prefer to at least apply the naming changes before merging. The
> additional board flavors could be added before merging or I could send
> them as follow-up patches. What do you think?
Let's change the naming and add new flavors as follow ups?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Joseph Lo @ 2019-07-16 7:24 UTC (permalink / raw)
To: Sowjanya Komatineni, Dmitry Osipenko
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pdeschrijver, pgaikwad,
sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <21266e4f-16b1-4c87-067a-16c07c803b6e@nvidia.com>
On 7/16/19 2:35 PM, Sowjanya Komatineni wrote:
>
> On 7/15/19 10:37 PM, Dmitry Osipenko wrote:
>> В Mon, 15 Jul 2019 21:37:09 -0700
>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>
>>> On 7/15/19 8:50 PM, Dmitry Osipenko wrote:
>>>> 16.07.2019 6:00, Sowjanya Komatineni пишет:
>>>>> On 7/15/19 5:35 PM, Sowjanya Komatineni wrote:
>>>>>> On 7/14/19 2:41 PM, Dmitry Osipenko wrote:
>>>>>>> 13.07.2019 8:54, Sowjanya Komatineni пишет:
>>>>>>>> On 6/29/19 8:10 AM, Dmitry Osipenko wrote:
>>>>>>>>> 28.06.2019 5:12, Sowjanya Komatineni пишет:
>>>>>>>>>> This patch adds system suspend and resume support for Tegra210
>>>>>>>>>> clocks.
>>>>>>>>>>
>>>>>>>>>> All the CAR controller settings are lost on suspend when core
>>>>>>>>>> power goes off.
>>>>>>>>>>
>>>>>>>>>> This patch has implementation for saving and restoring all
>>>>>>>>>> the PLLs and clocks context during system suspend and resume
>>>>>>>>>> to have the clocks back to same state for normal operation.
>>>>>>>>>>
>>>>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>>> ---
>>>>>>>>>> drivers/clk/tegra/clk-tegra210.c | 115
>>>>>>>>>> ++++++++++++++++++++++++++++++++++++++-
>>>>>>>>>> drivers/clk/tegra/clk.c | 14 +++++
>>>>>>>>>> drivers/clk/tegra/clk.h | 1 +
>>>>>>>>>> 3 files changed, 127 insertions(+), 3 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-tegra210.c
>>>>>>>>>> b/drivers/clk/tegra/clk-tegra210.c
>>>>>>>>>> index 1c08c53482a5..1b839544e086 100644
>>>>>>>>>> --- a/drivers/clk/tegra/clk-tegra210.c
>>>>>>>>>> +++ b/drivers/clk/tegra/clk-tegra210.c
>>>>>>>>>> @@ -9,10 +9,12 @@
>>>>>>>>>> #include <linux/clkdev.h>
>>>>>>>>>> #include <linux/of.h>
>>>>>>>>>> #include <linux/of_address.h>
>>>>>>>>>> +#include <linux/of_platform.h>
>>>>>>>>>> #include <linux/delay.h>
>>>>>>>>>> #include <linux/export.h>
>>>>>>>>>> #include <linux/mutex.h>
>>>>>>>>>> #include <linux/clk/tegra.h>
>>>>>>>>>> +#include <linux/syscore_ops.h>
>>>>>>>>>> #include <dt-bindings/clock/tegra210-car.h>
>>>>>>>>>> #include <dt-bindings/reset/tegra210-car.h>
>>>>>>>>>> #include <linux/iopoll.h>
>>>>>>>>>> @@ -20,6 +22,7 @@
>>>>>>>>>> #include <soc/tegra/pmc.h>
>>>>>>>>>> #include "clk.h"
>>>>>>>>>> +#include "clk-dfll.h"
>>>>>>>>>> #include "clk-id.h"
>>>>>>>>>> /*
>>>>>>>>>> @@ -225,6 +228,7 @@
>>>>>>>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_SET 0x2a8
>>>>>>>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_CLR 0x2ac
>>>>>>>>>> +#define CPU_SOFTRST_CTRL 0x380
>>>>>>>>>> #define LVL2_CLK_GATE_OVRA 0xf8
>>>>>>>>>> #define LVL2_CLK_GATE_OVRC 0x3a0
>>>>>>>>>> @@ -2820,6 +2824,7 @@ static int tegra210_enable_pllu(void)
>>>>>>>>>> struct tegra_clk_pll_freq_table *fentry;
>>>>>>>>>> struct tegra_clk_pll pllu;
>>>>>>>>>> u32 reg;
>>>>>>>>>> + int ret;
>>>>>>>>>> for (fentry = pll_u_freq_table; fentry->input_rate;
>>>>>>>>>> fentry++) {
>>>>>>>>>> if (fentry->input_rate == pll_ref_freq)
>>>>>>>>>> @@ -2847,10 +2852,10 @@ static int tegra210_enable_pllu(void)
>>>>>>>>>> fence_udelay(1, clk_base);
>>>>>>>>>> reg |= PLL_ENABLE;
>>>>>>>>>> writel(reg, clk_base + PLLU_BASE);
>>>>>>>>>> + fence_udelay(1, clk_base);
>>>>>>>>>> - readl_relaxed_poll_timeout_atomic(clk_base +
>>>>>>>>>> PLLU_BASE, reg,
>>>>>>>>>> - reg & PLL_BASE_LOCK, 2, 1000);
>>>>>>>>>> - if (!(reg & PLL_BASE_LOCK)) {
>>>>>>>>>> + ret = tegra210_wait_for_mask(&pllu, PLLU_BASE,
>>>>>>>>>> PLL_BASE_LOCK);
>>>>>>>>>> + if (ret) {
>>>>>>>>>> pr_err("Timed out waiting for PLL_U to lock\n");
>>>>>>>>>> return -ETIMEDOUT;
>>>>>>>>>> }
>>>>>>>>>> @@ -3283,6 +3288,103 @@ static void
>>>>>>>>>> tegra210_disable_cpu_clock(u32 cpu)
>>>>>>>>>> }
>>>>>>>>>> #ifdef CONFIG_PM_SLEEP
>>>>>>>>>> +static u32 cpu_softrst_ctx[3];
>>>>>>>>>> +static struct platform_device *dfll_pdev;
>>>>>>>>>> +#define car_readl(_base, _off) readl_relaxed(clk_base +
>>>>>>>>>> (_base) + ((_off) * 4))
>>>>>>>>>> +#define car_writel(_val, _base, _off) \
>>>>>>>>>> + writel_relaxed(_val, clk_base + (_base) + ((_off) *
>>>>>>>>>> 4)) +
>>>>>>>>>> +static int tegra210_clk_suspend(void)
>>>>>>>>>> +{
>>>>>>>>>> + unsigned int i;
>>>>>>>>>> + struct device_node *node;
>>>>>>>>>> +
>>>>>>>>>> + tegra_cclkg_burst_policy_save_context();
>>>>>>>>>> +
>>>>>>>>>> + if (!dfll_pdev) {
>>>>>>>>>> + node = of_find_compatible_node(NULL, NULL,
>>>>>>>>>> + "nvidia,tegra210-dfll");
>>>>>>>>>> + if (node)
>>>>>>>>>> + dfll_pdev = of_find_device_by_node(node);
>>>>>>>>>> +
>>>>>>>>>> + of_node_put(node);
>>>>>>>>>> + if (!dfll_pdev)
>>>>>>>>>> + pr_err("dfll node not found. no suspend for
>>>>>>>>>> dfll\n");
>>>>>>>>>> + }
>>>>>>>>>> +
>>>>>>>>>> + if (dfll_pdev)
>>>>>>>>>> + tegra_dfll_suspend(dfll_pdev);
>>>>>>>>>> +
>>>>>>>>>> + /* Enable PLLP_OUT_CPU after dfll suspend */
>>>>>>>>>> + tegra_clk_set_pllp_out_cpu(true);
>>>>>>>>>> +
>>>>>>>>>> + tegra_sclk_cclklp_burst_policy_save_context();
>>>>>>>>>> +
>>>>>>>>>> + clk_save_context();
>>>>>>>>>> +
>>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>>>>>>>>> + cpu_softrst_ctx[i] = car_readl(CPU_SOFTRST_CTRL, i);
>>>>>>>>>> +
>>>>>>>>>> + return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static void tegra210_clk_resume(void)
>>>>>>>>>> +{
>>>>>>>>>> + unsigned int i;
>>>>>>>>>> + struct clk_hw *parent;
>>>>>>>>>> + struct clk *clk;
>>>>>>>>>> +
>>>>>>>>>> + /*
>>>>>>>>>> + * clk_restore_context restores clocks as per the clock
>>>>>>>>>> tree.
>>>>>>>>>> + *
>>>>>>>>>> + * dfllCPU_out is first in the clock tree to get
>>>>>>>>>> restored and it
>>>>>>>>>> + * involves programming DFLL controller along with
>>>>>>>>>> restoring CPUG
>>>>>>>>>> + * clock burst policy.
>>>>>>>>>> + *
>>>>>>>>>> + * DFLL programming needs dfll_ref and dfll_soc
>>>>>>>>>> peripheral clocks
>>>>>>>>>> + * to be restores which are part ofthe peripheral
>>>>>>>>>> clocks.
>>>>>>> ^ white-space
>>>>>>>
>>>>>>> Please use spellchecker to avoid typos.
>>>>>>>>>> + * So, peripheral clocks restore should happen prior to
>>>>>>>>>> dfll clock
>>>>>>>>>> + * restore.
>>>>>>>>>> + */
>>>>>>>>>> +
>>>>>>>>>> + tegra_clk_osc_resume(clk_base);
>>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>>>>>>>>> + car_writel(cpu_softrst_ctx[i], CPU_SOFTRST_CTRL, i);
>>>>>>>>>> +
>>>>>>>>>> + /* restore all plls and peripheral clocks */
>>>>>>>>>> + tegra210_init_pllu();
>>>>>>>>>> + clk_restore_context();
>>>>>>>>>> +
>>>>>>>>>> + fence_udelay(5, clk_base);
>>>>>>>>>> +
>>>>>>>>>> + /* resume SCLK and CPULP clocks */
>>>>>>>>>> + tegra_sclk_cpulp_burst_policy_restore_context();
>>>>>>>>>> +
>>>>>>>>>> + /*
>>>>>>>>>> + * restore CPUG clocks:
>>>>>>>>>> + * - enable DFLL in open loop mode
>>>>>>>>>> + * - switch CPUG to DFLL clock source
>>>>>>>>>> + * - close DFLL loop
>>>>>>>>>> + * - sync PLLX state
>>>>>>>>>> + */
>>>>>>>>>> + if (dfll_pdev)
>>>>>>>>>> + tegra_dfll_resume(dfll_pdev, false);
>>>>>>>>>> +
>>>>>>>>>> + tegra_cclkg_burst_policy_restore_context();
>>>>>>>>>> + fence_udelay(2, clk_base);
>>>>>>>>>> +
>>>>>>>>>> + if (dfll_pdev)
>>>>>>>>>> + tegra_dfll_resume(dfll_pdev, true);
>>>>>>>>>> +
>>>>>>>>>> + parent =
>>>>>>>>>> clk_hw_get_parent(__clk_get_hw(clks[TEGRA210_CLK_CCLK_G]));
>>>>>>>>>> + clk = clks[TEGRA210_CLK_PLL_X];
>>>>>>>>>> + if (parent != __clk_get_hw(clk))
>>>>>>>>>> + tegra_clk_sync_state_pll(__clk_get_hw(clk));
>>>>>>>>>> +
>>>>>>>>>> + /* Disable PLL_OUT_CPU after DFLL resume */
>>>>>>>>>> + tegra_clk_set_pllp_out_cpu(false);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> static void tegra210_cpu_clock_suspend(void)
>>>>>>>>>> {
>>>>>>>>>> /* switch coresite to clk_m, save off original source
>>>>>>>>>> */ @@ -3298,6 +3400,11 @@ static void
>>>>>>>>>> tegra210_cpu_clock_resume(void) }
>>>>>>>>>> #endif
>>>>>>>>>> +static struct syscore_ops tegra_clk_syscore_ops = {
>>>>>>>>>> + .suspend = tegra210_clk_suspend,
>>>>>>>>>> + .resume = tegra210_clk_resume,
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> static struct tegra_cpu_car_ops tegra210_cpu_car_ops = {
>>>>>>>>>> .wait_for_reset = tegra210_wait_cpu_in_reset,
>>>>>>>>>> .disable_clock = tegra210_disable_cpu_clock,
>>>>>>>>>> @@ -3583,5 +3690,7 @@ static void __init
>>>>>>>>>> tegra210_clock_init(struct device_node *np)
>>>>>>>>>> tegra210_mbist_clk_init();
>>>>>>>>>> tegra_cpu_car_ops = &tegra210_cpu_car_ops;
>>>>>>>>>> +
>>>>>>>>>> + register_syscore_ops(&tegra_clk_syscore_ops);
>>>>>>>>>> }
>>>>>>>>> Is it really worthwhile to use syscore_ops for suspend/resume
>>>>>>>>> given that drivers for
>>>>>>>>> won't resume before the CLK driver anyway? Are there any other
>>>>>>>>> options for CLK
>>>>>>>>> suspend/resume?
>>>>>>>>>
>>>>>>>>> I'm also not sure whether PM runtime API could be used at all
>>>>>>>>> in the context of
>>>>>>>>> syscore_ops ..
>>>>>>>>>
>>>>>>>>> Secondly, what about to use generic clk_save_context() /
>>>>>>>>> clk_restore_context()
>>>>>>>>> helpers for the suspend-resume? It looks to me that some other
>>>>>>>>> essential (and proper)
>>>>>>>>> platform driver (soc/tegra/? PMC?) should suspend-resume the
>>>>>>>>> clocks using the generic
>>>>>>>>> CLK Framework API.
>>>>>>>> Clock resume should happen very early to restore peripheral and
>>>>>>>> cpu clocks very early than peripheral drivers resume happens.
>>>>>>> If all peripheral drivers properly requested all of the
>>>>>>> necessary clocks and CLK driver was a platform driver, then I
>>>>>>> guess the probe should have been naturally ordered. But that's
>>>>>>> not very achievable with the currently available infrastructure
>>>>>>> in the kernel, so I'm not arguing that the clocks should be
>>>>>>> explicitly resumed before the users.
>>>>>>>> this patch series uses clk_save_context and clk_restore_context
>>>>>>>> for corresponding divider, pll, pllout.. save and restore
>>>>>>>> context.
>>>>>>> Now I see that indeed this API is utilized in this patch, thank
>>>>>>> you for the clarification.
>>>>>>>> But as there is dependency on dfll resume and cpu and pllx
>>>>>>>> clocks restore, couldnt use clk_save_context and
>>>>>>>> clk_restore_context for dfll.
>>>>>>>>
>>>>>>>> So implemented recommended dfll resume sequence in main
>>>>>>>> Tegra210 clock driver along with invoking
>>>>>>>> clk_save_context/clk_restore_context where all other clocks
>>>>>>>> save/restore happens as per clock tree traversal.
>>>>>>> Could you please clarify what part of peripherals clocks is
>>>>>>> required for DFLL's restore? Couldn't DFLL driver be changed to
>>>>>>> avoid that quirkness and thus to make DFLL driver suspend/resume
>>>>>>> the clock?
>>>>>> DFLL source ref_clk and soc_clk need to be restored prior to dfll.
>>>>>>
>>>>>> I see dfllCPU_out parent to CCLK_G first in the clock tree and
>>>>>> dfll_ref and dfll_soc peripheral clocks are not resumed by the
>>>>>> time dfll resume happens first.
>>>>>>
>>>>>> ref_clk and soc_clk source is from pll_p and clock tree has these
>>>>>> registered under pll_p which happens later.
>>>>>>
>>>>>> tegra210_clock_init registers in order plls, peripheral clocks,
>>>>>> super_clk init for cclk_g during clock driver probe and dfll
>>>>>> probe and register happens later.
>>>>> One more thing, CLDVFS peripheral clock enable is also needed to be
>>>>> enabled to program DFLL Controller and all peripheral clock
>>>>> context is restored only after their PLL sources are restored.
>>>>>
>>>>> DFLL restore involves dfll source clock resume along with CLDVFS
>>>>> periheral clock enable and reset
>>>> I don't quite see why you can't simply add suspend/resume callbacks
>>>> to the CPUFreq driver to:
>>>>
>>>> On suspend:
>>>> 1. Switch CPU to PLLP (or whatever "safe" parent)
>>>> 2. Disable/teardown DFLL
>>>>
>>>> On resume:
>>>> 1. Enable/restore DFLL
>>>> 2. Switch CPU back to DFLL
>>> dfll runtime suspend/resume are already part of dfll_pm_ops. Don't we
>>> want to use it for suspend/resume as well?
>> Looks like no. Seems runtime PM of that driver is intended solely for
>> the DFLL's clk management.
>>
>>> currently no APIs are shared b/w clk/tegra driver and CPUFreq driver
>>> to invoke dfll suspend/resume in CPUFreq driver
>>>
>> Just add it. Also, please note that CPUFreq driver is optional and thus
>> you may need to switch CPU to a safe parent on clk-core suspend as
>> well in order to resume properly if CPU was running off unsafe parent
>> during boot and CPUFreq driver is disabled in kernel build (or failed
>> to load).
> OK, Will add to CPUFreq driver...
>>
>> The other thing that also need attention is that T124 CPUFreq driver
>> implicitly relies on DFLL driver to be probed first, which is icky.
>>
> Should I add check for successful dfll clk register explicitly in
> CPUFreq driver probe and defer till dfll clk registers?
Sorry, I didn't follow the mail thread. Just regarding the DFLL part.
As you know it, the DFLL clock is one of the CPU clock sources and
integrated with DVFS control logic with the regulator. We will not
switch CPU to other clock sources once we switched to DFLL. Because the
CPU has been regulated by the DFLL HW with the DVFS table (CVB or OPP
table you see in the driver.). We shouldn't reparent it to other sources
with unknew freq/volt pair. That's not guaranteed to work. We allow
switching to open-loop mode but different sources.
And I don't exactly understand why we need to switch to PLLP in CPU idle
driver. Just keep it on CL-DVFS mode all the time.
In SC7 entry, the dfll suspend function moves it the open-loop mode.
That's all. The sc7-entryfirmware will handle the rest of the sequence
to turn off the CPU power.
In SC7 resume, the warmboot code will handle the sequence to turn on
regulator and power up the CPU cluster. And leave it on PLL_P. After
resuming to the kernel, we re-init DFLL, restore the CPU clock policy
(CPU runs on DFLL open-loop mode) and then moving to close-loop mode.
The DFLL part looks good to me. BTW, change the patch subject to "Add
suspend-resume support" seems more appropriate to me.
Thanks.
^ permalink raw reply
* Re: [PATCH] dt-bindings: Ensure child nodes are of type 'object'
From: Miquel Raynal @ 2019-07-16 7:19 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, linux-kernel, Maxime Ripard, Chen-Yu Tsai,
David Woodhouse, Brian Norris, Marek Vasut, Richard Weinberger,
Vignesh Raghavendra, Linus Walleij, Maxime Coquelin,
Alexandre Torgue, Mark Brown, linux-mtd, linux-gpio, linux-stm32,
linux-spi
In-Reply-To: <20190715230457.3901-1-robh@kernel.org>
Hi Rob,
Rob Herring <robh@kernel.org> wrote on Mon, 15 Jul 2019 17:04:57 -0600:
> Properties which are child node definitions need to have an explict
> type. Otherwise, a matching (DT) property can silently match when an
> error is desired. Fix this up tree-wide. Once this is fixed, the
> meta-schema will enforce this on any child node definitions.
>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Chen-Yu Tsai <wens@csie.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: Marek Vasut <marek.vasut@gmail.com>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Vignesh Raghavendra <vigneshr@ti.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: linux-mtd@lists.infradead.org
> Cc: linux-gpio@vger.kernel.org
> Cc: linux-stm32@st-md-mailman.stormreply.com
> Cc: linux-spi@vger.kernel.org
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Please ack. I will take this via the DT tree.
>
> Rob
>
> .../devicetree/bindings/bus/allwinner,sun8i-a23-rsb.yaml | 1 +
> .../devicetree/bindings/mtd/allwinner,sun4i-a10-nand.yaml | 1 +
> Documentation/devicetree/bindings/mtd/nand-controller.yaml | 1 +
> .../devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml | 3 +++
> .../devicetree/bindings/spi/allwinner,sun4i-a10-spi.yaml | 1 +
> .../devicetree/bindings/spi/allwinner,sun6i-a31-spi.yaml | 1 +
> 6 files changed, 8 insertions(+)
>
[...]
> diff --git a/Documentation/devicetree/bindings/mtd/nand-controller.yaml b/Documentation/devicetree/bindings/mtd/nand-controller.yaml
> index 199ba5ac2a06..d261b7096c69 100644
> --- a/Documentation/devicetree/bindings/mtd/nand-controller.yaml
> +++ b/Documentation/devicetree/bindings/mtd/nand-controller.yaml
> @@ -40,6 +40,7 @@ properties:
>
> patternProperties:
> "^nand@[a-f0-9]$":
> + type: object
> properties:
> reg:
> description:
For the mtd .yaml:
Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
Thanks,
Miquèl
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-16 6:35 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pdeschrijver, pgaikwad,
sboyd, linux-clk, linux-gpio, jckuo, josephl, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <20190716083701.225f0fd9@dimatab>
On 7/15/19 10:37 PM, Dmitry Osipenko wrote:
> В Mon, 15 Jul 2019 21:37:09 -0700
> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>
>> On 7/15/19 8:50 PM, Dmitry Osipenko wrote:
>>> 16.07.2019 6:00, Sowjanya Komatineni пишет:
>>>> On 7/15/19 5:35 PM, Sowjanya Komatineni wrote:
>>>>> On 7/14/19 2:41 PM, Dmitry Osipenko wrote:
>>>>>> 13.07.2019 8:54, Sowjanya Komatineni пишет:
>>>>>>> On 6/29/19 8:10 AM, Dmitry Osipenko wrote:
>>>>>>>> 28.06.2019 5:12, Sowjanya Komatineni пишет:
>>>>>>>>> This patch adds system suspend and resume support for Tegra210
>>>>>>>>> clocks.
>>>>>>>>>
>>>>>>>>> All the CAR controller settings are lost on suspend when core
>>>>>>>>> power goes off.
>>>>>>>>>
>>>>>>>>> This patch has implementation for saving and restoring all
>>>>>>>>> the PLLs and clocks context during system suspend and resume
>>>>>>>>> to have the clocks back to same state for normal operation.
>>>>>>>>>
>>>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>> ---
>>>>>>>>> drivers/clk/tegra/clk-tegra210.c | 115
>>>>>>>>> ++++++++++++++++++++++++++++++++++++++-
>>>>>>>>> drivers/clk/tegra/clk.c | 14 +++++
>>>>>>>>> drivers/clk/tegra/clk.h | 1 +
>>>>>>>>> 3 files changed, 127 insertions(+), 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/clk/tegra/clk-tegra210.c
>>>>>>>>> b/drivers/clk/tegra/clk-tegra210.c
>>>>>>>>> index 1c08c53482a5..1b839544e086 100644
>>>>>>>>> --- a/drivers/clk/tegra/clk-tegra210.c
>>>>>>>>> +++ b/drivers/clk/tegra/clk-tegra210.c
>>>>>>>>> @@ -9,10 +9,12 @@
>>>>>>>>> #include <linux/clkdev.h>
>>>>>>>>> #include <linux/of.h>
>>>>>>>>> #include <linux/of_address.h>
>>>>>>>>> +#include <linux/of_platform.h>
>>>>>>>>> #include <linux/delay.h>
>>>>>>>>> #include <linux/export.h>
>>>>>>>>> #include <linux/mutex.h>
>>>>>>>>> #include <linux/clk/tegra.h>
>>>>>>>>> +#include <linux/syscore_ops.h>
>>>>>>>>> #include <dt-bindings/clock/tegra210-car.h>
>>>>>>>>> #include <dt-bindings/reset/tegra210-car.h>
>>>>>>>>> #include <linux/iopoll.h>
>>>>>>>>> @@ -20,6 +22,7 @@
>>>>>>>>> #include <soc/tegra/pmc.h>
>>>>>>>>> #include "clk.h"
>>>>>>>>> +#include "clk-dfll.h"
>>>>>>>>> #include "clk-id.h"
>>>>>>>>> /*
>>>>>>>>> @@ -225,6 +228,7 @@
>>>>>>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_SET 0x2a8
>>>>>>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_CLR 0x2ac
>>>>>>>>> +#define CPU_SOFTRST_CTRL 0x380
>>>>>>>>> #define LVL2_CLK_GATE_OVRA 0xf8
>>>>>>>>> #define LVL2_CLK_GATE_OVRC 0x3a0
>>>>>>>>> @@ -2820,6 +2824,7 @@ static int tegra210_enable_pllu(void)
>>>>>>>>> struct tegra_clk_pll_freq_table *fentry;
>>>>>>>>> struct tegra_clk_pll pllu;
>>>>>>>>> u32 reg;
>>>>>>>>> + int ret;
>>>>>>>>> for (fentry = pll_u_freq_table; fentry->input_rate;
>>>>>>>>> fentry++) {
>>>>>>>>> if (fentry->input_rate == pll_ref_freq)
>>>>>>>>> @@ -2847,10 +2852,10 @@ static int tegra210_enable_pllu(void)
>>>>>>>>> fence_udelay(1, clk_base);
>>>>>>>>> reg |= PLL_ENABLE;
>>>>>>>>> writel(reg, clk_base + PLLU_BASE);
>>>>>>>>> + fence_udelay(1, clk_base);
>>>>>>>>> - readl_relaxed_poll_timeout_atomic(clk_base +
>>>>>>>>> PLLU_BASE, reg,
>>>>>>>>> - reg & PLL_BASE_LOCK, 2, 1000);
>>>>>>>>> - if (!(reg & PLL_BASE_LOCK)) {
>>>>>>>>> + ret = tegra210_wait_for_mask(&pllu, PLLU_BASE,
>>>>>>>>> PLL_BASE_LOCK);
>>>>>>>>> + if (ret) {
>>>>>>>>> pr_err("Timed out waiting for PLL_U to lock\n");
>>>>>>>>> return -ETIMEDOUT;
>>>>>>>>> }
>>>>>>>>> @@ -3283,6 +3288,103 @@ static void
>>>>>>>>> tegra210_disable_cpu_clock(u32 cpu)
>>>>>>>>> }
>>>>>>>>> #ifdef CONFIG_PM_SLEEP
>>>>>>>>> +static u32 cpu_softrst_ctx[3];
>>>>>>>>> +static struct platform_device *dfll_pdev;
>>>>>>>>> +#define car_readl(_base, _off) readl_relaxed(clk_base +
>>>>>>>>> (_base) + ((_off) * 4))
>>>>>>>>> +#define car_writel(_val, _base, _off) \
>>>>>>>>> + writel_relaxed(_val, clk_base + (_base) + ((_off) *
>>>>>>>>> 4)) +
>>>>>>>>> +static int tegra210_clk_suspend(void)
>>>>>>>>> +{
>>>>>>>>> + unsigned int i;
>>>>>>>>> + struct device_node *node;
>>>>>>>>> +
>>>>>>>>> + tegra_cclkg_burst_policy_save_context();
>>>>>>>>> +
>>>>>>>>> + if (!dfll_pdev) {
>>>>>>>>> + node = of_find_compatible_node(NULL, NULL,
>>>>>>>>> + "nvidia,tegra210-dfll");
>>>>>>>>> + if (node)
>>>>>>>>> + dfll_pdev = of_find_device_by_node(node);
>>>>>>>>> +
>>>>>>>>> + of_node_put(node);
>>>>>>>>> + if (!dfll_pdev)
>>>>>>>>> + pr_err("dfll node not found. no suspend for
>>>>>>>>> dfll\n");
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + if (dfll_pdev)
>>>>>>>>> + tegra_dfll_suspend(dfll_pdev);
>>>>>>>>> +
>>>>>>>>> + /* Enable PLLP_OUT_CPU after dfll suspend */
>>>>>>>>> + tegra_clk_set_pllp_out_cpu(true);
>>>>>>>>> +
>>>>>>>>> + tegra_sclk_cclklp_burst_policy_save_context();
>>>>>>>>> +
>>>>>>>>> + clk_save_context();
>>>>>>>>> +
>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>>>>>>>> + cpu_softrst_ctx[i] = car_readl(CPU_SOFTRST_CTRL, i);
>>>>>>>>> +
>>>>>>>>> + return 0;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static void tegra210_clk_resume(void)
>>>>>>>>> +{
>>>>>>>>> + unsigned int i;
>>>>>>>>> + struct clk_hw *parent;
>>>>>>>>> + struct clk *clk;
>>>>>>>>> +
>>>>>>>>> + /*
>>>>>>>>> + * clk_restore_context restores clocks as per the clock
>>>>>>>>> tree.
>>>>>>>>> + *
>>>>>>>>> + * dfllCPU_out is first in the clock tree to get
>>>>>>>>> restored and it
>>>>>>>>> + * involves programming DFLL controller along with
>>>>>>>>> restoring CPUG
>>>>>>>>> + * clock burst policy.
>>>>>>>>> + *
>>>>>>>>> + * DFLL programming needs dfll_ref and dfll_soc
>>>>>>>>> peripheral clocks
>>>>>>>>> + * to be restores which are part ofthe peripheral
>>>>>>>>> clocks.
>>>>>> ^ white-space
>>>>>>
>>>>>> Please use spellchecker to avoid typos.
>>>>>>
>>>>>>>>> + * So, peripheral clocks restore should happen prior to
>>>>>>>>> dfll clock
>>>>>>>>> + * restore.
>>>>>>>>> + */
>>>>>>>>> +
>>>>>>>>> + tegra_clk_osc_resume(clk_base);
>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>>>>>>>> + car_writel(cpu_softrst_ctx[i], CPU_SOFTRST_CTRL, i);
>>>>>>>>> +
>>>>>>>>> + /* restore all plls and peripheral clocks */
>>>>>>>>> + tegra210_init_pllu();
>>>>>>>>> + clk_restore_context();
>>>>>>>>> +
>>>>>>>>> + fence_udelay(5, clk_base);
>>>>>>>>> +
>>>>>>>>> + /* resume SCLK and CPULP clocks */
>>>>>>>>> + tegra_sclk_cpulp_burst_policy_restore_context();
>>>>>>>>> +
>>>>>>>>> + /*
>>>>>>>>> + * restore CPUG clocks:
>>>>>>>>> + * - enable DFLL in open loop mode
>>>>>>>>> + * - switch CPUG to DFLL clock source
>>>>>>>>> + * - close DFLL loop
>>>>>>>>> + * - sync PLLX state
>>>>>>>>> + */
>>>>>>>>> + if (dfll_pdev)
>>>>>>>>> + tegra_dfll_resume(dfll_pdev, false);
>>>>>>>>> +
>>>>>>>>> + tegra_cclkg_burst_policy_restore_context();
>>>>>>>>> + fence_udelay(2, clk_base);
>>>>>>>>> +
>>>>>>>>> + if (dfll_pdev)
>>>>>>>>> + tegra_dfll_resume(dfll_pdev, true);
>>>>>>>>> +
>>>>>>>>> + parent =
>>>>>>>>> clk_hw_get_parent(__clk_get_hw(clks[TEGRA210_CLK_CCLK_G]));
>>>>>>>>> + clk = clks[TEGRA210_CLK_PLL_X];
>>>>>>>>> + if (parent != __clk_get_hw(clk))
>>>>>>>>> + tegra_clk_sync_state_pll(__clk_get_hw(clk));
>>>>>>>>> +
>>>>>>>>> + /* Disable PLL_OUT_CPU after DFLL resume */
>>>>>>>>> + tegra_clk_set_pllp_out_cpu(false);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> static void tegra210_cpu_clock_suspend(void)
>>>>>>>>> {
>>>>>>>>> /* switch coresite to clk_m, save off original source
>>>>>>>>> */ @@ -3298,6 +3400,11 @@ static void
>>>>>>>>> tegra210_cpu_clock_resume(void) }
>>>>>>>>> #endif
>>>>>>>>> +static struct syscore_ops tegra_clk_syscore_ops = {
>>>>>>>>> + .suspend = tegra210_clk_suspend,
>>>>>>>>> + .resume = tegra210_clk_resume,
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>> static struct tegra_cpu_car_ops tegra210_cpu_car_ops = {
>>>>>>>>> .wait_for_reset = tegra210_wait_cpu_in_reset,
>>>>>>>>> .disable_clock = tegra210_disable_cpu_clock,
>>>>>>>>> @@ -3583,5 +3690,7 @@ static void __init
>>>>>>>>> tegra210_clock_init(struct device_node *np)
>>>>>>>>> tegra210_mbist_clk_init();
>>>>>>>>> tegra_cpu_car_ops = &tegra210_cpu_car_ops;
>>>>>>>>> +
>>>>>>>>> + register_syscore_ops(&tegra_clk_syscore_ops);
>>>>>>>>> }
>>>>>>>> Is it really worthwhile to use syscore_ops for suspend/resume
>>>>>>>> given that drivers for
>>>>>>>> won't resume before the CLK driver anyway? Are there any other
>>>>>>>> options for CLK
>>>>>>>> suspend/resume?
>>>>>>>>
>>>>>>>> I'm also not sure whether PM runtime API could be used at all
>>>>>>>> in the context of
>>>>>>>> syscore_ops ..
>>>>>>>>
>>>>>>>> Secondly, what about to use generic clk_save_context() /
>>>>>>>> clk_restore_context()
>>>>>>>> helpers for the suspend-resume? It looks to me that some other
>>>>>>>> essential (and proper)
>>>>>>>> platform driver (soc/tegra/? PMC?) should suspend-resume the
>>>>>>>> clocks using the generic
>>>>>>>> CLK Framework API.
>>>>>>> Clock resume should happen very early to restore peripheral and
>>>>>>> cpu clocks very early than peripheral drivers resume happens.
>>>>>> If all peripheral drivers properly requested all of the
>>>>>> necessary clocks and CLK driver was a platform driver, then I
>>>>>> guess the probe should have been naturally ordered. But that's
>>>>>> not very achievable with the currently available infrastructure
>>>>>> in the kernel, so I'm not arguing that the clocks should be
>>>>>> explicitly resumed before the users.
>>>>>>> this patch series uses clk_save_context and clk_restore_context
>>>>>>> for corresponding divider, pll, pllout.. save and restore
>>>>>>> context.
>>>>>> Now I see that indeed this API is utilized in this patch, thank
>>>>>> you for the clarification.
>>>>>>
>>>>>>> But as there is dependency on dfll resume and cpu and pllx
>>>>>>> clocks restore, couldnt use clk_save_context and
>>>>>>> clk_restore_context for dfll.
>>>>>>>
>>>>>>> So implemented recommended dfll resume sequence in main
>>>>>>> Tegra210 clock driver along with invoking
>>>>>>> clk_save_context/clk_restore_context where all other clocks
>>>>>>> save/restore happens as per clock tree traversal.
>>>>>> Could you please clarify what part of peripherals clocks is
>>>>>> required for DFLL's restore? Couldn't DFLL driver be changed to
>>>>>> avoid that quirkness and thus to make DFLL driver suspend/resume
>>>>>> the clock?
>>>>> DFLL source ref_clk and soc_clk need to be restored prior to dfll.
>>>>>
>>>>> I see dfllCPU_out parent to CCLK_G first in the clock tree and
>>>>> dfll_ref and dfll_soc peripheral clocks are not resumed by the
>>>>> time dfll resume happens first.
>>>>>
>>>>> ref_clk and soc_clk source is from pll_p and clock tree has these
>>>>> registered under pll_p which happens later.
>>>>>
>>>>> tegra210_clock_init registers in order plls, peripheral clocks,
>>>>> super_clk init for cclk_g during clock driver probe and dfll
>>>>> probe and register happens later.
>>>>>
>>>> One more thing, CLDVFS peripheral clock enable is also needed to be
>>>> enabled to program DFLL Controller and all peripheral clock
>>>> context is restored only after their PLL sources are restored.
>>>>
>>>> DFLL restore involves dfll source clock resume along with CLDVFS
>>>> periheral clock enable and reset
>>>>
>>> I don't quite see why you can't simply add suspend/resume callbacks
>>> to the CPUFreq driver to:
>>>
>>> On suspend:
>>> 1. Switch CPU to PLLP (or whatever "safe" parent)
>>> 2. Disable/teardown DFLL
>>>
>>> On resume:
>>> 1. Enable/restore DFLL
>>> 2. Switch CPU back to DFLL
>> dfll runtime suspend/resume are already part of dfll_pm_ops. Don't we
>> want to use it for suspend/resume as well?
> Looks like no. Seems runtime PM of that driver is intended solely for
> the DFLL's clk management.
>
>> currently no APIs are shared b/w clk/tegra driver and CPUFreq driver
>> to invoke dfll suspend/resume in CPUFreq driver
>>
> Just add it. Also, please note that CPUFreq driver is optional and thus
> you may need to switch CPU to a safe parent on clk-core suspend as
> well in order to resume properly if CPU was running off unsafe parent
> during boot and CPUFreq driver is disabled in kernel build (or failed
> to load).
OK, Will add to CPUFreq driver...
>
> The other thing that also need attention is that T124 CPUFreq driver
> implicitly relies on DFLL driver to be probed first, which is icky.
>
Should I add check for successful dfll clk register explicitly in
CPUFreq driver probe and defer till dfll clk registers?
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-16 6:20 UTC (permalink / raw)
To: Sowjanya Komatineni
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pdeschrijver, pgaikwad,
sboyd, linux-clk, linux-gpio, jckuo, josephl, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <20190716083701.225f0fd9@dimatab>
В Tue, 16 Jul 2019 08:37:01 +0300
Dmitry Osipenko <digetx@gmail.com> пишет:
> В Mon, 15 Jul 2019 21:37:09 -0700
> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>
> > On 7/15/19 8:50 PM, Dmitry Osipenko wrote:
> > > 16.07.2019 6:00, Sowjanya Komatineni пишет:
> > >> On 7/15/19 5:35 PM, Sowjanya Komatineni wrote:
> > >>> On 7/14/19 2:41 PM, Dmitry Osipenko wrote:
> > >>>> 13.07.2019 8:54, Sowjanya Komatineni пишет:
> > >>>>> On 6/29/19 8:10 AM, Dmitry Osipenko wrote:
> > >>>>>> 28.06.2019 5:12, Sowjanya Komatineni пишет:
> > >>>>>>> This patch adds system suspend and resume support for
> > >>>>>>> Tegra210 clocks.
> > >>>>>>>
> > >>>>>>> All the CAR controller settings are lost on suspend when
> > >>>>>>> core power goes off.
> > >>>>>>>
> > >>>>>>> This patch has implementation for saving and restoring all
> > >>>>>>> the PLLs and clocks context during system suspend and resume
> > >>>>>>> to have the clocks back to same state for normal operation.
> > >>>>>>>
> > >>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
> > >>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> > >>>>>>> ---
> > >>>>>>> drivers/clk/tegra/clk-tegra210.c | 115
> > >>>>>>> ++++++++++++++++++++++++++++++++++++++-
> > >>>>>>> drivers/clk/tegra/clk.c | 14 +++++
> > >>>>>>> drivers/clk/tegra/clk.h | 1 +
> > >>>>>>> 3 files changed, 127 insertions(+), 3 deletions(-)
> > >>>>>>>
> > >>>>>>> diff --git a/drivers/clk/tegra/clk-tegra210.c
> > >>>>>>> b/drivers/clk/tegra/clk-tegra210.c
> > >>>>>>> index 1c08c53482a5..1b839544e086 100644
> > >>>>>>> --- a/drivers/clk/tegra/clk-tegra210.c
> > >>>>>>> +++ b/drivers/clk/tegra/clk-tegra210.c
> > >>>>>>> @@ -9,10 +9,12 @@
> > >>>>>>> #include <linux/clkdev.h>
> > >>>>>>> #include <linux/of.h>
> > >>>>>>> #include <linux/of_address.h>
> > >>>>>>> +#include <linux/of_platform.h>
> > >>>>>>> #include <linux/delay.h>
> > >>>>>>> #include <linux/export.h>
> > >>>>>>> #include <linux/mutex.h>
> > >>>>>>> #include <linux/clk/tegra.h>
> > >>>>>>> +#include <linux/syscore_ops.h>
> > >>>>>>> #include <dt-bindings/clock/tegra210-car.h>
> > >>>>>>> #include <dt-bindings/reset/tegra210-car.h>
> > >>>>>>> #include <linux/iopoll.h>
> > >>>>>>> @@ -20,6 +22,7 @@
> > >>>>>>> #include <soc/tegra/pmc.h>
> > >>>>>>> #include "clk.h"
> > >>>>>>> +#include "clk-dfll.h"
> > >>>>>>> #include "clk-id.h"
> > >>>>>>> /*
> > >>>>>>> @@ -225,6 +228,7 @@
> > >>>>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_SET 0x2a8
> > >>>>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_CLR 0x2ac
> > >>>>>>> +#define CPU_SOFTRST_CTRL 0x380
> > >>>>>>> #define LVL2_CLK_GATE_OVRA 0xf8
> > >>>>>>> #define LVL2_CLK_GATE_OVRC 0x3a0
> > >>>>>>> @@ -2820,6 +2824,7 @@ static int tegra210_enable_pllu(void)
> > >>>>>>> struct tegra_clk_pll_freq_table *fentry;
> > >>>>>>> struct tegra_clk_pll pllu;
> > >>>>>>> u32 reg;
> > >>>>>>> + int ret;
> > >>>>>>> for (fentry = pll_u_freq_table;
> > >>>>>>> fentry->input_rate; fentry++) {
> > >>>>>>> if (fentry->input_rate == pll_ref_freq)
> > >>>>>>> @@ -2847,10 +2852,10 @@ static int
> > >>>>>>> tegra210_enable_pllu(void) fence_udelay(1, clk_base);
> > >>>>>>> reg |= PLL_ENABLE;
> > >>>>>>> writel(reg, clk_base + PLLU_BASE);
> > >>>>>>> + fence_udelay(1, clk_base);
> > >>>>>>> - readl_relaxed_poll_timeout_atomic(clk_base +
> > >>>>>>> PLLU_BASE, reg,
> > >>>>>>> - reg & PLL_BASE_LOCK, 2, 1000);
> > >>>>>>> - if (!(reg & PLL_BASE_LOCK)) {
> > >>>>>>> + ret = tegra210_wait_for_mask(&pllu, PLLU_BASE,
> > >>>>>>> PLL_BASE_LOCK);
> > >>>>>>> + if (ret) {
> > >>>>>>> pr_err("Timed out waiting for PLL_U to lock\n");
> > >>>>>>> return -ETIMEDOUT;
> > >>>>>>> }
> > >>>>>>> @@ -3283,6 +3288,103 @@ static void
> > >>>>>>> tegra210_disable_cpu_clock(u32 cpu)
> > >>>>>>> }
> > >>>>>>> #ifdef CONFIG_PM_SLEEP
> > >>>>>>> +static u32 cpu_softrst_ctx[3];
> > >>>>>>> +static struct platform_device *dfll_pdev;
> > >>>>>>> +#define car_readl(_base, _off) readl_relaxed(clk_base +
> > >>>>>>> (_base) + ((_off) * 4))
> > >>>>>>> +#define car_writel(_val, _base, _off) \
> > >>>>>>> + writel_relaxed(_val, clk_base + (_base) + ((_off) *
> > >>>>>>> 4)) +
> > >>>>>>> +static int tegra210_clk_suspend(void)
> > >>>>>>> +{
> > >>>>>>> + unsigned int i;
> > >>>>>>> + struct device_node *node;
> > >>>>>>> +
> > >>>>>>> + tegra_cclkg_burst_policy_save_context();
> > >>>>>>> +
> > >>>>>>> + if (!dfll_pdev) {
> > >>>>>>> + node = of_find_compatible_node(NULL, NULL,
> > >>>>>>> + "nvidia,tegra210-dfll");
> > >>>>>>> + if (node)
> > >>>>>>> + dfll_pdev = of_find_device_by_node(node);
> > >>>>>>> +
> > >>>>>>> + of_node_put(node);
> > >>>>>>> + if (!dfll_pdev)
> > >>>>>>> + pr_err("dfll node not found. no suspend for
> > >>>>>>> dfll\n");
> > >>>>>>> + }
> > >>>>>>> +
> > >>>>>>> + if (dfll_pdev)
> > >>>>>>> + tegra_dfll_suspend(dfll_pdev);
> > >>>>>>> +
> > >>>>>>> + /* Enable PLLP_OUT_CPU after dfll suspend */
> > >>>>>>> + tegra_clk_set_pllp_out_cpu(true);
> > >>>>>>> +
> > >>>>>>> + tegra_sclk_cclklp_burst_policy_save_context();
> > >>>>>>> +
> > >>>>>>> + clk_save_context();
> > >>>>>>> +
> > >>>>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
> > >>>>>>> + cpu_softrst_ctx[i] = car_readl(CPU_SOFTRST_CTRL,
> > >>>>>>> i); +
> > >>>>>>> + return 0;
> > >>>>>>> +}
> > >>>>>>> +
> > >>>>>>> +static void tegra210_clk_resume(void)
> > >>>>>>> +{
> > >>>>>>> + unsigned int i;
> > >>>>>>> + struct clk_hw *parent;
> > >>>>>>> + struct clk *clk;
> > >>>>>>> +
> > >>>>>>> + /*
> > >>>>>>> + * clk_restore_context restores clocks as per the clock
> > >>>>>>> tree.
> > >>>>>>> + *
> > >>>>>>> + * dfllCPU_out is first in the clock tree to get
> > >>>>>>> restored and it
> > >>>>>>> + * involves programming DFLL controller along with
> > >>>>>>> restoring CPUG
> > >>>>>>> + * clock burst policy.
> > >>>>>>> + *
> > >>>>>>> + * DFLL programming needs dfll_ref and dfll_soc
> > >>>>>>> peripheral clocks
> > >>>>>>> + * to be restores which are part ofthe peripheral
> > >>>>>>> clocks.
> > >>>> ^ white-space
> > >>>>
> > >>>> Please use spellchecker to avoid typos.
> > >>>>
> > >>>>>>> + * So, peripheral clocks restore should happen prior to
> > >>>>>>> dfll clock
> > >>>>>>> + * restore.
> > >>>>>>> + */
> > >>>>>>> +
> > >>>>>>> + tegra_clk_osc_resume(clk_base);
> > >>>>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
> > >>>>>>> + car_writel(cpu_softrst_ctx[i], CPU_SOFTRST_CTRL,
> > >>>>>>> i); +
> > >>>>>>> + /* restore all plls and peripheral clocks */
> > >>>>>>> + tegra210_init_pllu();
> > >>>>>>> + clk_restore_context();
> > >>>>>>> +
> > >>>>>>> + fence_udelay(5, clk_base);
> > >>>>>>> +
> > >>>>>>> + /* resume SCLK and CPULP clocks */
> > >>>>>>> + tegra_sclk_cpulp_burst_policy_restore_context();
> > >>>>>>> +
> > >>>>>>> + /*
> > >>>>>>> + * restore CPUG clocks:
> > >>>>>>> + * - enable DFLL in open loop mode
> > >>>>>>> + * - switch CPUG to DFLL clock source
> > >>>>>>> + * - close DFLL loop
> > >>>>>>> + * - sync PLLX state
> > >>>>>>> + */
> > >>>>>>> + if (dfll_pdev)
> > >>>>>>> + tegra_dfll_resume(dfll_pdev, false);
> > >>>>>>> +
> > >>>>>>> + tegra_cclkg_burst_policy_restore_context();
> > >>>>>>> + fence_udelay(2, clk_base);
> > >>>>>>> +
> > >>>>>>> + if (dfll_pdev)
> > >>>>>>> + tegra_dfll_resume(dfll_pdev, true);
> > >>>>>>> +
> > >>>>>>> + parent =
> > >>>>>>> clk_hw_get_parent(__clk_get_hw(clks[TEGRA210_CLK_CCLK_G]));
> > >>>>>>> + clk = clks[TEGRA210_CLK_PLL_X];
> > >>>>>>> + if (parent != __clk_get_hw(clk))
> > >>>>>>> + tegra_clk_sync_state_pll(__clk_get_hw(clk));
> > >>>>>>> +
> > >>>>>>> + /* Disable PLL_OUT_CPU after DFLL resume */
> > >>>>>>> + tegra_clk_set_pllp_out_cpu(false);
> > >>>>>>> +}
> > >>>>>>> +
> > >>>>>>> static void tegra210_cpu_clock_suspend(void)
> > >>>>>>> {
> > >>>>>>> /* switch coresite to clk_m, save off original
> > >>>>>>> source */ @@ -3298,6 +3400,11 @@ static void
> > >>>>>>> tegra210_cpu_clock_resume(void) }
> > >>>>>>> #endif
> > >>>>>>> +static struct syscore_ops tegra_clk_syscore_ops = {
> > >>>>>>> + .suspend = tegra210_clk_suspend,
> > >>>>>>> + .resume = tegra210_clk_resume,
> > >>>>>>> +};
> > >>>>>>> +
> > >>>>>>> static struct tegra_cpu_car_ops tegra210_cpu_car_ops = {
> > >>>>>>> .wait_for_reset = tegra210_wait_cpu_in_reset,
> > >>>>>>> .disable_clock = tegra210_disable_cpu_clock,
> > >>>>>>> @@ -3583,5 +3690,7 @@ static void __init
> > >>>>>>> tegra210_clock_init(struct device_node *np)
> > >>>>>>> tegra210_mbist_clk_init();
> > >>>>>>> tegra_cpu_car_ops = &tegra210_cpu_car_ops;
> > >>>>>>> +
> > >>>>>>> + register_syscore_ops(&tegra_clk_syscore_ops);
> > >>>>>>> }
> > >>>>>> Is it really worthwhile to use syscore_ops for suspend/resume
> > >>>>>> given that drivers for
> > >>>>>> won't resume before the CLK driver anyway? Are there any
> > >>>>>> other options for CLK
> > >>>>>> suspend/resume?
> > >>>>>>
> > >>>>>> I'm also not sure whether PM runtime API could be used at all
> > >>>>>> in the context of
> > >>>>>> syscore_ops ..
> > >>>>>>
> > >>>>>> Secondly, what about to use generic clk_save_context() /
> > >>>>>> clk_restore_context()
> > >>>>>> helpers for the suspend-resume? It looks to me that some
> > >>>>>> other essential (and proper)
> > >>>>>> platform driver (soc/tegra/? PMC?) should suspend-resume the
> > >>>>>> clocks using the generic
> > >>>>>> CLK Framework API.
> > >>>>> Clock resume should happen very early to restore peripheral
> > >>>>> and cpu clocks very early than peripheral drivers resume
> > >>>>> happens.
> > >>>> If all peripheral drivers properly requested all of the
> > >>>> necessary clocks and CLK driver was a platform driver, then I
> > >>>> guess the probe should have been naturally ordered. But that's
> > >>>> not very achievable with the currently available infrastructure
> > >>>> in the kernel, so I'm not arguing that the clocks should be
> > >>>> explicitly resumed before the users.
> > >>>>> this patch series uses clk_save_context and
> > >>>>> clk_restore_context for corresponding divider, pll, pllout..
> > >>>>> save and restore context.
> > >>>> Now I see that indeed this API is utilized in this patch, thank
> > >>>> you for the clarification.
> > >>>>
> > >>>>> But as there is dependency on dfll resume and cpu and pllx
> > >>>>> clocks restore, couldnt use clk_save_context and
> > >>>>> clk_restore_context for dfll.
> > >>>>>
> > >>>>> So implemented recommended dfll resume sequence in main
> > >>>>> Tegra210 clock driver along with invoking
> > >>>>> clk_save_context/clk_restore_context where all other clocks
> > >>>>> save/restore happens as per clock tree traversal.
> > >>>> Could you please clarify what part of peripherals clocks is
> > >>>> required for DFLL's restore? Couldn't DFLL driver be changed to
> > >>>> avoid that quirkness and thus to make DFLL driver
> > >>>> suspend/resume the clock?
> > >>> DFLL source ref_clk and soc_clk need to be restored prior to
> > >>> dfll.
> > >>>
> > >>> I see dfllCPU_out parent to CCLK_G first in the clock tree and
> > >>> dfll_ref and dfll_soc peripheral clocks are not resumed by the
> > >>> time dfll resume happens first.
> > >>>
> > >>> ref_clk and soc_clk source is from pll_p and clock tree has
> > >>> these registered under pll_p which happens later.
> > >>>
> > >>> tegra210_clock_init registers in order plls, peripheral clocks,
> > >>> super_clk init for cclk_g during clock driver probe and dfll
> > >>> probe and register happens later.
> > >>>
> > >> One more thing, CLDVFS peripheral clock enable is also needed to
> > >> be enabled to program DFLL Controller and all peripheral clock
> > >> context is restored only after their PLL sources are restored.
> > >>
> > >> DFLL restore involves dfll source clock resume along with CLDVFS
> > >> periheral clock enable and reset
> > >>
> > > I don't quite see why you can't simply add suspend/resume
> > > callbacks to the CPUFreq driver to:
> > >
> > > On suspend:
> > > 1. Switch CPU to PLLP (or whatever "safe" parent)
> > > 2. Disable/teardown DFLL
> > >
> > > On resume:
> > > 1. Enable/restore DFLL
> > > 2. Switch CPU back to DFLL
> >
> > dfll runtime suspend/resume are already part of dfll_pm_ops. Don't
> > we want to use it for suspend/resume as well?
>
> Looks like no. Seems runtime PM of that driver is intended solely for
> the DFLL's clk management.
>
> > currently no APIs are shared b/w clk/tegra driver and CPUFreq driver
> > to invoke dfll suspend/resume in CPUFreq driver
> >
>
> Just add it. Also, please note that CPUFreq driver is optional and
> thus you may need to switch CPU to a safe parent on clk-core suspend
> as well in order to resume properly if CPU was running off unsafe
> parent during boot and CPUFreq driver is disabled in kernel build (or
> failed to load).
Although, if PLLs are restored before CCLK, then it should be fine
as-is.
> The other thing that also need attention is that T124 CPUFreq driver
> implicitly relies on DFLL driver to be probed first, which is icky.
>
^ permalink raw reply
* Re: [PATCH 1/3] iio: imu: st_lsm6sdx: move some register definitions to sensor_settings struct
From: Martin Kepplinger @ 2019-07-16 6:11 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: lorenzo.bianconi83, jic23, knaack.h, lars, pmeerw, linux-iio,
devicetree
In-Reply-To: <20190715211033.GA23126@localhost.localdomain>
On 15.07.19 23:10, Lorenzo Bianconi wrote:
>> Move some register definitions to the per-device array of struct
>> st_lsm6dsx_sensor_settings in order to simplify adding new sensor
>> devices to the driver.
>>
>> Also, remove completely unused register definitions.
>>
>
> Hi Martin,
>
> just few comments inline
>
> Regards,
> Lorenzo
>
>> Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
>> ---
>> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 6 ++++
>> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 31 ++++++++++++++------
>> 2 files changed, 28 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> index c14bf533b66b..f072ac14f213 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> @@ -196,6 +196,9 @@ struct st_lsm6dsx_ext_dev_settings {
>> /**
>> * struct st_lsm6dsx_settings - ST IMU sensor settings
>> * @wai: Sensor WhoAmI default value.
>> + * @reg_int1_addr: Control Register address for INT1
>> + * @reg_int2_addr: Control Register address for INT2
>> + * @reg_reset_addr: register address for reset/reboot
>> * @max_fifo_size: Sensor max fifo length in FIFO words.
>> * @id: List of hw id/device name supported by the driver configuration.
>> * @decimator: List of decimator register info (addr + mask).
>> @@ -206,6 +209,9 @@ struct st_lsm6dsx_ext_dev_settings {
>> */
>> struct st_lsm6dsx_settings {
>> u8 wai;
>> + u8 reg_int1_addr;
>> + u8 reg_int2_addr;
>> + u8 reg_reset_addr;
>
> could you please rename them in int1_addr, int2_addr and reset_addr in order to
> be inline with other st_lsm6dsx_settings fields?
>
sure. thanks for reviewing!
martin
^ permalink raw reply
* Re: [PATCH 2/3] iio: imu: st_lsm6dsx: add support for accel/gyro unit of lsm9sd1
From: Martin Kepplinger @ 2019-07-16 6:10 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: lorenzo.bianconi83, jic23, knaack.h, lars, pmeerw, linux-iio,
devicetree, linux-kernel
In-Reply-To: <20190715214920.GB23126@localhost.localdomain>
On 15.07.19 23:49, Lorenzo Bianconi wrote:
>> The LSM9DS1's accelerometer / gyroscope unit and it's magnetometer (separately
>> supported in iio/magnetometer/st_magn*) are located on a separate i2c addresses
>> on the bus.
>>
>> For the datasheet, see https://www.st.com/resource/en/datasheet/lsm9ds1.pdf
>>
>> Treat it just like the LSM6* devices and, despite it's name, hook it up
>> to the st_lsm6dsx driver, using it's basic functionality.
>>
>> Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
>> ---
>>
>> What do you think about an addition like this? How confusing is it to support
>> an LSM9 module by the lsm6 driver, despite it's name? It requires almost no
>> code, so why not think about it, right?
>
> I am fine with (it was on my ToDo list, so thanks for working on this).
> I have just posted the following series that will help you adding support for
> LSM9DS1
> https://patchwork.kernel.org/cover/11045061/
> I think you just need to take care of gyro channels allocating iio devices (you
> probably need to pass hw_id to st_lsm6dsx_alloc_iiodev())
yes. I'll look over allocation once more.
>
>>
>> Oh, I'm not 100% convinced by my new "if" in probe(), but even that is
>> not too confusing I guess.
>>
>> thanks,
>>
>> martin
>>
>> p.s.: todos:
>> * hook up the fifo / buffer / trigger functionality,
>> * (off-topic a bit) move the (currently strange) gyro-only support
>> for lsm9ds0 to this driver as well.
>
> Regarding FIFO I guess it is enough to set decimator factor to 1 for both accel
> and gyro.
>
> Regards,
> Lorenzo
>
>>
>>
>>
>> drivers/iio/imu/st_lsm6dsx/Kconfig | 3 +-
>> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 4 +
>> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 105 ++++++++++++++++++-
>> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c | 5 +
>> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c | 5 +
>> 5 files changed, 117 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
>> index 002a423eae52..0b5a568e4c16 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/Kconfig
>> +++ b/drivers/iio/imu/st_lsm6dsx/Kconfig
>> @@ -10,7 +10,8 @@ config IIO_ST_LSM6DSX
>> help
>> Say yes here to build support for STMicroelectronics LSM6DSx imu
>> sensor. Supported devices: lsm6ds3, lsm6ds3h, lsm6dsl, lsm6dsm,
>> - ism330dlc, lsm6dso, lsm6dsox, asm330lhh, lsm6dsr
>> + ism330dlc, lsm6dso, lsm6dsox, asm330lhh, lsm6dsr and the
>> + accelerometer/gyroscope of lsm9ds1.
>>
>> To compile this driver as a module, choose M here: the module
>> will be called st_lsm6dsx.
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> index f072ac14f213..8af9641260fa 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> @@ -22,6 +22,7 @@
>> #define ST_ASM330LHH_DEV_NAME "asm330lhh"
>> #define ST_LSM6DSOX_DEV_NAME "lsm6dsox"
>> #define ST_LSM6DSR_DEV_NAME "lsm6dsr"
>> +#define ST_LSM9DS1_DEV_NAME "lsm9ds1"
>>
>> enum st_lsm6dsx_hw_id {
>> ST_LSM6DS3_ID,
>> @@ -33,6 +34,7 @@ enum st_lsm6dsx_hw_id {
>> ST_ASM330LHH_ID,
>> ST_LSM6DSOX_ID,
>> ST_LSM6DSR_ID,
>> + ST_LSM9DS1_ID,
>> ST_LSM6DSX_MAX_ID,
>> };
>>
>> @@ -230,6 +232,8 @@ enum st_lsm6dsx_sensor_id {
>> ST_LSM6DSX_ID_EXT0,
>> ST_LSM6DSX_ID_EXT1,
>> ST_LSM6DSX_ID_EXT2,
>> + ST_LSM9DSX_ID_GYRO,
>> + ST_LSM9DSX_ID_ACC,
>> ST_LSM6DSX_ID_MAX,
>> };
>>
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> index 7a4fe70a8f20..6acfe63073de 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> @@ -10,6 +10,8 @@
>> * +-125/+-245/+-500/+-1000/+-2000 dps
>> * LSM6DSx series has an integrated First-In-First-Out (FIFO) buffer
>> * allowing dynamic batching of sensor data.
>> + * LSM9DSx series is similar but includes an additional magnetometer, handled
>> + * by a different driver.
>> *
>> * Supported sensors:
>> * - LSM6DS3:
>> @@ -30,6 +32,13 @@
>> * - Gyroscope supported full-scale [dps]: +-125/+-245/+-500/+-1000/+-2000
>> * - FIFO size: 3KB
>> *
>> + * - LSM9DS1:
>> + * - Accelerometer supported ODR [Hz]: 10, 50, 119, 238, 476, 952
>> + * - Accelerometer supported full-scale [g]: +-2/+-4/+-8/+-16
>> + * - Gyroscope supported ODR [Hz]: 15, 60, 119, 238, 476, 952
>> + * - Gyroscope supported full-scale [dps]: +-245/+-500/+-2000
>> + * - FIFO size: 32
>> + *
>> * Copyright 2016 STMicroelectronics Inc.
>> *
>> * Lorenzo Bianconi <lorenzo.bianconi@st.com>
>> @@ -64,6 +73,10 @@
>> #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR 0x24
>> #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR 0x26
>>
>> +#define ST_LSM9DSX_REG_GYRO_OUT_X_L_ADDR 0x18
>> +#define ST_LSM9DSX_REG_GYRO_OUT_Y_L_ADDR 0x1a
>> +#define ST_LSM9DSX_REG_GYRO_OUT_Z_L_ADDR 0x1c
>> +
>> static const struct st_lsm6dsx_odr_table_entry st_lsm6dsx_odr_table[] = {
>> [ST_LSM6DSX_ID_ACC] = {
>> .reg = {
>> @@ -88,6 +101,30 @@ static const struct st_lsm6dsx_odr_table_entry st_lsm6dsx_odr_table[] = {
>> .odr_avl[3] = { 104, 0x04 },
>> .odr_avl[4] = { 208, 0x05 },
>> .odr_avl[5] = { 416, 0x06 },
>> + },
>> + [ST_LSM9DSX_ID_ACC] = {
>> + .reg = {
>> + .addr = 0x20,
>> + .mask = GENMASK(7, 5),
>> + },
>> + .odr_avl[0] = { 10, 0x01 },
>> + .odr_avl[1] = { 50, 0x02 },
>> + .odr_avl[2] = { 119, 0x03 },
>> + .odr_avl[3] = { 238, 0x04 },
>> + .odr_avl[4] = { 476, 0x05 },
>> + .odr_avl[5] = { 952, 0x06 },
>> + },
>> + [ST_LSM9DSX_ID_GYRO] = {
>> + .reg = {
>> + .addr = 0x10,
>> + .mask = GENMASK(7, 5),
>> + },
>> + .odr_avl[0] = { 15, 0x01 },
>> + .odr_avl[1] = { 60, 0x02 },
>> + .odr_avl[2] = { 119, 0x03 },
>> + .odr_avl[3] = { 238, 0x04 },
>> + .odr_avl[4] = { 476, 0x05 },
>> + .odr_avl[5] = { 952, 0x06 },
>> }
>> };
>>
>> @@ -111,10 +148,43 @@ static const struct st_lsm6dsx_fs_table_entry st_lsm6dsx_fs_table[] = {
>> .fs_avl[1] = { IIO_DEGREE_TO_RAD(17500), 0x1 },
>> .fs_avl[2] = { IIO_DEGREE_TO_RAD(35000), 0x2 },
>> .fs_avl[3] = { IIO_DEGREE_TO_RAD(70000), 0x3 },
>> + },
>> + [ST_LSM9DSX_ID_ACC] = {
>> + .reg = {
>> + .addr = 0x20,
>> + .mask = GENMASK(4, 3),
>> + },
>> + .fs_avl[0] = { 599, 0x0 },
>> + .fs_avl[1] = { 1197, 0x2 },
>> + .fs_avl[2] = { 2394, 0x3 },
>> + .fs_avl[3] = { 4788, 0x1 },
>> + },
>> + [ST_LSM9DSX_ID_GYRO] = {
>> + .reg = {
>> + .addr = 0x10,
>> + .mask = GENMASK(4, 3),
>> + },
>> + .fs_avl[0] = { IIO_DEGREE_TO_RAD(245), 0x0 },
>> + .fs_avl[1] = { IIO_DEGREE_TO_RAD(500), 0x1 },
>> + .fs_avl[2] = { IIO_DEGREE_TO_RAD(0), 0x2 },
>> + .fs_avl[3] = { IIO_DEGREE_TO_RAD(2000), 0x3 },
>> }
>> };
>>
>> static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>> + {
>> + .wai = 0x68,
>> + .reg_int1_addr = 0x0c,
>> + .reg_int2_addr = 0x0d,
>> + .reg_reset_addr = 0x22,
>> + .max_fifo_size = 32,
>> + .id = {
>> + {
>> + .hw_id = ST_LSM9DS1_ID,
>> + .name = ST_LSM9DS1_DEV_NAME,
>> + },
>> + },
>> + },
>> {
>> .wai = 0x69,
>> .reg_int1_addr = 0x0d,
>> @@ -492,6 +562,16 @@ static const struct iio_chan_spec st_lsm6dsx_gyro_channels[] = {
>> IIO_CHAN_SOFT_TIMESTAMP(3),
>> };
>>
>> +static const struct iio_chan_spec st_lsm9dsx_gyro_channels[] = {
>> + ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, ST_LSM9DSX_REG_GYRO_OUT_X_L_ADDR,
>> + IIO_MOD_X, 0),
>> + ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, ST_LSM9DSX_REG_GYRO_OUT_Y_L_ADDR,
>> + IIO_MOD_Y, 1),
>> + ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, ST_LSM9DSX_REG_GYRO_OUT_Z_L_ADDR,
>> + IIO_MOD_Z, 2),
>> + IIO_CHAN_SOFT_TIMESTAMP(3),
>> +};
>> +
>> int st_lsm6dsx_set_page(struct st_lsm6dsx_hw *hw, bool enable)
>> {
>> const struct st_lsm6dsx_shub_settings *hub_settings;
>> @@ -1056,6 +1136,7 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
>>
>> switch (id) {
>> case ST_LSM6DSX_ID_ACC:
>> + case ST_LSM9DSX_ID_ACC:
>> iio_dev->channels = st_lsm6dsx_acc_channels;
>> iio_dev->num_channels = ARRAY_SIZE(st_lsm6dsx_acc_channels);
>> iio_dev->info = &st_lsm6dsx_acc_info;
>> @@ -1068,6 +1149,14 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
>> iio_dev->num_channels = ARRAY_SIZE(st_lsm6dsx_gyro_channels);
>> iio_dev->info = &st_lsm6dsx_gyro_info;
>>
>> + scnprintf(sensor->name, sizeof(sensor->name), "%s_gyro",
>> + name);
>> + break;
>> + case ST_LSM9DSX_ID_GYRO:
>> + iio_dev->channels = st_lsm9dsx_gyro_channels;
>> + iio_dev->num_channels = ARRAY_SIZE(st_lsm9dsx_gyro_channels);
>> + iio_dev->info = &st_lsm6dsx_gyro_info;
>> +
>> scnprintf(sensor->name, sizeof(sensor->name), "%s_gyro",
>> name);
>> break;
>> @@ -1109,10 +1198,18 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
>> if (err < 0)
>> return err;
>>
>> - for (i = 0; i < ST_LSM6DSX_ID_EXT0; i++) {
>> - hw->iio_devs[i] = st_lsm6dsx_alloc_iiodev(hw, i, name);
>> - if (!hw->iio_devs[i])
>> - return -ENOMEM;
>> + if (hw_id == ST_LSM9DS1_ID) {
>> + for (i = ST_LSM9DSX_ID_GYRO; i <= ST_LSM9DSX_ID_ACC; i++) {
>> + hw->iio_devs[i] = st_lsm6dsx_alloc_iiodev(hw, i, name);
>> + if (!hw->iio_devs[i])
>> + return -ENOMEM;
>> + }
>> + } else {
>> + for (i = 0; i < ST_LSM6DSX_ID_EXT0; i++) {
>> + hw->iio_devs[i] = st_lsm6dsx_alloc_iiodev(hw, i, name);
>> + if (!hw->iio_devs[i])
>> + return -ENOMEM;
>> + }
>> }
>>
>> err = st_lsm6dsx_init_device(hw);
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
>> index b3211e0ac07b..a684e7db1299 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
>> @@ -75,6 +75,10 @@ static const struct of_device_id st_lsm6dsx_i2c_of_match[] = {
>> .compatible = "st,lsm6dsr",
>> .data = (void *)ST_LSM6DSR_ID,
>> },
>> + {
>> + .compatible = "st,lsm9ds1",
>> + .data = (void *)ST_LSM9DS1_ID,
>> + },
>> {},
>> };
>> MODULE_DEVICE_TABLE(of, st_lsm6dsx_i2c_of_match);
>> @@ -89,6 +93,7 @@ static const struct i2c_device_id st_lsm6dsx_i2c_id_table[] = {
>> { ST_ASM330LHH_DEV_NAME, ST_ASM330LHH_ID },
>> { ST_LSM6DSOX_DEV_NAME, ST_LSM6DSOX_ID },
>> { ST_LSM6DSR_DEV_NAME, ST_LSM6DSR_ID },
>> + { ST_LSM9DS1_DEV_NAME, ST_LSM9DS1_ID },
>> {},
>> };
>> MODULE_DEVICE_TABLE(i2c, st_lsm6dsx_i2c_id_table);
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
>> index c9d3c4711018..709769177e91 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
>> @@ -75,6 +75,10 @@ static const struct of_device_id st_lsm6dsx_spi_of_match[] = {
>> .compatible = "st,lsm6dsr",
>> .data = (void *)ST_LSM6DSR_ID,
>> },
>> + {
>> + .compatible = "st,lsm9ds1",
>> + .data = (void *)ST_LSM9DS1_ID,
>> + },
>> {},
>> };
>> MODULE_DEVICE_TABLE(of, st_lsm6dsx_spi_of_match);
>> @@ -89,6 +93,7 @@ static const struct spi_device_id st_lsm6dsx_spi_id_table[] = {
>> { ST_ASM330LHH_DEV_NAME, ST_ASM330LHH_ID },
>> { ST_LSM6DSOX_DEV_NAME, ST_LSM6DSOX_ID },
>> { ST_LSM6DSR_DEV_NAME, ST_LSM6DSR_ID },
>> + { ST_LSM9DS1_DEV_NAME, ST_LSM9DS1_ID },
>> {},
>> };
>> MODULE_DEVICE_TABLE(spi, st_lsm6dsx_spi_id_table);
>> --
>> 2.20.1
>>
^ permalink raw reply
* [PATCH] of: resolver: Add of_node_put() before return and break
From: Nishka Dasgupta @ 2019-07-16 5:43 UTC (permalink / raw)
To: pantelis.antoniou, frowand.list, robh+dt, devicetree; +Cc: Nishka Dasgupta
Each iteration of for_each_child_of_node puts the previous node, but in
the case of a return or break from the middle of the loop, there is no
put, thus causing a memory leak. Hence add an of_node_put before the
return or break in three places.
Issue found with Coccinelle.
Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
---
drivers/of/resolver.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index c1b67dd7cd6e..83c766233181 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -206,16 +206,22 @@ static int adjust_local_phandle_references(struct device_node *local_fixups,
for_each_child_of_node(local_fixups, child) {
for_each_child_of_node(overlay, overlay_child)
- if (!node_name_cmp(child, overlay_child))
+ if (!node_name_cmp(child, overlay_child)) {
+ of_node_put(overlay_child);
break;
+ }
- if (!overlay_child)
+ if (!overlay_child) {
+ of_node_put(child);
return -EINVAL;
+ }
err = adjust_local_phandle_references(child, overlay_child,
phandle_delta);
- if (err)
+ if (err) {
+ of_node_put(child);
return err;
+ }
}
return 0;
--
2.19.1
^ permalink raw reply related
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-16 5:37 UTC (permalink / raw)
To: Sowjanya Komatineni
Cc: thierry.reding, jonathanh, tglx, jason, marc.zyngier,
linus.walleij, stefan, mark.rutland, pdeschrijver, pgaikwad,
sboyd, linux-clk, linux-gpio, jckuo, josephl, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <86fc07d5-ab2e-a52a-a570-b1dfff4c20fe@nvidia.com>
В Mon, 15 Jul 2019 21:37:09 -0700
Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
> On 7/15/19 8:50 PM, Dmitry Osipenko wrote:
> > 16.07.2019 6:00, Sowjanya Komatineni пишет:
> >> On 7/15/19 5:35 PM, Sowjanya Komatineni wrote:
> >>> On 7/14/19 2:41 PM, Dmitry Osipenko wrote:
> >>>> 13.07.2019 8:54, Sowjanya Komatineni пишет:
> >>>>> On 6/29/19 8:10 AM, Dmitry Osipenko wrote:
> >>>>>> 28.06.2019 5:12, Sowjanya Komatineni пишет:
> >>>>>>> This patch adds system suspend and resume support for Tegra210
> >>>>>>> clocks.
> >>>>>>>
> >>>>>>> All the CAR controller settings are lost on suspend when core
> >>>>>>> power goes off.
> >>>>>>>
> >>>>>>> This patch has implementation for saving and restoring all
> >>>>>>> the PLLs and clocks context during system suspend and resume
> >>>>>>> to have the clocks back to same state for normal operation.
> >>>>>>>
> >>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
> >>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> >>>>>>> ---
> >>>>>>> drivers/clk/tegra/clk-tegra210.c | 115
> >>>>>>> ++++++++++++++++++++++++++++++++++++++-
> >>>>>>> drivers/clk/tegra/clk.c | 14 +++++
> >>>>>>> drivers/clk/tegra/clk.h | 1 +
> >>>>>>> 3 files changed, 127 insertions(+), 3 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/clk/tegra/clk-tegra210.c
> >>>>>>> b/drivers/clk/tegra/clk-tegra210.c
> >>>>>>> index 1c08c53482a5..1b839544e086 100644
> >>>>>>> --- a/drivers/clk/tegra/clk-tegra210.c
> >>>>>>> +++ b/drivers/clk/tegra/clk-tegra210.c
> >>>>>>> @@ -9,10 +9,12 @@
> >>>>>>> #include <linux/clkdev.h>
> >>>>>>> #include <linux/of.h>
> >>>>>>> #include <linux/of_address.h>
> >>>>>>> +#include <linux/of_platform.h>
> >>>>>>> #include <linux/delay.h>
> >>>>>>> #include <linux/export.h>
> >>>>>>> #include <linux/mutex.h>
> >>>>>>> #include <linux/clk/tegra.h>
> >>>>>>> +#include <linux/syscore_ops.h>
> >>>>>>> #include <dt-bindings/clock/tegra210-car.h>
> >>>>>>> #include <dt-bindings/reset/tegra210-car.h>
> >>>>>>> #include <linux/iopoll.h>
> >>>>>>> @@ -20,6 +22,7 @@
> >>>>>>> #include <soc/tegra/pmc.h>
> >>>>>>> #include "clk.h"
> >>>>>>> +#include "clk-dfll.h"
> >>>>>>> #include "clk-id.h"
> >>>>>>> /*
> >>>>>>> @@ -225,6 +228,7 @@
> >>>>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_SET 0x2a8
> >>>>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_CLR 0x2ac
> >>>>>>> +#define CPU_SOFTRST_CTRL 0x380
> >>>>>>> #define LVL2_CLK_GATE_OVRA 0xf8
> >>>>>>> #define LVL2_CLK_GATE_OVRC 0x3a0
> >>>>>>> @@ -2820,6 +2824,7 @@ static int tegra210_enable_pllu(void)
> >>>>>>> struct tegra_clk_pll_freq_table *fentry;
> >>>>>>> struct tegra_clk_pll pllu;
> >>>>>>> u32 reg;
> >>>>>>> + int ret;
> >>>>>>> for (fentry = pll_u_freq_table; fentry->input_rate;
> >>>>>>> fentry++) {
> >>>>>>> if (fentry->input_rate == pll_ref_freq)
> >>>>>>> @@ -2847,10 +2852,10 @@ static int tegra210_enable_pllu(void)
> >>>>>>> fence_udelay(1, clk_base);
> >>>>>>> reg |= PLL_ENABLE;
> >>>>>>> writel(reg, clk_base + PLLU_BASE);
> >>>>>>> + fence_udelay(1, clk_base);
> >>>>>>> - readl_relaxed_poll_timeout_atomic(clk_base +
> >>>>>>> PLLU_BASE, reg,
> >>>>>>> - reg & PLL_BASE_LOCK, 2, 1000);
> >>>>>>> - if (!(reg & PLL_BASE_LOCK)) {
> >>>>>>> + ret = tegra210_wait_for_mask(&pllu, PLLU_BASE,
> >>>>>>> PLL_BASE_LOCK);
> >>>>>>> + if (ret) {
> >>>>>>> pr_err("Timed out waiting for PLL_U to lock\n");
> >>>>>>> return -ETIMEDOUT;
> >>>>>>> }
> >>>>>>> @@ -3283,6 +3288,103 @@ static void
> >>>>>>> tegra210_disable_cpu_clock(u32 cpu)
> >>>>>>> }
> >>>>>>> #ifdef CONFIG_PM_SLEEP
> >>>>>>> +static u32 cpu_softrst_ctx[3];
> >>>>>>> +static struct platform_device *dfll_pdev;
> >>>>>>> +#define car_readl(_base, _off) readl_relaxed(clk_base +
> >>>>>>> (_base) + ((_off) * 4))
> >>>>>>> +#define car_writel(_val, _base, _off) \
> >>>>>>> + writel_relaxed(_val, clk_base + (_base) + ((_off) *
> >>>>>>> 4)) +
> >>>>>>> +static int tegra210_clk_suspend(void)
> >>>>>>> +{
> >>>>>>> + unsigned int i;
> >>>>>>> + struct device_node *node;
> >>>>>>> +
> >>>>>>> + tegra_cclkg_burst_policy_save_context();
> >>>>>>> +
> >>>>>>> + if (!dfll_pdev) {
> >>>>>>> + node = of_find_compatible_node(NULL, NULL,
> >>>>>>> + "nvidia,tegra210-dfll");
> >>>>>>> + if (node)
> >>>>>>> + dfll_pdev = of_find_device_by_node(node);
> >>>>>>> +
> >>>>>>> + of_node_put(node);
> >>>>>>> + if (!dfll_pdev)
> >>>>>>> + pr_err("dfll node not found. no suspend for
> >>>>>>> dfll\n");
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + if (dfll_pdev)
> >>>>>>> + tegra_dfll_suspend(dfll_pdev);
> >>>>>>> +
> >>>>>>> + /* Enable PLLP_OUT_CPU after dfll suspend */
> >>>>>>> + tegra_clk_set_pllp_out_cpu(true);
> >>>>>>> +
> >>>>>>> + tegra_sclk_cclklp_burst_policy_save_context();
> >>>>>>> +
> >>>>>>> + clk_save_context();
> >>>>>>> +
> >>>>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
> >>>>>>> + cpu_softrst_ctx[i] = car_readl(CPU_SOFTRST_CTRL, i);
> >>>>>>> +
> >>>>>>> + return 0;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static void tegra210_clk_resume(void)
> >>>>>>> +{
> >>>>>>> + unsigned int i;
> >>>>>>> + struct clk_hw *parent;
> >>>>>>> + struct clk *clk;
> >>>>>>> +
> >>>>>>> + /*
> >>>>>>> + * clk_restore_context restores clocks as per the clock
> >>>>>>> tree.
> >>>>>>> + *
> >>>>>>> + * dfllCPU_out is first in the clock tree to get
> >>>>>>> restored and it
> >>>>>>> + * involves programming DFLL controller along with
> >>>>>>> restoring CPUG
> >>>>>>> + * clock burst policy.
> >>>>>>> + *
> >>>>>>> + * DFLL programming needs dfll_ref and dfll_soc
> >>>>>>> peripheral clocks
> >>>>>>> + * to be restores which are part ofthe peripheral
> >>>>>>> clocks.
> >>>> ^ white-space
> >>>>
> >>>> Please use spellchecker to avoid typos.
> >>>>
> >>>>>>> + * So, peripheral clocks restore should happen prior to
> >>>>>>> dfll clock
> >>>>>>> + * restore.
> >>>>>>> + */
> >>>>>>> +
> >>>>>>> + tegra_clk_osc_resume(clk_base);
> >>>>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
> >>>>>>> + car_writel(cpu_softrst_ctx[i], CPU_SOFTRST_CTRL, i);
> >>>>>>> +
> >>>>>>> + /* restore all plls and peripheral clocks */
> >>>>>>> + tegra210_init_pllu();
> >>>>>>> + clk_restore_context();
> >>>>>>> +
> >>>>>>> + fence_udelay(5, clk_base);
> >>>>>>> +
> >>>>>>> + /* resume SCLK and CPULP clocks */
> >>>>>>> + tegra_sclk_cpulp_burst_policy_restore_context();
> >>>>>>> +
> >>>>>>> + /*
> >>>>>>> + * restore CPUG clocks:
> >>>>>>> + * - enable DFLL in open loop mode
> >>>>>>> + * - switch CPUG to DFLL clock source
> >>>>>>> + * - close DFLL loop
> >>>>>>> + * - sync PLLX state
> >>>>>>> + */
> >>>>>>> + if (dfll_pdev)
> >>>>>>> + tegra_dfll_resume(dfll_pdev, false);
> >>>>>>> +
> >>>>>>> + tegra_cclkg_burst_policy_restore_context();
> >>>>>>> + fence_udelay(2, clk_base);
> >>>>>>> +
> >>>>>>> + if (dfll_pdev)
> >>>>>>> + tegra_dfll_resume(dfll_pdev, true);
> >>>>>>> +
> >>>>>>> + parent =
> >>>>>>> clk_hw_get_parent(__clk_get_hw(clks[TEGRA210_CLK_CCLK_G]));
> >>>>>>> + clk = clks[TEGRA210_CLK_PLL_X];
> >>>>>>> + if (parent != __clk_get_hw(clk))
> >>>>>>> + tegra_clk_sync_state_pll(__clk_get_hw(clk));
> >>>>>>> +
> >>>>>>> + /* Disable PLL_OUT_CPU after DFLL resume */
> >>>>>>> + tegra_clk_set_pllp_out_cpu(false);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> static void tegra210_cpu_clock_suspend(void)
> >>>>>>> {
> >>>>>>> /* switch coresite to clk_m, save off original source
> >>>>>>> */ @@ -3298,6 +3400,11 @@ static void
> >>>>>>> tegra210_cpu_clock_resume(void) }
> >>>>>>> #endif
> >>>>>>> +static struct syscore_ops tegra_clk_syscore_ops = {
> >>>>>>> + .suspend = tegra210_clk_suspend,
> >>>>>>> + .resume = tegra210_clk_resume,
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> static struct tegra_cpu_car_ops tegra210_cpu_car_ops = {
> >>>>>>> .wait_for_reset = tegra210_wait_cpu_in_reset,
> >>>>>>> .disable_clock = tegra210_disable_cpu_clock,
> >>>>>>> @@ -3583,5 +3690,7 @@ static void __init
> >>>>>>> tegra210_clock_init(struct device_node *np)
> >>>>>>> tegra210_mbist_clk_init();
> >>>>>>> tegra_cpu_car_ops = &tegra210_cpu_car_ops;
> >>>>>>> +
> >>>>>>> + register_syscore_ops(&tegra_clk_syscore_ops);
> >>>>>>> }
> >>>>>> Is it really worthwhile to use syscore_ops for suspend/resume
> >>>>>> given that drivers for
> >>>>>> won't resume before the CLK driver anyway? Are there any other
> >>>>>> options for CLK
> >>>>>> suspend/resume?
> >>>>>>
> >>>>>> I'm also not sure whether PM runtime API could be used at all
> >>>>>> in the context of
> >>>>>> syscore_ops ..
> >>>>>>
> >>>>>> Secondly, what about to use generic clk_save_context() /
> >>>>>> clk_restore_context()
> >>>>>> helpers for the suspend-resume? It looks to me that some other
> >>>>>> essential (and proper)
> >>>>>> platform driver (soc/tegra/? PMC?) should suspend-resume the
> >>>>>> clocks using the generic
> >>>>>> CLK Framework API.
> >>>>> Clock resume should happen very early to restore peripheral and
> >>>>> cpu clocks very early than peripheral drivers resume happens.
> >>>> If all peripheral drivers properly requested all of the
> >>>> necessary clocks and CLK driver was a platform driver, then I
> >>>> guess the probe should have been naturally ordered. But that's
> >>>> not very achievable with the currently available infrastructure
> >>>> in the kernel, so I'm not arguing that the clocks should be
> >>>> explicitly resumed before the users.
> >>>>> this patch series uses clk_save_context and clk_restore_context
> >>>>> for corresponding divider, pll, pllout.. save and restore
> >>>>> context.
> >>>> Now I see that indeed this API is utilized in this patch, thank
> >>>> you for the clarification.
> >>>>
> >>>>> But as there is dependency on dfll resume and cpu and pllx
> >>>>> clocks restore, couldnt use clk_save_context and
> >>>>> clk_restore_context for dfll.
> >>>>>
> >>>>> So implemented recommended dfll resume sequence in main
> >>>>> Tegra210 clock driver along with invoking
> >>>>> clk_save_context/clk_restore_context where all other clocks
> >>>>> save/restore happens as per clock tree traversal.
> >>>> Could you please clarify what part of peripherals clocks is
> >>>> required for DFLL's restore? Couldn't DFLL driver be changed to
> >>>> avoid that quirkness and thus to make DFLL driver suspend/resume
> >>>> the clock?
> >>> DFLL source ref_clk and soc_clk need to be restored prior to dfll.
> >>>
> >>> I see dfllCPU_out parent to CCLK_G first in the clock tree and
> >>> dfll_ref and dfll_soc peripheral clocks are not resumed by the
> >>> time dfll resume happens first.
> >>>
> >>> ref_clk and soc_clk source is from pll_p and clock tree has these
> >>> registered under pll_p which happens later.
> >>>
> >>> tegra210_clock_init registers in order plls, peripheral clocks,
> >>> super_clk init for cclk_g during clock driver probe and dfll
> >>> probe and register happens later.
> >>>
> >> One more thing, CLDVFS peripheral clock enable is also needed to be
> >> enabled to program DFLL Controller and all peripheral clock
> >> context is restored only after their PLL sources are restored.
> >>
> >> DFLL restore involves dfll source clock resume along with CLDVFS
> >> periheral clock enable and reset
> >>
> > I don't quite see why you can't simply add suspend/resume callbacks
> > to the CPUFreq driver to:
> >
> > On suspend:
> > 1. Switch CPU to PLLP (or whatever "safe" parent)
> > 2. Disable/teardown DFLL
> >
> > On resume:
> > 1. Enable/restore DFLL
> > 2. Switch CPU back to DFLL
>
> dfll runtime suspend/resume are already part of dfll_pm_ops. Don't we
> want to use it for suspend/resume as well?
Looks like no. Seems runtime PM of that driver is intended solely for
the DFLL's clk management.
> currently no APIs are shared b/w clk/tegra driver and CPUFreq driver
> to invoke dfll suspend/resume in CPUFreq driver
>
Just add it. Also, please note that CPUFreq driver is optional and thus
you may need to switch CPU to a safe parent on clk-core suspend as
well in order to resume properly if CPU was running off unsafe parent
during boot and CPUFreq driver is disabled in kernel build (or failed
to load).
The other thing that also need attention is that T124 CPUFreq driver
implicitly relies on DFLL driver to be probed first, which is icky.
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-16 4:37 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <0ee055ad-d397-32e5-60ee-d62c14c6f77b@gmail.com>
On 7/15/19 8:50 PM, Dmitry Osipenko wrote:
> 16.07.2019 6:00, Sowjanya Komatineni пишет:
>> On 7/15/19 5:35 PM, Sowjanya Komatineni wrote:
>>> On 7/14/19 2:41 PM, Dmitry Osipenko wrote:
>>>> 13.07.2019 8:54, Sowjanya Komatineni пишет:
>>>>> On 6/29/19 8:10 AM, Dmitry Osipenko wrote:
>>>>>> 28.06.2019 5:12, Sowjanya Komatineni пишет:
>>>>>>> This patch adds system suspend and resume support for Tegra210
>>>>>>> clocks.
>>>>>>>
>>>>>>> All the CAR controller settings are lost on suspend when core power
>>>>>>> goes off.
>>>>>>>
>>>>>>> This patch has implementation for saving and restoring all the PLLs
>>>>>>> and clocks context during system suspend and resume to have the
>>>>>>> clocks back to same state for normal operation.
>>>>>>>
>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>> ---
>>>>>>> drivers/clk/tegra/clk-tegra210.c | 115
>>>>>>> ++++++++++++++++++++++++++++++++++++++-
>>>>>>> drivers/clk/tegra/clk.c | 14 +++++
>>>>>>> drivers/clk/tegra/clk.h | 1 +
>>>>>>> 3 files changed, 127 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/clk/tegra/clk-tegra210.c
>>>>>>> b/drivers/clk/tegra/clk-tegra210.c
>>>>>>> index 1c08c53482a5..1b839544e086 100644
>>>>>>> --- a/drivers/clk/tegra/clk-tegra210.c
>>>>>>> +++ b/drivers/clk/tegra/clk-tegra210.c
>>>>>>> @@ -9,10 +9,12 @@
>>>>>>> #include <linux/clkdev.h>
>>>>>>> #include <linux/of.h>
>>>>>>> #include <linux/of_address.h>
>>>>>>> +#include <linux/of_platform.h>
>>>>>>> #include <linux/delay.h>
>>>>>>> #include <linux/export.h>
>>>>>>> #include <linux/mutex.h>
>>>>>>> #include <linux/clk/tegra.h>
>>>>>>> +#include <linux/syscore_ops.h>
>>>>>>> #include <dt-bindings/clock/tegra210-car.h>
>>>>>>> #include <dt-bindings/reset/tegra210-car.h>
>>>>>>> #include <linux/iopoll.h>
>>>>>>> @@ -20,6 +22,7 @@
>>>>>>> #include <soc/tegra/pmc.h>
>>>>>>> #include "clk.h"
>>>>>>> +#include "clk-dfll.h"
>>>>>>> #include "clk-id.h"
>>>>>>> /*
>>>>>>> @@ -225,6 +228,7 @@
>>>>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_SET 0x2a8
>>>>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_CLR 0x2ac
>>>>>>> +#define CPU_SOFTRST_CTRL 0x380
>>>>>>> #define LVL2_CLK_GATE_OVRA 0xf8
>>>>>>> #define LVL2_CLK_GATE_OVRC 0x3a0
>>>>>>> @@ -2820,6 +2824,7 @@ static int tegra210_enable_pllu(void)
>>>>>>> struct tegra_clk_pll_freq_table *fentry;
>>>>>>> struct tegra_clk_pll pllu;
>>>>>>> u32 reg;
>>>>>>> + int ret;
>>>>>>> for (fentry = pll_u_freq_table; fentry->input_rate;
>>>>>>> fentry++) {
>>>>>>> if (fentry->input_rate == pll_ref_freq)
>>>>>>> @@ -2847,10 +2852,10 @@ static int tegra210_enable_pllu(void)
>>>>>>> fence_udelay(1, clk_base);
>>>>>>> reg |= PLL_ENABLE;
>>>>>>> writel(reg, clk_base + PLLU_BASE);
>>>>>>> + fence_udelay(1, clk_base);
>>>>>>> - readl_relaxed_poll_timeout_atomic(clk_base + PLLU_BASE, reg,
>>>>>>> - reg & PLL_BASE_LOCK, 2, 1000);
>>>>>>> - if (!(reg & PLL_BASE_LOCK)) {
>>>>>>> + ret = tegra210_wait_for_mask(&pllu, PLLU_BASE, PLL_BASE_LOCK);
>>>>>>> + if (ret) {
>>>>>>> pr_err("Timed out waiting for PLL_U to lock\n");
>>>>>>> return -ETIMEDOUT;
>>>>>>> }
>>>>>>> @@ -3283,6 +3288,103 @@ static void tegra210_disable_cpu_clock(u32
>>>>>>> cpu)
>>>>>>> }
>>>>>>> #ifdef CONFIG_PM_SLEEP
>>>>>>> +static u32 cpu_softrst_ctx[3];
>>>>>>> +static struct platform_device *dfll_pdev;
>>>>>>> +#define car_readl(_base, _off) readl_relaxed(clk_base + (_base) +
>>>>>>> ((_off) * 4))
>>>>>>> +#define car_writel(_val, _base, _off) \
>>>>>>> + writel_relaxed(_val, clk_base + (_base) + ((_off) * 4))
>>>>>>> +
>>>>>>> +static int tegra210_clk_suspend(void)
>>>>>>> +{
>>>>>>> + unsigned int i;
>>>>>>> + struct device_node *node;
>>>>>>> +
>>>>>>> + tegra_cclkg_burst_policy_save_context();
>>>>>>> +
>>>>>>> + if (!dfll_pdev) {
>>>>>>> + node = of_find_compatible_node(NULL, NULL,
>>>>>>> + "nvidia,tegra210-dfll");
>>>>>>> + if (node)
>>>>>>> + dfll_pdev = of_find_device_by_node(node);
>>>>>>> +
>>>>>>> + of_node_put(node);
>>>>>>> + if (!dfll_pdev)
>>>>>>> + pr_err("dfll node not found. no suspend for dfll\n");
>>>>>>> + }
>>>>>>> +
>>>>>>> + if (dfll_pdev)
>>>>>>> + tegra_dfll_suspend(dfll_pdev);
>>>>>>> +
>>>>>>> + /* Enable PLLP_OUT_CPU after dfll suspend */
>>>>>>> + tegra_clk_set_pllp_out_cpu(true);
>>>>>>> +
>>>>>>> + tegra_sclk_cclklp_burst_policy_save_context();
>>>>>>> +
>>>>>>> + clk_save_context();
>>>>>>> +
>>>>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>>>>>> + cpu_softrst_ctx[i] = car_readl(CPU_SOFTRST_CTRL, i);
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void tegra210_clk_resume(void)
>>>>>>> +{
>>>>>>> + unsigned int i;
>>>>>>> + struct clk_hw *parent;
>>>>>>> + struct clk *clk;
>>>>>>> +
>>>>>>> + /*
>>>>>>> + * clk_restore_context restores clocks as per the clock tree.
>>>>>>> + *
>>>>>>> + * dfllCPU_out is first in the clock tree to get restored and it
>>>>>>> + * involves programming DFLL controller along with restoring
>>>>>>> CPUG
>>>>>>> + * clock burst policy.
>>>>>>> + *
>>>>>>> + * DFLL programming needs dfll_ref and dfll_soc peripheral
>>>>>>> clocks
>>>>>>> + * to be restores which are part ofthe peripheral clocks.
>>>> ^ white-space
>>>>
>>>> Please use spellchecker to avoid typos.
>>>>
>>>>>>> + * So, peripheral clocks restore should happen prior to dfll
>>>>>>> clock
>>>>>>> + * restore.
>>>>>>> + */
>>>>>>> +
>>>>>>> + tegra_clk_osc_resume(clk_base);
>>>>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>>>>>> + car_writel(cpu_softrst_ctx[i], CPU_SOFTRST_CTRL, i);
>>>>>>> +
>>>>>>> + /* restore all plls and peripheral clocks */
>>>>>>> + tegra210_init_pllu();
>>>>>>> + clk_restore_context();
>>>>>>> +
>>>>>>> + fence_udelay(5, clk_base);
>>>>>>> +
>>>>>>> + /* resume SCLK and CPULP clocks */
>>>>>>> + tegra_sclk_cpulp_burst_policy_restore_context();
>>>>>>> +
>>>>>>> + /*
>>>>>>> + * restore CPUG clocks:
>>>>>>> + * - enable DFLL in open loop mode
>>>>>>> + * - switch CPUG to DFLL clock source
>>>>>>> + * - close DFLL loop
>>>>>>> + * - sync PLLX state
>>>>>>> + */
>>>>>>> + if (dfll_pdev)
>>>>>>> + tegra_dfll_resume(dfll_pdev, false);
>>>>>>> +
>>>>>>> + tegra_cclkg_burst_policy_restore_context();
>>>>>>> + fence_udelay(2, clk_base);
>>>>>>> +
>>>>>>> + if (dfll_pdev)
>>>>>>> + tegra_dfll_resume(dfll_pdev, true);
>>>>>>> +
>>>>>>> + parent =
>>>>>>> clk_hw_get_parent(__clk_get_hw(clks[TEGRA210_CLK_CCLK_G]));
>>>>>>> + clk = clks[TEGRA210_CLK_PLL_X];
>>>>>>> + if (parent != __clk_get_hw(clk))
>>>>>>> + tegra_clk_sync_state_pll(__clk_get_hw(clk));
>>>>>>> +
>>>>>>> + /* Disable PLL_OUT_CPU after DFLL resume */
>>>>>>> + tegra_clk_set_pllp_out_cpu(false);
>>>>>>> +}
>>>>>>> +
>>>>>>> static void tegra210_cpu_clock_suspend(void)
>>>>>>> {
>>>>>>> /* switch coresite to clk_m, save off original source */
>>>>>>> @@ -3298,6 +3400,11 @@ static void tegra210_cpu_clock_resume(void)
>>>>>>> }
>>>>>>> #endif
>>>>>>> +static struct syscore_ops tegra_clk_syscore_ops = {
>>>>>>> + .suspend = tegra210_clk_suspend,
>>>>>>> + .resume = tegra210_clk_resume,
>>>>>>> +};
>>>>>>> +
>>>>>>> static struct tegra_cpu_car_ops tegra210_cpu_car_ops = {
>>>>>>> .wait_for_reset = tegra210_wait_cpu_in_reset,
>>>>>>> .disable_clock = tegra210_disable_cpu_clock,
>>>>>>> @@ -3583,5 +3690,7 @@ static void __init tegra210_clock_init(struct
>>>>>>> device_node *np)
>>>>>>> tegra210_mbist_clk_init();
>>>>>>> tegra_cpu_car_ops = &tegra210_cpu_car_ops;
>>>>>>> +
>>>>>>> + register_syscore_ops(&tegra_clk_syscore_ops);
>>>>>>> }
>>>>>> Is it really worthwhile to use syscore_ops for suspend/resume given
>>>>>> that drivers for
>>>>>> won't resume before the CLK driver anyway? Are there any other options
>>>>>> for CLK
>>>>>> suspend/resume?
>>>>>>
>>>>>> I'm also not sure whether PM runtime API could be used at all in the
>>>>>> context of
>>>>>> syscore_ops ..
>>>>>>
>>>>>> Secondly, what about to use generic clk_save_context() /
>>>>>> clk_restore_context()
>>>>>> helpers for the suspend-resume? It looks to me that some other
>>>>>> essential (and proper)
>>>>>> platform driver (soc/tegra/? PMC?) should suspend-resume the clocks
>>>>>> using the generic
>>>>>> CLK Framework API.
>>>>> Clock resume should happen very early to restore peripheral and cpu
>>>>> clocks very early than peripheral drivers resume happens.
>>>> If all peripheral drivers properly requested all of the necessary clocks
>>>> and CLK driver was a platform driver, then I guess the probe should have
>>>> been naturally ordered. But that's not very achievable with the
>>>> currently available infrastructure in the kernel, so I'm not arguing
>>>> that the clocks should be explicitly resumed before the users.
>>>>
>>>>> this patch series uses clk_save_context and clk_restore_context for
>>>>> corresponding divider, pll, pllout.. save and restore context.
>>>> Now I see that indeed this API is utilized in this patch, thank you for
>>>> the clarification.
>>>>
>>>>> But as there is dependency on dfll resume and cpu and pllx clocks
>>>>> restore, couldnt use clk_save_context and clk_restore_context for dfll.
>>>>>
>>>>> So implemented recommended dfll resume sequence in main Tegra210 clock
>>>>> driver along with invoking clk_save_context/clk_restore_context where
>>>>> all other clocks save/restore happens as per clock tree traversal.
>>>> Could you please clarify what part of peripherals clocks is required for
>>>> DFLL's restore? Couldn't DFLL driver be changed to avoid that quirkness
>>>> and thus to make DFLL driver suspend/resume the clock?
>>> DFLL source ref_clk and soc_clk need to be restored prior to dfll.
>>>
>>> I see dfllCPU_out parent to CCLK_G first in the clock tree and
>>> dfll_ref and dfll_soc peripheral clocks are not resumed by the time
>>> dfll resume happens first.
>>>
>>> ref_clk and soc_clk source is from pll_p and clock tree has these
>>> registered under pll_p which happens later.
>>>
>>> tegra210_clock_init registers in order plls, peripheral clocks,
>>> super_clk init for cclk_g during clock driver probe and dfll probe and
>>> register happens later.
>>>
>> One more thing, CLDVFS peripheral clock enable is also needed to be
>> enabled to program DFLL Controller and all peripheral clock context is
>> restored only after their PLL sources are restored.
>>
>> DFLL restore involves dfll source clock resume along with CLDVFS
>> periheral clock enable and reset
>>
> I don't quite see why you can't simply add suspend/resume callbacks to
> the CPUFreq driver to:
>
> On suspend:
> 1. Switch CPU to PLLP (or whatever "safe" parent)
> 2. Disable/teardown DFLL
>
> On resume:
> 1. Enable/restore DFLL
> 2. Switch CPU back to DFLL
dfll runtime suspend/resume are already part of dfll_pm_ops. Don't we
want to use it for suspend/resume as well?
currently no APIs are shared b/w clk/tegra driver and CPUFreq driver to
invoke dfll suspend/resume in CPUFreq driver
^ permalink raw reply
* Re: [PATCH v2 1/4] opp: core: add regulators enable and disable
From: Chanwoo Choi @ 2019-07-16 4:03 UTC (permalink / raw)
To: Kamil Konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <20190715120416.3561-2-k.konieczny@partner.samsung.com>
Hi Kamil,
On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
> Add enable regulators to dev_pm_opp_set_regulators() and disable
> regulators to dev_pm_opp_put_regulators(). This prepares for
> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
IMHO, it is not proper to mention the specific driver name.
If you explain the reason why enable the regulator before using it,
it is enough description.
>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> --
> Changes in v2:
>
> - move regulator enable and disable into loop
>
> ---
> drivers/opp/core.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0e7703fe733f..069c5cf8827e 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
> goto free_regulators;
> }
>
> + ret = regulator_enable(reg);
> + if (ret < 0)
> + goto disable;
> +
> opp_table->regulators[i] = reg;
> }
>
> @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>
> return opp_table;
>
> +disable:
> + regulator_put(reg);
> + --i;
> +
> free_regulators:
> - while (i != 0)
> - regulator_put(opp_table->regulators[--i]);
> + for (; i >= 0; --i) {
> + regulator_disable(opp_table->regulators[i]);
> + regulator_put(opp_table->regulators[i]);
> + }
>
> kfree(opp_table->regulators);
> opp_table->regulators = NULL;
> @@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
> /* Make sure there are no concurrent readers while updating opp_table */
> WARN_ON(!list_empty(&opp_table->opp_list));
>
> - for (i = opp_table->regulator_count - 1; i >= 0; i--)
> + for (i = opp_table->regulator_count - 1; i >= 0; i--) {
> + regulator_disable(opp_table->regulators[i]);
> regulator_put(opp_table->regulators[i]);
> + }
>
> _free_set_opp_data(opp_table);
>
>
I agree to enable the regulator before using it.
The bootloader might not enable the regulators
and the kernel need to enable regulator in order to increase
the reference count explicitly event if bootloader enables it.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
From: Chanwoo Choi @ 2019-07-16 3:56 UTC (permalink / raw)
To: Kamil Konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <20190715120416.3561-3-k.konieczny@partner.samsung.com>
Hi Kamil,
Looks good to me. But, this patch has some issue.
I added the detailed reviews.
I recommend that you make the separate patches as following
in order to clarify the role of which apply the dev_pm_opp_* function.
First patch,
Need to consolidate the following two function into one function.
because the original exynos-bus.c has the problem that the regulator
of parent devfreq device have to be enabled before enabling the clock.
This issue did not happen because bootloader enables the bus-related
regulators before kernel booting.
- exynos_bus_parse_of()
- exynos_bus_parent_parse_of()
Second patch,
Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
> Reuse opp core code for setting bus clock and voltage. As a side
> effect this allow useage of coupled regulators feature (required
> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
> uses regulator_set_voltage_triplet() for setting regulator voltage
> while the old code used regulator_set_voltage_tol() with fixed
> tolerance. This patch also removes no longer needed parsing of DT
> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
> it).
>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> ---
> drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
> 1 file changed, 66 insertions(+), 106 deletions(-)
>
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 486cc5b422f1..7fc4f76bd848 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -25,7 +25,6 @@
> #include <linux/slab.h>
>
> #define DEFAULT_SATURATION_RATIO 40
> -#define DEFAULT_VOLTAGE_TOLERANCE 2
>
> struct exynos_bus {
> struct device *dev;
> @@ -37,9 +36,9 @@ struct exynos_bus {
>
> unsigned long curr_freq;
>
> - struct regulator *regulator;
> + struct opp_table *opp_table;
> +
> struct clk *clk;
> - unsigned int voltage_tolerance;
> unsigned int ratio;
> };
>
> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
> {
> struct exynos_bus *bus = dev_get_drvdata(dev);
> struct dev_pm_opp *new_opp;
> - unsigned long old_freq, new_freq, new_volt, tol;
> int ret = 0;
> -
> - /* Get new opp-bus instance according to new bus clock */
> + /*
> + * New frequency for bus may not be exactly matched to opp, adjust
> + * *freq to correct value.
> + */
You better to change this comment with following styles
to keep the consistency:
/* Get correct frequency for bus ... */
> new_opp = devfreq_recommended_opp(dev, freq, flags);
> if (IS_ERR(new_opp)) {
> dev_err(dev, "failed to get recommended opp instance\n");
> return PTR_ERR(new_opp);
> }
>
> - new_freq = dev_pm_opp_get_freq(new_opp);
> - new_volt = dev_pm_opp_get_voltage(new_opp);
> dev_pm_opp_put(new_opp);
>
> - old_freq = bus->curr_freq;
> -
> - if (old_freq == new_freq)
> - return 0;
> - tol = new_volt * bus->voltage_tolerance / 100;
> -
> /* Change voltage and frequency according to new OPP level */
> mutex_lock(&bus->lock);
> + ret = dev_pm_opp_set_rate(dev, *freq);
> + if (!ret)
> + bus->curr_freq = *freq;
Have to print the error log if ret has minus error value.
Modify it as following:
if (ret < 0) {
dev_err(dev, "failed to set bus rate\n");
goto err:
}
bus->curr_freq = *freq;
err:
mutex_unlock(&bus->lock);
return ret;
>
> - if (old_freq < new_freq) {
> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
> - if (ret < 0) {
> - dev_err(bus->dev, "failed to set voltage\n");
> - goto out;
> - }
> - }
> -
> - ret = clk_set_rate(bus->clk, new_freq);
> - if (ret < 0) {
> - dev_err(dev, "failed to change clock of bus\n");
> - clk_set_rate(bus->clk, old_freq);
> - goto out;
> - }
> -
> - if (old_freq > new_freq) {
> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
> - if (ret < 0) {
> - dev_err(bus->dev, "failed to set voltage\n");
> - goto out;
> - }
> - }
> - bus->curr_freq = new_freq;
> -
> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
> - old_freq, new_freq, clk_get_rate(bus->clk));
> -out:
> mutex_unlock(&bus->lock);
>
> return ret;
> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
> if (ret < 0)
> dev_warn(dev, "failed to disable the devfreq-event devices\n");
>
> - if (bus->regulator)
> - regulator_disable(bus->regulator);
> + if (bus->opp_table)
> + dev_pm_opp_put_regulators(bus->opp_table);
Have to disable regulator after disabling the clock
to prevent the h/w fault.
I think that you should call them with following sequence:
clk_disable_unprepare(bus->clk);
if (bus->opp_table)
dev_pm_opp_put_regulators(bus->opp_table);
dev_pm_opp_of_remove_table(dev);
>
> dev_pm_opp_of_remove_table(dev);
> +
> clk_disable_unprepare(bus->clk);
> }
>
> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
> {
> struct exynos_bus *bus = dev_get_drvdata(dev);
> struct dev_pm_opp *new_opp;
> - unsigned long old_freq, new_freq;
> - int ret = 0;
> + int ret;
>
> - /* Get new opp-bus instance according to new bus clock */
> + /*
> + * New frequency for bus may not be exactly matched to opp, adjust
> + * *freq to correct value.
> + */
You better to change this comment with following styles
to keep the consistency:
/* Get correct frequency for bus ... */
> new_opp = devfreq_recommended_opp(dev, freq, flags);
> if (IS_ERR(new_opp)) {
> dev_err(dev, "failed to get recommended opp instance\n");
> return PTR_ERR(new_opp);
> }
>
> - new_freq = dev_pm_opp_get_freq(new_opp);
> dev_pm_opp_put(new_opp);
>
> - old_freq = bus->curr_freq;
> -
> - if (old_freq == new_freq)
> - return 0;
> -
> /* Change the frequency according to new OPP level */
> mutex_lock(&bus->lock);
> + ret = dev_pm_opp_set_rate(dev, *freq);
> + if (!ret)
> + bus->curr_freq = *freq;
ditto. Have to print the error log, check above comment.
>
> - ret = clk_set_rate(bus->clk, new_freq);
> - if (ret < 0) {
> - dev_err(dev, "failed to set the clock of bus\n");
> - goto out;
> - }
> -
> - *freq = new_freq;
> - bus->curr_freq = new_freq;
> -
> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
> - old_freq, new_freq, clk_get_rate(bus->clk));
> -out:
> mutex_unlock(&bus->lock);
>
> return ret;
> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> struct exynos_bus *bus)
> {
> struct device *dev = bus->dev;
> - int i, ret, count, size;
> -
> - /* Get the regulator to provide each bus with the power */
> - bus->regulator = devm_regulator_get(dev, "vdd");
> - if (IS_ERR(bus->regulator)) {
> - dev_err(dev, "failed to get VDD regulator\n");
> - return PTR_ERR(bus->regulator);
> - }
> -
> - ret = regulator_enable(bus->regulator);
> - if (ret < 0) {
> - dev_err(dev, "failed to enable VDD regulator\n");
> - return ret;
> - }
> + int i, count, size;
>
> /*
> * Get the devfreq-event devices to get the current utilization of
> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> count = devfreq_event_get_edev_count(dev);
> if (count < 0) {
> dev_err(dev, "failed to get the count of devfreq-event dev\n");
> - ret = count;
> - goto err_regulator;
> + return count;
> }
> +
> bus->edev_count = count;
>
> size = sizeof(*bus->edev) * count;
> bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
> - if (!bus->edev) {
> - ret = -ENOMEM;
> - goto err_regulator;
> - }
> + if (!bus->edev)
> + return -ENOMEM;
>
> for (i = 0; i < count; i++) {
> bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
> - if (IS_ERR(bus->edev[i])) {
> - ret = -EPROBE_DEFER;
> - goto err_regulator;
> - }
> + if (IS_ERR(bus->edev[i]))
> + return -EPROBE_DEFER;
> }
>
> /*
> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
> bus->ratio = DEFAULT_SATURATION_RATIO;
>
> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
> - &bus->voltage_tolerance))
> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
> -
> return 0;
> -
> -err_regulator:
> - regulator_disable(bus->regulator);
> -
> - return ret;
> }
>
> static int exynos_bus_parse_of(struct device_node *np,
> - struct exynos_bus *bus)
> + struct exynos_bus *bus, bool passive)
> {
> struct device *dev = bus->dev;
> + struct opp_table *opp_table;
> + const char *vdd = "vdd";
> struct dev_pm_opp *opp;
> unsigned long rate;
> int ret;
> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
> return ret;
> }
>
> + if (!passive) {
> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
> + if (IS_ERR(opp_table)) {
> + ret = PTR_ERR(opp_table);
> + dev_err(dev, "failed to set regulators %d\n", ret);
> + goto err_clk;/
> + }
> +
> + bus->opp_table = opp_table;
> + }
This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
because the regulator is only used by parent devfreq device.
> +
> /* Get the freq and voltage from OPP table to scale the bus freq */
> ret = dev_pm_opp_of_add_table(dev);
> if (ret < 0) {
> dev_err(dev, "failed to get OPP table\n");
> - goto err_clk;
> + goto err_regulator;
> }
>
> rate = clk_get_rate(bus->clk);
> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
> ret = PTR_ERR(opp);
> goto err_opp;
> }
> +
> bus->curr_freq = dev_pm_opp_get_freq(opp);
> dev_pm_opp_put(opp);
>
> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>
> err_opp:
> dev_pm_opp_of_remove_table(dev);
> +
> +err_regulator:
> + if (bus->opp_table) {
> + dev_pm_opp_put_regulators(bus->opp_table);
> + bus->opp_table = NULL;
> + }
As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
after removing the opp_table by dev_pm_opp_of_remove_table().
> +
> err_clk:
> clk_disable_unprepare(bus->clk);
>
> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
> struct exynos_bus *bus;
> int ret, max_state;
> unsigned long min_freq, max_freq;
> + bool passive = false;
>
> if (!np) {
> dev_err(dev, "failed to find devicetree node\n");
> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
> bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> if (!bus)
> return -ENOMEM;
> +
> mutex_init(&bus->lock);
> bus->dev = &pdev->dev;
> platform_set_drvdata(pdev, bus);
> + node = of_parse_phandle(dev->of_node, "devfreq", 0);
> + if (node) {
> + of_node_put(node);
> + passive = true;
> + }
>
> /* Parse the device-tree to get the resource information */
> - ret = exynos_bus_parse_of(np, bus);
> + ret = exynos_bus_parse_of(np, bus, passive);
> if (ret < 0)
> return ret;
>
> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
> goto err;
> }
>
> - node = of_parse_phandle(dev->of_node, "devfreq", 0);
> - if (node) {
> - of_node_put(node);
> + if (passive)
> goto passive;
> - } else {
> - ret = exynos_bus_parent_parse_of(np, bus);
> - }
> +
> + ret = exynos_bus_parent_parse_of(np, bus);
>
Remove unneeded blank line.
> if (ret < 0)
> goto err;
> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>
> err:
> dev_pm_opp_of_remove_table(dev);
> + if (bus->opp_table) {
> + dev_pm_opp_put_regulators(bus->opp_table);
> + bus->opp_table = NULL;
> + }
> +
ditto.
Have to disable regulator after disabling the clock
to prevent the h/w fault.
I think that you should call them with following sequence:
clk_disable_unprepare(bus->clk);
if (bus->opp_table)
dev_pm_opp_put_regulators(bus->opp_table);
dev_pm_opp_of_remove_table(dev);
> clk_disable_unprepare(bus->clk);
>
> return ret;
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-16 3:50 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <932d4d50-120c-9191-6a9a-23bf9c96633b@nvidia.com>
16.07.2019 6:00, Sowjanya Komatineni пишет:
>
> On 7/15/19 5:35 PM, Sowjanya Komatineni wrote:
>>
>> On 7/14/19 2:41 PM, Dmitry Osipenko wrote:
>>> 13.07.2019 8:54, Sowjanya Komatineni пишет:
>>>> On 6/29/19 8:10 AM, Dmitry Osipenko wrote:
>>>>> 28.06.2019 5:12, Sowjanya Komatineni пишет:
>>>>>> This patch adds system suspend and resume support for Tegra210
>>>>>> clocks.
>>>>>>
>>>>>> All the CAR controller settings are lost on suspend when core power
>>>>>> goes off.
>>>>>>
>>>>>> This patch has implementation for saving and restoring all the PLLs
>>>>>> and clocks context during system suspend and resume to have the
>>>>>> clocks back to same state for normal operation.
>>>>>>
>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>> ---
>>>>>> drivers/clk/tegra/clk-tegra210.c | 115
>>>>>> ++++++++++++++++++++++++++++++++++++++-
>>>>>> drivers/clk/tegra/clk.c | 14 +++++
>>>>>> drivers/clk/tegra/clk.h | 1 +
>>>>>> 3 files changed, 127 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/clk/tegra/clk-tegra210.c
>>>>>> b/drivers/clk/tegra/clk-tegra210.c
>>>>>> index 1c08c53482a5..1b839544e086 100644
>>>>>> --- a/drivers/clk/tegra/clk-tegra210.c
>>>>>> +++ b/drivers/clk/tegra/clk-tegra210.c
>>>>>> @@ -9,10 +9,12 @@
>>>>>> #include <linux/clkdev.h>
>>>>>> #include <linux/of.h>
>>>>>> #include <linux/of_address.h>
>>>>>> +#include <linux/of_platform.h>
>>>>>> #include <linux/delay.h>
>>>>>> #include <linux/export.h>
>>>>>> #include <linux/mutex.h>
>>>>>> #include <linux/clk/tegra.h>
>>>>>> +#include <linux/syscore_ops.h>
>>>>>> #include <dt-bindings/clock/tegra210-car.h>
>>>>>> #include <dt-bindings/reset/tegra210-car.h>
>>>>>> #include <linux/iopoll.h>
>>>>>> @@ -20,6 +22,7 @@
>>>>>> #include <soc/tegra/pmc.h>
>>>>>> #include "clk.h"
>>>>>> +#include "clk-dfll.h"
>>>>>> #include "clk-id.h"
>>>>>> /*
>>>>>> @@ -225,6 +228,7 @@
>>>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_SET 0x2a8
>>>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_CLR 0x2ac
>>>>>> +#define CPU_SOFTRST_CTRL 0x380
>>>>>> #define LVL2_CLK_GATE_OVRA 0xf8
>>>>>> #define LVL2_CLK_GATE_OVRC 0x3a0
>>>>>> @@ -2820,6 +2824,7 @@ static int tegra210_enable_pllu(void)
>>>>>> struct tegra_clk_pll_freq_table *fentry;
>>>>>> struct tegra_clk_pll pllu;
>>>>>> u32 reg;
>>>>>> + int ret;
>>>>>> for (fentry = pll_u_freq_table; fentry->input_rate;
>>>>>> fentry++) {
>>>>>> if (fentry->input_rate == pll_ref_freq)
>>>>>> @@ -2847,10 +2852,10 @@ static int tegra210_enable_pllu(void)
>>>>>> fence_udelay(1, clk_base);
>>>>>> reg |= PLL_ENABLE;
>>>>>> writel(reg, clk_base + PLLU_BASE);
>>>>>> + fence_udelay(1, clk_base);
>>>>>> - readl_relaxed_poll_timeout_atomic(clk_base + PLLU_BASE, reg,
>>>>>> - reg & PLL_BASE_LOCK, 2, 1000);
>>>>>> - if (!(reg & PLL_BASE_LOCK)) {
>>>>>> + ret = tegra210_wait_for_mask(&pllu, PLLU_BASE, PLL_BASE_LOCK);
>>>>>> + if (ret) {
>>>>>> pr_err("Timed out waiting for PLL_U to lock\n");
>>>>>> return -ETIMEDOUT;
>>>>>> }
>>>>>> @@ -3283,6 +3288,103 @@ static void tegra210_disable_cpu_clock(u32
>>>>>> cpu)
>>>>>> }
>>>>>> #ifdef CONFIG_PM_SLEEP
>>>>>> +static u32 cpu_softrst_ctx[3];
>>>>>> +static struct platform_device *dfll_pdev;
>>>>>> +#define car_readl(_base, _off) readl_relaxed(clk_base + (_base) +
>>>>>> ((_off) * 4))
>>>>>> +#define car_writel(_val, _base, _off) \
>>>>>> + writel_relaxed(_val, clk_base + (_base) + ((_off) * 4))
>>>>>> +
>>>>>> +static int tegra210_clk_suspend(void)
>>>>>> +{
>>>>>> + unsigned int i;
>>>>>> + struct device_node *node;
>>>>>> +
>>>>>> + tegra_cclkg_burst_policy_save_context();
>>>>>> +
>>>>>> + if (!dfll_pdev) {
>>>>>> + node = of_find_compatible_node(NULL, NULL,
>>>>>> + "nvidia,tegra210-dfll");
>>>>>> + if (node)
>>>>>> + dfll_pdev = of_find_device_by_node(node);
>>>>>> +
>>>>>> + of_node_put(node);
>>>>>> + if (!dfll_pdev)
>>>>>> + pr_err("dfll node not found. no suspend for dfll\n");
>>>>>> + }
>>>>>> +
>>>>>> + if (dfll_pdev)
>>>>>> + tegra_dfll_suspend(dfll_pdev);
>>>>>> +
>>>>>> + /* Enable PLLP_OUT_CPU after dfll suspend */
>>>>>> + tegra_clk_set_pllp_out_cpu(true);
>>>>>> +
>>>>>> + tegra_sclk_cclklp_burst_policy_save_context();
>>>>>> +
>>>>>> + clk_save_context();
>>>>>> +
>>>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>>>>> + cpu_softrst_ctx[i] = car_readl(CPU_SOFTRST_CTRL, i);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static void tegra210_clk_resume(void)
>>>>>> +{
>>>>>> + unsigned int i;
>>>>>> + struct clk_hw *parent;
>>>>>> + struct clk *clk;
>>>>>> +
>>>>>> + /*
>>>>>> + * clk_restore_context restores clocks as per the clock tree.
>>>>>> + *
>>>>>> + * dfllCPU_out is first in the clock tree to get restored and it
>>>>>> + * involves programming DFLL controller along with restoring
>>>>>> CPUG
>>>>>> + * clock burst policy.
>>>>>> + *
>>>>>> + * DFLL programming needs dfll_ref and dfll_soc peripheral
>>>>>> clocks
>>>>>> + * to be restores which are part ofthe peripheral clocks.
>>> ^ white-space
>>>
>>> Please use spellchecker to avoid typos.
>>>
>>>>>> + * So, peripheral clocks restore should happen prior to dfll
>>>>>> clock
>>>>>> + * restore.
>>>>>> + */
>>>>>> +
>>>>>> + tegra_clk_osc_resume(clk_base);
>>>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>>>>> + car_writel(cpu_softrst_ctx[i], CPU_SOFTRST_CTRL, i);
>>>>>> +
>>>>>> + /* restore all plls and peripheral clocks */
>>>>>> + tegra210_init_pllu();
>>>>>> + clk_restore_context();
>>>>>> +
>>>>>> + fence_udelay(5, clk_base);
>>>>>> +
>>>>>> + /* resume SCLK and CPULP clocks */
>>>>>> + tegra_sclk_cpulp_burst_policy_restore_context();
>>>>>> +
>>>>>> + /*
>>>>>> + * restore CPUG clocks:
>>>>>> + * - enable DFLL in open loop mode
>>>>>> + * - switch CPUG to DFLL clock source
>>>>>> + * - close DFLL loop
>>>>>> + * - sync PLLX state
>>>>>> + */
>>>>>> + if (dfll_pdev)
>>>>>> + tegra_dfll_resume(dfll_pdev, false);
>>>>>> +
>>>>>> + tegra_cclkg_burst_policy_restore_context();
>>>>>> + fence_udelay(2, clk_base);
>>>>>> +
>>>>>> + if (dfll_pdev)
>>>>>> + tegra_dfll_resume(dfll_pdev, true);
>>>>>> +
>>>>>> + parent =
>>>>>> clk_hw_get_parent(__clk_get_hw(clks[TEGRA210_CLK_CCLK_G]));
>>>>>> + clk = clks[TEGRA210_CLK_PLL_X];
>>>>>> + if (parent != __clk_get_hw(clk))
>>>>>> + tegra_clk_sync_state_pll(__clk_get_hw(clk));
>>>>>> +
>>>>>> + /* Disable PLL_OUT_CPU after DFLL resume */
>>>>>> + tegra_clk_set_pllp_out_cpu(false);
>>>>>> +}
>>>>>> +
>>>>>> static void tegra210_cpu_clock_suspend(void)
>>>>>> {
>>>>>> /* switch coresite to clk_m, save off original source */
>>>>>> @@ -3298,6 +3400,11 @@ static void tegra210_cpu_clock_resume(void)
>>>>>> }
>>>>>> #endif
>>>>>> +static struct syscore_ops tegra_clk_syscore_ops = {
>>>>>> + .suspend = tegra210_clk_suspend,
>>>>>> + .resume = tegra210_clk_resume,
>>>>>> +};
>>>>>> +
>>>>>> static struct tegra_cpu_car_ops tegra210_cpu_car_ops = {
>>>>>> .wait_for_reset = tegra210_wait_cpu_in_reset,
>>>>>> .disable_clock = tegra210_disable_cpu_clock,
>>>>>> @@ -3583,5 +3690,7 @@ static void __init tegra210_clock_init(struct
>>>>>> device_node *np)
>>>>>> tegra210_mbist_clk_init();
>>>>>> tegra_cpu_car_ops = &tegra210_cpu_car_ops;
>>>>>> +
>>>>>> + register_syscore_ops(&tegra_clk_syscore_ops);
>>>>>> }
>>>>> Is it really worthwhile to use syscore_ops for suspend/resume given
>>>>> that drivers for
>>>>> won't resume before the CLK driver anyway? Are there any other options
>>>>> for CLK
>>>>> suspend/resume?
>>>>>
>>>>> I'm also not sure whether PM runtime API could be used at all in the
>>>>> context of
>>>>> syscore_ops ..
>>>>>
>>>>> Secondly, what about to use generic clk_save_context() /
>>>>> clk_restore_context()
>>>>> helpers for the suspend-resume? It looks to me that some other
>>>>> essential (and proper)
>>>>> platform driver (soc/tegra/? PMC?) should suspend-resume the clocks
>>>>> using the generic
>>>>> CLK Framework API.
>>>> Clock resume should happen very early to restore peripheral and cpu
>>>> clocks very early than peripheral drivers resume happens.
>>> If all peripheral drivers properly requested all of the necessary clocks
>>> and CLK driver was a platform driver, then I guess the probe should have
>>> been naturally ordered. But that's not very achievable with the
>>> currently available infrastructure in the kernel, so I'm not arguing
>>> that the clocks should be explicitly resumed before the users.
>>>
>>>> this patch series uses clk_save_context and clk_restore_context for
>>>> corresponding divider, pll, pllout.. save and restore context.
>>> Now I see that indeed this API is utilized in this patch, thank you for
>>> the clarification.
>>>
>>>> But as there is dependency on dfll resume and cpu and pllx clocks
>>>> restore, couldnt use clk_save_context and clk_restore_context for dfll.
>>>>
>>>> So implemented recommended dfll resume sequence in main Tegra210 clock
>>>> driver along with invoking clk_save_context/clk_restore_context where
>>>> all other clocks save/restore happens as per clock tree traversal.
>>> Could you please clarify what part of peripherals clocks is required for
>>> DFLL's restore? Couldn't DFLL driver be changed to avoid that quirkness
>>> and thus to make DFLL driver suspend/resume the clock?
>>
>> DFLL source ref_clk and soc_clk need to be restored prior to dfll.
>>
>> I see dfllCPU_out parent to CCLK_G first in the clock tree and
>> dfll_ref and dfll_soc peripheral clocks are not resumed by the time
>> dfll resume happens first.
>>
>> ref_clk and soc_clk source is from pll_p and clock tree has these
>> registered under pll_p which happens later.
>>
>> tegra210_clock_init registers in order plls, peripheral clocks,
>> super_clk init for cclk_g during clock driver probe and dfll probe and
>> register happens later.
>>
> One more thing, CLDVFS peripheral clock enable is also needed to be
> enabled to program DFLL Controller and all peripheral clock context is
> restored only after their PLL sources are restored.
>
> DFLL restore involves dfll source clock resume along with CLDVFS
> periheral clock enable and reset
>
I don't quite see why you can't simply add suspend/resume callbacks to
the CPUFreq driver to:
On suspend:
1. Switch CPU to PLLP (or whatever "safe" parent)
2. Disable/teardown DFLL
On resume:
1. Enable/restore DFLL
2. Switch CPU back to DFLL
^ permalink raw reply
* RE: [EXT] Re: [v4,1/2] rtc/fsl: add FTM alarm driver as the wakeup source
From: Biwen Li @ 2019-07-16 3:49 UTC (permalink / raw)
To: Alexandre Belloni
Cc: a.zummo@towertech.it, Leo Li, robh+dt@kernel.org,
linux-rtc@vger.kernel.org, linux-kernel@vger.kernel.org,
Xiaobo Xie, Jiafei Pan, Ran Wang, mark.rutland@arm.com,
devicetree@vger.kernel.org
In-Reply-To: <20190715145637.GG4732@piout.net>
> Caution: EXT Email
>
> On 15/07/2019 18:15:19+0800, Biwen Li wrote:
> > + device_init_wakeup(&pdev->dev, true);
> > + rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, "ftm-alarm",
> > +
> &ftm_rtc_ops,
> > +
> THIS_MODULE);
>
> To be clear, I want you to not use devm_rtc_device_register and use
> devm_rtc_allocate_device and rtc_register_device.
I will correct it in v5.
>
> > + if (IS_ERR(rtc->rtc_dev)) {
> > + dev_err(&pdev->dev, "can't register rtc device\n");
> > + return PTR_ERR(rtc->rtc_dev);
> > + }
> > +
>
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin.
> com&data=02%7C01%7Cbiwen.li%40nxp.com%7Cc5b1382ed1fb493cf69
> f08d7093513b2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63
> 6987995881848030&sdata=T71z2Rjw%2FZL444U8C1ow3WcoyDYl76Mq
> niLIupXbXtI%3D&reserved=0
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-16 3:41 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <932d4d50-120c-9191-6a9a-23bf9c96633b@nvidia.com>
On 7/15/19 8:00 PM, Sowjanya Komatineni wrote:
>
> On 7/15/19 5:35 PM, Sowjanya Komatineni wrote:
>>
>> On 7/14/19 2:41 PM, Dmitry Osipenko wrote:
>>> 13.07.2019 8:54, Sowjanya Komatineni пишет:
>>>> On 6/29/19 8:10 AM, Dmitry Osipenko wrote:
>>>>> 28.06.2019 5:12, Sowjanya Komatineni пишет:
>>>>>> This patch adds system suspend and resume support for Tegra210
>>>>>> clocks.
>>>>>>
>>>>>> All the CAR controller settings are lost on suspend when core power
>>>>>> goes off.
>>>>>>
>>>>>> This patch has implementation for saving and restoring all the PLLs
>>>>>> and clocks context during system suspend and resume to have the
>>>>>> clocks back to same state for normal operation.
>>>>>>
>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>> ---
>>>>>> drivers/clk/tegra/clk-tegra210.c | 115
>>>>>> ++++++++++++++++++++++++++++++++++++++-
>>>>>> drivers/clk/tegra/clk.c | 14 +++++
>>>>>> drivers/clk/tegra/clk.h | 1 +
>>>>>> 3 files changed, 127 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/clk/tegra/clk-tegra210.c
>>>>>> b/drivers/clk/tegra/clk-tegra210.c
>>>>>> index 1c08c53482a5..1b839544e086 100644
>>>>>> --- a/drivers/clk/tegra/clk-tegra210.c
>>>>>> +++ b/drivers/clk/tegra/clk-tegra210.c
>>>>>> @@ -9,10 +9,12 @@
>>>>>> #include <linux/clkdev.h>
>>>>>> #include <linux/of.h>
>>>>>> #include <linux/of_address.h>
>>>>>> +#include <linux/of_platform.h>
>>>>>> #include <linux/delay.h>
>>>>>> #include <linux/export.h>
>>>>>> #include <linux/mutex.h>
>>>>>> #include <linux/clk/tegra.h>
>>>>>> +#include <linux/syscore_ops.h>
>>>>>> #include <dt-bindings/clock/tegra210-car.h>
>>>>>> #include <dt-bindings/reset/tegra210-car.h>
>>>>>> #include <linux/iopoll.h>
>>>>>> @@ -20,6 +22,7 @@
>>>>>> #include <soc/tegra/pmc.h>
>>>>>> #include "clk.h"
>>>>>> +#include "clk-dfll.h"
>>>>>> #include "clk-id.h"
>>>>>> /*
>>>>>> @@ -225,6 +228,7 @@
>>>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_SET 0x2a8
>>>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_CLR 0x2ac
>>>>>> +#define CPU_SOFTRST_CTRL 0x380
>>>>>> #define LVL2_CLK_GATE_OVRA 0xf8
>>>>>> #define LVL2_CLK_GATE_OVRC 0x3a0
>>>>>> @@ -2820,6 +2824,7 @@ static int tegra210_enable_pllu(void)
>>>>>> struct tegra_clk_pll_freq_table *fentry;
>>>>>> struct tegra_clk_pll pllu;
>>>>>> u32 reg;
>>>>>> + int ret;
>>>>>> for (fentry = pll_u_freq_table; fentry->input_rate;
>>>>>> fentry++) {
>>>>>> if (fentry->input_rate == pll_ref_freq)
>>>>>> @@ -2847,10 +2852,10 @@ static int tegra210_enable_pllu(void)
>>>>>> fence_udelay(1, clk_base);
>>>>>> reg |= PLL_ENABLE;
>>>>>> writel(reg, clk_base + PLLU_BASE);
>>>>>> + fence_udelay(1, clk_base);
>>>>>> - readl_relaxed_poll_timeout_atomic(clk_base + PLLU_BASE, reg,
>>>>>> - reg & PLL_BASE_LOCK, 2, 1000);
>>>>>> - if (!(reg & PLL_BASE_LOCK)) {
>>>>>> + ret = tegra210_wait_for_mask(&pllu, PLLU_BASE, PLL_BASE_LOCK);
>>>>>> + if (ret) {
>>>>>> pr_err("Timed out waiting for PLL_U to lock\n");
>>>>>> return -ETIMEDOUT;
>>>>>> }
>>>>>> @@ -3283,6 +3288,103 @@ static void
>>>>>> tegra210_disable_cpu_clock(u32 cpu)
>>>>>> }
>>>>>> #ifdef CONFIG_PM_SLEEP
>>>>>> +static u32 cpu_softrst_ctx[3];
>>>>>> +static struct platform_device *dfll_pdev;
>>>>>> +#define car_readl(_base, _off) readl_relaxed(clk_base + (_base) +
>>>>>> ((_off) * 4))
>>>>>> +#define car_writel(_val, _base, _off) \
>>>>>> + writel_relaxed(_val, clk_base + (_base) + ((_off) * 4))
>>>>>> +
>>>>>> +static int tegra210_clk_suspend(void)
>>>>>> +{
>>>>>> + unsigned int i;
>>>>>> + struct device_node *node;
>>>>>> +
>>>>>> + tegra_cclkg_burst_policy_save_context();
>>>>>> +
>>>>>> + if (!dfll_pdev) {
>>>>>> + node = of_find_compatible_node(NULL, NULL,
>>>>>> + "nvidia,tegra210-dfll");
>>>>>> + if (node)
>>>>>> + dfll_pdev = of_find_device_by_node(node);
>>>>>> +
>>>>>> + of_node_put(node);
>>>>>> + if (!dfll_pdev)
>>>>>> + pr_err("dfll node not found. no suspend for dfll\n");
>>>>>> + }
>>>>>> +
>>>>>> + if (dfll_pdev)
>>>>>> + tegra_dfll_suspend(dfll_pdev);
>>>>>> +
>>>>>> + /* Enable PLLP_OUT_CPU after dfll suspend */
>>>>>> + tegra_clk_set_pllp_out_cpu(true);
>>>>>> +
>>>>>> + tegra_sclk_cclklp_burst_policy_save_context();
>>>>>> +
>>>>>> + clk_save_context();
>>>>>> +
>>>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>>>>> + cpu_softrst_ctx[i] = car_readl(CPU_SOFTRST_CTRL, i);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static void tegra210_clk_resume(void)
>>>>>> +{
>>>>>> + unsigned int i;
>>>>>> + struct clk_hw *parent;
>>>>>> + struct clk *clk;
>>>>>> +
>>>>>> + /*
>>>>>> + * clk_restore_context restores clocks as per the clock tree.
>>>>>> + *
>>>>>> + * dfllCPU_out is first in the clock tree to get restored
>>>>>> and it
>>>>>> + * involves programming DFLL controller along with restoring
>>>>>> CPUG
>>>>>> + * clock burst policy.
>>>>>> + *
>>>>>> + * DFLL programming needs dfll_ref and dfll_soc peripheral
>>>>>> clocks
>>>>>> + * to be restores which are part ofthe peripheral clocks.
>>> ^ white-space
>>>
>>> Please use spellchecker to avoid typos.
>>>
>>>>>> + * So, peripheral clocks restore should happen prior to dfll
>>>>>> clock
>>>>>> + * restore.
>>>>>> + */
>>>>>> +
>>>>>> + tegra_clk_osc_resume(clk_base);
>>>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>>>>> + car_writel(cpu_softrst_ctx[i], CPU_SOFTRST_CTRL, i);
>>>>>> +
>>>>>> + /* restore all plls and peripheral clocks */
>>>>>> + tegra210_init_pllu();
>>>>>> + clk_restore_context();
>>>>>> +
>>>>>> + fence_udelay(5, clk_base);
>>>>>> +
>>>>>> + /* resume SCLK and CPULP clocks */
>>>>>> + tegra_sclk_cpulp_burst_policy_restore_context();
>>>>>> +
>>>>>> + /*
>>>>>> + * restore CPUG clocks:
>>>>>> + * - enable DFLL in open loop mode
>>>>>> + * - switch CPUG to DFLL clock source
>>>>>> + * - close DFLL loop
>>>>>> + * - sync PLLX state
>>>>>> + */
>>>>>> + if (dfll_pdev)
>>>>>> + tegra_dfll_resume(dfll_pdev, false);
>>>>>> +
>>>>>> + tegra_cclkg_burst_policy_restore_context();
>>>>>> + fence_udelay(2, clk_base);
>>>>>> +
>>>>>> + if (dfll_pdev)
>>>>>> + tegra_dfll_resume(dfll_pdev, true);
>>>>>> +
>>>>>> + parent =
>>>>>> clk_hw_get_parent(__clk_get_hw(clks[TEGRA210_CLK_CCLK_G]));
>>>>>> + clk = clks[TEGRA210_CLK_PLL_X];
>>>>>> + if (parent != __clk_get_hw(clk))
>>>>>> + tegra_clk_sync_state_pll(__clk_get_hw(clk));
>>>>>> +
>>>>>> + /* Disable PLL_OUT_CPU after DFLL resume */
>>>>>> + tegra_clk_set_pllp_out_cpu(false);
>>>>>> +}
>>>>>> +
>>>>>> static void tegra210_cpu_clock_suspend(void)
>>>>>> {
>>>>>> /* switch coresite to clk_m, save off original source */
>>>>>> @@ -3298,6 +3400,11 @@ static void tegra210_cpu_clock_resume(void)
>>>>>> }
>>>>>> #endif
>>>>>> +static struct syscore_ops tegra_clk_syscore_ops = {
>>>>>> + .suspend = tegra210_clk_suspend,
>>>>>> + .resume = tegra210_clk_resume,
>>>>>> +};
>>>>>> +
>>>>>> static struct tegra_cpu_car_ops tegra210_cpu_car_ops = {
>>>>>> .wait_for_reset = tegra210_wait_cpu_in_reset,
>>>>>> .disable_clock = tegra210_disable_cpu_clock,
>>>>>> @@ -3583,5 +3690,7 @@ static void __init tegra210_clock_init(struct
>>>>>> device_node *np)
>>>>>> tegra210_mbist_clk_init();
>>>>>> tegra_cpu_car_ops = &tegra210_cpu_car_ops;
>>>>>> +
>>>>>> + register_syscore_ops(&tegra_clk_syscore_ops);
>>>>>> }
>>>>> Is it really worthwhile to use syscore_ops for suspend/resume given
>>>>> that drivers for
>>>>> won't resume before the CLK driver anyway? Are there any other
>>>>> options
>>>>> for CLK
>>>>> suspend/resume?
>>>>>
>>>>> I'm also not sure whether PM runtime API could be used at all in the
>>>>> context of
>>>>> syscore_ops ..
>>>>>
>>>>> Secondly, what about to use generic clk_save_context() /
>>>>> clk_restore_context()
>>>>> helpers for the suspend-resume? It looks to me that some other
>>>>> essential (and proper)
>>>>> platform driver (soc/tegra/? PMC?) should suspend-resume the clocks
>>>>> using the generic
>>>>> CLK Framework API.
>>>> Clock resume should happen very early to restore peripheral and cpu
>>>> clocks very early than peripheral drivers resume happens.
>>> If all peripheral drivers properly requested all of the necessary
>>> clocks
>>> and CLK driver was a platform driver, then I guess the probe should
>>> have
>>> been naturally ordered. But that's not very achievable with the
>>> currently available infrastructure in the kernel, so I'm not arguing
>>> that the clocks should be explicitly resumed before the users.
>>>
>>>> this patch series uses clk_save_context and clk_restore_context for
>>>> corresponding divider, pll, pllout.. save and restore context.
>>> Now I see that indeed this API is utilized in this patch, thank you for
>>> the clarification.
>>>
>>>> But as there is dependency on dfll resume and cpu and pllx clocks
>>>> restore, couldnt use clk_save_context and clk_restore_context for
>>>> dfll.
>>>>
>>>> So implemented recommended dfll resume sequence in main Tegra210 clock
>>>> driver along with invoking clk_save_context/clk_restore_context where
>>>> all other clocks save/restore happens as per clock tree traversal.
>>> Could you please clarify what part of peripherals clocks is required
>>> for
>>> DFLL's restore? Couldn't DFLL driver be changed to avoid that quirkness
>>> and thus to make DFLL driver suspend/resume the clock?
>>
>> DFLL source ref_clk and soc_clk need to be restored prior to dfll.
>>
>> I see dfllCPU_out parent to CCLK_G first in the clock tree and
>> dfll_ref and dfll_soc peripheral clocks are not resumed by the time
>> dfll resume happens first.
>>
>> ref_clk and soc_clk source is from pll_p and clock tree has these
>> registered under pll_p which happens later.
>>
>> tegra210_clock_init registers in order plls, peripheral clocks,
>> super_clk init for cclk_g during clock driver probe and dfll probe
>> and register happens later.
>>
> One more thing, CLDVFS peripheral clock enable is also needed to be
> enabled to program DFLL Controller and all peripheral clock context is
> restored only after their PLL sources are restored.
>
> DFLL restore involves dfll source clock resume along with CLDVFS
> periheral clock enable and reset
>
Will try with dfll_pm_ops instead of dfll suspend/resume in tegra210
clock driver...
^ permalink raw reply
* [PATCH] arm64: dts: imx8mm: Correct SAI3 RXC/TXFS pin's mux option #1
From: Anson.Huang @ 2019-07-16 3:09 UTC (permalink / raw)
To: robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam,
ping.bai, aisheng.dong, linus.walleij, devicetree,
linux-arm-kernel, linux-kernel
Cc: Linux-imx
From: Anson Huang <Anson.Huang@nxp.com>
According to i.MX8MM reference manual Rev.1, 03/2019:
SAI3_RXC pin's mux option #1 should be GPT1_CLK, NOT GPT1_CAPTURE2;
SAI3_TXFS pin's mux option #1 should be GPT1_CAPTURE2, NOT GPT1_CLK.
Fixes: c1c9d41319c3 ("dt-bindings: imx: Add pinctrl binding doc for imx8mm")
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
arch/arm64/boot/dts/freescale/imx8mm-pinfunc.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/freescale/imx8mm-pinfunc.h b/arch/arm64/boot/dts/freescale/imx8mm-pinfunc.h
index e25f7fc..cffa899 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-pinfunc.h
+++ b/arch/arm64/boot/dts/freescale/imx8mm-pinfunc.h
@@ -462,7 +462,7 @@
#define MX8MM_IOMUXC_SAI3_RXFS_GPIO4_IO28 0x1CC 0x434 0x000 0x5 0x0
#define MX8MM_IOMUXC_SAI3_RXFS_TPSMP_HTRANS0 0x1CC 0x434 0x000 0x7 0x0
#define MX8MM_IOMUXC_SAI3_RXC_SAI3_RX_BCLK 0x1D0 0x438 0x000 0x0 0x0
-#define MX8MM_IOMUXC_SAI3_RXC_GPT1_CAPTURE2 0x1D0 0x438 0x000 0x1 0x0
+#define MX8MM_IOMUXC_SAI3_RXC_GPT1_CLK 0x1D0 0x438 0x000 0x1 0x0
#define MX8MM_IOMUXC_SAI3_RXC_SAI5_RX_BCLK 0x1D0 0x438 0x4D0 0x2 0x2
#define MX8MM_IOMUXC_SAI3_RXC_GPIO4_IO29 0x1D0 0x438 0x000 0x5 0x0
#define MX8MM_IOMUXC_SAI3_RXC_TPSMP_HTRANS1 0x1D0 0x438 0x000 0x7 0x0
@@ -472,7 +472,7 @@
#define MX8MM_IOMUXC_SAI3_RXD_GPIO4_IO30 0x1D4 0x43C 0x000 0x5 0x0
#define MX8MM_IOMUXC_SAI3_RXD_TPSMP_HDATA0 0x1D4 0x43C 0x000 0x7 0x0
#define MX8MM_IOMUXC_SAI3_TXFS_SAI3_TX_SYNC 0x1D8 0x440 0x000 0x0 0x0
-#define MX8MM_IOMUXC_SAI3_TXFS_GPT1_CLK 0x1D8 0x440 0x000 0x1 0x0
+#define MX8MM_IOMUXC_SAI3_TXFS_GPT1_CAPTURE2 0x1D8 0x440 0x000 0x1 0x0
#define MX8MM_IOMUXC_SAI3_TXFS_SAI5_RX_DATA1 0x1D8 0x440 0x4D8 0x2 0x2
#define MX8MM_IOMUXC_SAI3_TXFS_GPIO4_IO31 0x1D8 0x440 0x000 0x5 0x0
#define MX8MM_IOMUXC_SAI3_TXFS_TPSMP_HDATA1 0x1D8 0x440 0x000 0x7 0x0
--
2.7.4
^ permalink raw reply related
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-16 3:00 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <3938092a-bbc7-b304-641d-31677539598d@nvidia.com>
On 7/15/19 5:35 PM, Sowjanya Komatineni wrote:
>
> On 7/14/19 2:41 PM, Dmitry Osipenko wrote:
>> 13.07.2019 8:54, Sowjanya Komatineni пишет:
>>> On 6/29/19 8:10 AM, Dmitry Osipenko wrote:
>>>> 28.06.2019 5:12, Sowjanya Komatineni пишет:
>>>>> This patch adds system suspend and resume support for Tegra210
>>>>> clocks.
>>>>>
>>>>> All the CAR controller settings are lost on suspend when core power
>>>>> goes off.
>>>>>
>>>>> This patch has implementation for saving and restoring all the PLLs
>>>>> and clocks context during system suspend and resume to have the
>>>>> clocks back to same state for normal operation.
>>>>>
>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>> ---
>>>>> drivers/clk/tegra/clk-tegra210.c | 115
>>>>> ++++++++++++++++++++++++++++++++++++++-
>>>>> drivers/clk/tegra/clk.c | 14 +++++
>>>>> drivers/clk/tegra/clk.h | 1 +
>>>>> 3 files changed, 127 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/clk/tegra/clk-tegra210.c
>>>>> b/drivers/clk/tegra/clk-tegra210.c
>>>>> index 1c08c53482a5..1b839544e086 100644
>>>>> --- a/drivers/clk/tegra/clk-tegra210.c
>>>>> +++ b/drivers/clk/tegra/clk-tegra210.c
>>>>> @@ -9,10 +9,12 @@
>>>>> #include <linux/clkdev.h>
>>>>> #include <linux/of.h>
>>>>> #include <linux/of_address.h>
>>>>> +#include <linux/of_platform.h>
>>>>> #include <linux/delay.h>
>>>>> #include <linux/export.h>
>>>>> #include <linux/mutex.h>
>>>>> #include <linux/clk/tegra.h>
>>>>> +#include <linux/syscore_ops.h>
>>>>> #include <dt-bindings/clock/tegra210-car.h>
>>>>> #include <dt-bindings/reset/tegra210-car.h>
>>>>> #include <linux/iopoll.h>
>>>>> @@ -20,6 +22,7 @@
>>>>> #include <soc/tegra/pmc.h>
>>>>> #include "clk.h"
>>>>> +#include "clk-dfll.h"
>>>>> #include "clk-id.h"
>>>>> /*
>>>>> @@ -225,6 +228,7 @@
>>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_SET 0x2a8
>>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_CLR 0x2ac
>>>>> +#define CPU_SOFTRST_CTRL 0x380
>>>>> #define LVL2_CLK_GATE_OVRA 0xf8
>>>>> #define LVL2_CLK_GATE_OVRC 0x3a0
>>>>> @@ -2820,6 +2824,7 @@ static int tegra210_enable_pllu(void)
>>>>> struct tegra_clk_pll_freq_table *fentry;
>>>>> struct tegra_clk_pll pllu;
>>>>> u32 reg;
>>>>> + int ret;
>>>>> for (fentry = pll_u_freq_table; fentry->input_rate;
>>>>> fentry++) {
>>>>> if (fentry->input_rate == pll_ref_freq)
>>>>> @@ -2847,10 +2852,10 @@ static int tegra210_enable_pllu(void)
>>>>> fence_udelay(1, clk_base);
>>>>> reg |= PLL_ENABLE;
>>>>> writel(reg, clk_base + PLLU_BASE);
>>>>> + fence_udelay(1, clk_base);
>>>>> - readl_relaxed_poll_timeout_atomic(clk_base + PLLU_BASE, reg,
>>>>> - reg & PLL_BASE_LOCK, 2, 1000);
>>>>> - if (!(reg & PLL_BASE_LOCK)) {
>>>>> + ret = tegra210_wait_for_mask(&pllu, PLLU_BASE, PLL_BASE_LOCK);
>>>>> + if (ret) {
>>>>> pr_err("Timed out waiting for PLL_U to lock\n");
>>>>> return -ETIMEDOUT;
>>>>> }
>>>>> @@ -3283,6 +3288,103 @@ static void tegra210_disable_cpu_clock(u32
>>>>> cpu)
>>>>> }
>>>>> #ifdef CONFIG_PM_SLEEP
>>>>> +static u32 cpu_softrst_ctx[3];
>>>>> +static struct platform_device *dfll_pdev;
>>>>> +#define car_readl(_base, _off) readl_relaxed(clk_base + (_base) +
>>>>> ((_off) * 4))
>>>>> +#define car_writel(_val, _base, _off) \
>>>>> + writel_relaxed(_val, clk_base + (_base) + ((_off) * 4))
>>>>> +
>>>>> +static int tegra210_clk_suspend(void)
>>>>> +{
>>>>> + unsigned int i;
>>>>> + struct device_node *node;
>>>>> +
>>>>> + tegra_cclkg_burst_policy_save_context();
>>>>> +
>>>>> + if (!dfll_pdev) {
>>>>> + node = of_find_compatible_node(NULL, NULL,
>>>>> + "nvidia,tegra210-dfll");
>>>>> + if (node)
>>>>> + dfll_pdev = of_find_device_by_node(node);
>>>>> +
>>>>> + of_node_put(node);
>>>>> + if (!dfll_pdev)
>>>>> + pr_err("dfll node not found. no suspend for dfll\n");
>>>>> + }
>>>>> +
>>>>> + if (dfll_pdev)
>>>>> + tegra_dfll_suspend(dfll_pdev);
>>>>> +
>>>>> + /* Enable PLLP_OUT_CPU after dfll suspend */
>>>>> + tegra_clk_set_pllp_out_cpu(true);
>>>>> +
>>>>> + tegra_sclk_cclklp_burst_policy_save_context();
>>>>> +
>>>>> + clk_save_context();
>>>>> +
>>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>>>> + cpu_softrst_ctx[i] = car_readl(CPU_SOFTRST_CTRL, i);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static void tegra210_clk_resume(void)
>>>>> +{
>>>>> + unsigned int i;
>>>>> + struct clk_hw *parent;
>>>>> + struct clk *clk;
>>>>> +
>>>>> + /*
>>>>> + * clk_restore_context restores clocks as per the clock tree.
>>>>> + *
>>>>> + * dfllCPU_out is first in the clock tree to get restored and it
>>>>> + * involves programming DFLL controller along with restoring
>>>>> CPUG
>>>>> + * clock burst policy.
>>>>> + *
>>>>> + * DFLL programming needs dfll_ref and dfll_soc peripheral
>>>>> clocks
>>>>> + * to be restores which are part ofthe peripheral clocks.
>> ^ white-space
>>
>> Please use spellchecker to avoid typos.
>>
>>>>> + * So, peripheral clocks restore should happen prior to dfll
>>>>> clock
>>>>> + * restore.
>>>>> + */
>>>>> +
>>>>> + tegra_clk_osc_resume(clk_base);
>>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>>>> + car_writel(cpu_softrst_ctx[i], CPU_SOFTRST_CTRL, i);
>>>>> +
>>>>> + /* restore all plls and peripheral clocks */
>>>>> + tegra210_init_pllu();
>>>>> + clk_restore_context();
>>>>> +
>>>>> + fence_udelay(5, clk_base);
>>>>> +
>>>>> + /* resume SCLK and CPULP clocks */
>>>>> + tegra_sclk_cpulp_burst_policy_restore_context();
>>>>> +
>>>>> + /*
>>>>> + * restore CPUG clocks:
>>>>> + * - enable DFLL in open loop mode
>>>>> + * - switch CPUG to DFLL clock source
>>>>> + * - close DFLL loop
>>>>> + * - sync PLLX state
>>>>> + */
>>>>> + if (dfll_pdev)
>>>>> + tegra_dfll_resume(dfll_pdev, false);
>>>>> +
>>>>> + tegra_cclkg_burst_policy_restore_context();
>>>>> + fence_udelay(2, clk_base);
>>>>> +
>>>>> + if (dfll_pdev)
>>>>> + tegra_dfll_resume(dfll_pdev, true);
>>>>> +
>>>>> + parent =
>>>>> clk_hw_get_parent(__clk_get_hw(clks[TEGRA210_CLK_CCLK_G]));
>>>>> + clk = clks[TEGRA210_CLK_PLL_X];
>>>>> + if (parent != __clk_get_hw(clk))
>>>>> + tegra_clk_sync_state_pll(__clk_get_hw(clk));
>>>>> +
>>>>> + /* Disable PLL_OUT_CPU after DFLL resume */
>>>>> + tegra_clk_set_pllp_out_cpu(false);
>>>>> +}
>>>>> +
>>>>> static void tegra210_cpu_clock_suspend(void)
>>>>> {
>>>>> /* switch coresite to clk_m, save off original source */
>>>>> @@ -3298,6 +3400,11 @@ static void tegra210_cpu_clock_resume(void)
>>>>> }
>>>>> #endif
>>>>> +static struct syscore_ops tegra_clk_syscore_ops = {
>>>>> + .suspend = tegra210_clk_suspend,
>>>>> + .resume = tegra210_clk_resume,
>>>>> +};
>>>>> +
>>>>> static struct tegra_cpu_car_ops tegra210_cpu_car_ops = {
>>>>> .wait_for_reset = tegra210_wait_cpu_in_reset,
>>>>> .disable_clock = tegra210_disable_cpu_clock,
>>>>> @@ -3583,5 +3690,7 @@ static void __init tegra210_clock_init(struct
>>>>> device_node *np)
>>>>> tegra210_mbist_clk_init();
>>>>> tegra_cpu_car_ops = &tegra210_cpu_car_ops;
>>>>> +
>>>>> + register_syscore_ops(&tegra_clk_syscore_ops);
>>>>> }
>>>> Is it really worthwhile to use syscore_ops for suspend/resume given
>>>> that drivers for
>>>> won't resume before the CLK driver anyway? Are there any other options
>>>> for CLK
>>>> suspend/resume?
>>>>
>>>> I'm also not sure whether PM runtime API could be used at all in the
>>>> context of
>>>> syscore_ops ..
>>>>
>>>> Secondly, what about to use generic clk_save_context() /
>>>> clk_restore_context()
>>>> helpers for the suspend-resume? It looks to me that some other
>>>> essential (and proper)
>>>> platform driver (soc/tegra/? PMC?) should suspend-resume the clocks
>>>> using the generic
>>>> CLK Framework API.
>>> Clock resume should happen very early to restore peripheral and cpu
>>> clocks very early than peripheral drivers resume happens.
>> If all peripheral drivers properly requested all of the necessary clocks
>> and CLK driver was a platform driver, then I guess the probe should have
>> been naturally ordered. But that's not very achievable with the
>> currently available infrastructure in the kernel, so I'm not arguing
>> that the clocks should be explicitly resumed before the users.
>>
>>> this patch series uses clk_save_context and clk_restore_context for
>>> corresponding divider, pll, pllout.. save and restore context.
>> Now I see that indeed this API is utilized in this patch, thank you for
>> the clarification.
>>
>>> But as there is dependency on dfll resume and cpu and pllx clocks
>>> restore, couldnt use clk_save_context and clk_restore_context for dfll.
>>>
>>> So implemented recommended dfll resume sequence in main Tegra210 clock
>>> driver along with invoking clk_save_context/clk_restore_context where
>>> all other clocks save/restore happens as per clock tree traversal.
>> Could you please clarify what part of peripherals clocks is required for
>> DFLL's restore? Couldn't DFLL driver be changed to avoid that quirkness
>> and thus to make DFLL driver suspend/resume the clock?
>
> DFLL source ref_clk and soc_clk need to be restored prior to dfll.
>
> I see dfllCPU_out parent to CCLK_G first in the clock tree and
> dfll_ref and dfll_soc peripheral clocks are not resumed by the time
> dfll resume happens first.
>
> ref_clk and soc_clk source is from pll_p and clock tree has these
> registered under pll_p which happens later.
>
> tegra210_clock_init registers in order plls, peripheral clocks,
> super_clk init for cclk_g during clock driver probe and dfll probe and
> register happens later.
>
One more thing, CLDVFS peripheral clock enable is also needed to be
enabled to program DFLL Controller and all peripheral clock context is
restored only after their PLL sources are restored.
DFLL restore involves dfll source clock resume along with CLDVFS
periheral clock enable and reset
^ permalink raw reply
* Re: [PATCH v3 2/4] of/platform: Add functional dependency link from DT bindings
From: Frank Rowand @ 2019-07-16 1:05 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel@vger.kernel.org, David Collins, Android Kernel Team
In-Reply-To: <CAGETcx8YCCGxgXnByenVUb+q8pHPPTjwAjK3L_+9mwoCe=9SbA@mail.gmail.com>
On 7/15/19 11:40 AM, Saravana Kannan wrote:
> Replying again because the previous email accidentally included HTML.
>
> Thanks for taking the time to reconsider the wording Frank. Your
> intention was clear to me in the first email too.
>
> A kernel command line option can also completely disable this
> functionality easily and cleanly. Can we pick that as an option? I've
> an implementation of that in the v5 series I sent out last week.
Yes, Rob suggested a command line option for debugging, and I am fine with
that. But even with that, I would like a lot of testing so that we have a
chance of finding systems that have trouble with the changes and could
potentially be fixed before impacting a large number of users.
-Frank
>
> -Saravana
>
> On Mon, Jul 15, 2019 at 7:39 AM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 7/15/19 7:26 AM, Frank Rowand wrote:
>>> HiRob,
>>>
>>> Sorry for such a late reply...
>>>
>>>
>>> On 7/1/19 8:25 PM, Saravana Kannan wrote:
>>>> On Mon, Jul 1, 2019 at 6:32 PM Rob Herring <robh+dt@kernel.org> wrote:
>>>>>
>>>>> On Mon, Jul 1, 2019 at 6:48 PM Saravana Kannan <saravanak@google.com> wrote:
>>>>>>
>>>>>> Add device-links after the devices are created (but before they are
>>>>>> probed) by looking at common DT bindings like clocks and
>>>>>> interconnects.
>>>>>>
>>>>>> Automatically adding device-links for functional dependencies at the
>>>>>> framework level provides the following benefits:
>>>>>>
>>>>>> - Optimizes device probe order and avoids the useless work of
>>>>>> attempting probes of devices that will not probe successfully
>>>>>> (because their suppliers aren't present or haven't probed yet).
>>>>>>
>>>>>> For example, in a commonly available mobile SoC, registering just
>>>>>> one consumer device's driver at an initcall level earlier than the
>>>>>> supplier device's driver causes 11 failed probe attempts before the
>>>>>> consumer device probes successfully. This was with a kernel with all
>>>>>> the drivers statically compiled in. This problem gets a lot worse if
>>>>>> all the drivers are loaded as modules without direct symbol
>>>>>> dependencies.
>>>>>>
>>>>>> - Supplier devices like clock providers, interconnect providers, etc
>>>>>> need to keep the resources they provide active and at a particular
>>>>>> state(s) during boot up even if their current set of consumers don't
>>>>>> request the resource to be active. This is because the rest of the
>>>>>> consumers might not have probed yet and turning off the resource
>>>>>> before all the consumers have probed could lead to a hang or
>>>>>> undesired user experience.
>>>>>>
>>>>>> Some frameworks (Eg: regulator) handle this today by turning off
>>>>>> "unused" resources at late_initcall_sync and hoping all the devices
>>>>>> have probed by then. This is not a valid assumption for systems with
>>>>>> loadable modules. Other frameworks (Eg: clock) just don't handle
>>>>>> this due to the lack of a clear signal for when they can turn off
>>>>>> resources. This leads to downstream hacks to handle cases like this
>>>>>> that can easily be solved in the upstream kernel.
>>>>>>
>>>>>> By linking devices before they are probed, we give suppliers a clear
>>>>>> count of the number of dependent consumers. Once all of the
>>>>>> consumers are active, the suppliers can turn off the unused
>>>>>> resources without making assumptions about the number of consumers.
>>>>>>
>>>>>> By default we just add device-links to track "driver presence" (probe
>>>>>> succeeded) of the supplier device. If any other functionality provided
>>>>>> by device-links are needed, it is left to the consumer/supplier
>>>>>> devices to change the link when they probe.
>>>>>>
>>>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>>>>> ---
>>>>>> drivers/of/Kconfig | 9 ++++++++
>>>>>> drivers/of/platform.c | 52 +++++++++++++++++++++++++++++++++++++++++++
>>>>>> 2 files changed, 61 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>>>>>> index 37c2ccbefecd..7c7fa7394b4c 100644
>>>>>> --- a/drivers/of/Kconfig
>>>>>> +++ b/drivers/of/Kconfig
>>>>>> @@ -103,4 +103,13 @@ config OF_OVERLAY
>>>>>> config OF_NUMA
>>>>>> bool
>>>>>>
>>>>>> +config OF_DEVLINKS
>>>>>
>>>>> I'd prefer this not be a config option. After all, we want one kernel
>>>>> build that works for all platforms.
>>>>
>>>> We need a lot more changes before one kernel build can work for all
>>>> platforms. At least until then, I think we need this. Lot less chance
>>>> of breaking existing platforms before all the missing pieces are
>>>> created.
>>>>
>>>>> A kernel command line option to disable might be useful for debugging.
>>>>
>>>> Or we can have a command line to enable this for platforms that want
>>>> to use it and have it default off.
>>>
>>
>>> Given the fragility of the current boot sequence (without this patch set)
>>> and the potential breakage of existing systems, I think that if we choose
>>> to accept this patch set that it should first bake in the -next tree for
>>> at least one major release cycle. Maybe even two major release cycles.
>>
>> I probably didn't state that very well. I was trying to not sound like
>> I was accusing this patch series of being fragile. The issue is that
>> adding the patches to systems that weren't expecting the new ordering
>> may cause boot problems for some systems. I'm not concerned with
>> pointing fingers, just want to make sure that we proceed cautiously
>> until we know that the resulting system is robust.
>>
>> -Frank
>>
>>>
>>> -Frank
>>>
>>>
>>>>
>>>>>> + bool "Device links from DT bindings"
>>>>>> + help
>>>>>> + Common DT bindings like clocks, interconnects, etc represent a
>>>>>> + consumer device's dependency on suppliers devices. This option
>>>>>> + creates device links from these common bindings so that consumers are
>>>>>> + probed only after all their suppliers are active and suppliers can
>>>>>> + tell when all their consumers are active.
>>>>>> +
>>>>>> endif # OF
>>>>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>>>>> index 04ad312fd85b..a53717168aca 100644
>>>>>> --- a/drivers/of/platform.c
>>>>>> +++ b/drivers/of/platform.c
>>>>>> @@ -61,6 +61,57 @@ struct platform_device *of_find_device_by_node(struct device_node *np)
>>>>>> EXPORT_SYMBOL(of_find_device_by_node);
>>>>>>
>>>>>> #ifdef CONFIG_OF_ADDRESS
>>>>>> +static int of_link_binding(struct device *dev, char *binding, char *cell)
>>>>>
>>>>> Under CONFIG_OF_ADDRESS seems like a strange location.
>>>>
>>>> Yeah, but the rest of the file seems to be under this. So I'm not
>>>> touching that. I can probably move this function further down (close
>>>> to platform populate) if you want that.
>>>>>
>>>>>> +{
>>>>>> + struct of_phandle_args sup_args;
>>>>>> + struct platform_device *sup_dev;
>>>>>> + unsigned int i = 0, links = 0;
>>>>>> + u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
>>>>>> +
>>>>>> + while (!of_parse_phandle_with_args(dev->of_node, binding, cell, i,
>>>>>> + &sup_args)) {
>>>>>> + i++;
>>>>>> + sup_dev = of_find_device_by_node(sup_args.np);
>>>>>> + if (!sup_dev)
>>>>>> + continue;
>>>>>> + if (device_link_add(dev, &sup_dev->dev, dl_flags))
>>>>>> + links++;
>>>>>> + put_device(&sup_dev->dev);
>>>>>> + }
>>>>>> + if (links < i)
>>>>>> + return -ENODEV;
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * List of bindings and their cell names (use NULL if no cell names) from which
>>>>>> + * device links need to be created.
>>>>>> + */
>>>>>> +static char *link_bindings[] = {
>>>>>
>>>>> const
>>>>
>>>> Ack
>>>>
>>>>>
>>>>>> +#ifdef CONFIG_OF_DEVLINKS
>>>>>> + "clocks", "#clock-cells",
>>>>>> + "interconnects", "#interconnect-cells",
>>>>>
>>>>> Planning to add others?
>>>>
>>>> Not in this patch.
>>>>
>>>> Regulators are the other big missing piece that I'm aware of now but
>>>> they need a lot of discussion (see email from David and my reply).
>>>>
>>>> Not sure what other resources are shared where they can be "turned
>>>> off" and cause devices set up at boot to fail. For example, I don't
>>>> think interrupts need functional dependency tracking because they
>>>> aren't really turned off by consumer 1 in a way that breaks things for
>>>> consumer 2. Just masked and the consumer 2 can unmask and use it once
>>>> it probes.
>>>>
>>>> I'm only intimately familiar with clocks, interconnects and regulators
>>>> (to some extent). I'm open to adding other supplier categories in
>>>> future patches as I educate myself of those or if other people want to
>>>> add support for more categories.
>>>>
>>>> -Saravana
>>>>
>>>>>> +#endif
>>>>>> +};
>>>>>> +
>>>>>> +static int of_link_to_suppliers(struct device *dev)
>>>>>> +{
>>>>>> + unsigned int i = 0;
>>>>>> + bool done = true;
>>>>>> +
>>>>>> + if (unlikely(!dev->of_node))
>>>>>> + return 0;
>>>>>> +
>>>>>> + for (i = 0; i < ARRAY_SIZE(link_bindings) / 2; i++)
>>>>>> + if (of_link_binding(dev, link_bindings[i * 2],
>>>>>> + link_bindings[i * 2 + 1]))
>>>>>> + done = false;
>>>>>> +
>>>>>> + if (!done)
>>>>>> + return -ENODEV;
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> /*
>>>>>> * The following routines scan a subtree and registers a device for
>>>>>> * each applicable node.
>>>>>> @@ -524,6 +575,7 @@ static int __init of_platform_default_populate_init(void)
>>>>>> if (!of_have_populated_dt())
>>>>>> return -ENODEV;
>>>>>>
>>>>>> + platform_bus_type.add_links = of_link_to_suppliers;
>>>>>> /*
>>>>>> * Handle certain compatibles explicitly, since we don't want to create
>>>>>> * platform_devices for every node in /reserved-memory with a
>>>>>> --
>>>>>> 2.22.0.410.gd8fdbe21b5-goog
>>>>>>
>>>>
>>>
>>>
>>
>
^ permalink raw reply
* Re: [PATCH v3 6/6] interconnect: Add OPP table support for interconnects
From: Saravana Kannan @ 2019-07-16 0:55 UTC (permalink / raw)
To: Vincent Guittot
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, Sweeney, Sean,
daidavid1, Rajendra Nayak, sibis, Bjorn Andersson, Evan Green,
Android Kernel Team, open list:THERMAL,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel
In-Reply-To: <CAKfTPtBE7e+hc55TY43JC0XPONvrS4FBPkZcRZ4EbzyCJKNhfg@mail.gmail.com>
On Mon, Jul 15, 2019 at 1:16 AM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Tue, 9 Jul 2019 at 21:03, Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Tue, Jul 9, 2019 at 12:25 AM Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> > >
> > > On Sun, 7 Jul 2019 at 23:48, Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > On Thu, Jul 4, 2019 at 12:12 AM Vincent Guittot
> > > > <vincent.guittot@linaro.org> wrote:
> > > > >
> > > > > On Wed, 3 Jul 2019 at 23:33, Saravana Kannan <saravanak@google.com> wrote:
> > > > > >
> > > > > > On Tue, Jul 2, 2019 at 11:45 PM Vincent Guittot
> > > > > > <vincent.guittot@linaro.org> wrote:
> > > > > > >
> > > > > > > On Wed, 3 Jul 2019 at 03:10, Saravana Kannan <saravanak@google.com> wrote:
> > > > > > > >
> > > > > > > > Interconnect paths can have different performance points. Now that OPP
> > > > > > > > framework supports bandwidth OPP tables, add OPP table support for
> > > > > > > > interconnects.
> > > > > > > >
> > > > > > > > Devices can use the interconnect-opp-table DT property to specify OPP
> > > > > > > > tables for interconnect paths. And the driver can obtain the OPP table for
> > > > > > > > an interconnect path by calling icc_get_opp_table().
> > > > > > >
> > > > > > > The opp table of a path must come from the aggregation of OPP tables
> > > > > > > of the interconnect providers.
> > > > > >
> > > > > > The aggregation of OPP tables of the providers is certainly the
> > > > > > superset of what a path can achieve, but to say that OPPs for
> > > > > > interconnect path should match that superset is an oversimplification
> > > > > > of the reality in hardware.
> > > > > >
> > > > > > There are lots of reasons an interconnect path might not want to use
> > > > > > all the available bandwidth options across all the interconnects in
> > > > > > the route.
> > > > > >
> > > > > > 1. That particular path might not have been validated or verified
> > > > > > during the HW design process for some of the frequencies/bandwidth
> > > > > > combinations of the providers.
> > > > >
> > > > > All these constraint are provider's constraints and not consumer's one
> > > > >
> > > > > The consumer asks for a bandwidth according to its needs and then the
> > > > > providers select the optimal bandwidth of each interconnect after
> > > > > aggregating all the request and according to what OPP have been
> > > > > validated
> > > >
> > > > Not really. The screening can be a consumer specific issue. The
> > > > consumer IP itself might have some issue with using too low of a
> > > > bandwidth or bandwidth that's not within some range. It should not be
> > >
> > > How can an IP ask for not enough bandwidth ?
> > > It asks the needed bandwidth based on its requirements
> >
> > The "enough bandwidth" is not always obvious. It's only for very
> > simple cases that you can calculate the required bandwidth. Even for
> > cases that you think might be "obvious/easy" aren't always easy.
> >
> > For example, you'd think a display IP would have a fixed bandwidth
> > requirement for a fixed resolution screen. But that's far from the
> > truth. It can also change as the number of layers change per frame.
> > For video decoder/encoder, it depends on how well the frames compress
> > with a specific compression scheme.
> > So the "required" bandwidth is often a heuristic based on the IP
> > frequency or traffic measurement.
> >
> > But that's not even the point I was making in this specific "bullet".
> >
> > A hardware IP might be screen/verified with only certain bandwidth
> > levels. Or it might have hardware bugs that prevent it from using
> > lower bandwidths even though it's technically sufficient. We need a
> > way to capture that per path. This is not even a fictional case. This
> > has been true multiple times over widely used IPs.
>
> here you are mixing HW constraint on the soc and OPP screening with
> bandwidth request from consumer
> ICC framework is about getting bandwidth request not trying to fix
> some HW/voltage dependency of the SoC
>
> >
> > > > the provider's job to take into account all the IP that might be
> > > > connected to the interconnects. If the interconnect HW itself didn't
> > >
> > > That's not what I'm saying. The provider knows which bandwidth the
> > > interconnect can provide as it is the ones which configures it. So if
> > > the interconnect has a finite number of bandwidth point based probably
> > > on the possible clock frequency and others config of the interconnect,
> > > it selects the best final config after aggregating the request of the
> > > consumer.
> >
> > I completely agree with this. What you are stating above is how it
> > should work and that's the whole point of the interconnect framework.
> >
> > But this is orthogonal to the point I'm making.
>
> It's not orthogonal because you want to add a OPP table pointer in the
> ICC path structure to fix your platform HW constraint whereas it's not
> the purpose of the framework IMO
>
> >
> > > > change, the provider driver shouldn't need to change. By your
> > > > definition, a provider driver will have to account for all the
> > > > possible bus masters that might be connected to it across all SoCs.
> > >
> > > you didn't catch my point
> >
> > Same. I think we are talking over each other. Let me try again.
> >
> > You are trying to describe how and interconnect provider and framework
> > should work. There's no disagreement there.
> >
> > My point is that consumers might not want to or can not always use all
> > the available bandwidth levels offered by the providers. There can be
> > many reasons for that (which is what I listed in my earlier emails)
> > and we need a good and generic way to capture that so that everyone
> > isn't trying to invent their own property.
>
> And my point is that you want to describe some platform or even UCs
> specific constraint in the ICC framework which is not the place to do.
>
> If the consumers might not want to use all available bandwidth because
> this is not power efficient as an example, this should be describe
> somewhere else to express that there is a shared power domain
> between some devices and we shoudl ensure that all devices in this
> power domain should use the Optimal Operating Point (optimal freq for
> a voltage)
My patch series has nothing to do with shared power domains. I think
the examples have made it amply clear.
> ICC framework describes the bandwidth request that are expressed by
> the consumers for the current running state of their IP but it doesn't
> reflect the fact that on platform A, the consumer should use bandwidth
> X because it will select a voltage level of a shared power domain that
> is optimized for the other devices B, C ... . It's up to the provider
> to know HW details of the bus that it drives and to make such
> decision; the consumer should always request the same
The change to ICC framework is practically just this. I don't have any
future changes planned for the ICC framework. This is the entirety of
it.
+ opp_node = of_parse_phandle(np, "interconnect-opp-table", idx);
+ if (opp_node) {
+ path->opp_table = dev_pm_opp_of_find_table_from_node(opp_node);
+ of_node_put(opp_node);
+ }
It's quite a stretch and bit hyperbolic to say this one change is
getting ICC framework to do all the things you claim above.
It's literally a simple helper function so that the consumer doesn't
have to make assumptions about indices and it's a bit more explicit
about which OPP table of the device (a device can have multiple OPP
tables) corresponds to which ICC path.
Going by your extreme argument, one can also claim that it's not the
ICC framework's job to make it easy for consumers to figure out the
source/destination endpoints or give them names and delete the
interconnect and interconnect-names properties. That's clearly just as
absurd a claim.
-Saravana
> > > > That's not good design nor is it scalable.
> > > >
> > > > > >
> > > > > > 2. Similarly during parts screening in the factory, some of the
> > > > > > combinations might not have been screened and can't be guaranteed
> > > > > > to work.
> > > > >
> > > > > As above, it's the provider's job to select the final bandwidth
> > > > > according to its constraint
> > > >
> > > > Same reply as above.
> > > >
> > > > > >
> > > > > > 3. Only a certain set of bandwidth levels might make sense to use from
> > > > > > a power/performance balance given the device using it. For example:
> > > > > > - The big CPU might not want to use some of the lower bandwidths
> > > > > > but the little CPU might want to.
> > > > > > - The big CPU might not want to use some intermediate bandwidth
> > > > > > points if they don't save a lot of power compared to a higher
> > > > > > bandwidth levels, but the little CPU might want to.
> > > > > > - The little CPU might never want to use the higher set of
> > > > > > bandwidth levels since they won't be power efficient for the use
> > > > > > cases that might run on it.
> > > > >
> > > > > These example are quite vague about the reasons why little might never
> > > > > want to use higher bandwidth.
> > > >
> > > > How is it vague? I just said because of power/performance balance.
> > > >
> > > > > But then, if little doesn't ask high bandwidth it will not use them.
> > > >
> > > > If you are running a heuristics based algorithm to pick bandwidth,
> > > > this is how it'll know NOT to use some of the bandwidth levels.
> > >
> > > so you want to set a bandwidth according to the cpu frequency which is
> > > what has been proposed in other thread
> >
> > Nope, that's just one heuristic. Often times it's based on hardware
> > monitors measuring interconnect activity. If you go look at the SDM845
> > in a Pixel 3, almost nothing is directly tied to the CPU frequency.
> >
> > Even if you are scaling bandwidth based on other hardware
> > measurements, you might want to avoid some bandwidth level provided by
> > the interconnect providers because it's suboptimal.
> >
> > For example, when making bandwidth votes to accommodate the big CPUs,
> > you might never want to use some of the lower bandwidth levels because
> > they are not power efficient for any CPU frequency or any bandwidth
> > level. Because at those levels the memory/interconnect is so slow that
> > it has a non-trivial utilization increase (because the CPU is
> > stalling) of the big CPUs.
> >
> > Again, this is completely different from what the providers/icc
> > framework does. Which is, once the request is made, they aggregate and
> > set the actual interconnect frequencies correctly.
> >
> > > >
> > > > > >
> > > > > > 4. It might not make sense from a system level power perspective.
> > > > > > Let's take an example of a path S (source) -> A -> B -> C -> D
> > > > > > (destination).
> > > > > > - A supports only 2, 5, 7 and 10 GB/s. B supports 1, 2 ... 10 GB/s.
> > > > > > C supports 5 and 10 GB/s
> > > > > > - If you combine and list the superset of bandwidth levels
> > > > > > supported in that path, that'd be 1, 2, 3, ... 10 GB/s.
> > > > > > - Which set of bandwidth levels make sense will depend on the
> > > > > > hardware characteristics of the interconnects.
> > > > > > - If B is the biggest power sink, then you might want to use all 10
> > > > > > levels.
> > > > > > - If A is the biggest power sink, then you might want to use all 2,
> > > > > > 5 and 10 GB/s of the levels.
> > > > > > - If C is the biggest power sink then you might only want to use 5
> > > > > > and 10 GB/s
> > > > > > - The more hops and paths you get the more convoluted this gets.
> > > > > >
> > > > > > 5. The design of the interconnects themselves might have an impact on
> > > > > > which bandwidth levels are used.
> > > > > > - For example, the FIFO depth between two specific interconnects
> > > > > > might affect the valid bandwidth levels for a specific path.
> > > > > > - Say S1 -> A -> B -> D1, S2 -> C -> B -> D1 and S2 -> C -> D2 are
> > > > > > three paths.
> > > > > > - If C <-> B FIFO depth is small, then there might be a requirement
> > > > > > that C and B be closely performance matched to avoid system level
> > > > > > congestion due to back pressure.
> > > > > > - So S2 -> D1 path can't use all the bandwidth levels supported by
> > > > > > C-B combination.
> > > > > > - But S2 -> D2 can use all the bandwidth levels supported by C.
> > > > > > - And S1 -> D1 can use all the levels supported by A-B combination.
> > > > > >
> > > > >
> > > > > All the examples above makes sense but have to be handle by the
> > > > > provider not the consumer. The consumer asks for a bandwidth according
> > > > > to its constraints. Then the provider which is the driver that manages
> > > > > the interconnect IP, should manage all this hardware and platform
> > > > > specific stuff related to the interconnect IP in order to set the
> > > > > optimal bandwidth that fit both consumer constraint and platform
> > > > > specific configuration.
> > > >
> > > > Sure, but the provider itself can have interconnect properties to
> > > > indicate which other interconnects it's tied to. And the provider will
> > > > still need the interconnect-opp-table to denote which bandwidth levels
> > > > are sensible to use with each of its connections.
> >
> > You seem to have missed this comment.
> >
> > Thanks,
> > Saravana
> >
> > > > So in some instances the interconnect-opp-table covers the needs of
> > > > purely consumers and in some instances purely providers. But in either
> > > > case, it's still needed to describe the hardware properly.
> > > >
> > > > -Saravana
> > > >
> > > > > > These are just some of the reasons I could recollect in a few minutes.
> > > > > > These are all real world cases I had to deal with in the past several
> > > > > > years of dealing with scaling interconnects. I'm sure vendors and SoCs
> > > > > > I'm not familiar with have other good reasons I'm not aware of.
> > > > > >
> > > > > > Trying to figure this all out by aggregating OPP tables of
> > > > > > interconnect providers just isn't feasible nor is it efficient. The
> > > > > > OPP tables for an interconnect path is describing the valid BW levels
> > > > > > supported by that path and verified in hardware and makes a lot of
> > > > > > sense to capture it clearly in DT.
> > > > > >
> > > > > > > So such kind of OPP table should be at
> > > > > > > provider level but not at path level.
> > > > > >
> > > > > > They can also use it if they want to, but they'll probably want to use
> > > > > > a frequency OPP table.
> > > > > >
> > > > > >
> > > > > > -Saravana
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > > > > ---
> > > > > > > > drivers/interconnect/core.c | 27 ++++++++++++++++++++++++++-
> > > > > > > > include/linux/interconnect.h | 7 +++++++
> > > > > > > > 2 files changed, 33 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> > > > > > > > index 871eb4bc4efc..881bac80bc1e 100644
> > > > > > > > --- a/drivers/interconnect/core.c
> > > > > > > > +++ b/drivers/interconnect/core.c
> > > > > > > > @@ -47,6 +47,7 @@ struct icc_req {
> > > > > > > > */
> > > > > > > > struct icc_path {
> > > > > > > > size_t num_nodes;
> > > > > > > > + struct opp_table *opp_table;
> > > > > > > > struct icc_req reqs[];
> > > > > > > > };
> > > > > > > >
> > > > > > > > @@ -313,7 +314,7 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
> > > > > > > > {
> > > > > > > > struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
> > > > > > > > struct icc_node *src_node, *dst_node;
> > > > > > > > - struct device_node *np = NULL;
> > > > > > > > + struct device_node *np = NULL, *opp_node;
> > > > > > > > struct of_phandle_args src_args, dst_args;
> > > > > > > > int idx = 0;
> > > > > > > > int ret;
> > > > > > > > @@ -381,10 +382,34 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
> > > > > > > > dev_err(dev, "%s: invalid path=%ld\n", __func__, PTR_ERR(path));
> > > > > > > > mutex_unlock(&icc_lock);
> > > > > > > >
> > > > > > > > + opp_node = of_parse_phandle(np, "interconnect-opp-table", idx);
> > > > > > > > + if (opp_node) {
> > > > > > > > + path->opp_table = dev_pm_opp_of_find_table_from_node(opp_node);
> > > > > > > > + of_node_put(opp_node);
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > +
> > > > > > > > return path;
> > > > > > > > }
> > > > > > > > EXPORT_SYMBOL_GPL(of_icc_get);
> > > > > > > >
> > > > > > > > +/**
> > > > > > > > + * icc_get_opp_table() - Get the OPP table that corresponds to a path
> > > > > > > > + * @path: reference to the path returned by icc_get()
> > > > > > > > + *
> > > > > > > > + * This function will return the OPP table that corresponds to a path handle.
> > > > > > > > + * If the interconnect API is disabled, NULL is returned and the consumer
> > > > > > > > + * drivers will still build. Drivers are free to handle this specifically, but
> > > > > > > > + * they don't have to.
> > > > > > > > + *
> > > > > > > > + * Return: opp_table pointer on success. NULL is returned when the API is
> > > > > > > > + * disabled or the OPP table is missing.
> > > > > > > > + */
> > > > > > > > +struct opp_table *icc_get_opp_table(struct icc_path *path)
> > > > > > > > +{
> > > > > > > > + return path->opp_table;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > /**
> > > > > > > > * icc_set_bw() - set bandwidth constraints on an interconnect path
> > > > > > > > * @path: reference to the path returned by icc_get()
> > > > > > > > diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
> > > > > > > > index dc25864755ba..0c0bc55f0e89 100644
> > > > > > > > --- a/include/linux/interconnect.h
> > > > > > > > +++ b/include/linux/interconnect.h
> > > > > > > > @@ -9,6 +9,7 @@
> > > > > > > >
> > > > > > > > #include <linux/mutex.h>
> > > > > > > > #include <linux/types.h>
> > > > > > > > +#include <linux/pm_opp.h>
> > > > > > > >
> > > > > > > > /* macros for converting to icc units */
> > > > > > > > #define Bps_to_icc(x) ((x) / 1000)
> > > > > > > > @@ -28,6 +29,7 @@ struct device;
> > > > > > > > struct icc_path *icc_get(struct device *dev, const int src_id,
> > > > > > > > const int dst_id);
> > > > > > > > struct icc_path *of_icc_get(struct device *dev, const char *name);
> > > > > > > > +struct opp_table *icc_get_opp_table(struct icc_path *path);
> > > > > > > > void icc_put(struct icc_path *path);
> > > > > > > > int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw);
> > > > > > > >
> > > > > > > > @@ -49,6 +51,11 @@ static inline void icc_put(struct icc_path *path)
> > > > > > > > {
> > > > > > > > }
> > > > > > > >
> > > > > > > > +static inline struct opp_table *icc_get_opp_table(struct icc_path *path)
> > > > > > > > +{
> > > > > > > > + return NULL;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > static inline int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
> > > > > > > > {
> > > > > > > > return 0;
> > > > > > > > --
> > > > > > > > 2.22.0.410.gd8fdbe21b5-goog
> > > > > > > >
> > > > >
> > > > > --
> > > > > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> > > > >
^ permalink raw reply
* Re: [PATCH] dt-bindings: pinctrl: aspeed: Fix 'compatible' schema errors
From: Andrew Jeffery @ 2019-07-16 0:39 UTC (permalink / raw)
To: Joel Stanley, Rob Herring
Cc: devicetree, Linus Walleij, Linux ARM, linux-aspeed, linux-gpio
In-Reply-To: <CACPK8Xdz98CQzgE2KCjz8eOhPtx=H8jTe1hVT7LvP77U_gGASQ@mail.gmail.com>
On Tue, 16 Jul 2019, at 08:47, Joel Stanley wrote:
> On Mon, 15 Jul 2019 at 22:37, Rob Herring <robh@kernel.org> wrote:
> >
> > The Aspeed pinctl schema have errors in the 'compatible' schema:
> >
> > Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml: \
> > properties:compatible:enum: ['aspeed', 'ast2400-pinctrl', 'aspeed', 'g4-pinctrl'] has non-unique elements
> > Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml: \
> > properties:compatible:enum: ['aspeed', 'ast2500-pinctrl', 'aspeed', 'g5-pinctrl'] has non-unique elements
> >
> > Flow style sequences have to be quoted if the vales contain ','. Fix
> > this by using the more common one line per entry formatting.
>
> >
> > properties:
> > compatible:
> > - enum: [ aspeed,ast2400-pinctrl, aspeed,g4-pinctrl ]
> > + enum:
> > + - aspeed,ast2400-pinctrl
> > + - aspeed,g4-pinctrl
>
> Thanks for the fix. However, we've standardised on the first form for
> all of our device trees, so we can drop the second compatible string
> from the bindings.
>
> I think Andrew already has a patch cooking to do this.
Yes, I have a series in the works. Will send patches soon.
Andrew
^ permalink raw reply
* Re: [PATCH] dt-bindings: pinctrl: aspeed: Fix 'compatible' schema errors
From: Andrew Jeffery @ 2019-07-16 0:37 UTC (permalink / raw)
To: Rob Herring, devicetree, Linus Walleij
Cc: linux-arm-kernel, linux-gpio, Joel Stanley, linux-aspeed
In-Reply-To: <20190715223725.12924-1-robh@kernel.org>
On Tue, 16 Jul 2019, at 08:07, Rob Herring wrote:
> The Aspeed pinctl schema have errors in the 'compatible' schema:
>
> Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml: \
> properties:compatible:enum: ['aspeed', 'ast2400-pinctrl', 'aspeed',
> 'g4-pinctrl'] has non-unique elements
> Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml: \
> properties:compatible:enum: ['aspeed', 'ast2500-pinctrl', 'aspeed',
> 'g5-pinctrl'] has non-unique elements
>
> Flow style sequences have to be quoted if the vales contain ','. Fix
> this by using the more common one line per entry formatting.
>
> Fixes: 0a617de16730 ("dt-bindings: pinctrl: aspeed: Convert AST2500
> bindings to json-schema")
> Fixes: 07457937bb5c ("dt-bindings: pinctrl: aspeed: Convert AST2400
> bindings to json-schema")
> Cc: Andrew Jeffery <andrew@aj.id.au>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: linux-aspeed@lists.ozlabs.org
> Cc: linux-gpio@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Rob Herring <robh@kernel.org>
Acked-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> .../devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml | 4 +++-
> .../devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml | 4 +++-
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml
> b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml
> index 61a110a7db8a..125599a2dc5e 100644
> ---
> a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml
> +++
> b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml
> @@ -22,7 +22,9 @@ description: |+
>
> properties:
> compatible:
> - enum: [ aspeed,ast2400-pinctrl, aspeed,g4-pinctrl ]
> + enum:
> + - aspeed,ast2400-pinctrl
> + - aspeed,g4-pinctrl
>
> patternProperties:
> '^.*$':
> diff --git
> a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
> b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
> index cf561bd55128..a464cfa0cba3 100644
> ---
> a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
> +++
> b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
> @@ -22,7 +22,9 @@ description: |+
>
> properties:
> compatible:
> - enum: [ aspeed,ast2500-pinctrl, aspeed,g5-pinctrl ]
> + enum:
> + - aspeed,ast2500-pinctrl
> + - aspeed,g5-pinctrl
> aspeed,external-nodes:
> minItems: 2
> maxItems: 2
> --
> 2.20.1
>
>
^ permalink raw reply
* Re: [PATCH] dt-bindings: pinctrl: aspeed: Fix AST2500 example errors
From: Andrew Jeffery @ 2019-07-16 0:37 UTC (permalink / raw)
To: Rob Herring, devicetree, Linus Walleij
Cc: linux-arm-kernel, linux-gpio, Joel Stanley, linux-aspeed
In-Reply-To: <20190715224841.6771-1-robh@kernel.org>
On Tue, 16 Jul 2019, at 08:18, Rob Herring wrote:
> The schema examples are now validated against the schema itself. The
> AST2500 pinctrl schema has a couple of errors:
>
> Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.example.dt.yaml: \
> example-0: $nodename:0: 'example-0' does not match
> '^(bus|soc|axi|ahb|apb)(@[0-9a-f]+)?$'
> Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.example.dt.yaml: \
> pinctrl: aspeed,external-nodes: [[1, 2]] is too short
>
> Fixes: 0a617de16730 ("dt-bindings: pinctrl: aspeed: Convert AST2500
> bindings to json-schema")
> Cc: Andrew Jeffery <andrew@aj.id.au>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: linux-aspeed@lists.ozlabs.org
> Cc: linux-gpio@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Rob Herring <robh@kernel.org>
Acked-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> .../devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
> b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
> index a464cfa0cba3..3e6d85318577 100644
> ---
> a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
> +++
> b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
> @@ -76,9 +76,6 @@ required:
>
> examples:
> - |
> - compatible = "simple-bus";
> - ranges;
> -
> apb {
> compatible = "simple-bus";
> #address-cells = <1>;
> @@ -91,7 +88,7 @@ examples:
>
> pinctrl: pinctrl {
> compatible = "aspeed,g5-pinctrl";
> - aspeed,external-nodes = <&gfx &lhc>;
> + aspeed,external-nodes = <&gfx>, <&lhc>;
>
> pinctrl_i2c3_default: i2c3_default {
> function = "I2C3";
> --
> 2.20.1
>
>
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller
From: Andrew Jeffery @ 2019-07-16 0:36 UTC (permalink / raw)
To: Rob Herring
Cc: linux-mmc, Ulf Hansson, Mark Rutland, Joel Stanley, Adrian Hunter,
devicetree,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-aspeed, linux-kernel@vger.kernel.org, Ryan Chen
In-Reply-To: <CAL_JsqLkOtsAxj9NvNB=EEkH00k-dtNedNY042uuntSmcjhDhA@mail.gmail.com>
On Tue, 16 Jul 2019, at 07:47, Rob Herring wrote:
> On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the
> > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit
> > data bus if only a single slot is enabled.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> > In v2:
> >
> > * Rename to aspeed,sdhci.yaml
> > * Rename sd-controller compatible
> > * Add `maxItems: 1` for reg properties
> > * Move sdhci subnode description to patternProperties
> > * Drop sdhci compatible requirement
> > * #address-cells and #size-cells are required
> > * Prevent additional properties
> > * Implement explicit ranges in example
> > * Remove slot property
> >
> > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++
> > 1 file changed, 90 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > new file mode 100644
> > index 000000000000..67a691c3348c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > @@ -0,0 +1,90 @@
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ASPEED SD/SDIO/eMMC Controller
> > +
> > +maintainers:
> > + - Andrew Jeffery <andrew@aj.id.au>
> > + - Ryan Chen <ryanchen.aspeed@gmail.com>
> > +
> > +description: |+
> > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO
> > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if
> > + only a single slot is enabled.
> > +
> > + The two slots are supported by a common configuration area. As the SDHCIs for
> > + the slots are dependent on the common configuration area, they are described
> > + as child nodes.
> > +
> > +properties:
> > + compatible:
> > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ]
>
> This is actually a list of 4 strings. Please reformat to 1 per line.
On reflection that's obvious, but also a somewhat subtle interaction with the
preference for no quotes (the obvious caveat being "except where required").
Thanks for pointing it out.
I have been running `make dt_binding_check` and `make dtbs_check` over
these, looks like I need to up my game a bit though. Do you do additional things
in your workflow?
Andrew
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-16 0:35 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <e9d4bc0e-fd5d-ae02-2d67-86c7f7c9620f@gmail.com>
On 7/14/19 2:41 PM, Dmitry Osipenko wrote:
> 13.07.2019 8:54, Sowjanya Komatineni пишет:
>> On 6/29/19 8:10 AM, Dmitry Osipenko wrote:
>>> 28.06.2019 5:12, Sowjanya Komatineni пишет:
>>>> This patch adds system suspend and resume support for Tegra210
>>>> clocks.
>>>>
>>>> All the CAR controller settings are lost on suspend when core power
>>>> goes off.
>>>>
>>>> This patch has implementation for saving and restoring all the PLLs
>>>> and clocks context during system suspend and resume to have the
>>>> clocks back to same state for normal operation.
>>>>
>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>> ---
>>>> drivers/clk/tegra/clk-tegra210.c | 115
>>>> ++++++++++++++++++++++++++++++++++++++-
>>>> drivers/clk/tegra/clk.c | 14 +++++
>>>> drivers/clk/tegra/clk.h | 1 +
>>>> 3 files changed, 127 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/tegra/clk-tegra210.c
>>>> b/drivers/clk/tegra/clk-tegra210.c
>>>> index 1c08c53482a5..1b839544e086 100644
>>>> --- a/drivers/clk/tegra/clk-tegra210.c
>>>> +++ b/drivers/clk/tegra/clk-tegra210.c
>>>> @@ -9,10 +9,12 @@
>>>> #include <linux/clkdev.h>
>>>> #include <linux/of.h>
>>>> #include <linux/of_address.h>
>>>> +#include <linux/of_platform.h>
>>>> #include <linux/delay.h>
>>>> #include <linux/export.h>
>>>> #include <linux/mutex.h>
>>>> #include <linux/clk/tegra.h>
>>>> +#include <linux/syscore_ops.h>
>>>> #include <dt-bindings/clock/tegra210-car.h>
>>>> #include <dt-bindings/reset/tegra210-car.h>
>>>> #include <linux/iopoll.h>
>>>> @@ -20,6 +22,7 @@
>>>> #include <soc/tegra/pmc.h>
>>>> #include "clk.h"
>>>> +#include "clk-dfll.h"
>>>> #include "clk-id.h"
>>>> /*
>>>> @@ -225,6 +228,7 @@
>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_SET 0x2a8
>>>> #define CLK_RST_CONTROLLER_RST_DEV_Y_CLR 0x2ac
>>>> +#define CPU_SOFTRST_CTRL 0x380
>>>> #define LVL2_CLK_GATE_OVRA 0xf8
>>>> #define LVL2_CLK_GATE_OVRC 0x3a0
>>>> @@ -2820,6 +2824,7 @@ static int tegra210_enable_pllu(void)
>>>> struct tegra_clk_pll_freq_table *fentry;
>>>> struct tegra_clk_pll pllu;
>>>> u32 reg;
>>>> + int ret;
>>>> for (fentry = pll_u_freq_table; fentry->input_rate; fentry++) {
>>>> if (fentry->input_rate == pll_ref_freq)
>>>> @@ -2847,10 +2852,10 @@ static int tegra210_enable_pllu(void)
>>>> fence_udelay(1, clk_base);
>>>> reg |= PLL_ENABLE;
>>>> writel(reg, clk_base + PLLU_BASE);
>>>> + fence_udelay(1, clk_base);
>>>> - readl_relaxed_poll_timeout_atomic(clk_base + PLLU_BASE, reg,
>>>> - reg & PLL_BASE_LOCK, 2, 1000);
>>>> - if (!(reg & PLL_BASE_LOCK)) {
>>>> + ret = tegra210_wait_for_mask(&pllu, PLLU_BASE, PLL_BASE_LOCK);
>>>> + if (ret) {
>>>> pr_err("Timed out waiting for PLL_U to lock\n");
>>>> return -ETIMEDOUT;
>>>> }
>>>> @@ -3283,6 +3288,103 @@ static void tegra210_disable_cpu_clock(u32 cpu)
>>>> }
>>>> #ifdef CONFIG_PM_SLEEP
>>>> +static u32 cpu_softrst_ctx[3];
>>>> +static struct platform_device *dfll_pdev;
>>>> +#define car_readl(_base, _off) readl_relaxed(clk_base + (_base) +
>>>> ((_off) * 4))
>>>> +#define car_writel(_val, _base, _off) \
>>>> + writel_relaxed(_val, clk_base + (_base) + ((_off) * 4))
>>>> +
>>>> +static int tegra210_clk_suspend(void)
>>>> +{
>>>> + unsigned int i;
>>>> + struct device_node *node;
>>>> +
>>>> + tegra_cclkg_burst_policy_save_context();
>>>> +
>>>> + if (!dfll_pdev) {
>>>> + node = of_find_compatible_node(NULL, NULL,
>>>> + "nvidia,tegra210-dfll");
>>>> + if (node)
>>>> + dfll_pdev = of_find_device_by_node(node);
>>>> +
>>>> + of_node_put(node);
>>>> + if (!dfll_pdev)
>>>> + pr_err("dfll node not found. no suspend for dfll\n");
>>>> + }
>>>> +
>>>> + if (dfll_pdev)
>>>> + tegra_dfll_suspend(dfll_pdev);
>>>> +
>>>> + /* Enable PLLP_OUT_CPU after dfll suspend */
>>>> + tegra_clk_set_pllp_out_cpu(true);
>>>> +
>>>> + tegra_sclk_cclklp_burst_policy_save_context();
>>>> +
>>>> + clk_save_context();
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>>> + cpu_softrst_ctx[i] = car_readl(CPU_SOFTRST_CTRL, i);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void tegra210_clk_resume(void)
>>>> +{
>>>> + unsigned int i;
>>>> + struct clk_hw *parent;
>>>> + struct clk *clk;
>>>> +
>>>> + /*
>>>> + * clk_restore_context restores clocks as per the clock tree.
>>>> + *
>>>> + * dfllCPU_out is first in the clock tree to get restored and it
>>>> + * involves programming DFLL controller along with restoring CPUG
>>>> + * clock burst policy.
>>>> + *
>>>> + * DFLL programming needs dfll_ref and dfll_soc peripheral clocks
>>>> + * to be restores which are part ofthe peripheral clocks.
> ^ white-space
>
> Please use spellchecker to avoid typos.
>
>>>> + * So, peripheral clocks restore should happen prior to dfll clock
>>>> + * restore.
>>>> + */
>>>> +
>>>> + tegra_clk_osc_resume(clk_base);
>>>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>>>> + car_writel(cpu_softrst_ctx[i], CPU_SOFTRST_CTRL, i);
>>>> +
>>>> + /* restore all plls and peripheral clocks */
>>>> + tegra210_init_pllu();
>>>> + clk_restore_context();
>>>> +
>>>> + fence_udelay(5, clk_base);
>>>> +
>>>> + /* resume SCLK and CPULP clocks */
>>>> + tegra_sclk_cpulp_burst_policy_restore_context();
>>>> +
>>>> + /*
>>>> + * restore CPUG clocks:
>>>> + * - enable DFLL in open loop mode
>>>> + * - switch CPUG to DFLL clock source
>>>> + * - close DFLL loop
>>>> + * - sync PLLX state
>>>> + */
>>>> + if (dfll_pdev)
>>>> + tegra_dfll_resume(dfll_pdev, false);
>>>> +
>>>> + tegra_cclkg_burst_policy_restore_context();
>>>> + fence_udelay(2, clk_base);
>>>> +
>>>> + if (dfll_pdev)
>>>> + tegra_dfll_resume(dfll_pdev, true);
>>>> +
>>>> + parent =
>>>> clk_hw_get_parent(__clk_get_hw(clks[TEGRA210_CLK_CCLK_G]));
>>>> + clk = clks[TEGRA210_CLK_PLL_X];
>>>> + if (parent != __clk_get_hw(clk))
>>>> + tegra_clk_sync_state_pll(__clk_get_hw(clk));
>>>> +
>>>> + /* Disable PLL_OUT_CPU after DFLL resume */
>>>> + tegra_clk_set_pllp_out_cpu(false);
>>>> +}
>>>> +
>>>> static void tegra210_cpu_clock_suspend(void)
>>>> {
>>>> /* switch coresite to clk_m, save off original source */
>>>> @@ -3298,6 +3400,11 @@ static void tegra210_cpu_clock_resume(void)
>>>> }
>>>> #endif
>>>> +static struct syscore_ops tegra_clk_syscore_ops = {
>>>> + .suspend = tegra210_clk_suspend,
>>>> + .resume = tegra210_clk_resume,
>>>> +};
>>>> +
>>>> static struct tegra_cpu_car_ops tegra210_cpu_car_ops = {
>>>> .wait_for_reset = tegra210_wait_cpu_in_reset,
>>>> .disable_clock = tegra210_disable_cpu_clock,
>>>> @@ -3583,5 +3690,7 @@ static void __init tegra210_clock_init(struct
>>>> device_node *np)
>>>> tegra210_mbist_clk_init();
>>>> tegra_cpu_car_ops = &tegra210_cpu_car_ops;
>>>> +
>>>> + register_syscore_ops(&tegra_clk_syscore_ops);
>>>> }
>>> Is it really worthwhile to use syscore_ops for suspend/resume given
>>> that drivers for
>>> won't resume before the CLK driver anyway? Are there any other options
>>> for CLK
>>> suspend/resume?
>>>
>>> I'm also not sure whether PM runtime API could be used at all in the
>>> context of
>>> syscore_ops ..
>>>
>>> Secondly, what about to use generic clk_save_context() /
>>> clk_restore_context()
>>> helpers for the suspend-resume? It looks to me that some other
>>> essential (and proper)
>>> platform driver (soc/tegra/? PMC?) should suspend-resume the clocks
>>> using the generic
>>> CLK Framework API.
>> Clock resume should happen very early to restore peripheral and cpu
>> clocks very early than peripheral drivers resume happens.
> If all peripheral drivers properly requested all of the necessary clocks
> and CLK driver was a platform driver, then I guess the probe should have
> been naturally ordered. But that's not very achievable with the
> currently available infrastructure in the kernel, so I'm not arguing
> that the clocks should be explicitly resumed before the users.
>
>> this patch series uses clk_save_context and clk_restore_context for
>> corresponding divider, pll, pllout.. save and restore context.
> Now I see that indeed this API is utilized in this patch, thank you for
> the clarification.
>
>> But as there is dependency on dfll resume and cpu and pllx clocks
>> restore, couldnt use clk_save_context and clk_restore_context for dfll.
>>
>> So implemented recommended dfll resume sequence in main Tegra210 clock
>> driver along with invoking clk_save_context/clk_restore_context where
>> all other clocks save/restore happens as per clock tree traversal.
> Could you please clarify what part of peripherals clocks is required for
> DFLL's restore? Couldn't DFLL driver be changed to avoid that quirkness
> and thus to make DFLL driver suspend/resume the clock?
DFLL source ref_clk and soc_clk need to be restored prior to dfll.
I see dfllCPU_out parent to CCLK_G first in the clock tree and dfll_ref
and dfll_soc peripheral clocks are not resumed by the time dfll resume
happens first.
ref_clk and soc_clk source is from pll_p and clock tree has these
registered under pll_p which happens later.
tegra210_clock_init registers in order plls, peripheral clocks,
super_clk init for cclk_g during clock driver probe and dfll probe and
register happens later.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox