Date:   Tue, 3 Mar 2020 19:31:09 +0530
From:   Anand Moon <linux.amoon@...il.com>
To:     Kevin Hilman <khilman@...libre.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        devicetree <devicetree@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-amlogic@...ts.infradead.org,
        Linux Kernel <linux-kernel@...r.kernel.org>,
        "open list:COMMON CLK FRAMEWORK" <linux-clk@...r.kernel.org>
Subject: Re: [PATCHv2 2/2] clk: meson: g12a: set cpub_clk flags to CLK_IS_CRITICAL

Hi Kevin,

Thanks for your review comments,
Plz see my inline comment.
Let me try to explain with the logs from my side.

On Mon, 2 Mar 2020 at 22:31, Kevin Hilman <khilman@...libre.com> wrote:
>
> Anand Moon <linux.amoon@...il.com> writes:
>
> > On Odroid n2, cpub_clk is not geting enable, which lead the stalling
> > at booting of the device,
>
> First, how is the CPU_B clk related to the SD card issue described in
> the cover letter?  I think this patch is attempting to fix something
> unrelated to the SD card.  Please separate from this series (or describe
> in detail how it's related to the SD card booting.)
>
Yes you are correct, its not related to sdcard,
It's related to *cpub_clk* clock not getting enabled,
we can check this in clk_summary of the board.

> Also, we're missing lots of details here to be able to help.  Are you
> using the u-boot from hardkernel?  your own?  something else?  What's
> the version?

I am using Archlinux distro with mainline latest kernel 5.6-rc4 and
mainline u-boot (U-Boot 2020.04-rc3-00043-g5b07eb8ee6-dirty (Mar 03
2020 - 11:30:28 +0530) odroid-n2)
for my testing.

>
> Can you share logs (including u-boot logs) showing how your kernel is
> booting and full kernel boot log (including the stalls.)

I am using Archlinux distro to test. So here is the log for pre-compiled
images booting from sdcard, which fails.

Here is the boot log. Using sdcard failed to boot.

[0] https://pastebin.com/6zgFR8Zj        (odroid_n2_sdcard_stalled)

Using same image with eMMC it will boot to login prompt.

[1] https://pastebin.com/wyZqAtjw       (odroid_n2_emmc_booting)

>
> > updating flags to CLK_IS_CRITICAL which help enable all the parent for
> > cpub_clk.
>
> With current mainline, I've tested DVFS using CPUfreq on both clusters
> on odroid-n2, and both clusters are booting, so I don't understand the
> need for this patch.
>

Here is the *clk_summary* of the mainline kernel it shows that
cpub_clk is not getting enabled before this patch.

[2] https://pastebin.com/WFYcwtTa        (clk_summary_emmc_before.txt)

After my patch changes here is the clk_summary for eMMC and microSD cards.
it's observed that cpub_clk and it's parent clock are getting enabled.

[3] https://pastebin.com/PLzHjSXj        (clk_summary.eMMC_after my changes)
[4] https://pastebin.com/yBqwTCvZ        (clk_summary.microSD_after my changes)

> It's not related to your problem (I don't think) but for the regulators
> used by each cluster, the PWM driver is needed, and there's a bug/race
> in the probing of the PWM regulators used for CPU_B.  If you make the
> PWM regulators, built-in this problem goes away for CPUfreq.
>
> Just for kicks, can you build your kernel with CONFIG_PWM_MESON=y
> (currently defaults to =n) and see if you have any better results with
> booting.

I have build the kernel with CONFIG_PWM_MESON=n
Here is the boot log at my end on microsd card.

[5] https://pastebin.com/dv45U4EM        (odroid_n2_sdcard_stalled_PWM_MESON=n)

I have also build the kernel with CONFIG_PWM_MESON=y
Here is the boot log at my end on microsd card

[6] https://pastebin.com/XWmdFwnf        (odroid_n2_sdcard_PWM_MESON=y)

>
> And FYI, any use of CLK_IS_CRITICAL will be very highly scrutinized.
> You will need detailed justification for adding this flag since it most
> often is just masking some other bug.
>

