| Message ID | 1936935.FVZdWRYuBf@diego |
|---|---|
| State | New |
| Headers | show |
On Saturday 05 December 2015 01:53:38 Heiko Stübner wrote: > SMP special case for the rk3036 and making sure the arch-timer > supply is enabled, similar to the rk3288. That is a rather ugly hack, I'd prefer if this could be done cleaner rather than duplicated into another place. Sorry for not seeing this earlier. Can you replace the hardcoded RK3036_TIMER_PHYS and RK3288_TIMER6_7_PHYS constants with a DT lookup, to make it somewhat less hacky? Arnd
Hi Arnd, Am Samstag, 12. Dezember 2015, 01:30:17 schrieb Arnd Bergmann: > On Saturday 05 December 2015 01:53:38 Heiko Stübner wrote: > > SMP special case for the rk3036 and making sure the arch-timer > > supply is enabled, similar to the rk3288. > > That is a rather ugly hack, I'd prefer if this could be done cleaner > rather than duplicated into another place. I do agree that it is rather ugly :-) . The general opinion seems to be, that firmware is supposed to make sure this timer is enabled and at the time when it got introduced the consensus was to not hack up the arch-timer to facilitate this and "just" put it into the soc code for the affected cpu. It seems like the really new rk3228 (quad-core A7 and using psci now) actually gets this right now, in having its firmware taking care of this. So it looks like this hopefully won't be needed even for future arm32 socs. I guess I should look at what the new and shiny mainline uboot support does for this, but I do think it might actually do it right as well. So this is really only a hack for flaky vendor-bootloaders. Which brings me to ... > Sorry for not seeing this earlier. Can you replace the hardcoded > RK3036_TIMER_PHYS and RK3288_TIMER6_7_PHYS constants with a DT > lookup, to make it somewhat less hacky? I'm not really sure how that is supposed to look like. Technically nothing should ever touch that timer, as it will only make the system hang if it gets disabled. We do have a binding for the timer ip block (rockchip,rk3288-timer) but of course cannot use that, to make sure the regular timer driver doesn't bind to it. So I guess we could do something like: timer@200440a0 { compatible = "rockchip,arch-timer-supply"; reg = <0x200440a0 0x20>; }; try to find this and enable it, but duplicating the hack and spreading it into the dts as well somehow doesn't feel like an improvement ;-) . But I maybe you have a nicer idea on how to do this, than me. Thanks Heiko
Heiko, On Fri, Dec 11, 2015 at 4:57 PM, Heiko Stübner <heiko@sntech.de> wrote: > Hi Arnd, > > Am Samstag, 12. Dezember 2015, 01:30:17 schrieb Arnd Bergmann: >> On Saturday 05 December 2015 01:53:38 Heiko Stübner wrote: >> > SMP special case for the rk3036 and making sure the arch-timer >> > supply is enabled, similar to the rk3288. >> >> That is a rather ugly hack, I'd prefer if this could be done cleaner >> rather than duplicated into another place. > > I do agree that it is rather ugly :-) . > > The general opinion seems to be, that firmware is supposed to make sure this > timer is enabled and at the time when it got introduced the consensus was to > not hack up the arch-timer to facilitate this and "just" put it into the soc > code for the affected cpu. > > It seems like the really new rk3228 (quad-core A7 and using psci now) actually > gets this right now, in having its firmware taking care of this. So it looks > like this hopefully won't be needed even for future arm32 socs. > > I guess I should look at what the new and shiny mainline uboot support does > for this, but I do think it might actually do it right as well. > > So this is really only a hack for flaky vendor-bootloaders. > > > Which brings me to ... > >> Sorry for not seeing this earlier. Can you replace the hardcoded >> RK3036_TIMER_PHYS and RK3288_TIMER6_7_PHYS constants with a DT >> lookup, to make it somewhat less hacky? > > I'm not really sure how that is supposed to look like. Technically nothing > should ever touch that timer, as it will only make the system hang if it gets > disabled. > > We do have a binding for the timer ip block (rockchip,rk3288-timer) but of > course cannot use that, to make sure the regular timer driver doesn't bind to > it. So I guess we could do something like: > > > timer@200440a0 { > compatible = "rockchip,arch-timer-supply"; > reg = <0x200440a0 0x20>; > }; > > try to find this and enable it, but duplicating the hack and spreading it into > the dts as well somehow doesn't feel like an improvement ;-) . > > But I maybe you have a nicer idea on how to do this, than me. In a kernel tree drop from Rockchip I noticed that they're still carrying around the patch "ARM: rockchip: make sure timer5 is enabled on rk3036 platforms" as if it made it upstream, but as per this thread it didn't. Does someone have ownership resolving this? Perhaps everyone has updated to a bootloader that avoids the need for this patch now? -Doug
Am Montag, 7. März 2016, 16:46:04 schrieb Doug Anderson: > On Fri, Dec 11, 2015 at 4:57 PM, Heiko Stübner <heiko@sntech.de> wrote: > > Hi Arnd, > > > > Am Samstag, 12. Dezember 2015, 01:30:17 schrieb Arnd Bergmann: > >> On Saturday 05 December 2015 01:53:38 Heiko Stübner wrote: > >> > SMP special case for the rk3036 and making sure the arch-timer > >> > supply is enabled, similar to the rk3288. > >> > >> That is a rather ugly hack, I'd prefer if this could be done cleaner > >> rather than duplicated into another place. > > > > I do agree that it is rather ugly :-) . > > > > The general opinion seems to be, that firmware is supposed to make sure > > this timer is enabled and at the time when it got introduced the > > consensus was to not hack up the arch-timer to facilitate this and "just" > > put it into the soc code for the affected cpu. > > > > It seems like the really new rk3228 (quad-core A7 and using psci now) > > actually gets this right now, in having its firmware taking care of this. > > So it looks like this hopefully won't be needed even for future arm32 > > socs. > > > > I guess I should look at what the new and shiny mainline uboot support > > does > > for this, but I do think it might actually do it right as well. > > > > So this is really only a hack for flaky vendor-bootloaders. > > > > > > Which brings me to ... > > > >> Sorry for not seeing this earlier. Can you replace the hardcoded > >> RK3036_TIMER_PHYS and RK3288_TIMER6_7_PHYS constants with a DT > >> lookup, to make it somewhat less hacky? > > > > I'm not really sure how that is supposed to look like. Technically nothing > > should ever touch that timer, as it will only make the system hang if it > > gets disabled. > > > > We do have a binding for the timer ip block (rockchip,rk3288-timer) but of > > course cannot use that, to make sure the regular timer driver doesn't bind > > to it. So I guess we could do something like: > > > > > > timer@200440a0 { > > > > compatible = "rockchip,arch-timer-supply"; > > reg = <0x200440a0 0x20>; > > > > }; > > > > try to find this and enable it, but duplicating the hack and spreading it > > into the dts as well somehow doesn't feel like an improvement ;-) . > > > > But I maybe you have a nicer idea on how to do this, than me. > > In a kernel tree drop from Rockchip I noticed that they're still > carrying around the patch "ARM: rockchip: make sure timer5 is enabled > on rk3036 platforms" as if it made it upstream, but as per this thread > it didn't. > > Does someone have ownership resolving this? Perhaps everyone has > updated to a bootloader that avoids the need for this patch now? running on kylin, with the provided uboot sources (mainline uboot + some patches) the rk3036 runs just fine, without that timer enablement being needed. So if nobody shouts (and does the needed work), I intend to just let it drop. Heiko
Heiko, On Mon, Mar 7, 2016 at 4:50 PM, Heiko Stübner <heiko@sntech.de> wrote: > Am Montag, 7. März 2016, 16:46:04 schrieb Doug Anderson: >> On Fri, Dec 11, 2015 at 4:57 PM, Heiko Stübner <heiko@sntech.de> wrote: >> > Hi Arnd, >> > >> > Am Samstag, 12. Dezember 2015, 01:30:17 schrieb Arnd Bergmann: >> >> On Saturday 05 December 2015 01:53:38 Heiko Stübner wrote: >> >> > SMP special case for the rk3036 and making sure the arch-timer >> >> > supply is enabled, similar to the rk3288. >> >> >> >> That is a rather ugly hack, I'd prefer if this could be done cleaner >> >> rather than duplicated into another place. >> > >> > I do agree that it is rather ugly :-) . >> > >> > The general opinion seems to be, that firmware is supposed to make sure >> > this timer is enabled and at the time when it got introduced the >> > consensus was to not hack up the arch-timer to facilitate this and "just" >> > put it into the soc code for the affected cpu. >> > >> > It seems like the really new rk3228 (quad-core A7 and using psci now) >> > actually gets this right now, in having its firmware taking care of this. >> > So it looks like this hopefully won't be needed even for future arm32 >> > socs. >> > >> > I guess I should look at what the new and shiny mainline uboot support >> > does >> > for this, but I do think it might actually do it right as well. >> > >> > So this is really only a hack for flaky vendor-bootloaders. >> > >> > >> > Which brings me to ... >> > >> >> Sorry for not seeing this earlier. Can you replace the hardcoded >> >> RK3036_TIMER_PHYS and RK3288_TIMER6_7_PHYS constants with a DT >> >> lookup, to make it somewhat less hacky? >> > >> > I'm not really sure how that is supposed to look like. Technically nothing >> > should ever touch that timer, as it will only make the system hang if it >> > gets disabled. >> > >> > We do have a binding for the timer ip block (rockchip,rk3288-timer) but of >> > course cannot use that, to make sure the regular timer driver doesn't bind >> > to it. So I guess we could do something like: >> > >> > >> > timer@200440a0 { >> > >> > compatible = "rockchip,arch-timer-supply"; >> > reg = <0x200440a0 0x20>; >> > >> > }; >> > >> > try to find this and enable it, but duplicating the hack and spreading it >> > into the dts as well somehow doesn't feel like an improvement ;-) . >> > >> > But I maybe you have a nicer idea on how to do this, than me. >> >> In a kernel tree drop from Rockchip I noticed that they're still >> carrying around the patch "ARM: rockchip: make sure timer5 is enabled >> on rk3036 platforms" as if it made it upstream, but as per this thread >> it didn't. >> >> Does someone have ownership resolving this? Perhaps everyone has >> updated to a bootloader that avoids the need for this patch now? > > running on kylin, with the provided uboot sources (mainline uboot + some > patches) the rk3036 runs just fine, without that timer enablement being needed. > > So if nobody shouts (and does the needed work), I intend to just let it drop. Sounds like a great plan to me. Thanks! :) -Doug
Hi Arnd, Kevin, Olof, please find below rockchip soc changes for 4.5 . There exists one possible merge conflict with armsoc/cleanup, due to the newly added const/__initconst attributes in the smp_operations. But I've already also adapted the new smp operation struct accordingly. The following changes since commit 8005c49d9aea74d382f474ce11afbbc7d7130bec: Linux 4.4-rc1 (2015-11-15 17:00:27 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git tags/v4.5-rockchip-soc for you to fetch changes up to 24fdb345d7a9e1ee8dc6fe138a250c63f918fed5: ARM: rockchip: use const and __initconst for rk3036 smp_operations (2015-12-04 18:36:02 +0100) ---------------------------------------------------------------- SMP special case for the rk3036 and making sure the arch-timer supply is enabled, similar to the rk3288. ---------------------------------------------------------------- Heiko Stuebner (2): ARM: rockchip: add support smp for rk3036 ARM: rockchip: use const and __initconst for rk3036 smp_operations Xing Zheng (1): ARM: rockchip: make sure timer5 is enabled on rk3036 platforms Documentation/devicetree/bindings/arm/cpus.txt | 1 + arch/arm/mach-rockchip/platsmp.c | 45 +++++++++++++++++++------- arch/arm/mach-rockchip/rockchip.c | 44 +++++++++++++++---------- 3 files changed, 61 insertions(+), 29 deletions(-)