| Message ID | 1406824943-24052-7-git-send-email-matthias.bgg@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 31, 2014 at 6:42 PM, Matthias Brugger <matthias.bgg@gmail.com> wrote: > We enable GTP6 which ungates the arch timer clock. Apart we write the > frequency with which the timer is running in the CNTFREQ register. > In the future this should be done in the bootloader. > > Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com> (...) > + if (of_machine_is_compatible("mediatek,mt6589")) { > + /* set cntfreq register which is not done in bootloader */ > + asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (13000000)); I guess this is for something like the ARM arch timer in drivers/clocksource/arm_arch_timer.c Instead of doing this, use the DT property "clock-frequency" on the ARM arch timer node, as that overrides the CP15 setting. > + > + /* turn on GPT6 which ungates arch timer clocks */ > + gpt_base = ioremap(GPT6_CON_MT65xx, 0x04); > + } > + > + /* enabel clock and set to free-run */ > + if (gpt_base) > + writel(0x31, gpt_base); Why is this not done properly in the GPT driver (I guess in drivers/clocksource/mtk_timer.c) instead of remapping it and fiddling around in the machine? If it only affects the mt6589 just add another compatible string to that driver like "mediatek,mt6589-timer" as it properly reflects the fact that this timer is slightly different on the 6589. Also use a #define for the magic constant 0x31. Yours, Linus Walleij
2014-08-11 9:15 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: > On Thu, Jul 31, 2014 at 6:42 PM, Matthias Brugger > <matthias.bgg@gmail.com> wrote: > >> We enable GTP6 which ungates the arch timer clock. Apart we write the >> frequency with which the timer is running in the CNTFREQ register. >> In the future this should be done in the bootloader. >> >> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com> > (...) > >> + if (of_machine_is_compatible("mediatek,mt6589")) { >> + /* set cntfreq register which is not done in bootloader */ >> + asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (13000000)); > > I guess this is for something like the ARM arch timer in > drivers/clocksource/arm_arch_timer.c > > Instead of doing this, use the DT property "clock-frequency" on > the ARM arch timer node, as that overrides the CP15 setting. > >> + >> + /* turn on GPT6 which ungates arch timer clocks */ >> + gpt_base = ioremap(GPT6_CON_MT65xx, 0x04); >> + } >> + >> + /* enabel clock and set to free-run */ >> + if (gpt_base) >> + writel(0x31, gpt_base); > > Why is this not done properly in the GPT driver (I guess > in drivers/clocksource/mtk_timer.c) instead of remapping > it and fiddling around in the machine? I didn't put it in the GPT driver, because that would mean, that you need to have the mtk_timer to be able to use the ARM arch timer. From my understanding this are two independent blocks in the SoC. Apart from that, as stated in the commit message, this patch is a workaround until we have a bootloader which does this for us. Therefore I thought the init_time function would be the better place to do this. Thanks, Matthias > > If it only affects the mt6589 just add another compatible string > to that driver like "mediatek,mt6589-timer" as it properly reflects the > fact that this timer is slightly different on the 6589. > > Also use a #define for the magic constant 0x31. > > Yours, > Linus Walleij
On Tue, Aug 12, 2014 at 4:02 AM, Matthias Brugger <matthias.bgg@gmail.com> wrote: > 2014-08-11 9:15 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: >> On Thu, Jul 31, 2014 at 6:42 PM, Matthias Brugger >> <matthias.bgg@gmail.com> wrote: >> >>> We enable GTP6 which ungates the arch timer clock. Apart we write the >>> frequency with which the timer is running in the CNTFREQ register. >>> In the future this should be done in the bootloader. >>> >>> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com> >> (...) >> >>> + if (of_machine_is_compatible("mediatek,mt6589")) { >>> + /* set cntfreq register which is not done in bootloader */ >>> + asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (13000000)); >> >> I guess this is for something like the ARM arch timer in >> drivers/clocksource/arm_arch_timer.c >> >> Instead of doing this, use the DT property "clock-frequency" on >> the ARM arch timer node, as that overrides the CP15 setting. >> >>> + >>> + /* turn on GPT6 which ungates arch timer clocks */ >>> + gpt_base = ioremap(GPT6_CON_MT65xx, 0x04); >>> + } >>> + >>> + /* enabel clock and set to free-run */ >>> + if (gpt_base) >>> + writel(0x31, gpt_base); >> >> Why is this not done properly in the GPT driver (I guess >> in drivers/clocksource/mtk_timer.c) instead of remapping >> it and fiddling around in the machine? > > I didn't put it in the GPT driver, because that would mean, that you > need to have the mtk_timer to be able to use the ARM arch timer. > From my understanding this are two independent blocks in the SoC. > Apart from that, as stated in the commit message, this patch is a > workaround until we have a bootloader which does this for us. > Therefore I thought the init_time function would be the better place to do this. I agree that the bootloader is the right place, and the fixup in the machine code until that is in place is the right place for the fixup. Rob
On Tue, Aug 12, 2014 at 11:02 AM, Matthias Brugger <matthias.bgg@gmail.com> wrote: > 2014-08-11 9:15 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: >> On Thu, Jul 31, 2014 at 6:42 PM, Matthias Brugger >> <matthias.bgg@gmail.com> wrote: >> >>> We enable GTP6 which ungates the arch timer clock. Apart we write the >>> frequency with which the timer is running in the CNTFREQ register. >>> In the future this should be done in the bootloader. >>> >>> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com> >> (...) >> >>> + if (of_machine_is_compatible("mediatek,mt6589")) { >>> + /* set cntfreq register which is not done in bootloader */ >>> + asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (13000000)); >> >> I guess this is for something like the ARM arch timer in >> drivers/clocksource/arm_arch_timer.c >> >> Instead of doing this, use the DT property "clock-frequency" on >> the ARM arch timer node, as that overrides the CP15 setting. >> >>> + >>> + /* turn on GPT6 which ungates arch timer clocks */ >>> + gpt_base = ioremap(GPT6_CON_MT65xx, 0x04); >>> + } >>> + >>> + /* enabel clock and set to free-run */ >>> + if (gpt_base) >>> + writel(0x31, gpt_base); >> >> Why is this not done properly in the GPT driver (I guess >> in drivers/clocksource/mtk_timer.c) instead of remapping >> it and fiddling around in the machine? > > I didn't put it in the GPT driver, because that would mean, that you > need to have the mtk_timer to be able to use the ARM arch timer. > From my understanding this are two independent blocks in the SoC. > Apart from that, as stated in the commit message, this patch is a > workaround until we have a bootloader which does this for us. > Therefore I thought the init_time function would be the better place to do this. OK I get the point... What about the ARM arch timer fixup then, will that also be fixed in the boot loader eventually? Yours, Linus Walleij
2014-08-13 10:47 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: > On Tue, Aug 12, 2014 at 11:02 AM, Matthias Brugger > <matthias.bgg@gmail.com> wrote: >> 2014-08-11 9:15 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: >>> On Thu, Jul 31, 2014 at 6:42 PM, Matthias Brugger >>> <matthias.bgg@gmail.com> wrote: >>> >>>> We enable GTP6 which ungates the arch timer clock. Apart we write the >>>> frequency with which the timer is running in the CNTFREQ register. >>>> In the future this should be done in the bootloader. >>>> >>>> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com> >>> (...) >>> >>>> + if (of_machine_is_compatible("mediatek,mt6589")) { >>>> + /* set cntfreq register which is not done in bootloader */ >>>> + asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (13000000)); >>> >>> I guess this is for something like the ARM arch timer in >>> drivers/clocksource/arm_arch_timer.c >>> >>> Instead of doing this, use the DT property "clock-frequency" on >>> the ARM arch timer node, as that overrides the CP15 setting. >>> >>>> + >>>> + /* turn on GPT6 which ungates arch timer clocks */ >>>> + gpt_base = ioremap(GPT6_CON_MT65xx, 0x04); >>>> + } >>>> + >>>> + /* enabel clock and set to free-run */ >>>> + if (gpt_base) >>>> + writel(0x31, gpt_base); >>> >>> Why is this not done properly in the GPT driver (I guess >>> in drivers/clocksource/mtk_timer.c) instead of remapping >>> it and fiddling around in the machine? >> >> I didn't put it in the GPT driver, because that would mean, that you >> need to have the mtk_timer to be able to use the ARM arch timer. >> From my understanding this are two independent blocks in the SoC. >> Apart from that, as stated in the commit message, this patch is a >> workaround until we have a bootloader which does this for us. >> Therefore I thought the init_time function would be the better place to do this. > > OK I get the point... > > What about the ARM arch timer fixup then, will that also be > fixed in the boot loader eventually? Do you refer to clock frequency in the cntfreq register? I think that this should go in the DTS as you said. Same holds for the magic 0x31, which should be done through defines. Cheers, Matthias > > Yours, > Linus Walleij
On Tue, 2014-08-12 at 11:02 +0200, Matthias Brugger wrote: > 2014-08-11 9:15 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: > > On Thu, Jul 31, 2014 at 6:42 PM, Matthias Brugger > > <matthias.bgg@gmail.com> wrote: > > > >> We enable GTP6 which ungates the arch timer clock. Apart we write the > >> frequency with which the timer is running in the CNTFREQ register. > >> In the future this should be done in the bootloader. > >> > >> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com> > > (...) > > > >> + if (of_machine_is_compatible("mediatek,mt6589")) { > >> + /* set cntfreq register which is not done in bootloader */ > >> + asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (13000000)); > > > > I guess this is for something like the ARM arch timer in > > drivers/clocksource/arm_arch_timer.c > > > > Instead of doing this, use the DT property "clock-frequency" on > > the ARM arch timer node, as that overrides the CP15 setting. > > > >> + > >> + /* turn on GPT6 which ungates arch timer clocks */ > >> + gpt_base = ioremap(GPT6_CON_MT65xx, 0x04); > >> + } > >> + > >> + /* enabel clock and set to free-run */ > >> + if (gpt_base) > >> + writel(0x31, gpt_base); > > > > Why is this not done properly in the GPT driver (I guess > > in drivers/clocksource/mtk_timer.c) instead of remapping > > it and fiddling around in the machine? > > I didn't put it in the GPT driver, because that would mean, that you > need to have the mtk_timer to be able to use the ARM arch timer. > From my understanding this are two independent blocks in the SoC. > Apart from that, as stated in the commit message, this patch is a > workaround until we have a bootloader which does this for us. > Therefore I thought the init_time function would be the better place to do this. Can we have a separate node like mt6577-timerfixup then? I'm preparing patch for mt8127 and mt8135. They all need this and use same address and. In current form, I'll have to make this check against a compatible string array. Joe.C
2014-08-14 4:24 GMT+02:00 Joe.C <srv_yingjoe.chen@mediatek.com>: > On Tue, 2014-08-12 at 11:02 +0200, Matthias Brugger wrote: >> 2014-08-11 9:15 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: >> > On Thu, Jul 31, 2014 at 6:42 PM, Matthias Brugger >> > <matthias.bgg@gmail.com> wrote: >> > >> >> We enable GTP6 which ungates the arch timer clock. Apart we write the >> >> frequency with which the timer is running in the CNTFREQ register. >> >> In the future this should be done in the bootloader. >> >> >> >> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com> >> > (...) >> > >> >> + if (of_machine_is_compatible("mediatek,mt6589")) { >> >> + /* set cntfreq register which is not done in bootloader */ >> >> + asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (13000000)); >> > >> > I guess this is for something like the ARM arch timer in >> > drivers/clocksource/arm_arch_timer.c >> > >> > Instead of doing this, use the DT property "clock-frequency" on >> > the ARM arch timer node, as that overrides the CP15 setting. >> > >> >> + >> >> + /* turn on GPT6 which ungates arch timer clocks */ >> >> + gpt_base = ioremap(GPT6_CON_MT65xx, 0x04); >> >> + } >> >> + >> >> + /* enabel clock and set to free-run */ >> >> + if (gpt_base) >> >> + writel(0x31, gpt_base); >> > >> > Why is this not done properly in the GPT driver (I guess >> > in drivers/clocksource/mtk_timer.c) instead of remapping >> > it and fiddling around in the machine? >> >> I didn't put it in the GPT driver, because that would mean, that you >> need to have the mtk_timer to be able to use the ARM arch timer. >> From my understanding this are two independent blocks in the SoC. >> Apart from that, as stated in the commit message, this patch is a >> workaround until we have a bootloader which does this for us. >> Therefore I thought the init_time function would be the better place to do this. > > Can we have a separate node like mt6577-timerfixup then? > I'm preparing patch for mt8127 and mt8135. They all need this and use > same address and. In current form, I'll have to make this check against > a compatible string array. The way to go is to add another of_machine_is_compatible for every SoC. If the address is the same just use logical or. Cheers, Matthias > > Joe.C > >
Hi Matthias, I didn't saw this in v3.18-rc1, are you working on version 2 of this? mt8135 and m8127 also need this fixup and we need this to have SMP working. Joe.C
Hi Joe, 2014-10-24 15:53 GMT+02:00 Yingjoe Chen <srv_yingjoe.chen@mediatek.com>: > > Hi Matthias, > > I didn't saw this in v3.18-rc1, are you working on version 2 of this? > mt8135 and m8127 also need this fixup and we need this to have SMP > working. I had a discussion with Mark Rutland from ARM on this [0]. At that point he convinced me that this was not critical. If we need this for SMP, things really change here I think. Cheers, Matthias [0] https://lkml.org/lkml/2014/8/21/170 > > Joe.C > >
Hi Matthias, On Mon, 2014-10-27 at 11:28 +0100, Matthias Brugger wrote: > Hi Joe, > > 2014-10-24 15:53 GMT+02:00 Yingjoe Chen <srv_yingjoe.chen@mediatek.com>: > > > > Hi Matthias, > > > > I didn't saw this in v3.18-rc1, are you working on version 2 of this? > > mt8135 and m8127 also need this fixup and we need this to have SMP > > working. > > I had a discussion with Mark Rutland from ARM on this [0]. > At that point he convinced me that this was not critical. > > If we need this for SMP, things really change here I think. We have some driver now, and we started to implement SMP enable method internally. I think it is reasonable to discuss this now. We could include this patch in our SMP patch series. Do you prefer we do this? Joe.C
2014-10-30 4:23 GMT+01:00 Yingjoe Chen <yingjoe.chen@mediatek.com>: > Hi Matthias, > > On Mon, 2014-10-27 at 11:28 +0100, Matthias Brugger wrote: >> Hi Joe, >> >> 2014-10-24 15:53 GMT+02:00 Yingjoe Chen <srv_yingjoe.chen@mediatek.com>: >> > >> > Hi Matthias, >> > >> > I didn't saw this in v3.18-rc1, are you working on version 2 of this? >> > mt8135 and m8127 also need this fixup and we need this to have SMP >> > working. >> >> I had a discussion with Mark Rutland from ARM on this [0]. >> At that point he convinced me that this was not critical. >> >> If we need this for SMP, things really change here I think. > > We have some driver now, and we started to implement SMP enable method > internally. I think it is reasonable to discuss this now. > We could include this patch in our SMP patch series. Do you prefer we do > this? Alright. Please remind that we don't need to set the cntfreq register here but define the clock frequency in the dts. Thanks, Matthias > > Joe.C > > >
diff --git a/arch/arm/mach-mediatek/mediatek.c b/arch/arm/mach-mediatek/mediatek.c index f2acf07..b7c5c60 100644 --- a/arch/arm/mach-mediatek/mediatek.c +++ b/arch/arm/mach-mediatek/mediatek.c @@ -16,6 +16,32 @@ */ #include <linux/init.h> #include <asm/mach/arch.h> +#include <linux/of.h> +#include <linux/clk-provider.h> +#include <linux/clocksource.h> + + +#define GPT6_CON_MT65xx 0x10008060 + +static void __init mediatek_timer_init(void) +{ + static void __iomem *gpt_base; + + if (of_machine_is_compatible("mediatek,mt6589")) { + /* set cntfreq register which is not done in bootloader */ + asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (13000000)); + + /* turn on GPT6 which ungates arch timer clocks */ + gpt_base = ioremap(GPT6_CON_MT65xx, 0x04); + } + + /* enabel clock and set to free-run */ + if (gpt_base) + writel(0x31, gpt_base); + + of_clk_init(NULL); + clocksource_of_init(); +}; static const char * const mediatek_board_dt_compat[] = { "mediatek,mt6589", @@ -24,4 +50,5 @@ static const char * const mediatek_board_dt_compat[] = { DT_MACHINE_START(MEDIATEK_DT, "Mediatek Cortex-A7 (Device Tree)") .dt_compat = mediatek_board_dt_compat, + .init_time = mediatek_timer_init, MACHINE_END
We enable GTP6 which ungates the arch timer clock. Apart we write the frequency with which the timer is running in the CNTFREQ register. In the future this should be done in the bootloader. Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com> --- arch/arm/mach-mediatek/mediatek.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)