Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
To:     Christian Hewitt <christianshewitt@gmail.com>
From:   Michael Turquette <mturquette@baylibre.com>
In-Reply-To: <1539425086-10997-1-git-send-email-christianshewitt@gmail.com>
Cc:     christianshewitt@gmail.com,
        Neil Armstrong <narmstrong@baylibre.com>,
        Jerome Brunet <jbrunet@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Carlo Caione <carlo@caione.org>,
        Kevin Hilman <khilman@baylibre.com>,
        linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
References: <1539425086-10997-1-git-send-email-christianshewitt@gmail.com>
Message-ID: <20181013160810.88481.14237@resonance>
User-Agent: alot/0.7
Subject: Re: [PATCH] clk: meson-gxbb: set fclk_div3 as CLK_IS_CRITICAL
Date:   Sat, 13 Oct 2018 18:08:10 +0200
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Christian Hewitt (2018-10-13 12:04:46)
> On the Khadas VIM2 (GXM) and LePotato (GXL) board there are problems
> with reboot; e.g. a ~60 second delay between issuing reboot and the
> board power cycling (and in some OS configurations reboot will fail
> and require manual power cycling).
> =

> Similar to 'commit c987ac6f1f088663b6dad39281071aeb31d450a8 ("clk:
> meson-gxbb: set fclk_div2 as CLK_IS_CRITICAL")' the SCPI Cortex-M4
> Co-Processor seems to depend on FCLK_DIV3 being operational.
> =

> Bisect gives 'commit 05f814402d6174369b3b29832cbb5eb5ed287059 ("clk:
> meson: add fdiv clock gates") between 4.16 and 4.16-rc1 as the first
> bad commit. This added support for the missing clock gates before the
> fixed PLL fixed dividers (FCLK_DIVx) and the clock framework which
> disabled all the unused fixed dividers, thus it disabled a critical
> clock path for the SCPI Co-Processor.
> =

> This change simply sets the FCLK_DIV3 gate as critical to ensure
> nothing can disable it.

I'm a bit skeptical of this. If FCLK_DIV3 is gated at run-time, there is
no side effect other than long and/or failed reboot?

Seems like someone should be managing this clock, and simply leaving it
on all the time isn't necessarily the right approach. Any chance that
you can dig into this behavior to better understand it?

It's easy to solve issues by leaving clocks on all the time, but this
becomes a problem later on when trying to tune a device for power.

Also, if this commit really is the right fix, it should include a
comment for FCLK_DIV3 stating why the CLK_IS_CRITICAL flag was set,
which may be helpful later on to know if it is safe to remove it. Same
is true for FCLK_DIV2 in c987ac6f1f088663b6dad39281071aeb31d450a8.

Regards,
Mike

> =

> Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
> ---
>  drivers/clk/meson/gxbb.c | 1 +
>  1 file changed, 1 insertion(+)
> =

> diff --git a/drivers/clk/meson/gxbb.c b/drivers/clk/meson/gxbb.c
> index 86d3ae5..4c8925d 100644
> --- a/drivers/clk/meson/gxbb.c
> +++ b/drivers/clk/meson/gxbb.c
> @@ -509,6 +509,7 @@ static struct clk_fixed_factor gxbb_fclk_div3_div =3D=
 {
>                 .ops =3D &clk_fixed_factor_ops,
>                 .parent_names =3D (const char *[]){ "fixed_pll" },
>                 .num_parents =3D 1,
> +               .flags =3D CLK_IS_CRITICAL,
>         },
>  };
>  =

> -- =

> 2.7.4