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 wrote: > > Anand Moon 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 wrote: > > Anand Moon 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 > > Cc: Jerome Brunet > > Cc: Neil Armstrong > > Suggested-by: Neil Armstrong > > Signed-off-by: Anand Moon > > --- > > 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