Re: [PATCH 4/7] dt-bindings: chosen: Add clocksource and clockevent selection
On 10.09.2019 17:32, Sudeep Holla wrote: > External E-Mail > > > On Tue, Sep 10, 2019 at 04:47:13PM +0300, Claudiu Beznea wrote: >> From: Alexandre Belloni >> >> Some timer drivers may behave either as clocksource or clockevent >> or both. Until now, in case of platforms with multiple hardware >> resources of the same type, the drivers were chosing the first >> registered hardware resource as clocksource/clockevent and the >> next one as clockevent/clocksource. Other were using different >> compatibles (one for each functionality, although its about the >> same hardware). Add DT bindings to be able to choose the >> functionality of a timer. >> > > Is the piece of hardware not capable of serving as both clocksource > and clockevent or is it just the platform choice ? In my case, the hardware is not capable of serving at the same time a clocksource device and a clockevent device. First, I published v1 for a hardware device having this behavior at [1] requesting 1st probed hardware device to work as clocksource and the 2nd one to work as clockevent. The discussion at [1] ended up with the idea of having a mechanism to specify which hardware device behaves as clocksource and which one behaves as clockevent. Thank you, Claudiu Beznea [1] https://lore.kernel.org/lkml/1552580772-8499-1-git-send-email-claudiu.bez...@microchip.com/ > > -- > Regards, > Sudeep > > ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 2/7] clocksource: change timer registration macros
On 10.09.2019 17:49, Marc Zyngier wrote: > External E-Mail > > > [crazy Cc list, not sure it'll go anywhere] It is what get_maintainer.pl script returned to. > > On Tue, 10 Sep 2019 14:47:11 +0100, > Claudiu Beznea wrote: >> >> Change timer registration macros (TIMER_OF_DECLARE() and >> CLOCKSOURCE_OF_DECLARE()) by adding a new argument. This new argument >> is a pointer to an object of type struct timer_of and is used in >> timer_probe(). Based on the flags filled in the struct timer_of object >> the probing process will parse different DT bindings. Later on the >> drivers will use the result of this parsing. Even at the moment only >> few drivers are using this functionality there are other that could >> be converted to use it. >> >> Signed-off-by: Claudiu Beznea > > Why don't you introduce a new registration macro that does what you > want instead of creating this unnecessary churn all over the place? > I though it would be good to keep only one interface for all drivers. I'll keep in mind your proposal for next version. Thank you, Claudiu Beznea > M. > ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 0/7] add support for clocksource/clockevent DT selection
On 10.09.2019 19:05, John Stultz wrote: > External E-Mail > > > On Tue, Sep 10, 2019 at 6:47 AM Claudiu Beznea > wrote: >> >> This series adds support to permit the selection of clocksource/clockevent >> via DT. > > Sorry about this, but could you try to include more of a rational for > *why* this would be useful in your cover-letter/commit messages? > Sorry for not being to clear in the cover letter. The case I am trying to solve here is as follows: The timer hardware for which I publish a driver at [1] cannot work at the same time as a clocksource and clockevent. On some of our platforms we have more than one such a timer. So we could use one hardware resource as clocksource and one as clockevent but not one for both. Due to this, I proposed in the driver at [1] to have 1st probed hardware to work as clocksource and the 2nd one to work as clockevent. There are also other timer drivers that uses this approach. While working on this series I noticed that there are others that are using even different compatibles (although it looks to be related to the same hardware). Due to this Daniel proposed to have an unified mechanism for this scenario, see [2], (something like what I proposed in this series), such that to have a determinism b/w the function that the hardware resources would behave (either clocksource or clockevent or both). The description I gave in cover letter was not the best one. Because, actually, at this time, the clocksource/clockevent of the system would not be the one pointed by DT bindings, these DT bindings would chose only the function that a timer would have. Because if more than one clocksource/clockevent would be registered in a system the rating field of struct clocksource/struct clockevent would be the one that would decide the chosen clocksource/clockevent. [1] https://lore.kernel.org/lkml/1552580772-8499-1-git-send-email-claudiu.bez...@microchip.com/ [2] https://lore.kernel.org/lkml/2f831f1b-c87d-48bd-cf02-2ebb334b9...@linaro.org/ > I'm not sure I understand the limitation that requires such an option > to be added to the dts. > > thanks > -john > ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 7/7] clocksource/drivers/integrator-ap: parse the chosen node
On 11.09.2019 02:48, Linus Walleij wrote: > External E-Mail > > > On Tue, Sep 10, 2019 at 2:50 PM Claudiu Beznea > wrote: >> From: Alexandre Belloni >> >> The driver currently uses aliases to know whether the timer is the >> clocksource or the clockevent. > > OK maybe that wasn't the most elegant solution. > >> Add the /chosen/linux,clocksource and >> /chosen/linux,clockevent parsing while keeping backward compatibility. > > This is not how I would solve this today. > > I would simply remove/comment out the IRQ from the timer > that cannot be used for clockevent from the device tree > (apparently it doesn't work anyway), and make the code only > pick a timer with a valid interrupt assigned as clock event, > while a timer without interrupt can be used for clock source. This could also be used but it will not be compatible with old device trees. There are different ideas implemented in timer drivers with regards to this issue. Some of them are registering 1st timer to work as clocksource/clockevent and the 2nd one to work as clockevent/clocksource. Some are using different compatibles, one for clocksource, one for clockevent although these compatible seems to be for the same hardware type (I can point drivers/clocksource/timer-sprd.c). The idea with this series was, at it has been proposed in [1] to have one single mechanism for this kind of situations. Thank you, Claudiu Beznea [1] https://lore.kernel.org/lkml/2f831f1b-c87d-48bd-cf02-2ebb334b9...@linaro.org/ > > This has the upside of not needing any special aliases or > chosen things. > > Yours, > Linus Walleij > ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 4/7] dt-bindings: chosen: Add clocksource and clockevent selection
On 11.09.2019 03:03, Linus Walleij wrote:
> External E-Mail
>
>
> On Tue, Sep 10, 2019 at 4:11 PM Alexandre Belloni
> wrote:
>> On 10/09/2019 16:08:26+0100, Sudeep Holla wrote:
>>> On Tue, Sep 10, 2019 at 02:51:50PM +, claudiu.bez...@microchip.com
>>> wrote:
>
>>> In that case, why can't we identify capability that with the compatibles
>>> for this timer IP ?
>>>
>>> IOW, I don't like the proposal as it's hardware limitation.
>>
>> To be clear, bot timers are exactly the same but can't be clocksource
>> and clockevent at the same time. Why would we have different compatibles
>> for the exact same IP?
>
> In that case why not just pick the first one you find as clocksource
> and the second one as clock event? As they all come to the
> same timer of init function two simple local state variables can
> solve that:
>
> static bool registered_clocksource;
> static bool registered_clockevent;
>
> probe(timer) {
>if (!registered_clocksource) {
>register_clocksource(timer);
>registrered_clocksource = true;
>return;
>}
>if (!registered_clockevent) {
>register_clockevent(timer);
>registered_clockevent = true;
>return;
>}
>pr_info("surplus timer %p\n", timer);
> }
>
That was also my proposal for the driver I'm sending this series for (see
[1]) but it has been proposed to implement a mechanism similar to this one
in this series (see [2] and [3]).
Thank you,
Claudiu Beznea
[1]
https://lore.kernel.org/lkml/1552580772-8499-1-git-send-email-claudiu.bez...@microchip.com/
[2]
https://lore.kernel.org/lkml/a738fce5-1108-34d7-d255-dfcb86f51...@linaro.org/
[3]
https://lore.kernel.org/lkml/2f831f1b-c87d-48bd-cf02-2ebb334b9...@linaro.org/
> Clocksource and clockevent are natural singletons so there is
> no need to handle more than one of each in a driver for identical
> hardware.
>
> With the Integrator AP timer there is a real reason to select one over
> the other but as I replied to that patch it is pretty easy to just identify
> which block has this limitation by simply commenting out the IRQ
> line for it from the device tree.
>
> Maybe there is something about this I don't understand.
>
> Yours,
> Linus Walleij
>
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 0/7] add support for clocksource/clockevent DT selection
On 25.09.2019 20:19, Daniel Lezcano wrote: > External E-Mail > > > Hi Claudiu, > > On 10/09/2019 15:47, Claudiu Beznea wrote: >> Hi, >> >> This series adds support to permit the selection of clocksource/clockevent >> via DT. > > Thanks for the proposal and taking care of making some progress on this. > > I just wanted to let you know I've been traveling but the series is in > my pipe and I did not forget it. I'll comment it next week. Hi Daniel, No problem. Thank you for letting me know. Claudiu > > -- Daniel > > ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 4/7] dt-bindings: chosen: Add clocksource and clockevent selection
On 30.09.2019 17:32, Rob Herring wrote:
> On Wed, Sep 11, 2019 at 07:18:07AM +, claudiu.bez...@microchip.com wrote:
>>
>>
>> On 11.09.2019 03:03, Linus Walleij wrote:
>>> External E-Mail
>>>
>>>
>>> On Tue, Sep 10, 2019 at 4:11 PM Alexandre Belloni
>>> wrote:
On 10/09/2019 16:08:26+0100, Sudeep Holla wrote:
> On Tue, Sep 10, 2019 at 02:51:50PM +, claudiu.bez...@microchip.com
> wrote:
>>>
> In that case, why can't we identify capability that with the compatibles
> for this timer IP ?
>
> IOW, I don't like the proposal as it's hardware limitation.
To be clear, bot timers are exactly the same but can't be clocksource
and clockevent at the same time. Why would we have different compatibles
for the exact same IP?
>>>
>>> In that case why not just pick the first one you find as clocksource
>>> and the second one as clock event? As they all come to the
>>> same timer of init function two simple local state variables can
>>> solve that:
>>>
>>> static bool registered_clocksource;
>>> static bool registered_clockevent;
>>>
>>> probe(timer) {
>>>if (!registered_clocksource) {
>>>register_clocksource(timer);
>>>registrered_clocksource = true;
>>>return;
>>>}
>>>if (!registered_clockevent) {
>>>register_clockevent(timer);
>>>registered_clockevent = true;
>>>return;
>>>}
>>>pr_info("surplus timer %p\n", timer);
>>> }
>>>
>>
>> That was also my proposal for the driver I'm sending this series for (see
>> [1]) but it has been proposed to implement a mechanism similar to this one
>> in this series (see [2] and [3]).
>
> This comes up over and over, and the answer is still no. Either each
> block is identical and doesn't matter which one is used for what or
> there is some h/w difference that you should describe.
There are no hardware differences in my case. The block just cannot work at
the same time as clocksource and clockevent. And on SAM9X60's we want to
use it as clockevent for high resolution timers support.
>
> If you want something that would even be considered to put into DT,
> then define something BSD or other OS's could use too. (That's not a
> suggestion to respin this with generalized names.)
>
> Rob
>
>
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 0/7] add support for clocksource/clockevent DT selection
Hi Daniel, Taking into account that Rob doesn't agree with the solution proposed in this series do you think there is a chance to merge this driver as is? If you have other suggestion I am open to try it. Thank you, Claudiu Beznea On 26.09.2019 11:42, Claudiu Beznea wrote: > > > On 25.09.2019 20:19, Daniel Lezcano wrote: >> External E-Mail >> >> >> Hi Claudiu, >> >> On 10/09/2019 15:47, Claudiu Beznea wrote: >>> Hi, >>> >>> This series adds support to permit the selection of clocksource/clockevent >>> via DT. >> >> Thanks for the proposal and taking care of making some progress on this. >> >> I just wanted to let you know I've been traveling but the series is in >> my pipe and I did not forget it. I'll comment it next week. > > Hi Daniel, > > No problem. Thank you for letting me know. > > Claudiu > >> >> -- Daniel >> >> ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 0/7] add support for clocksource/clockevent DT selection
On 02.10.2019 16:35, Claudiu Beznea wrote: > Hi Daniel, > > Taking into account that Rob doesn't agree with the solution proposed in > this series do you think there is a chance to merge this driver as is? Sorry, I was talking here about the driver at [1]. [1] https://lore.kernel.org/lkml/1552580772-8499-1-git-send-email-claudiu.bez...@microchip.com/ > > If you have other suggestion I am open to try it. > > Thank you, > Claudiu Beznea > > On 26.09.2019 11:42, Claudiu Beznea wrote: >> >> >> On 25.09.2019 20:19, Daniel Lezcano wrote: >>> External E-Mail >>> >>> >>> Hi Claudiu, >>> >>> On 10/09/2019 15:47, Claudiu Beznea wrote: Hi, This series adds support to permit the selection of clocksource/clockevent via DT. >>> >>> Thanks for the proposal and taking care of making some progress on this. >>> >>> I just wanted to let you know I've been traveling but the series is in >>> my pipe and I did not forget it. I'll comment it next week. >> >> Hi Daniel, >> >> No problem. Thank you for letting me know. >> >> Claudiu >> >>> >>> -- Daniel >>> >>> ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 0/7] add support for clocksource/clockevent DT selection
Hi Daniel, On 13.10.2019 21:16, Daniel Lezcano wrote: > Hi Claudiu, > > sorry for the delay, I was OoO again. No problem, thank you for your reply. > > On 03/10/2019 12:43, claudiu.bez...@microchip.com wrote: >> >> >> On 02.10.2019 16:35, Claudiu Beznea wrote: >>> Hi Daniel, >>> >>> Taking into account that Rob doesn't agree with the solution proposed in >>> this series do you think there is a chance to merge this driver as is? >> >> Sorry, I was talking here about the driver at [1]. >> >> [1] >> https://lore.kernel.org/lkml/1552580772-8499-1-git-send-email-claudiu.bez...@microchip.com/ > > Damn! 7 months old. I'm truly sorry we do not have progress on this. Let > fix this once and for all. > > In the driver: > > ret = of_property_read_u32(node, "clock-frequency", &freq); > > It is unclear how is used this property. It should be the frequency > driving the timer, but can we get from a clk_get_rate() and a fixed divider? > The timer could be driven by 2 clock sources, one is called peripheral clock and is the clock that is also driving the IP itself, and one is called generic clock (this could drive only the timer itself) and should be at least 3 times lower than the peripheral clock. We could choose the clock driving the timer by setting the PIT64B_MR.SGCLK bit (0 - means the timer itself is driven by peripheral clock, 1 - means the timer is driven by the generic clock). The timer clock source could be divided by MR.PRES + 1. So, I used the clock-frequency DT binding to let user choose the timer's frequency. Based on the value provided via this DT binding the best clock source and prescaler is chosen via mchp_pit64b_pres_prepare() function. As the datasheet for the product that is using this IP is open now, I'm inserting here a link to it [1]. Thank you, Claudiu Beznea [1] http://ww1.microchip.com/downloads/en/DeviceDoc/SAM9X60-Data-Sheet-DS60001579A.pdf > ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 0/7] add support for clocksource/clockevent DT selection
Hi Daniel, On 18.10.2019 23:24, Daniel Lezcano wrote: > Hi Claudiu, > > On 15/10/2019 11:23, claudiu.bez...@microchip.com wrote: > > [ ... ] > >> The timer clock source could be divided by MR.PRES + 1. >> >> So, I used the clock-frequency DT binding to let user choose the timer's >> frequency. Based on the value provided via this DT binding the best clock >> source and prescaler is chosen via mchp_pit64b_pres_prepare() function. > > I'm willing to take the driver but I doubt the purpose of the > clock-frequency is to let the user choose the frequency. > I found this approach in the following already integrated drivers: drivers/clocksource/armv7m_systick.c drivers/clocksource/bcm2835_timer.c drivers/clocksource/bcm_kona_timer.c drivers/clocksource/mips-gic-timer.c drivers/clocksource/mps2-timer.c drivers/clocksource/timer-qcom.c drivers/clocksource/arm_arch_timer.c Looking through the documentation of these, most of them document this DT property as the frequency of the clock that drivers the timer, but none of them seems to have some IP internal dividers so that the timer to tick at different frequency than the clock that feeds the IP. From the documentation of the above drivers; drivers/clocksource/armv7m_systick.c - clock-frequency : The rate in HZ in input of the ARM SysTick drivers/clocksource/bcm2835_timer.c - clock-frequency : The frequency of the clock that drives the counter, in Hz. drivers/clocksource/bcm_kona_timer.c - clock-frequency: frequency that the clock operates drivers/clocksource/mips-gic-timer.c clock-frequency : Clock frequency at which the GIC timers operate. drivers/clocksource/mps2-timer.c - clock-frequency : The rate in HZ in input of the ARM MPS2 timer drivers/clocksource/timer-qcom.c - clock-frequency : The frequency of the debug timer and the general purpose timer(s) in Hz in that order. This is why I also chose this DT bindings. If you want I can stick to a fixed frequency hard coded in the driver. Please let me know if this would be OK for you. Thank you, Claudiu Beznea > > [ ... ] > > ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
