| Message ID | 1444074528-3905-1-git-send-email-balbi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Felipe, On 10/05/2015 02:48 PM, Balbi, Felipe wrote: > We actually want these devices to be created because > we will be moving timers to drivers/clocksource and > this will prevent them from probing. This logic is also used to remove the secure timer from being registered to the kernel on HS devices, while allowing the timer to be available on GP devices. Your patch actually would break that functionality. I suggest that you look at the history of the code that originally added this logic - this function seems to be designed to actually remove the node. The OMAP DMTimer provides an API to request timers, and I think this logic was used to eliminate giving out the timers used for clocksource and clockevent when the driver got adapted to DT. > > Signed-off-by: Felipe Balbi <balbi@ti.com> > --- > > Tony, I wonder if you can get merged as a fix, or do you > prefer receiving it together with my timer series ? NAK for rc, as it breaks other stuff. regards Suman > > arch/arm/mach-omap2/timer.c | 11 +---------- > 1 file changed, 1 insertion(+), 10 deletions(-) > > diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c > index 0ff676273b4b..0c00138d7bd5 100644 > --- a/arch/arm/mach-omap2/timer.c > +++ b/arch/arm/mach-omap2/timer.c > @@ -136,12 +136,6 @@ static struct clock_event_device clockevent_gpt = { > .tick_resume = omap2_gp_timer_shutdown, > }; > > -static struct property device_disabled = { > - .name = "status", > - .length = sizeof("disabled"), > - .value = "disabled", > -}; > - > static const struct of_device_id omap_timer_match[] __initconst = { > { .compatible = "ti,omap2420-timer", }, > { .compatible = "ti,omap3430-timer", }, > @@ -161,9 +155,7 @@ static const struct of_device_id omap_timer_match[] __initconst = { > * > * Helper function to get a timer during early boot using device-tree for use > * as kernel system timer. Optionally, the property argument can be used to > - * select a timer with a specific property. Once a timer is found then mark > - * the timer node in device-tree as disabled, to prevent the kernel from > - * registering this timer as a platform device and so no one else can use it. > + * select a timer with a specific property. > */ > static struct device_node * __init omap_get_timer_dt(const struct of_device_id *match, > const char *property) > @@ -183,7 +175,6 @@ static struct device_node * __init omap_get_timer_dt(const struct of_device_id * > of_get_property(np, "ti,timer-secure", NULL))) > continue; > > - of_add_property(np, &device_disabled); > return np; > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Suman Anna <s-anna@ti.com> writes: > Hi Felipe, > > On 10/05/2015 02:48 PM, Balbi, Felipe wrote: >> We actually want these devices to be created because >> we will be moving timers to drivers/clocksource and >> this will prevent them from probing. > > This logic is also used to remove the secure timer from being > registered to the kernel on HS devices, while allowing the timer to be > available on GP devices. Your patch actually would break that > functionality. I suggest that you look at the history of the code that heh, that's a silly way of doing so. Could just detect if we're running on HS or not. > originally added this logic - this function seems to be designed to > actually remove the node. The OMAP DMTimer provides an API to request > timers, and I think this logic was used to eliminate giving out the > timers used for clocksource and clockevent when the driver got adapted > to DT. again not the best way of achieving what you want. In any case, other than ir-rx51.c who's using that API ? Seems like we could just pass a phandle to ir-rx51 and be done with it. >> Signed-off-by: Felipe Balbi <balbi@ti.com> >> --- >> >> Tony, I wonder if you can get merged as a fix, or do you >> prefer receiving it together with my timer series ? > > NAK for rc, as it breaks other stuff. a single stuff which is likely easy enough to fix. But fair enough -- balbi
Hi Felipe, >> >> On 10/05/2015 02:48 PM, Balbi, Felipe wrote: >>> We actually want these devices to be created because >>> we will be moving timers to drivers/clocksource and >>> this will prevent them from probing. >> >> This logic is also used to remove the secure timer from being >> registered to the kernel on HS devices, while allowing the timer to be >> available on GP devices. Your patch actually would break that >> functionality. I suggest that you look at the history of the code that > > heh, that's a silly way of doing so. Could just detect if we're running > on HS or not. This function is invoked only after detecting that we are running on a HS device. > >> originally added this logic - this function seems to be designed to >> actually remove the node. The OMAP DMTimer provides an API to request >> timers, and I think this logic was used to eliminate giving out the >> timers used for clocksource and clockevent when the driver got adapted >> to DT. > > again not the best way of achieving what you want. In any case, other > than ir-rx51.c who's using that API ? Seems like we could just pass a > phandle to ir-rx51 and be done with it. We will gain a user from OMAP remoteproc driver as well (out of tree at the moment, but it does follow the DT phandle and omap_dm_timer_request_by_node semantics). I do not think ir-rx51.c is converted to DT, and also some of the API are alive just because of the continued OMAP3 legacy boot support. Guess, it is a just a question of not breaking existing API until we kill some of them. > >>> Signed-off-by: Felipe Balbi <balbi@ti.com> >>> --- >>> >>> Tony, I wonder if you can get merged as a fix, or do you >>> prefer receiving it together with my timer series ? >> >> NAK for rc, as it breaks other stuff. > > a single stuff which is likely easy enough to fix. But fair enough Don't think this patch is fixing any critical bug either, better to club it with your cleanup series. regards Suman -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Oct 05, 2015 at 06:41:37PM -0500, Suman Anna wrote: > We will gain a user from OMAP remoteproc driver as well (out of tree at > the moment, but it does follow the DT phandle and > omap_dm_timer_request_by_node semantics). I do not think ir-rx51.c is > converted to DT, and also some of the API are alive just because of the > continued OMAP3 legacy boot support. Guess, it is a just a question of > not breaking existing API until we kill some of them. Right, ir-rx51 has not yet been converted to DT. Additionally it depends !ARCH_MULTIPLATFORM, since it requires <plat/dmtimer.h>. I remember a patchset adding a omap-pwm driver using dmtimers. If that was added to the kernel, ir-rx51 could probably be switched to the PWM API + hrtimer API instead of depending on the dmtimer API. At the end not much is gained, though, since then the PWM driver would probably require the dmtimer API. -- Sebastian
Suman Anna <s-anna@ti.com> writes: > Hi Felipe, > >>> >>> On 10/05/2015 02:48 PM, Balbi, Felipe wrote: >>>> We actually want these devices to be created because >>>> we will be moving timers to drivers/clocksource and >>>> this will prevent them from probing. >>> >>> This logic is also used to remove the secure timer from being >>> registered to the kernel on HS devices, while allowing the timer to be >>> available on GP devices. Your patch actually would break that >>> functionality. I suggest that you look at the history of the code that >> >> heh, that's a silly way of doing so. Could just detect if we're running >> on HS or not. > > This function is invoked only after detecting that we are running on a > HS device. What actually disables the timer is omap_get_timer_dt() and that's called in more than one place. >>> originally added this logic - this function seems to be designed to >>> actually remove the node. The OMAP DMTimer provides an API to request >>> timers, and I think this logic was used to eliminate giving out the >>> timers used for clocksource and clockevent when the driver got adapted >>> to DT. >> >> again not the best way of achieving what you want. In any case, other >> than ir-rx51.c who's using that API ? Seems like we could just pass a >> phandle to ir-rx51 and be done with it. > > We will gain a user from OMAP remoteproc driver as well (out of tree at > the moment, but it does follow the DT phandle and > omap_dm_timer_request_by_node semantics). I do not think ir-rx51.c is > converted to DT, and also some of the API are alive just because of the > continued OMAP3 legacy boot support. Guess, it is a just a question of > not breaking existing API until we kill some of them. sure, but until remoteproc gets upstream, it's only 1 user ;-) >>>> Signed-off-by: Felipe Balbi <balbi@ti.com> >>>> --- >>>> >>>> Tony, I wonder if you can get merged as a fix, or do you >>>> prefer receiving it together with my timer series ? >>> >>> NAK for rc, as it breaks other stuff. >> >> a single stuff which is likely easy enough to fix. But fair enough > > Don't think this patch is fixing any critical bug either, better to club > it with your cleanup series. it is kinda critical when you consider that the timer is disabled outside of the soc type detection.
Hi, Sebastian Reichel <sre@kernel.org> writes: > On Mon, Oct 05, 2015 at 06:41:37PM -0500, Suman Anna wrote: >> We will gain a user from OMAP remoteproc driver as well (out of tree at >> the moment, but it does follow the DT phandle and >> omap_dm_timer_request_by_node semantics). I do not think ir-rx51.c is >> converted to DT, and also some of the API are alive just because of the >> continued OMAP3 legacy boot support. Guess, it is a just a question of >> not breaking existing API until we kill some of them. > > Right, ir-rx51 has not yet been converted to DT. Additionally it > depends !ARCH_MULTIPLATFORM, since it requires <plat/dmtimer.h>. > > I remember a patchset adding a omap-pwm driver using dmtimers. If > that was added to the kernel, ir-rx51 could probably be switched to > the PWM API + hrtimer API instead of depending on the dmtimer API. > At the end not much is gained, though, since then the PWM driver > would probably require the dmtimer API. Tony has been talking about an HRTimer API for some time, but I guess it didn't happen yet. Until then, cleanups will have to continue to cope with legacy users.
* Felipe Balbi <balbi@ti.com> [151005 18:02]: > > Hi, > > Sebastian Reichel <sre@kernel.org> writes: > > On Mon, Oct 05, 2015 at 06:41:37PM -0500, Suman Anna wrote: > >> We will gain a user from OMAP remoteproc driver as well (out of tree at > >> the moment, but it does follow the DT phandle and > >> omap_dm_timer_request_by_node semantics). I do not think ir-rx51.c is > >> converted to DT, and also some of the API are alive just because of the > >> continued OMAP3 legacy boot support. Guess, it is a just a question of > >> not breaking existing API until we kill some of them. > > > > Right, ir-rx51 has not yet been converted to DT. Additionally it > > depends !ARCH_MULTIPLATFORM, since it requires <plat/dmtimer.h>. > > > > I remember a patchset adding a omap-pwm driver using dmtimers. If > > that was added to the kernel, ir-rx51 could probably be switched to > > the PWM API + hrtimer API instead of depending on the dmtimer API. > > At the end not much is gained, though, since then the PWM driver > > would probably require the dmtimer API. > > Tony has been talking about an HRTimer API for some time, but I guess it > didn't happen yet. Until then, cleanups will have to continue to cope > with legacy users. Yeh.. Well meanwhile, here's how we can easily support hardware timers for pwm in a non-intrusive way: 1. We add irqchip support to gptimer code 2. We create omap-gptimer-pwm.c and pass hwtimer_get/set type function pointers in pdata to avoid exposing the gptimer custom functions to drivers until we have a hwtimer framework 3. Then omap-gptimer-pwm.c does request_irq on the hardware timer(s) configured in the board specific dts file to get the right timer 4. Then ir-rx51 can just use he Linux PWM API and omap-gptimer-pwm.c 5. We get rid of the pdata when we have a generic API available Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Felipe, >>>> >>>> On 10/05/2015 02:48 PM, Balbi, Felipe wrote: >>>>> We actually want these devices to be created because >>>>> we will be moving timers to drivers/clocksource and >>>>> this will prevent them from probing. >>>> >>>> This logic is also used to remove the secure timer from being >>>> registered to the kernel on HS devices, while allowing the timer to be >>>> available on GP devices. Your patch actually would break that >>>> functionality. I suggest that you look at the history of the code that >>> >>> heh, that's a silly way of doing so. Could just detect if we're running >>> on HS or not. >> >> This function is invoked only after detecting that we are running on a >> HS device. > > What actually disables the timer is omap_get_timer_dt() and that's > called in more than one place. Yes indeed, and this patch is changing the behavior of omap_get_timer_dt(), so you have to check all call-sites. Also, one another thing is that this trick was used for DT boots so that the timers used for clocksource and clockevent are eliminated from the omap_dm_timer_request() API. The legacy boot used a specific API to mark those as reserved so that the _request API does not give them out. Granted that it may not have been the best approach, but that needs a solution if you change the behavior of this API. > >>>> originally added this logic - this function seems to be designed to >>>> actually remove the node. The OMAP DMTimer provides an API to request >>>> timers, and I think this logic was used to eliminate giving out the >>>> timers used for clocksource and clockevent when the driver got adapted >>>> to DT. >>> >>> again not the best way of achieving what you want. In any case, other >>> than ir-rx51.c who's using that API ? Seems like we could just pass a >>> phandle to ir-rx51 and be done with it. >> >> We will gain a user from OMAP remoteproc driver as well (out of tree at >> the moment, but it does follow the DT phandle and >> omap_dm_timer_request_by_node semantics). I do not think ir-rx51.c is >> converted to DT, and also some of the API are alive just because of the >> continued OMAP3 legacy boot support. Guess, it is a just a question of >> not breaking existing API until we kill some of them. > > sure, but until remoteproc gets upstream, it's only 1 user ;-) > >>>>> Signed-off-by: Felipe Balbi <balbi@ti.com> >>>>> --- >>>>> >>>>> Tony, I wonder if you can get merged as a fix, or do you >>>>> prefer receiving it together with my timer series ? >>>> >>>> NAK for rc, as it breaks other stuff. >>> >>> a single stuff which is likely easy enough to fix. But fair enough >> >> Don't think this patch is fixing any critical bug either, better to club >> it with your cleanup series. > > it is kinda critical when you consider that the timer is disabled > outside of the soc type detection. This has been like this since DMTimer is converted to DT (from 3.8 kernel), and is a need for your counter32k clocksource series. The problem seems to stem from the reuse of omap_get_timer_dt for counter32k nodes. A better solution would be to remove the omap_get_timer_dt() from omap2_sync32k_clocksource_init() code and just replace it with of_find_compatible_node - you seem to be limiting the majority of that function to legacy mode in Patch 10 of your RFC series anyways. regards Suman -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Suman Anna <s-anna@ti.com> writes: >>>>> On 10/05/2015 02:48 PM, Balbi, Felipe wrote: >>>>>> We actually want these devices to be created because >>>>>> we will be moving timers to drivers/clocksource and >>>>>> this will prevent them from probing. >>>>> >>>>> This logic is also used to remove the secure timer from being >>>>> registered to the kernel on HS devices, while allowing the timer to be >>>>> available on GP devices. Your patch actually would break that >>>>> functionality. I suggest that you look at the history of the code that >>>> >>>> heh, that's a silly way of doing so. Could just detect if we're running >>>> on HS or not. >>> >>> This function is invoked only after detecting that we are running on a >>> HS device. >> >> What actually disables the timer is omap_get_timer_dt() and that's >> called in more than one place. > > Yes indeed, and this patch is changing the behavior of > omap_get_timer_dt(), so you have to check all call-sites. Also, one > another thing is that this trick was used for DT boots so that the > timers used for clocksource and clockevent are eliminated from the > omap_dm_timer_request() API. The legacy boot used a specific API to mark > those as reserved so that the _request API does not give them out. > Granted that it may not have been the best approach, but that needs a > solution if you change the behavior of this API. no doubt this needs a solution, but seesm like a solution here will have to be incremental. See new revision of my series. I'm now skipping 32k only and keeping it enabled so drivers/clocksource/ can use it. >>>>>> Signed-off-by: Felipe Balbi <balbi@ti.com> >>>>>> --- >>>>>> >>>>>> Tony, I wonder if you can get merged as a fix, or do you >>>>>> prefer receiving it together with my timer series ? >>>>> >>>>> NAK for rc, as it breaks other stuff. >>>> >>>> a single stuff which is likely easy enough to fix. But fair enough >>> >>> Don't think this patch is fixing any critical bug either, better to club >>> it with your cleanup series. >> >> it is kinda critical when you consider that the timer is disabled >> outside of the soc type detection. > > This has been like this since DMTimer is converted to DT (from 3.8 > kernel), and is a need for your counter32k clocksource series. The > problem seems to stem from the reuse of omap_get_timer_dt for counter32k > nodes. A better solution would be to remove the omap_get_timer_dt() from > omap2_sync32k_clocksource_init() code and just replace it with > of_find_compatible_node - you seem to be limiting the majority of that > function to legacy mode in Patch 10 of your RFC series anyways. I still need omap_hwmod_enable() to be called, that's why it's still used. I don't think replacing it with of_find_compatible_node() will buy us much, we will still need to check for of_device_is_available() anyway. Sure we skip all the timer flags (alwon, dsp, pwm, secure), but that really isn't big deal. When converting gptimer to clocksource, then I'll look at what I can do for the timer request side of things. For now, things are working, apparently without regressions (or at least I couldn't catch any so far) and it seems clean enough that it could possibly be queued for v4.4 merge window. For v4.5 I'll look at this again and try to figure out how to handle gptimer.
On 10/06/2015 12:14 PM, Felipe Balbi wrote: > > Hi, > > Suman Anna <s-anna@ti.com> writes: >>>>>> On 10/05/2015 02:48 PM, Balbi, Felipe wrote: >>>>>>> We actually want these devices to be created because >>>>>>> we will be moving timers to drivers/clocksource and >>>>>>> this will prevent them from probing. >>>>>> >>>>>> This logic is also used to remove the secure timer from being >>>>>> registered to the kernel on HS devices, while allowing the timer to be >>>>>> available on GP devices. Your patch actually would break that >>>>>> functionality. I suggest that you look at the history of the code that >>>>> >>>>> heh, that's a silly way of doing so. Could just detect if we're running >>>>> on HS or not. >>>> >>>> This function is invoked only after detecting that we are running on a >>>> HS device. >>> >>> What actually disables the timer is omap_get_timer_dt() and that's >>> called in more than one place. >> >> Yes indeed, and this patch is changing the behavior of >> omap_get_timer_dt(), so you have to check all call-sites. Also, one >> another thing is that this trick was used for DT boots so that the >> timers used for clocksource and clockevent are eliminated from the >> omap_dm_timer_request() API. The legacy boot used a specific API to mark >> those as reserved so that the _request API does not give them out. >> Granted that it may not have been the best approach, but that needs a >> solution if you change the behavior of this API. > > no doubt this needs a solution, but seesm like a solution here will have > to be incremental. See new revision of my series. I'm now skipping 32k > only and keeping it enabled so drivers/clocksource/ can use it. Yeah, have noticed, and it's better for the current problem at hand than this patch. > >>>>>>> Signed-off-by: Felipe Balbi <balbi@ti.com> >>>>>>> --- >>>>>>> >>>>>>> Tony, I wonder if you can get merged as a fix, or do you >>>>>>> prefer receiving it together with my timer series ? >>>>>> >>>>>> NAK for rc, as it breaks other stuff. >>>>> >>>>> a single stuff which is likely easy enough to fix. But fair enough >>>> >>>> Don't think this patch is fixing any critical bug either, better to club >>>> it with your cleanup series. >>> >>> it is kinda critical when you consider that the timer is disabled >>> outside of the soc type detection. >> >> This has been like this since DMTimer is converted to DT (from 3.8 >> kernel), and is a need for your counter32k clocksource series. The >> problem seems to stem from the reuse of omap_get_timer_dt for counter32k >> nodes. A better solution would be to remove the omap_get_timer_dt() from >> omap2_sync32k_clocksource_init() code and just replace it with >> of_find_compatible_node - you seem to be limiting the majority of that >> function to legacy mode in Patch 10 of your RFC series anyways. > > I still need omap_hwmod_enable() to be called, that's why it's still > used. I don't think replacing it with of_find_compatible_node() will buy > us much, we will still need to check for of_device_is_available() > anyway. Sure we skip all the timer flags (alwon, dsp, pwm, secure), but > that really isn't big deal. Yeah, I would think the counter_32k and the timer code will have to be decoupled in the future anyway, so it is best to localize the change now rather than later, so that the omap2_sync32k_clocksource_init() function can be decoupled cleanly from the timr code. The check for of_device_is_available may be moot if this device is always needed. But as long as current timer functionality is unchanged, I am not too particular about this. If it is always needed, the check for of_device regards Suman > When converting gptimer to clocksource, then I'll look at what I can do > for the timer request side of things. For now, things are working, > apparently without regressions (or at least I couldn't catch any so far) > and it seems clean enough that it could possibly be queued for v4.4 > merge window. For v4.5 I'll look at this again and try to figure out how > to handle gptimer. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index 0ff676273b4b..0c00138d7bd5 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -136,12 +136,6 @@ static struct clock_event_device clockevent_gpt = { .tick_resume = omap2_gp_timer_shutdown, }; -static struct property device_disabled = { - .name = "status", - .length = sizeof("disabled"), - .value = "disabled", -}; - static const struct of_device_id omap_timer_match[] __initconst = { { .compatible = "ti,omap2420-timer", }, { .compatible = "ti,omap3430-timer", }, @@ -161,9 +155,7 @@ static const struct of_device_id omap_timer_match[] __initconst = { * * Helper function to get a timer during early boot using device-tree for use * as kernel system timer. Optionally, the property argument can be used to - * select a timer with a specific property. Once a timer is found then mark - * the timer node in device-tree as disabled, to prevent the kernel from - * registering this timer as a platform device and so no one else can use it. + * select a timer with a specific property. */ static struct device_node * __init omap_get_timer_dt(const struct of_device_id *match, const char *property) @@ -183,7 +175,6 @@ static struct device_node * __init omap_get_timer_dt(const struct of_device_id * of_get_property(np, "ti,timer-secure", NULL))) continue; - of_add_property(np, &device_disabled); return np; }
We actually want these devices to be created because we will be moving timers to drivers/clocksource and this will prevent them from probing. Signed-off-by: Felipe Balbi <balbi@ti.com> --- Tony, I wonder if you can get merged as a fix, or do you prefer receiving it together with my timer series ? arch/arm/mach-omap2/timer.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)