| Message ID | 554BD33D.7050907@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 7, 2015 at 2:03 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 05/07/15 08:17, Kevin Hilman wrote: >> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >>> On 05/01/15 15:07, Heiko Stübner wrote: >>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd: >>>> >>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the >>>>>> following patch? >>>>> It looks like it will work at least, but still I'd prefer to keep the >>>>> orphan check contained to clk.c. How about this compile tested only patch? >>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree >>>> looks the same and it also still defers nicely in the scenario I needed it >>>> for. The implementation also looks nice - and of course much more compact than >>>> my check in two places :-) . I don't know if you want to put this as follow-up >>>> on top or fold it into the original orphan-check, so in any case >>>> >>>> Tested-by: Heiko Stuebner <heiko@sntech.de> >>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de> >>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with >>> my patch and a note that it's based on an earlier patch from you. >> It appears this has landed in linux-next in the form of 882667c1fcf1 >> clk: prevent orphan clocks from being used. A bunch of boot failures >> for sunxi in today's linux-next[1] were bisected down to that patch. >> >> I confirmed that reverting that commit on top of next/master gets >> sunxi booting again. >> >> > > Thanks for the report. I've removed the two clk orphan patches from > clk-next. Would it be possible to try with next-20150507 and > clk_ignore_unused on the command line? That doesn't help. I tried on cubieboard2 and bananapi. > Also we can try to see if > critical clocks aren't being forced on by applying this patch and > looking for clk_get() failures From cubieboard2, there's a few that look rather important: [ 0.000000] Additional per-CPU info printed with stalls. [ 0.000000] Build-time adjustment of leaf fanout to 32. [ 0.000000] RCU restricting CPUs from NR_CPUS=16 to nr_cpu_ids=2. [ 0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=32, nr_cpu_ids=2 [ 0.000000] NR_IRQS:16 nr_irqs:16 16 [ 0.000000] clk: couldn't get parent clock 0 for /clocks/ahb@01c20054 [ 0.000000] Failed to enable critical clock cpu [ 0.000000] Failed to enable critical clock pll5_ddr [ 0.000000] Failed to enable critical clock ahb_sdram [ 0.000000] Architected cp15 timer(s) running at 24.00MHz (virt). Kevin
On 05/07, Kevin Hilman wrote: > On Thu, May 7, 2015 at 2:03 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > On 05/07/15 08:17, Kevin Hilman wrote: > >> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > >>> On 05/01/15 15:07, Heiko Stübner wrote: > >>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd: > >>>> > >>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the > >>>>>> following patch? > >>>>> It looks like it will work at least, but still I'd prefer to keep the > >>>>> orphan check contained to clk.c. How about this compile tested only patch? > >>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree > >>>> looks the same and it also still defers nicely in the scenario I needed it > >>>> for. The implementation also looks nice - and of course much more compact than > >>>> my check in two places :-) . I don't know if you want to put this as follow-up > >>>> on top or fold it into the original orphan-check, so in any case > >>>> > >>>> Tested-by: Heiko Stuebner <heiko@sntech.de> > >>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de> > >>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with > >>> my patch and a note that it's based on an earlier patch from you. > >> It appears this has landed in linux-next in the form of 882667c1fcf1 > >> clk: prevent orphan clocks from being used. A bunch of boot failures > >> for sunxi in today's linux-next[1] were bisected down to that patch. > >> > >> I confirmed that reverting that commit on top of next/master gets > >> sunxi booting again. > >> > >> > > > > Thanks for the report. I've removed the two clk orphan patches from > > clk-next. Would it be possible to try with next-20150507 and > > clk_ignore_unused on the command line? > > That doesn't help. I tried on cubieboard2 and bananapi. Thanks for trying. > > > Also we can try to see if > > critical clocks aren't being forced on by applying this patch and > > looking for clk_get() failures > > From cubieboard2, there's a few that look rather important: > > [ 0.000000] Additional per-CPU info printed with stalls. > [ 0.000000] Build-time adjustment of leaf fanout to 32. > [ 0.000000] RCU restricting CPUs from NR_CPUS=16 to nr_cpu_ids=2. > [ 0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=32, nr_cpu_ids=2 > [ 0.000000] NR_IRQS:16 nr_irqs:16 16 > [ 0.000000] clk: couldn't get parent clock 0 for /clocks/ahb@01c20054 > [ 0.000000] Failed to enable critical clock cpu > [ 0.000000] Failed to enable critical clock pll5_ddr > [ 0.000000] Failed to enable critical clock ahb_sdram > [ 0.000000] Architected cp15 timer(s) running at 24.00MHz (virt). Ok. So it seems we need to come up with some solution to the "critical clocks" problem that doesn't require the individual clock drivers to call clk_prepare_enable().
On Thu, May 07, 2015 at 11:53:18PM -0700, Stephen Boyd wrote: > On 05/07, Kevin Hilman wrote: > > On Thu, May 7, 2015 at 2:03 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > > On 05/07/15 08:17, Kevin Hilman wrote: > > >> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > >>> On 05/01/15 15:07, Heiko Stübner wrote: > > >>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd: > > >>>> > > >>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the > > >>>>>> following patch? > > >>>>> It looks like it will work at least, but still I'd prefer to keep the > > >>>>> orphan check contained to clk.c. How about this compile tested only patch? > > >>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree > > >>>> looks the same and it also still defers nicely in the scenario I needed it > > >>>> for. The implementation also looks nice - and of course much more compact than > > >>>> my check in two places :-) . I don't know if you want to put this as follow-up > > >>>> on top or fold it into the original orphan-check, so in any case > > >>>> > > >>>> Tested-by: Heiko Stuebner <heiko@sntech.de> > > >>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de> > > >>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with > > >>> my patch and a note that it's based on an earlier patch from you. > > >> It appears this has landed in linux-next in the form of 882667c1fcf1 > > >> clk: prevent orphan clocks from being used. A bunch of boot failures > > >> for sunxi in today's linux-next[1] were bisected down to that patch. > > >> > > >> I confirmed that reverting that commit on top of next/master gets > > >> sunxi booting again. > > >> > > >> > > > > > > Thanks for the report. I've removed the two clk orphan patches from > > > clk-next. Would it be possible to try with next-20150507 and > > > clk_ignore_unused on the command line? > > > > That doesn't help. I tried on cubieboard2 and bananapi. > > Thanks for trying. > > > > > > Also we can try to see if > > > critical clocks aren't being forced on by applying this patch and > > > looking for clk_get() failures > > > > From cubieboard2, there's a few that look rather important: > > > > [ 0.000000] Additional per-CPU info printed with stalls. > > [ 0.000000] Build-time adjustment of leaf fanout to 32. > > [ 0.000000] RCU restricting CPUs from NR_CPUS=16 to nr_cpu_ids=2. > > [ 0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=32, nr_cpu_ids=2 > > [ 0.000000] NR_IRQS:16 nr_irqs:16 16 > > [ 0.000000] clk: couldn't get parent clock 0 for /clocks/ahb@01c20054 > > [ 0.000000] Failed to enable critical clock cpu > > [ 0.000000] Failed to enable critical clock pll5_ddr > > [ 0.000000] Failed to enable critical clock ahb_sdram > > [ 0.000000] Architected cp15 timer(s) running at 24.00MHz (virt). > > Ok. So it seems we need to come up with some solution to the > "critical clocks" problem that doesn't require the individual > clock drivers to call clk_prepare_enable(). I'm getting more and more unsure if we can really handle the complexity we get by allowing to register orphaned clocks. On one hand we can't handle the orphaned clocks properly when we do a clk_prepare/enable on them, on the other hand we run into trouble when we forbid to prepare/enable them. The fact that clocks can become orphans by reparenting them makes it even more complicated. Maybe allowing orphans is something that has to be revisited. Sascha
Am Freitag, 8. Mai 2015, 10:13:55 schrieb Sascha Hauer: > On Thu, May 07, 2015 at 11:53:18PM -0700, Stephen Boyd wrote: > > On 05/07, Kevin Hilman wrote: > > > On Thu, May 7, 2015 at 2:03 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > > > On 05/07/15 08:17, Kevin Hilman wrote: > > > >> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > > >>> On 05/01/15 15:07, Heiko Stübner wrote: > > > >>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd: > > > >>>>>> Instead I guess we could hook it less deep into clk_get_sys, like > > > >>>>>> in the > > > >>>>>> following patch? > > > >>>>> > > > >>>>> It looks like it will work at least, but still I'd prefer to keep > > > >>>>> the > > > >>>>> orphan check contained to clk.c. How about this compile tested > > > >>>>> only patch? > > > >>>> > > > >>>> I gave this a spin on my rk3288-firefly board. It still boots, the > > > >>>> clock tree looks the same and it also still defers nicely in the > > > >>>> scenario I needed it for. The implementation also looks nice - and > > > >>>> of course much more compact than my check in two places :-) . I > > > >>>> don't know if you want to put this as follow-up on top or fold it > > > >>>> into the original orphan-check, so in any case > > > >>>> > > > >>>> Tested-by: Heiko Stuebner <heiko@sntech.de> > > > >>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de> > > > >>> > > > >>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it > > > >>> with > > > >>> my patch and a note that it's based on an earlier patch from you. > > > >> > > > >> It appears this has landed in linux-next in the form of 882667c1fcf1 > > > >> clk: prevent orphan clocks from being used. A bunch of boot failures > > > >> for sunxi in today's linux-next[1] were bisected down to that patch. > > > >> > > > >> I confirmed that reverting that commit on top of next/master gets > > > >> sunxi booting again. > > > > > > > > Thanks for the report. I've removed the two clk orphan patches from > > > > clk-next. Would it be possible to try with next-20150507 and > > > > clk_ignore_unused on the command line? > > > > > > That doesn't help. I tried on cubieboard2 and bananapi. > > > > Thanks for trying. > > > > > > Also we can try to see if > > > > critical clocks aren't being forced on by applying this patch and > > > > looking for clk_get() failures > > > > > > From cubieboard2, there's a few that look rather important: > > > > > > [ 0.000000] Additional per-CPU info printed with stalls. > > > [ 0.000000] Build-time adjustment of leaf fanout to 32. > > > [ 0.000000] RCU restricting CPUs from NR_CPUS=16 to nr_cpu_ids=2. > > > [ 0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=32, > > > nr_cpu_ids=2 > > > [ 0.000000] NR_IRQS:16 nr_irqs:16 16 > > > [ 0.000000] clk: couldn't get parent clock 0 for /clocks/ahb@01c20054 > > > [ 0.000000] Failed to enable critical clock cpu > > > [ 0.000000] Failed to enable critical clock pll5_ddr > > > [ 0.000000] Failed to enable critical clock ahb_sdram > > > [ 0.000000] Architected cp15 timer(s) running at 24.00MHz (virt). > > > > Ok. So it seems we need to come up with some solution to the > > "critical clocks" problem that doesn't require the individual > > clock drivers to call clk_prepare_enable(). > > I'm getting more and more unsure if we can really handle the complexity > we get by allowing to register orphaned clocks. On one hand we can't > handle the orphaned clocks properly when we do a clk_prepare/enable on > them, on the other hand we run into trouble when we forbid to > prepare/enable them. The fact that clocks can become orphans by > reparenting them makes it even more complicated. > Maybe allowing orphans is something that has to be revisited. hmm, I don't see it this drastic. I was expecting a lot more fallout from changing the behaviour of orphaned clocks over all arches using the CCF. From the kernelci-boards only the Sunxi-ones seem to have been affected at all and also only because they need a "regulator-always-on" equivalent in the CCF, which currently hijacks prepare/enable inside the clock driver to achive this. On the Rockchip clocks we have something similar, with the only difference that it is done after all clocks are registered and does not need to access the 3 orphans we have at this point due to their source clock coming from an external i2c-connected chip that gets probed a lot later.
On Fri, May 08, 2015 at 11:30:19AM +0200, Heiko Stübner wrote: > Am Freitag, 8. Mai 2015, 10:13:55 schrieb Sascha Hauer: > > On Thu, May 07, 2015 at 11:53:18PM -0700, Stephen Boyd wrote: > > > On 05/07, Kevin Hilman wrote: > > > > On Thu, May 7, 2015 at 2:03 PM, Stephen Boyd <sboyd@codeaurora.org> > wrote: > > > > > On 05/07/15 08:17, Kevin Hilman wrote: > > > > >> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <sboyd@codeaurora.org> > wrote: > > > > >>> On 05/01/15 15:07, Heiko Stübner wrote: > > > > >>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd: > > > > >>>>>> Instead I guess we could hook it less deep into clk_get_sys, like > > > > >>>>>> in the > > > > >>>>>> following patch? > > > > >>>>> > > > > >>>>> It looks like it will work at least, but still I'd prefer to keep > > > > >>>>> the > > > > >>>>> orphan check contained to clk.c. How about this compile tested > > > > >>>>> only patch? > > > > >>>> > > > > >>>> I gave this a spin on my rk3288-firefly board. It still boots, the > > > > >>>> clock tree looks the same and it also still defers nicely in the > > > > >>>> scenario I needed it for. The implementation also looks nice - and > > > > >>>> of course much more compact than my check in two places :-) . I > > > > >>>> don't know if you want to put this as follow-up on top or fold it > > > > >>>> into the original orphan-check, so in any case > > > > >>>> > > > > >>>> Tested-by: Heiko Stuebner <heiko@sntech.de> > > > > >>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de> > > > > >>> > > > > >>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it > > > > >>> with > > > > >>> my patch and a note that it's based on an earlier patch from you. > > > > >> > > > > >> It appears this has landed in linux-next in the form of 882667c1fcf1 > > > > >> clk: prevent orphan clocks from being used. A bunch of boot failures > > > > >> for sunxi in today's linux-next[1] were bisected down to that patch. > > > > >> > > > > >> I confirmed that reverting that commit on top of next/master gets > > > > >> sunxi booting again. > > > > > > > > > > Thanks for the report. I've removed the two clk orphan patches from > > > > > clk-next. Would it be possible to try with next-20150507 and > > > > > clk_ignore_unused on the command line? > > > > > > > > That doesn't help. I tried on cubieboard2 and bananapi. > > > > > > Thanks for trying. > > > > > > > > Also we can try to see if > > > > > critical clocks aren't being forced on by applying this patch and > > > > > looking for clk_get() failures > > > > > > > > From cubieboard2, there's a few that look rather important: > > > > > > > > [ 0.000000] Additional per-CPU info printed with stalls. > > > > [ 0.000000] Build-time adjustment of leaf fanout to 32. > > > > [ 0.000000] RCU restricting CPUs from NR_CPUS=16 to nr_cpu_ids=2. > > > > [ 0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=32, > > > > nr_cpu_ids=2 > > > > [ 0.000000] NR_IRQS:16 nr_irqs:16 16 > > > > [ 0.000000] clk: couldn't get parent clock 0 for /clocks/ahb@01c20054 > > > > [ 0.000000] Failed to enable critical clock cpu > > > > [ 0.000000] Failed to enable critical clock pll5_ddr > > > > [ 0.000000] Failed to enable critical clock ahb_sdram > > > > [ 0.000000] Architected cp15 timer(s) running at 24.00MHz (virt). > > > > > > Ok. So it seems we need to come up with some solution to the > > > "critical clocks" problem that doesn't require the individual > > > clock drivers to call clk_prepare_enable(). > > > > I'm getting more and more unsure if we can really handle the complexity > > we get by allowing to register orphaned clocks. On one hand we can't > > handle the orphaned clocks properly when we do a clk_prepare/enable on > > them, on the other hand we run into trouble when we forbid to > > prepare/enable them. The fact that clocks can become orphans by > > reparenting them makes it even more complicated. > > Maybe allowing orphans is something that has to be revisited. > > hmm, I don't see it this drastic. I was expecting a lot more fallout from > changing the behaviour of orphaned clocks over all arches using the CCF. > > From the kernelci-boards only the Sunxi-ones seem to have been affected at all > and also only because they need a "regulator-always-on" equivalent in the CCF, > which currently hijacks prepare/enable inside the clock driver to achive this. Mediatek has the same case. Here we needs some clock to stay always on and these clocks get registered before the PLLs they depend on. This means they can't be enabled right after registration. Keeping the prepare/enable count correct and syncing the software state with the hardware state is made quite complicated with orphaned clocks and I suspect more bugs here. Even now it's hard to move in the clock framework without breaking other stuff, this won't get easier with more bug fixes. So I think asking what allowing orphaned clocks really buys us is valid. Of course we would need to find a way to get the initialisation order straight which will probably cause some pain. Sascha
On Thu, May 07, 2015 at 02:03:57PM -0700, Stephen Boyd wrote: > On 05/07/15 08:17, Kevin Hilman wrote: > > On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > >> On 05/01/15 15:07, Heiko Stübner wrote: > >>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd: > >>> > >>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the > >>>>> following patch? > >>>> It looks like it will work at least, but still I'd prefer to keep the > >>>> orphan check contained to clk.c. How about this compile tested only patch? > >>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree > >>> looks the same and it also still defers nicely in the scenario I needed it > >>> for. The implementation also looks nice - and of course much more compact than > >>> my check in two places :-) . I don't know if you want to put this as follow-up > >>> on top or fold it into the original orphan-check, so in any case > >>> > >>> Tested-by: Heiko Stuebner <heiko@sntech.de> > >>> Reviewed-by: Heiko Stuebner <heiko@sntech.de> > >> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with > >> my patch and a note that it's based on an earlier patch from you. > > It appears this has landed in linux-next in the form of 882667c1fcf1 > > clk: prevent orphan clocks from being used. A bunch of boot failures > > for sunxi in today's linux-next[1] were bisected down to that patch. > > > > I confirmed that reverting that commit on top of next/master gets > > sunxi booting again. > > > > > > Thanks for the report. I've removed the two clk orphan patches from > clk-next. Would it be possible to try with next-20150507 and > clk_ignore_unused on the command line? This makes it work, but it's not really an option. > Also we can try to see if critical clocks aren't being forced on by > applying this patch and looking for clk_get() failures And that shows that the CPU and DDR clocks are not protected, which obviously is pretty mad. I've mass converted all our probing code to use OF_CLK_DECLARE, and make things work again. http://code.bulix.org/5goa5j-88345?raw Is this an acceptable solution? We were already moving to this, I'm not really fond of doing this like that, but I guess this whole debacle makes it necessary. Maxime
On 05/08/15 03:02, Maxime Ripard wrote: > On Thu, May 07, 2015 at 02:03:57PM -0700, Stephen Boyd wrote: >> On 05/07/15 08:17, Kevin Hilman wrote: >>> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >>>> On 05/01/15 15:07, Heiko Stübner wrote: >>>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd: >>>>> >>>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the >>>>>>> following patch? >>>>>> It looks like it will work at least, but still I'd prefer to keep the >>>>>> orphan check contained to clk.c. How about this compile tested only patch? >>>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree >>>>> looks the same and it also still defers nicely in the scenario I needed it >>>>> for. The implementation also looks nice - and of course much more compact than >>>>> my check in two places :-) . I don't know if you want to put this as follow-up >>>>> on top or fold it into the original orphan-check, so in any case >>>>> >>>>> Tested-by: Heiko Stuebner <heiko@sntech.de> >>>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de> >>>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with >>>> my patch and a note that it's based on an earlier patch from you. >>> It appears this has landed in linux-next in the form of 882667c1fcf1 >>> clk: prevent orphan clocks from being used. A bunch of boot failures >>> for sunxi in today's linux-next[1] were bisected down to that patch. >>> >>> I confirmed that reverting that commit on top of next/master gets >>> sunxi booting again. >>> >>> >> Thanks for the report. I've removed the two clk orphan patches from >> clk-next. Would it be possible to try with next-20150507 and >> clk_ignore_unused on the command line? > This makes it work, but it's not really an option. > Hmm.. I thought it didn't fix it for Kevin. Confused. >> Also we can try to see if critical clocks aren't being forced on by >> applying this patch and looking for clk_get() failures > And that shows that the CPU and DDR clocks are not protected, which > obviously is pretty mad. > > I've mass converted all our probing code to use OF_CLK_DECLARE, and > make things work again. > > http://code.bulix.org/5goa5j-88345?raw > > Is this an acceptable solution? > > We were already moving to this, I'm not really fond of doing this like > that, but I guess this whole debacle makes it necessary. > I wonder why we can't switch out the clk_ops on the affected platforms + clocks to be read-only (at least for the enable/disable part)? That would fix it just the same right? I wasn't around for the original discussion regarding this always-on stuff so perhaps I've missed something.
On Tue, May 12, 2015 at 03:35:50PM -0700, Stephen Boyd wrote: > On 05/08/15 03:02, Maxime Ripard wrote: > > On Thu, May 07, 2015 at 02:03:57PM -0700, Stephen Boyd wrote: > >> On 05/07/15 08:17, Kevin Hilman wrote: > >>> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > >>>> On 05/01/15 15:07, Heiko Stübner wrote: > >>>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd: > >>>>> > >>>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the > >>>>>>> following patch? > >>>>>> It looks like it will work at least, but still I'd prefer to keep the > >>>>>> orphan check contained to clk.c. How about this compile tested only patch? > >>>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree > >>>>> looks the same and it also still defers nicely in the scenario I needed it > >>>>> for. The implementation also looks nice - and of course much more compact than > >>>>> my check in two places :-) . I don't know if you want to put this as follow-up > >>>>> on top or fold it into the original orphan-check, so in any case > >>>>> > >>>>> Tested-by: Heiko Stuebner <heiko@sntech.de> > >>>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de> > >>>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with > >>>> my patch and a note that it's based on an earlier patch from you. > >>> It appears this has landed in linux-next in the form of 882667c1fcf1 > >>> clk: prevent orphan clocks from being used. A bunch of boot failures > >>> for sunxi in today's linux-next[1] were bisected down to that patch. > >>> > >>> I confirmed that reverting that commit on top of next/master gets > >>> sunxi booting again. > >>> > >>> > >> Thanks for the report. I've removed the two clk orphan patches from > >> clk-next. Would it be possible to try with next-20150507 and > >> clk_ignore_unused on the command line? > > This makes it work, but it's not really an option. > > > > Hmm.. I thought it didn't fix it for Kevin. Confused. I'm too, but it does fix things here. > >> Also we can try to see if critical clocks aren't being forced on by > >> applying this patch and looking for clk_get() failures > > And that shows that the CPU and DDR clocks are not protected, which > > obviously is pretty mad. > > > > I've mass converted all our probing code to use OF_CLK_DECLARE, and > > make things work again. > > > > http://code.bulix.org/5goa5j-88345?raw > > > > Is this an acceptable solution? > > > > We were already moving to this, I'm not really fond of doing this like > > that, but I guess this whole debacle makes it necessary. > > > > I wonder why we can't switch out the clk_ops on the affected platforms + > clocks to be read-only (at least for the enable/disable part)? That > would fix it just the same right? I wasn't around for the original > discussion regarding this always-on stuff so perhaps I've missed something. We're using clk-gate, so that would require to recode our own driver. That change seemed less invasive, while fixing the issue and ensuring that we wouldn't have any orphan clock, which seems to be hunted down these days... Maxime
Maxime Ripard <maxime.ripard@free-electrons.com> writes: > On Tue, May 12, 2015 at 03:35:50PM -0700, Stephen Boyd wrote: >> On 05/08/15 03:02, Maxime Ripard wrote: >> > On Thu, May 07, 2015 at 02:03:57PM -0700, Stephen Boyd wrote: >> >> On 05/07/15 08:17, Kevin Hilman wrote: >> >>> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> >>>> On 05/01/15 15:07, Heiko Stübner wrote: >> >>>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd: >> >>>>> >> >>>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the >> >>>>>>> following patch? >> >>>>>> It looks like it will work at least, but still I'd prefer to keep the >> >>>>>> orphan check contained to clk.c. How about this compile tested only patch? >> >>>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree >> >>>>> looks the same and it also still defers nicely in the scenario I needed it >> >>>>> for. The implementation also looks nice - and of course much more compact than >> >>>>> my check in two places :-) . I don't know if you want to put this as follow-up >> >>>>> on top or fold it into the original orphan-check, so in any case >> >>>>> >> >>>>> Tested-by: Heiko Stuebner <heiko@sntech.de> >> >>>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de> >> >>>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with >> >>>> my patch and a note that it's based on an earlier patch from you. >> >>> It appears this has landed in linux-next in the form of 882667c1fcf1 >> >>> clk: prevent orphan clocks from being used. A bunch of boot failures >> >>> for sunxi in today's linux-next[1] were bisected down to that patch. >> >>> >> >>> I confirmed that reverting that commit on top of next/master gets >> >>> sunxi booting again. >> >>> >> >>> >> >> Thanks for the report. I've removed the two clk orphan patches from >> >> clk-next. Would it be possible to try with next-20150507 and >> >> clk_ignore_unused on the command line? >> > This makes it work, but it's not really an option. >> > >> >> Hmm.. I thought it didn't fix it for Kevin. Confused. > > I'm too, but it does fix things here. To be more precise on what I tested. I used next-20150507 and tested on 4 different sunxi platforms. First test was "normal" commandline, second was with clk_ignore_unused appended: - cubie: fail, fail - cubie2: fail, fail - bananpi: fail, pass - cubietruck: fail, pass So it seems to have some effect, but by itself, doesn't fix the issue. Kevin
On Wed, May 13, 2015 at 07:33:54AM -0700, Kevin Hilman wrote: > Maxime Ripard <maxime.ripard@free-electrons.com> writes: > > > On Tue, May 12, 2015 at 03:35:50PM -0700, Stephen Boyd wrote: > >> On 05/08/15 03:02, Maxime Ripard wrote: > >> > On Thu, May 07, 2015 at 02:03:57PM -0700, Stephen Boyd wrote: > >> >> On 05/07/15 08:17, Kevin Hilman wrote: > >> >>> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > >> >>>> On 05/01/15 15:07, Heiko Stübner wrote: > >> >>>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd: > >> >>>>> > >> >>>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the > >> >>>>>>> following patch? > >> >>>>>> It looks like it will work at least, but still I'd prefer to keep the > >> >>>>>> orphan check contained to clk.c. How about this compile tested only patch? > >> >>>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree > >> >>>>> looks the same and it also still defers nicely in the scenario I needed it > >> >>>>> for. The implementation also looks nice - and of course much more compact than > >> >>>>> my check in two places :-) . I don't know if you want to put this as follow-up > >> >>>>> on top or fold it into the original orphan-check, so in any case > >> >>>>> > >> >>>>> Tested-by: Heiko Stuebner <heiko@sntech.de> > >> >>>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de> > >> >>>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with > >> >>>> my patch and a note that it's based on an earlier patch from you. > >> >>> It appears this has landed in linux-next in the form of 882667c1fcf1 > >> >>> clk: prevent orphan clocks from being used. A bunch of boot failures > >> >>> for sunxi in today's linux-next[1] were bisected down to that patch. > >> >>> > >> >>> I confirmed that reverting that commit on top of next/master gets > >> >>> sunxi booting again. > >> >>> > >> >>> > >> >> Thanks for the report. I've removed the two clk orphan patches from > >> >> clk-next. Would it be possible to try with next-20150507 and > >> >> clk_ignore_unused on the command line? > >> > This makes it work, but it's not really an option. > >> > > >> > >> Hmm.. I thought it didn't fix it for Kevin. Confused. > > > > I'm too, but it does fix things here. > > To be more precise on what I tested. I used next-20150507 and tested on > 4 different sunxi platforms. First test was "normal" commandline, > second was with clk_ignore_unused appended: > > - cubie: fail, fail > - cubie2: fail, fail > - bananpi: fail, pass > - cubietruck: fail, pass > > So it seems to have some effect, but by itself, doesn't fix the issue. It's very odd, I actually tried with a cubie2 here... I'm booting on an initramfs and not MMC though, but I can't see how that can be related to our issue... Maxime
On Wed, May 13, 2015 at 1:14 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Wed, May 13, 2015 at 07:33:54AM -0700, Kevin Hilman wrote: >> Maxime Ripard <maxime.ripard@free-electrons.com> writes: >> >> > On Tue, May 12, 2015 at 03:35:50PM -0700, Stephen Boyd wrote: >> >> On 05/08/15 03:02, Maxime Ripard wrote: >> >> > On Thu, May 07, 2015 at 02:03:57PM -0700, Stephen Boyd wrote: >> >> >> On 05/07/15 08:17, Kevin Hilman wrote: >> >> >>> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> >> >>>> On 05/01/15 15:07, Heiko Stübner wrote: >> >> >>>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd: >> >> >>>>> >> >> >>>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the >> >> >>>>>>> following patch? >> >> >>>>>> It looks like it will work at least, but still I'd prefer to keep the >> >> >>>>>> orphan check contained to clk.c. How about this compile tested only patch? >> >> >>>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree >> >> >>>>> looks the same and it also still defers nicely in the scenario I needed it >> >> >>>>> for. The implementation also looks nice - and of course much more compact than >> >> >>>>> my check in two places :-) . I don't know if you want to put this as follow-up >> >> >>>>> on top or fold it into the original orphan-check, so in any case >> >> >>>>> >> >> >>>>> Tested-by: Heiko Stuebner <heiko@sntech.de> >> >> >>>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de> >> >> >>>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with >> >> >>>> my patch and a note that it's based on an earlier patch from you. >> >> >>> It appears this has landed in linux-next in the form of 882667c1fcf1 >> >> >>> clk: prevent orphan clocks from being used. A bunch of boot failures >> >> >>> for sunxi in today's linux-next[1] were bisected down to that patch. >> >> >>> >> >> >>> I confirmed that reverting that commit on top of next/master gets >> >> >>> sunxi booting again. >> >> >>> >> >> >>> >> >> >> Thanks for the report. I've removed the two clk orphan patches from >> >> >> clk-next. Would it be possible to try with next-20150507 and >> >> >> clk_ignore_unused on the command line? >> >> > This makes it work, but it's not really an option. >> >> > >> >> >> >> Hmm.. I thought it didn't fix it for Kevin. Confused. >> > >> > I'm too, but it does fix things here. >> >> To be more precise on what I tested. I used next-20150507 and tested on >> 4 different sunxi platforms. First test was "normal" commandline, >> second was with clk_ignore_unused appended: >> >> - cubie: fail, fail >> - cubie2: fail, fail >> - bananpi: fail, pass >> - cubietruck: fail, pass >> >> So it seems to have some effect, but by itself, doesn't fix the issue. > > It's very odd, I actually tried with a cubie2 here... > > I'm booting on an initramfs and not MMC though, but I can't see how > that can be related to our issue... I'm booting an initramfs too. Kevin
On Wed, May 13, 2015 at 01:44:50PM -0700, Kevin Hilman wrote: > On Wed, May 13, 2015 at 1:14 PM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > On Wed, May 13, 2015 at 07:33:54AM -0700, Kevin Hilman wrote: > >> Maxime Ripard <maxime.ripard@free-electrons.com> writes: > >> > >> > On Tue, May 12, 2015 at 03:35:50PM -0700, Stephen Boyd wrote: > >> >> On 05/08/15 03:02, Maxime Ripard wrote: > >> >> > On Thu, May 07, 2015 at 02:03:57PM -0700, Stephen Boyd wrote: > >> >> >> On 05/07/15 08:17, Kevin Hilman wrote: > >> >> >>> On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > >> >> >>>> On 05/01/15 15:07, Heiko Stübner wrote: > >> >> >>>>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd: > >> >> >>>>> > >> >> >>>>>>> Instead I guess we could hook it less deep into clk_get_sys, like in the > >> >> >>>>>>> following patch? > >> >> >>>>>> It looks like it will work at least, but still I'd prefer to keep the > >> >> >>>>>> orphan check contained to clk.c. How about this compile tested only patch? > >> >> >>>>> I gave this a spin on my rk3288-firefly board. It still boots, the clock tree > >> >> >>>>> looks the same and it also still defers nicely in the scenario I needed it > >> >> >>>>> for. The implementation also looks nice - and of course much more compact than > >> >> >>>>> my check in two places :-) . I don't know if you want to put this as follow-up > >> >> >>>>> on top or fold it into the original orphan-check, so in any case > >> >> >>>>> > >> >> >>>>> Tested-by: Heiko Stuebner <heiko@sntech.de> > >> >> >>>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de> > >> >> >>>> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it with > >> >> >>>> my patch and a note that it's based on an earlier patch from you. > >> >> >>> It appears this has landed in linux-next in the form of 882667c1fcf1 > >> >> >>> clk: prevent orphan clocks from being used. A bunch of boot failures > >> >> >>> for sunxi in today's linux-next[1] were bisected down to that patch. > >> >> >>> > >> >> >>> I confirmed that reverting that commit on top of next/master gets > >> >> >>> sunxi booting again. > >> >> >>> > >> >> >>> > >> >> >> Thanks for the report. I've removed the two clk orphan patches from > >> >> >> clk-next. Would it be possible to try with next-20150507 and > >> >> >> clk_ignore_unused on the command line? > >> >> > This makes it work, but it's not really an option. > >> >> > > >> >> > >> >> Hmm.. I thought it didn't fix it for Kevin. Confused. > >> > > >> > I'm too, but it does fix things here. > >> > >> To be more precise on what I tested. I used next-20150507 and tested on > >> 4 different sunxi platforms. First test was "normal" commandline, > >> second was with clk_ignore_unused appended: > >> > >> - cubie: fail, fail > >> - cubie2: fail, fail > >> - bananpi: fail, pass > >> - cubietruck: fail, pass > >> > >> So it seems to have some effect, but by itself, doesn't fix the issue. > > > > It's very odd, I actually tried with a cubie2 here... > > > > I'm booting on an initramfs and not MMC though, but I can't see how > > that can be related to our issue... > > I'm booting an initramfs too. Then I don't know :)
Hi Maxime, Stephen, Am Freitag, 8. Mai 2015, 12:02:47 schrieb Maxime Ripard: > On Thu, May 07, 2015 at 02:03:57PM -0700, Stephen Boyd wrote: > > On 05/07/15 08:17, Kevin Hilman wrote: > > > On Fri, May 1, 2015 at 4:40 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > >> On 05/01/15 15:07, Heiko Stübner wrote: > > >>> Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd: > > >>>>> Instead I guess we could hook it less deep into clk_get_sys, like in > > >>>>> the > > >>>>> following patch? > > >>>> > > >>>> It looks like it will work at least, but still I'd prefer to keep the > > >>>> orphan check contained to clk.c. How about this compile tested only > > >>>> patch? > > >>> > > >>> I gave this a spin on my rk3288-firefly board. It still boots, the > > >>> clock tree looks the same and it also still defers nicely in the > > >>> scenario I needed it for. The implementation also looks nice - and of > > >>> course much more compact than my check in two places :-) . I don't > > >>> know if you want to put this as follow-up on top or fold it into the > > >>> original orphan-check, so in any case > > >>> > > >>> Tested-by: Heiko Stuebner <heiko@sntech.de> > > >>> Reviewed-by: Heiko Stuebner <heiko@sntech.de> > > >> > > >> Thanks. I'm leaning towards tossing your patch 2/2 and replacing it > > >> with > > >> my patch and a note that it's based on an earlier patch from you. > > > > > > It appears this has landed in linux-next in the form of 882667c1fcf1 > > > clk: prevent orphan clocks from being used. A bunch of boot failures > > > for sunxi in today's linux-next[1] were bisected down to that patch. > > > > > > I confirmed that reverting that commit on top of next/master gets > > > sunxi booting again. > > > > Thanks for the report. I've removed the two clk orphan patches from > > clk-next. Would it be possible to try with next-20150507 and > > clk_ignore_unused on the command line? > > This makes it work, but it's not really an option. > > > Also we can try to see if critical clocks aren't being forced on by > > applying this patch and looking for clk_get() failures > > And that shows that the CPU and DDR clocks are not protected, which > obviously is pretty mad. > > I've mass converted all our probing code to use OF_CLK_DECLARE, and > make things work again. > > http://code.bulix.org/5goa5j-88345?raw > > Is this an acceptable solution? > > We were already moving to this, I'm not really fond of doing this like > that, but I guess this whole debacle makes it necessary. did this lead anywhere meanwhile. Last I remember the change to orphan handling made sunxi fail, but I'm still hoping to get this usable at some point :-) Thanks Heiko
Hi Heiko, On Mon, Jul 27, 2015 at 10:57:33AM +0200, Heiko Stübner wrote: > > > Also we can try to see if critical clocks aren't being forced on by > > > applying this patch and looking for clk_get() failures > > > > And that shows that the CPU and DDR clocks are not protected, which > > obviously is pretty mad. > > > > I've mass converted all our probing code to use OF_CLK_DECLARE, and > > make things work again. > > > > http://code.bulix.org/5goa5j-88345?raw > > > > Is this an acceptable solution? > > > > We were already moving to this, I'm not really fond of doing this like > > that, but I guess this whole debacle makes it necessary. > > > did this lead anywhere meanwhile. > > Last I remember the change to orphan handling made sunxi fail, but > I'm still hoping to get this usable at some point :-) To be honest, I don't know what the current status is. I haven't get any news since that mail. I started to move a significant portion of our clocks to CLK_OF_DECLARE, but not all of them are (which probably make the situation worse for the time being). Maxime
On 07/30, Maxime Ripard wrote: > On Mon, Jul 27, 2015 at 10:57:33AM +0200, Heiko Stübner wrote: > > > > did this lead anywhere meanwhile. > > > > Last I remember the change to orphan handling made sunxi fail, but > > I'm still hoping to get this usable at some point :-) > > To be honest, I don't know what the current status is. I haven't get > any news since that mail. > > I started to move a significant portion of our clocks to > CLK_OF_DECLARE, but not all of them are (which probably make the > situation worse for the time being). > Sorry, I think we're still waiting for the critical clocks stuff to settle down so that Sunxi can use that instead of calling clk_prepare_enable() in init code. For now, I've put the first patch of the two into clk-next so that we can have proper orphan tracking. Hopefully we resolve critical clocks soon so that we can merge the defer part.
Am Dienstag, 11. August 2015, 15:34:09 schrieb Stephen Boyd: > On 07/30, Maxime Ripard wrote: > > On Mon, Jul 27, 2015 at 10:57:33AM +0200, Heiko Stübner wrote: > > > did this lead anywhere meanwhile. > > > > > > Last I remember the change to orphan handling made sunxi fail, but > > > I'm still hoping to get this usable at some point :-) > > > > To be honest, I don't know what the current status is. I haven't get > > any news since that mail. > > > > I started to move a significant portion of our clocks to > > CLK_OF_DECLARE, but not all of them are (which probably make the > > situation worse for the time being). > > Sorry, I think we're still waiting for the critical clocks stuff > to settle down so that Sunxi can use that instead of calling > clk_prepare_enable() in init code. For now, I've put the first > patch of the two into clk-next so that we can have proper orphan > tracking. Hopefully we resolve critical clocks soon so that we > can merge the defer part. great :-) For the defer-part you came up with a better solution than mine anyway, if I remember correctly.
diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c index 7e1e2bd189b6..d88585b680bb 100644 --- a/drivers/clk/sunxi/clk-sunxi.c +++ b/drivers/clk/sunxi/clk-sunxi.c @@ -1347,6 +1347,9 @@ static void __init sunxi_init_clocks(const char *clocks[], int nclocks) if (!IS_ERR(clk)) clk_prepare_enable(clk); + else + pr_err("Failed to enable critical clock %s\n", + clocks[i]); } }