FYI, I had done code walk through with the clk frame work with the driver
and S922X datasheet, but could not find effective way enable this cpub_clk
clock dynamically, I am not much aware of how clk get enable / disable in the
first place. It's most likely, that bug could be some missing code changes
or other clock that could trigger this.

But I tried to add some core debug prints to clk frame work, but with
my changes I was not able to boot the kernel.

Yes I am aware masking CLK_IS_CRITICAL to flags might not be appropriate.
But is their any another alternate way to debug further.
Plz let me know. I will test this other approach.

Plz Plz Plz let me know how to debug further.

-Anand

> Kevin
>

On Mon, 2 Mar 2020 at 22:31, Kevin Hilman <khilman@...libre.com> wrote:
>
> Anand Moon <linux.amoon@...il.com> writes:
>
> > On Odroid n2, cpub_clk is not geting enable, which lead the stalling
> > at booting of the device,
>
> First, how is the CPU_B clk related to the SD card issue described in
> the cover letter?  I think this patch is attempting to fix something
> unrelated to the SD card.  Please separate from this series (or describe
> in detail how it's related to the SD card booting.)
>
> Also, we're missing lots of details here to be able to help.  Are you
> using the u-boot from hardkernel?  your own?  something else?  What's
> the version?
>
> Can you share logs (including u-boot logs) showing how your kernel is
> booting and full kernel boot log (including the stalls.)
>
> > updating flags to CLK_IS_CRITICAL which help enable all the parent for
> > cpub_clk.
>
> With current mainline, I've tested DVFS using CPUfreq on both clusters
> on odroid-n2, and both clusters are booting, so I don't understand the
> need for this patch.
>
> It's not related to your problem (I don't think) but for the regulators
> used by each cluster, the PWM driver is needed, and there's a bug/race
> in the probing of the PWM regulators used for CPU_B.  If you make the
> PWM regulators, built-in this problem goes away for CPUfreq.
>
> Just for kicks, can you build your kernel with CONFIG_PWM_MESON=y
> (currently defaults to =n) and see if you have any better results with
> booting.
>
> And FYI, any use of CLK_IS_CRITICAL will be very highly scrutinized.
> You will need detailed justification for adding this flag since it most
> often is just masking some other bug.
>
> Kevin
>
> > Fixes: ffae8475b90c (clk: meson: g12a: add notifiers to handle cpu clock change);
> > Cc: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
> > Cc: Jerome Brunet <jbrunet@...libre.com>
> > Cc: Neil Armstrong <narmstrong@...libre.com>
> > Suggested-by: Neil Armstrong <narmstrong@...libre.com>
> > Signed-off-by: Anand Moon <linux.amoon@...il.com>
> > ---
> > Previous changes
> >       fix the commit $subject and $message as previously I was
> >         wrong on the my findings.
> >         Added the Fixed tags to the commit.
> >
> > Following Neil's suggestion, I have prepared this patch.
> > https://patchwork.kernel.org/patch/11177441/#22964889
> > ---
> >  drivers/clk/meson/g12a.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
> > index d2760a021301..7237d08b4112 100644
> > --- a/drivers/clk/meson/g12a.c
> > +++ b/drivers/clk/meson/g12a.c
> > @@ -681,7 +681,7 @@ static struct clk_regmap g12b_cpub_clk = {
> >                       &g12a_sys_pll.hw
> >               },
> >               .num_parents = 2,
> > -             .flags = CLK_SET_RATE_PARENT,
> > +             .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
> >       },
> >  };
> >
> > --
> > 2.25.1

View attachment "odroid_n2_sdcard_stalled.txt" of type "text/plain" (22110 bytes)

View attachment "odroid_n2_emmc_booting.txt" of type "text/plain" (40741 bytes)

View attachment "clk_summary.eMMC_after my changes.txt" of type "text/plain" (43594 bytes)

View attachment "clk_summary.microSD_after my changes.txt" of type "text/plain" (43593 bytes)

View attachment "odroid_n2_sdcard_stalled_PWM_MESON=n.txt" of type "text/plain" (10124 bytes)

View attachment "odroid_n2_sdcard_PWM_MESON=y.txt" of type "text/plain" (17597 bytes)

Powered by blists - more mailing lists