| Message ID | 1403167145-5267-1-git-send-email-amit.daniel@samsung.com |
|---|---|
| State | New |
| Headers | show |
On 06/19/2014 10:39 AM, Amit Daniel Kachhap wrote: > This patch register the exynos mct clocksource as the current timer > as it has constant clock rate. This will generate correct udelay for the > exynos platform and avoid using unnecessary calibrated jiffies. This change > has been tested on exynos5420 based board and udelay is very close to > expected. > > Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com> > --- > Changes in V2: > * Added #defines for ARM and ARM64 as pointed by Doug Anderson. > > Patches from David Riley confirmed that udelay is broken in exynos5420. > Link to those patches are, > 1) https://patchwork.kernel.org/patch/4344911/ > 2) https://patchwork.kernel.org/patch/4344881/ > > drivers/clocksource/exynos_mct.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c > index f71d55f..02927e2 100644 > --- a/drivers/clocksource/exynos_mct.c > +++ b/drivers/clocksource/exynos_mct.c > @@ -195,10 +195,25 @@ static u64 notrace exynos4_read_sched_clock(void) > return exynos4_frc_read(&mct_frc); > } > > +static struct delay_timer exynos4_delay_timer; > + > +static unsigned long exynos4_read_current_timer(void) > +{ > +#ifdef ARM > + return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L); > +#else /* ARM64, etc */ > + return exynos4_frc_read(&mct_frc); > +#endif > +} > + There isn't another solution than that ? macros definitions in C file are avoided as much as possible. > static void __init exynos4_clocksource_init(void) > { > exynos4_mct_frc_start(); > > + exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer; &exynos4_read_current_timer ? > + exynos4_delay_timer.freq = clk_rate; > + register_current_timer_delay(&exynos4_delay_timer); > + > if (clocksource_register_hz(&mct_frc, clk_rate)) > panic("%s: can't register clocksource\n", mct_frc.name); > >
Hi Amit, Please see my comments inline. On 19.06.2014 10:39, Amit Daniel Kachhap wrote: > This patch register the exynos mct clocksource as the current timer > as it has constant clock rate. This will generate correct udelay for the > exynos platform and avoid using unnecessary calibrated jiffies. This change > has been tested on exynos5420 based board and udelay is very close to > expected. > > Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com> > --- > Changes in V2: > * Added #defines for ARM and ARM64 as pointed by Doug Anderson. > > Patches from David Riley confirmed that udelay is broken in exynos5420. > Link to those patches are, > 1) https://patchwork.kernel.org/patch/4344911/ > 2) https://patchwork.kernel.org/patch/4344881/ > > drivers/clocksource/exynos_mct.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c > index f71d55f..02927e2 100644 > --- a/drivers/clocksource/exynos_mct.c > +++ b/drivers/clocksource/exynos_mct.c > @@ -195,10 +195,25 @@ static u64 notrace exynos4_read_sched_clock(void) > return exynos4_frc_read(&mct_frc); > } > > +static struct delay_timer exynos4_delay_timer; > + > +static unsigned long exynos4_read_current_timer(void) > +{ > +#ifdef ARM > + return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L); > +#else /* ARM64, etc */ > + return exynos4_frc_read(&mct_frc); > +#endif > +} > + No need for anything like this. Even if running on ARM64, the delay timer code should be able to cope with different timer widths. For delays, 32 bits are enough, so just always read the lower part. Also use of raw accessors in drivers is discouraged - please use readl_relaxed(). Btw. I don't even see support for this on ARM64 in mainline, where arch timer is always used for delays and AFAIK this is a platform requirement. > static void __init exynos4_clocksource_init(void) > { > exynos4_mct_frc_start(); > > + exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer; nit: No need for & for function pointers. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 19, 2014 at 2:41 PM, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > On 06/19/2014 10:39 AM, Amit Daniel Kachhap wrote: >> >> This patch register the exynos mct clocksource as the current timer >> as it has constant clock rate. This will generate correct udelay for the >> exynos platform and avoid using unnecessary calibrated jiffies. This >> change >> has been tested on exynos5420 based board and udelay is very close to >> expected. >> >> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com> >> --- >> Changes in V2: >> * Added #defines for ARM and ARM64 as pointed by Doug Anderson. >> >> Patches from David Riley confirmed that udelay is broken in exynos5420. >> Link to those patches are, >> 1) https://patchwork.kernel.org/patch/4344911/ >> 2) https://patchwork.kernel.org/patch/4344881/ >> >> drivers/clocksource/exynos_mct.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/drivers/clocksource/exynos_mct.c >> b/drivers/clocksource/exynos_mct.c >> index f71d55f..02927e2 100644 >> --- a/drivers/clocksource/exynos_mct.c >> +++ b/drivers/clocksource/exynos_mct.c >> @@ -195,10 +195,25 @@ static u64 notrace exynos4_read_sched_clock(void) >> return exynos4_frc_read(&mct_frc); >> } >> >> +static struct delay_timer exynos4_delay_timer; >> + >> +static unsigned long exynos4_read_current_timer(void) >> +{ >> +#ifdef ARM >> + return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L); >> +#else /* ARM64, etc */ >> + return exynos4_frc_read(&mct_frc); >> +#endif >> +} >> + > > > There isn't another solution than that ? macros definitions in C file are > avoided as much as possible. I also didn't want to use macros but used as a last option. you want me to put more comments here? Or something like below is also possible for checking the size of (unsigned long) in runtime. unsigned long x; unsigned int size = (char *)(&x + 1) - (char *)(&x); if (size == 4) return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L); else return exynos4_frc_read(&mct_frc); But this involves extra computation which should not be used for time critical functions. Any suggestion? > > >> static void __init exynos4_clocksource_init(void) >> { >> exynos4_mct_frc_start(); >> >> + exynos4_delay_timer.read_current_timer = >> &exynos4_read_current_timer; > > > &exynos4_read_current_timer ? Any issue in the naming? > > >> + exynos4_delay_timer.freq = clk_rate; >> + register_current_timer_delay(&exynos4_delay_timer); >> + >> if (clocksource_register_hz(&mct_frc, clk_rate)) >> panic("%s: can't register clocksource\n", mct_frc.name); >> >> > > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" > in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/19/2014 12:21 PM, amit daniel kachhap wrote: > On Thu, Jun 19, 2014 at 2:41 PM, Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> On 06/19/2014 10:39 AM, Amit Daniel Kachhap wrote: >>> >>> This patch register the exynos mct clocksource as the current timer >>> as it has constant clock rate. This will generate correct udelay for the >>> exynos platform and avoid using unnecessary calibrated jiffies. This >>> change >>> has been tested on exynos5420 based board and udelay is very close to >>> expected. >>> >>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com> >>> --- >>> Changes in V2: >>> * Added #defines for ARM and ARM64 as pointed by Doug Anderson. >>> >>> Patches from David Riley confirmed that udelay is broken in exynos5420. >>> Link to those patches are, >>> 1) https://patchwork.kernel.org/patch/4344911/ >>> 2) https://patchwork.kernel.org/patch/4344881/ >>> >>> drivers/clocksource/exynos_mct.c | 15 +++++++++++++++ >>> 1 file changed, 15 insertions(+) >>> >>> diff --git a/drivers/clocksource/exynos_mct.c >>> b/drivers/clocksource/exynos_mct.c >>> index f71d55f..02927e2 100644 >>> --- a/drivers/clocksource/exynos_mct.c >>> +++ b/drivers/clocksource/exynos_mct.c >>> @@ -195,10 +195,25 @@ static u64 notrace exynos4_read_sched_clock(void) >>> return exynos4_frc_read(&mct_frc); >>> } >>> >>> +static struct delay_timer exynos4_delay_timer; >>> + >>> +static unsigned long exynos4_read_current_timer(void) >>> +{ >>> +#ifdef ARM >>> + return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L); >>> +#else /* ARM64, etc */ >>> + return exynos4_frc_read(&mct_frc); >>> +#endif >>> +} >>> + >> >> >> There isn't another solution than that ? macros definitions in C file are >> avoided as much as possible. > I also didn't want to use macros but used as a last option. you want > me to put more comments here? > Or something like below is also possible for checking the size of > (unsigned long) in runtime. > > unsigned long x; > unsigned int size = (char *)(&x + 1) - (char *)(&x); > if (size == 4) > return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L); > else > return exynos4_frc_read(&mct_frc); > > But this involves extra computation which should not be used for time > critical functions. > Any suggestion? AFAIU, Doug may change the exynos4_frc_read to be 32 bits. So may not need those macros as 'exynos4_frc_read' could be used instead. Doug may confirm this. >>> static void __init exynos4_clocksource_init(void) >>> { >>> exynos4_mct_frc_start(); >>> >>> + exynos4_delay_timer.read_current_timer = >>> &exynos4_read_current_timer; >> >> >> &exynos4_read_current_timer ? > Any issue in the naming? No problem with the naming. As Tomasz pointed also, you are passing '&' for the function pointer. >>> + exynos4_delay_timer.freq = clk_rate; >>> + register_current_timer_delay(&exynos4_delay_timer); >>> + >>> if (clocksource_register_hz(&mct_frc, clk_rate)) >>> panic("%s: can't register clocksource\n", mct_frc.name); >>> >>> >> >> >> -- >> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs >> >> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | >> <http://twitter.com/#!/linaroorg> Twitter | >> <http://www.linaro.org/linaro-blog/> Blog >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" >> in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 19, 2014 at 4:00 PM, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > On 06/19/2014 12:21 PM, amit daniel kachhap wrote: >> >> On Thu, Jun 19, 2014 at 2:41 PM, Daniel Lezcano >> <daniel.lezcano@linaro.org> wrote: >>> >>> On 06/19/2014 10:39 AM, Amit Daniel Kachhap wrote: >>>> >>>> >>>> This patch register the exynos mct clocksource as the current timer >>>> as it has constant clock rate. This will generate correct udelay for the >>>> exynos platform and avoid using unnecessary calibrated jiffies. This >>>> change >>>> has been tested on exynos5420 based board and udelay is very close to >>>> expected. >>>> >>>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com> >>>> --- >>>> Changes in V2: >>>> * Added #defines for ARM and ARM64 as pointed by Doug Anderson. >>>> >>>> Patches from David Riley confirmed that udelay is broken in exynos5420. >>>> Link to those patches are, >>>> 1) https://patchwork.kernel.org/patch/4344911/ >>>> 2) https://patchwork.kernel.org/patch/4344881/ >>>> >>>> drivers/clocksource/exynos_mct.c | 15 +++++++++++++++ >>>> 1 file changed, 15 insertions(+) >>>> >>>> diff --git a/drivers/clocksource/exynos_mct.c >>>> b/drivers/clocksource/exynos_mct.c >>>> index f71d55f..02927e2 100644 >>>> --- a/drivers/clocksource/exynos_mct.c >>>> +++ b/drivers/clocksource/exynos_mct.c >>>> @@ -195,10 +195,25 @@ static u64 notrace exynos4_read_sched_clock(void) >>>> return exynos4_frc_read(&mct_frc); >>>> } >>>> >>>> +static struct delay_timer exynos4_delay_timer; >>>> + >>>> +static unsigned long exynos4_read_current_timer(void) >>>> +{ >>>> +#ifdef ARM >>>> + return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L); >>>> +#else /* ARM64, etc */ >>>> + return exynos4_frc_read(&mct_frc); >>>> +#endif >>>> +} >>>> + >>> >>> >>> >>> There isn't another solution than that ? macros definitions in C file are >>> avoided as much as possible. >> >> I also didn't want to use macros but used as a last option. you want >> me to put more comments here? >> Or something like below is also possible for checking the size of >> (unsigned long) in runtime. >> >> unsigned long x; >> unsigned int size = (char *)(&x + 1) - (char *)(&x); >> if (size == 4) >> return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L); >> else >> return exynos4_frc_read(&mct_frc); >> >> But this involves extra computation which should not be used for time >> critical functions. >> Any suggestion? > > > AFAIU, Doug may change the exynos4_frc_read to be 32 bits. So may not need > those macros as 'exynos4_frc_read' could be used instead. right, its better to wait till those patches are out. > > Doug may confirm this. > > >>>> static void __init exynos4_clocksource_init(void) >>>> { >>>> exynos4_mct_frc_start(); >>>> >>>> + exynos4_delay_timer.read_current_timer = >>>> &exynos4_read_current_timer; >>> >>> >>> >>> &exynos4_read_current_timer ? >> >> Any issue in the naming? > > > No problem with the naming. As Tomasz pointed also, you are passing '&' for > the function pointer. yes my mistake. > > >>>> + exynos4_delay_timer.freq = clk_rate; >>>> + register_current_timer_delay(&exynos4_delay_timer); >>>> + >>>> if (clocksource_register_hz(&mct_frc, clk_rate)) >>>> panic("%s: can't register clocksource\n", >>>> mct_frc.name); >>>> >>>> >>> >>> >>> -- >>> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs >>> >>> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | >>> <http://twitter.com/#!/linaroorg> Twitter | >>> <http://www.linaro.org/linaro-blog/> Blog >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe >>> linux-samsung-soc" >>> in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" > in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Hi, On Thu, Jun 19, 2014 at 3:21 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >> +static struct delay_timer exynos4_delay_timer; >> + >> +static unsigned long exynos4_read_current_timer(void) Note: I think this should return a cycles_t, not an unsigned long. They're the same (right now), but probably shouldn't be (see below). >> +{ >> +#ifdef ARM >> + return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L); >> +#else /* ARM64, etc */ >> + return exynos4_frc_read(&mct_frc); >> +#endif >> +} >> + > > No need for anything like this. Even if running on ARM64, the delay > timer code should be able to cope with different timer widths. For > delays, 32 bits are enough, so just always read the lower part. I agree that the timer code should cope but it doesn't appear to. I see: cycles_t start = get_cycles(); while ((get_cycles() - start) < cycles) cpu_relax(); Right now cycles_t is defined as "unsigned long". If that's 64-bits on ARM64 then this function will have problems with wraparound. My personal vote would be to submit a patch to change "cycles_t" to always be 32-bits. Given that 32-bits was fine for udelay() for ARM that seems sane and simple. If someone later comes up with a super compelling reason why we need 64-bit timers for udelay (really??) then they can later add all the complexity needed. Amit: can you code up such a patch and add it to the series? I know it changes code that touches all ARM devices but I still think it's the right thing to do and actually only really changes behavior on ARM64. > Also use of raw accessors in drivers is discouraged - please use > readl_relaxed(). It doesn't seem like that should happen in the same patch. Perhaps Amit can do a cleanup patch first that changes all instances of __raw_readl / __raw_writel in this file, then submit his patch atop that. > Btw. I don't even see support for this on ARM64 in mainline, where arch > timer is always used for delays and AFAIK this is a platform requirement. Yeah, I'd vote for not using MCT on ARM64, but it I suppose it doesn't hurt to keep it working. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19.06.2014 18:01, Doug Anderson wrote: > Hi, > > On Thu, Jun 19, 2014 at 3:21 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >>> +static struct delay_timer exynos4_delay_timer; >>> + >>> +static unsigned long exynos4_read_current_timer(void) > > Note: I think this should return a cycles_t, not an unsigned long. > They're the same (right now), but probably shouldn't be (see below). > > >>> +{ >>> +#ifdef ARM >>> + return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L); >>> +#else /* ARM64, etc */ >>> + return exynos4_frc_read(&mct_frc); >>> +#endif >>> +} >>> + >> >> No need for anything like this. Even if running on ARM64, the delay >> timer code should be able to cope with different timer widths. For >> delays, 32 bits are enough, so just always read the lower part. > > I agree that the timer code should cope but it doesn't appear to. I see: > > cycles_t start = get_cycles(); > while ((get_cycles() - start) < cycles) > cpu_relax(); > > Right now cycles_t is defined as "unsigned long". If that's 64-bits > on ARM64 then this function will have problems with wraparound. Well, first of all, I'm not sure why we're discussing ARM64 here, if it doesn't have support for register_current_timer_delay() at all. Moreover, the only user if it is ARM32 and, if I remember correctly, the assumption was that you need a >= 32 bit timer to use it for delays and so with 32 bit unsigned long everything should work. Now if ARM64 also wants to use this infrastructure, instead of hacking drivers now and encouraging ARM64 people to continue using this kind of hackery, I believe we should indicate that ARM64 delay timer code needs to be implemented correctly and cope for different timer widths. Of course the best way would be to redesign this interface right now to consider time width (it is not just about 32 or 64 bits, there are various timers, possibly 48- or 52-bit) and change ARM32 implementation of it as well, if there are any volunteers. Changing the code to always use 32-bit type regardless of arch is also an option. > > My personal vote would be to submit a patch to change "cycles_t" to > always be 32-bits. Given that 32-bits was fine for udelay() for ARM > that seems sane and simple. If someone later comes up with a super > compelling reason why we need 64-bit timers for udelay (really??) then > they can later add all the complexity needed. Yes, this could work. I'm not sure what else cycles_t is used for, though. > > Amit: can you code up such a patch and add it to the series? I know > it changes code that touches all ARM devices but I still think it's > the right thing to do and actually only really changes behavior on > ARM64. > > >> Also use of raw accessors in drivers is discouraged - please use >> readl_relaxed(). > > It doesn't seem like that should happen in the same patch. Perhaps > Amit can do a cleanup patch first that changes all instances of > __raw_readl / __raw_writel in this file, then submit his patch atop > that. Why not? The patch adding exynos4_read_current_timer() (and so the call to __raw_readl() was Amit's patch. I'm not asking to fix this in the whole driver, just to do things correctly in new code. > > >> Btw. I don't even see support for this on ARM64 in mainline, where arch >> timer is always used for delays and AFAIK this is a platform requirement. > > Yeah, I'd vote for not using MCT on ARM64, but it I suppose it doesn't > hurt to keep it working. > Right now there is no need for this, so let's just keep things simple. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tomasz, On Thu, Jun 19, 2014 at 9:17 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > On 19.06.2014 18:01, Doug Anderson wrote: >> Hi, >> >> On Thu, Jun 19, 2014 at 3:21 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >>>> +static struct delay_timer exynos4_delay_timer; >>>> + >>>> +static unsigned long exynos4_read_current_timer(void) >> >> Note: I think this should return a cycles_t, not an unsigned long. >> They're the same (right now), but probably shouldn't be (see below). >> >> >>>> +{ >>>> +#ifdef ARM >>>> + return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L); >>>> +#else /* ARM64, etc */ >>>> + return exynos4_frc_read(&mct_frc); >>>> +#endif >>>> +} >>>> + >>> >>> No need for anything like this. Even if running on ARM64, the delay >>> timer code should be able to cope with different timer widths. For >>> delays, 32 bits are enough, so just always read the lower part. >> >> I agree that the timer code should cope but it doesn't appear to. I see: >> >> cycles_t start = get_cycles(); >> while ((get_cycles() - start) < cycles) >> cpu_relax(); >> >> Right now cycles_t is defined as "unsigned long". If that's 64-bits >> on ARM64 then this function will have problems with wraparound. > > Well, first of all, I'm not sure why we're discussing ARM64 here, if it > doesn't have support for register_current_timer_delay() at all. > Moreover, the only user if it is ARM32 and, if I remember correctly, the > assumption was that you need a >= 32 bit timer to use it for delays and > so with 32 bit unsigned long everything should work. Now if ARM64 also > wants to use this infrastructure, instead of hacking drivers now and > encouraging ARM64 people to continue using this kind of hackery, I > believe we should indicate that ARM64 delay timer code needs to be > implemented correctly and cope for different timer widths. > > Of course the best way would be to redesign this interface right now to > consider time width (it is not just about 32 or 64 bits, there are > various timers, possibly 48- or 52-bit) and change ARM32 implementation > of it as well, if there are any volunteers. I was arguing that it was a lot of extra complexity to add to the code to support different timer sizes for udelay when 32-bits is enough (and it's trivial for timer implementations to truncate down to 32-bits). > Changing the code to always > use 32-bit type regardless of arch is also an option. Right, that's what I was arguing for. >> My personal vote would be to submit a patch to change "cycles_t" to >> always be 32-bits. Given that 32-bits was fine for udelay() for ARM >> that seems sane and simple. If someone later comes up with a super >> compelling reason why we need 64-bit timers for udelay (really??) then >> they can later add all the complexity needed. > > Yes, this could work. I'm not sure what else cycles_t is used for, though. True, it is a bit questionable to change this since it's a type that's not obviously just for udelay(). Perhaps a better option would be to make a new typedef for the result of read_current_timer(). ...or just change it to return a u32? >> Amit: can you code up such a patch and add it to the series? I know >> it changes code that touches all ARM devices but I still think it's >> the right thing to do and actually only really changes behavior on >> ARM64. >> >> >>> Also use of raw accessors in drivers is discouraged - please use >>> readl_relaxed(). >> >> It doesn't seem like that should happen in the same patch. Perhaps >> Amit can do a cleanup patch first that changes all instances of >> __raw_readl / __raw_writel in this file, then submit his patch atop >> that. > > Why not? The patch adding exynos4_read_current_timer() (and so the call > to __raw_readl() was Amit's patch. I'm not asking to fix this in the > whole driver, just to do things correctly in new code. IMHO consistency within a file is more important. Having a single readl_relaxed() in a file that uses all __raw_readl() doesn't seem beneficial to me. ...but I certainly won't NAK the patch if that's what others want. >>> Btw. I don't even see support for this on ARM64 in mainline, where arch >>> timer is always used for delays and AFAIK this is a platform requirement. >> >> Yeah, I'd vote for not using MCT on ARM64, but it I suppose it doesn't >> hurt to keep it working. > > Right now there is no need for this, so let's just keep things simple. It should at least have a comment saying that it's broken in subtle ways (only on udelays that happen to span the 32-bit wraparound) on anything where cycle_t is not 32-bits. Perhaps at least a: WARN_ON_ONCE(sizeof(cycles_t) != sizeof(u32)); The above should be optimized by the compiler. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19.06.2014 18:31, Doug Anderson wrote: > Tomasz, > > On Thu, Jun 19, 2014 at 9:17 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >> On 19.06.2014 18:01, Doug Anderson wrote: >>> Hi, >>> >>> On Thu, Jun 19, 2014 at 3:21 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >>>>> +static struct delay_timer exynos4_delay_timer; >>>>> + >>>>> +static unsigned long exynos4_read_current_timer(void) >>> >>> Note: I think this should return a cycles_t, not an unsigned long. >>> They're the same (right now), but probably shouldn't be (see below). >>> >>> >>>>> +{ >>>>> +#ifdef ARM >>>>> + return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L); >>>>> +#else /* ARM64, etc */ >>>>> + return exynos4_frc_read(&mct_frc); >>>>> +#endif >>>>> +} >>>>> + >>>> >>>> No need for anything like this. Even if running on ARM64, the delay >>>> timer code should be able to cope with different timer widths. For >>>> delays, 32 bits are enough, so just always read the lower part. >>> >>> I agree that the timer code should cope but it doesn't appear to. I see: >>> >>> cycles_t start = get_cycles(); >>> while ((get_cycles() - start) < cycles) >>> cpu_relax(); >>> >>> Right now cycles_t is defined as "unsigned long". If that's 64-bits >>> on ARM64 then this function will have problems with wraparound. >> >> Well, first of all, I'm not sure why we're discussing ARM64 here, if it >> doesn't have support for register_current_timer_delay() at all. >> Moreover, the only user if it is ARM32 and, if I remember correctly, the >> assumption was that you need a >= 32 bit timer to use it for delays and >> so with 32 bit unsigned long everything should work. Now if ARM64 also >> wants to use this infrastructure, instead of hacking drivers now and >> encouraging ARM64 people to continue using this kind of hackery, I >> believe we should indicate that ARM64 delay timer code needs to be >> implemented correctly and cope for different timer widths. >> >> Of course the best way would be to redesign this interface right now to >> consider time width (it is not just about 32 or 64 bits, there are >> various timers, possibly 48- or 52-bit) and change ARM32 implementation >> of it as well, if there are any volunteers. > > I was arguing that it was a lot of extra complexity to add to the code > to support different timer sizes for udelay when 32-bits is enough > (and it's trivial for timer implementations to truncate down to > 32-bits). > > >> Changing the code to always >> use 32-bit type regardless of arch is also an option. > > Right, that's what I was arguing for. > > >>> My personal vote would be to submit a patch to change "cycles_t" to >>> always be 32-bits. Given that 32-bits was fine for udelay() for ARM >>> that seems sane and simple. If someone later comes up with a super >>> compelling reason why we need 64-bit timers for udelay (really??) then >>> they can later add all the complexity needed. >> >> Yes, this could work. I'm not sure what else cycles_t is used for, though. > > True, it is a bit questionable to change this since it's a type that's > not obviously just for udelay(). Perhaps a better option would be to > make a new typedef for the result of read_current_timer(). ...or just > change it to return a u32? > Sounds good to me, but let's hear other opinions. I'm adding Will and Jonathan as they wrote the ARM delay timer code. > >>> Amit: can you code up such a patch and add it to the series? I know >>> it changes code that touches all ARM devices but I still think it's >>> the right thing to do and actually only really changes behavior on >>> ARM64. >>> >>> >>>> Also use of raw accessors in drivers is discouraged - please use >>>> readl_relaxed(). >>> >>> It doesn't seem like that should happen in the same patch. Perhaps >>> Amit can do a cleanup patch first that changes all instances of >>> __raw_readl / __raw_writel in this file, then submit his patch atop >>> that. >> >> Why not? The patch adding exynos4_read_current_timer() (and so the call >> to __raw_readl() was Amit's patch. I'm not asking to fix this in the >> whole driver, just to do things correctly in new code. > > IMHO consistency within a file is more important. Having a single > readl_relaxed() in a file that uses all __raw_readl() doesn't seem > beneficial to me. ...but I certainly won't NAK the patch if that's > what others want. > It depends whether you prefer consistency over correctness or the other way around. I should have marked this comment as a nitpick. :) > >>>> Btw. I don't even see support for this on ARM64 in mainline, where arch >>>> timer is always used for delays and AFAIK this is a platform requirement. >>> >>> Yeah, I'd vote for not using MCT on ARM64, but it I suppose it doesn't >>> hurt to keep it working. >> >> Right now there is no need for this, so let's just keep things simple. > > It should at least have a comment saying that it's broken in subtle > ways (only on udelays that happen to span the 32-bit wraparound) on > anything where cycle_t is not 32-bits. Perhaps at least a: > WARN_ON_ONCE(sizeof(cycles_t) != sizeof(u32)); > > The above should be optimized by the compiler. Sounds good as well. Or maybe even BUILD_BUG_ON_MSG() with proper error message? Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Thu, Jun 19, 2014 at 05:40:49PM +0100, Tomasz Figa wrote: > On 19.06.2014 18:31, Doug Anderson wrote: > >>> My personal vote would be to submit a patch to change "cycles_t" to > >>> always be 32-bits. Given that 32-bits was fine for udelay() for ARM > >>> that seems sane and simple. If someone later comes up with a super > >>> compelling reason why we need 64-bit timers for udelay (really??) then > >>> they can later add all the complexity needed. > >> > >> Yes, this could work. I'm not sure what else cycles_t is used for, though. > > > > True, it is a bit questionable to change this since it's a type that's > > not obviously just for udelay(). Perhaps a better option would be to > > make a new typedef for the result of read_current_timer(). ...or just > > change it to return a u32? > > > > Sounds good to me, but let's hear other opinions. I'm adding Will and > Jonathan as they wrote the ARM delay timer code. I think cycles_t is only used for small interval calculations at the moment, but I remember Ted mentioning something about using it (or something similar) as a source of early entropy, in which case the more bits the better. That said, I can't find any code in the tree to that effect. Will -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c index f71d55f..02927e2 100644 --- a/drivers/clocksource/exynos_mct.c +++ b/drivers/clocksource/exynos_mct.c @@ -195,10 +195,25 @@ static u64 notrace exynos4_read_sched_clock(void) return exynos4_frc_read(&mct_frc); } +static struct delay_timer exynos4_delay_timer; + +static unsigned long exynos4_read_current_timer(void) +{ +#ifdef ARM + return __raw_readl(reg_base + EXYNOS4_MCT_G_CNT_L); +#else /* ARM64, etc */ + return exynos4_frc_read(&mct_frc); +#endif +} + static void __init exynos4_clocksource_init(void) { exynos4_mct_frc_start(); + exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer; + exynos4_delay_timer.freq = clk_rate; + register_current_timer_delay(&exynos4_delay_timer); + if (clocksource_register_hz(&mct_frc, clk_rate)) panic("%s: can't register clocksource\n", mct_frc.name);
This patch register the exynos mct clocksource as the current timer as it has constant clock rate. This will generate correct udelay for the exynos platform and avoid using unnecessary calibrated jiffies. This change has been tested on exynos5420 based board and udelay is very close to expected. Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com> --- Changes in V2: * Added #defines for ARM and ARM64 as pointed by Doug Anderson. Patches from David Riley confirmed that udelay is broken in exynos5420. Link to those patches are, 1) https://patchwork.kernel.org/patch/4344911/ 2) https://patchwork.kernel.org/patch/4344881/ drivers/clocksource/exynos_mct.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)